Thread: Possible patch for better index name choosing

Possible patch for better index name choosing

From
Tom Lane
Date:
Attached is a WIP patch for addressing the problems mentioned in this
thread:
http://archives.postgresql.org/pgsql-hackers/2009-12/msg01764.php

The main things that it does are (1) consider all index columns, not
just the first one as formerly; and (2) try to generate a usable name
for index expression columns, rather than just ignoring them which was
the effective behavior formerly.

There are several changes in the regression test outputs, mostly as a
result of choice (1).  I've not bothered to update the expected files
yet but just attached the output diffs to show what happens.

There is one thing that is not terribly nice about the behavior, which
is that CREATE TABLE LIKE INCLUDING INDEXES is unable to generate smart
names for expression indexes; it falls back to "expr", as for example
in

regression=# create table foo (f1 text, exclude (lower(f1) with =));
NOTICE:  CREATE TABLE / EXCLUDE will create implicit index "foo_lower_exclusion" for table "foo"
CREATE TABLE
regression=# create table foo2 (like foo including all);
NOTICE:  CREATE TABLE / EXCLUDE will create implicit index "foo2_expr_exclusion" for table "foo2"
CREATE TABLE

The reason for this is that the patch depends on FigureColname which
works on untransformed parse trees, and we don't have access to such
a tree when copying an existing index.  There seem to be three possible
responses to that:

1. Decide this isn't worth worrying about and use the patch as-is.

2. Change FigureColname to work on already-transformed expressions.
I don't care for this idea much, for two reasons.  First, FigureColname
would become significantly slower (eg, it would have to do catalog
lookups to resolve names of Vars, instead of just pulling the name out
of a ColumnRef), and this is objectionable considering it's part of
the required parsing path for even very simple commands.  Second, there
are various corner cases where we'd get different results, which would
likely break applications that are expecting specific result column
names from given queries.

3. Implement a separate FigureIndexColname function that works as much
like FigureColname as it can, but takes a transformed parse tree.
This fixes the LIKE case and also removes the need for the iexprname
field that the attached patch adds to IndexElem.  I think it largely
overcomes the two objections to idea #2, since an extra few lookups
during index creation are hardly a performance problem, and exact
application compatibility shouldn't be an issue here either.  It's
a bit ugly to have to keep two such functions in sync though.

I'm not real sure whether to go with the patch as-is or use idea #3.
It seems to depend on how annoyed you are by the LIKE behavior.

A different consideration is whether it's really a good idea to be
messing with default index names at all.  As illustrated in the attached
regression diffs, this does impact the error messages returned to
applications for unique-index failures.  I don't think this is a serious
problem across a major version update, but maybe someone thinks
differently.

Comments?

            regards, tom lane

Index: src/backend/bootstrap/bootparse.y
===================================================================
RCS file: /cvsroot/pgsql/src/backend/bootstrap/bootparse.y,v
retrieving revision 1.101
diff -c -r1.101 bootparse.y
*** src/backend/bootstrap/bootparse.y    7 Dec 2009 05:22:21 -0000    1.101
--- src/backend/bootstrap/bootparse.y    21 Dec 2009 02:47:04 -0000
***************
*** 322,328 ****
                  {
                      IndexElem *n = makeNode(IndexElem);
                      n->name = $1;
!                     n->expr = NULL;
                      n->opclass = list_make1(makeString($2));
                      n->ordering = SORTBY_DEFAULT;
                      n->nulls_ordering = SORTBY_NULLS_DEFAULT;
--- 322,329 ----
                  {
                      IndexElem *n = makeNode(IndexElem);
                      n->name = $1;
!                     n->iexpr = NULL;
!                     n->iexprname = NULL;
                      n->opclass = list_make1(makeString($2));
                      n->ordering = SORTBY_DEFAULT;
                      n->nulls_ordering = SORTBY_NULLS_DEFAULT;
Index: src/backend/catalog/index.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/catalog/index.c,v
retrieving revision 1.326
diff -c -r1.326 index.c
*** src/backend/catalog/index.c    9 Dec 2009 21:57:50 -0000    1.326
--- src/backend/catalog/index.c    21 Dec 2009 02:47:04 -0000
***************
*** 217,224 ****
              indexpr_item = lnext(indexpr_item);

              /*
!              * Make the attribute's name "pg_expresssion_nnn" (maybe think of
!              * something better later)
               */
              sprintf(NameStr(to->attname), "pg_expression_%d", i + 1);

--- 217,226 ----
              indexpr_item = lnext(indexpr_item);

              /*
!              * Make the attribute's name "pg_expression_nnn" (maybe think of
!              * something better later?  we're kind of locked in now, though,
!              * because pg_dump exposes these names when dumping comments on
!              * index columns)
               */
              sprintf(NameStr(to->attname), "pg_expression_%d", i + 1);

Index: src/backend/commands/indexcmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v
retrieving revision 1.188
diff -c -r1.188 indexcmds.c
*** src/backend/commands/indexcmds.c    7 Dec 2009 05:22:21 -0000    1.188
--- src/backend/commands/indexcmds.c    21 Dec 2009 02:47:05 -0000
***************
*** 252,282 ****
       */
      if (indexRelationName == NULL)
      {
          if (primary)
          {
!             indexRelationName = ChooseRelationName(RelationGetRelationName(rel),
                                                     NULL,
                                                     "pkey",
                                                     namespaceId);
          }
          else if (exclusionOpNames != NIL)
          {
!             IndexElem  *iparam = (IndexElem *) linitial(attributeList);
!
!             indexRelationName = ChooseRelationName(RelationGetRelationName(rel),
!                                                    iparam->name,
                                                     "exclusion",
                                                     namespaceId);
          }
!         else
          {
!             IndexElem  *iparam = (IndexElem *) linitial(attributeList);
!
!             indexRelationName = ChooseRelationName(RelationGetRelationName(rel),
!                                                    iparam->name,
                                                     "key",
                                                     namespaceId);
          }
      }

      /*
--- 252,288 ----
       */
      if (indexRelationName == NULL)
      {
+         char   *tabname = RelationGetRelationName(rel);
+
          if (primary)
          {
!             /* the pkey name does not depend on the specific column(s) */
!             indexRelationName = ChooseRelationName(tabname,
                                                     NULL,
                                                     "pkey",
                                                     namespaceId);
          }
          else if (exclusionOpNames != NIL)
          {
!             indexRelationName = ChooseRelationName(tabname,
!                                                    ChooseIndexNameAddition(attributeList),
                                                     "exclusion",
                                                     namespaceId);
          }
!         else if (isconstraint)
          {
!             indexRelationName = ChooseRelationName(tabname,
!                                                    ChooseIndexNameAddition(attributeList),
                                                     "key",
                                                     namespaceId);
          }
+         else
+         {
+             indexRelationName = ChooseRelationName(tabname,
+                                                    ChooseIndexNameAddition(attributeList),
+                                                    "idx",
+                                                    namespaceId);
+         }
      }

      /*
***************
*** 884,890 ****
              HeapTuple    atttuple;
              Form_pg_attribute attform;

!             Assert(attribute->expr == NULL);
              atttuple = SearchSysCacheAttName(relId, attribute->name);
              if (!HeapTupleIsValid(atttuple))
              {
--- 890,896 ----
              HeapTuple    atttuple;
              Form_pg_attribute attform;

!             Assert(attribute->iexpr == NULL);
              atttuple = SearchSysCacheAttName(relId, attribute->name);
              if (!HeapTupleIsValid(atttuple))
              {
***************
*** 905,915 ****
              atttype = attform->atttypid;
              ReleaseSysCache(atttuple);
          }
!         else if (attribute->expr && IsA(attribute->expr, Var) &&
!                  ((Var *) attribute->expr)->varattno != InvalidAttrNumber)
          {
              /* Tricky tricky, he wrote (column) ... treat as simple attr */
!             Var           *var = (Var *) attribute->expr;

              indexInfo->ii_KeyAttrNumbers[attn] = var->varattno;
              atttype = get_atttype(relId, var->varattno);
--- 911,921 ----
              atttype = attform->atttypid;
              ReleaseSysCache(atttuple);
          }
!         else if (attribute->iexpr && IsA(attribute->iexpr, Var) &&
!                  ((Var *) attribute->iexpr)->varattno != InvalidAttrNumber)
          {
              /* Tricky tricky, he wrote (column) ... treat as simple attr */
!             Var           *var = (Var *) attribute->iexpr;

              indexInfo->ii_KeyAttrNumbers[attn] = var->varattno;
              atttype = get_atttype(relId, var->varattno);
***************
*** 917,938 ****
          else
          {
              /* Index expression */
!             Assert(attribute->expr != NULL);
              indexInfo->ii_KeyAttrNumbers[attn] = 0;        /* marks expression */
              indexInfo->ii_Expressions = lappend(indexInfo->ii_Expressions,
!                                                 attribute->expr);
!             atttype = exprType(attribute->expr);

              /*
               * We don't currently support generation of an actual query plan
               * for an index expression, only simple scalar expressions; hence
               * these restrictions.
               */
!             if (contain_subplans(attribute->expr))
                  ereport(ERROR,
                          (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                           errmsg("cannot use subquery in index expression")));
!             if (contain_agg_clause(attribute->expr))
                  ereport(ERROR,
                          (errcode(ERRCODE_GROUPING_ERROR),
                  errmsg("cannot use aggregate function in index expression")));
--- 923,944 ----
          else
          {
              /* Index expression */
!             Assert(attribute->iexpr != NULL);
              indexInfo->ii_KeyAttrNumbers[attn] = 0;        /* marks expression */
              indexInfo->ii_Expressions = lappend(indexInfo->ii_Expressions,
!                                                 attribute->iexpr);
!             atttype = exprType(attribute->iexpr);

              /*
               * We don't currently support generation of an actual query plan
               * for an index expression, only simple scalar expressions; hence
               * these restrictions.
               */
!             if (contain_subplans(attribute->iexpr))
                  ereport(ERROR,
                          (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                           errmsg("cannot use subquery in index expression")));
!             if (contain_agg_clause(attribute->iexpr))
                  ereport(ERROR,
                          (errcode(ERRCODE_GROUPING_ERROR),
                  errmsg("cannot use aggregate function in index expression")));
***************
*** 942,948 ****
               * if you aren't going to get the same result for the same data
               * every time, it's not clear what the index entries mean at all.
               */
!             if (contain_mutable_functions(attribute->expr))
                  ereport(ERROR,
                          (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                           errmsg("functions in index expression must be marked IMMUTABLE")));
--- 948,954 ----
               * if you aren't going to get the same result for the same data
               * every time, it's not clear what the index entries mean at all.
               */
!             if (contain_mutable_functions(attribute->iexpr))
                  ereport(ERROR,
                          (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
                           errmsg("functions in index expression must be marked IMMUTABLE")));
***************
*** 1392,1397 ****
--- 1398,1446 ----
  }

  /*
+  * Generate "name2" for a new index given the list of IndexElems for it.
+  * This will be passed to ChooseRelationName along with the parent table
+  * name and a suitable label.
+  *
+  * We know that less than NAMEDATALEN characters will actually be used,
+  * so we can truncate the result once we've generated that many.
+  */
+ char *
+ ChooseIndexNameAddition(List *indexElems)
+ {
+     char        buf[NAMEDATALEN * 2];
+     int            buflen = 0;
+     ListCell   *lc;
+
+     buf[0] = '\0';
+     foreach(lc, indexElems)
+     {
+         IndexElem  *ielem = (IndexElem *) lfirst(lc);
+         const char *name;
+
+         if (ielem->name)
+             name = ielem->name;                /* simple column reference */
+         else if (ielem->iexprname)
+             name = ielem->iexprname;        /* expression */
+         else
+             name = "expr";                    /* default name for expression */
+
+         if (buflen > 0)
+             buf[buflen++] = '_';            /* insert _ between names */
+
+         /*
+          * At this point we have buflen <= NAMEDATALEN.  name should be less
+          * than NAMEDATALEN already, but use strlcpy for paranoia.
+          */
+         strlcpy(buf + buflen, name, NAMEDATALEN);
+         buflen += strlen(buf + buflen);
+         if (buflen >= NAMEDATALEN)
+             break;
+     }
+     return pstrdup(buf);
+ }
+
+ /*
   * relationHasPrimaryKey -
   *
   *    See whether an existing relation has a primary key.
Index: src/backend/nodes/copyfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v
retrieving revision 1.454
diff -c -r1.454 copyfuncs.c
*** src/backend/nodes/copyfuncs.c    15 Dec 2009 17:57:46 -0000    1.454
--- src/backend/nodes/copyfuncs.c    21 Dec 2009 02:47:05 -0000
***************
*** 2120,2126 ****
      IndexElem  *newnode = makeNode(IndexElem);

      COPY_STRING_FIELD(name);
!     COPY_NODE_FIELD(expr);
      COPY_NODE_FIELD(opclass);
      COPY_SCALAR_FIELD(ordering);
      COPY_SCALAR_FIELD(nulls_ordering);
--- 2120,2127 ----
      IndexElem  *newnode = makeNode(IndexElem);

      COPY_STRING_FIELD(name);
!     COPY_NODE_FIELD(iexpr);
!     COPY_STRING_FIELD(iexprname);
      COPY_NODE_FIELD(opclass);
      COPY_SCALAR_FIELD(ordering);
      COPY_SCALAR_FIELD(nulls_ordering);
Index: src/backend/nodes/equalfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.376
diff -c -r1.376 equalfuncs.c
*** src/backend/nodes/equalfuncs.c    15 Dec 2009 17:57:46 -0000    1.376
--- src/backend/nodes/equalfuncs.c    21 Dec 2009 02:47:05 -0000
***************
*** 2071,2077 ****
  _equalIndexElem(IndexElem *a, IndexElem *b)
  {
      COMPARE_STRING_FIELD(name);
!     COMPARE_NODE_FIELD(expr);
      COMPARE_NODE_FIELD(opclass);
      COMPARE_SCALAR_FIELD(ordering);
      COMPARE_SCALAR_FIELD(nulls_ordering);
--- 2071,2078 ----
  _equalIndexElem(IndexElem *a, IndexElem *b)
  {
      COMPARE_STRING_FIELD(name);
!     COMPARE_NODE_FIELD(iexpr);
!     COMPARE_STRING_FIELD(iexprname);
      COMPARE_NODE_FIELD(opclass);
      COMPARE_SCALAR_FIELD(ordering);
      COMPARE_SCALAR_FIELD(nulls_ordering);
Index: src/backend/nodes/outfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/nodes/outfuncs.c,v
retrieving revision 1.375
diff -c -r1.375 outfuncs.c
*** src/backend/nodes/outfuncs.c    15 Dec 2009 17:57:46 -0000    1.375
--- src/backend/nodes/outfuncs.c    21 Dec 2009 02:47:05 -0000
***************
*** 1945,1951 ****
      WRITE_NODE_TYPE("INDEXELEM");

      WRITE_STRING_FIELD(name);
!     WRITE_NODE_FIELD(expr);
      WRITE_NODE_FIELD(opclass);
      WRITE_ENUM_FIELD(ordering, SortByDir);
      WRITE_ENUM_FIELD(nulls_ordering, SortByNulls);
--- 1945,1952 ----
      WRITE_NODE_TYPE("INDEXELEM");

      WRITE_STRING_FIELD(name);
!     WRITE_NODE_FIELD(iexpr);
!     WRITE_STRING_FIELD(iexprname);
      WRITE_NODE_FIELD(opclass);
      WRITE_ENUM_FIELD(ordering, SortByDir);
      WRITE_ENUM_FIELD(nulls_ordering, SortByNulls);
Index: src/backend/parser/gram.y
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.697
diff -c -r2.697 gram.y
*** src/backend/parser/gram.y    15 Dec 2009 17:57:47 -0000    2.697
--- src/backend/parser/gram.y    21 Dec 2009 02:47:05 -0000
***************
*** 4886,4892 ****
                  {
                      $$ = makeNode(IndexElem);
                      $$->name = $1;
!                     $$->expr = NULL;
                      $$->opclass = $2;
                      $$->ordering = $3;
                      $$->nulls_ordering = $4;
--- 4886,4893 ----
                  {
                      $$ = makeNode(IndexElem);
                      $$->name = $1;
!                     $$->iexpr = NULL;
!                     $$->iexprname = NULL;
                      $$->opclass = $2;
                      $$->ordering = $3;
                      $$->nulls_ordering = $4;
***************
*** 4895,4901 ****
                  {
                      $$ = makeNode(IndexElem);
                      $$->name = NULL;
!                     $$->expr = $1;
                      $$->opclass = $2;
                      $$->ordering = $3;
                      $$->nulls_ordering = $4;
--- 4896,4903 ----
                  {
                      $$ = makeNode(IndexElem);
                      $$->name = NULL;
!                     $$->iexpr = $1;
!                     $$->iexprname = NULL;
                      $$->opclass = $2;
                      $$->ordering = $3;
                      $$->nulls_ordering = $4;
***************
*** 4904,4910 ****
                  {
                      $$ = makeNode(IndexElem);
                      $$->name = NULL;
!                     $$->expr = $2;
                      $$->opclass = $4;
                      $$->ordering = $5;
                      $$->nulls_ordering = $6;
--- 4906,4913 ----
                  {
                      $$ = makeNode(IndexElem);
                      $$->name = NULL;
!                     $$->iexpr = $2;
!                     $$->iexprname = NULL;
                      $$->opclass = $4;
                      $$->ordering = $5;
                      $$->nulls_ordering = $6;
Index: src/backend/parser/parse_target.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_target.c,v
retrieving revision 1.174
diff -c -r1.174 parse_target.c
*** src/backend/parser/parse_target.c    31 Oct 2009 01:41:31 -0000    1.174
--- src/backend/parser/parse_target.c    21 Dec 2009 02:47:05 -0000
***************
*** 1417,1422 ****
--- 1417,1449 ----
      return "?column?";
  }

+ /*
+  * FigureIndexColname -
+  *      choose the name for an expression column in an index
+  *
+  * This is actually just like FigureColname, except we return NULL if
+  * we can't pick a good name.
+  */
+ char *
+ FigureIndexColname(Node *node)
+ {
+     char       *name = NULL;
+
+     FigureColnameInternal(node, &name);
+     return name;
+ }
+
+ /*
+  * FigureColnameInternal -
+  *      internal workhorse for FigureColname
+  *
+  * Return value indicates strength of confidence in result:
+  *        0 - no information
+  *        1 - second-best name choice
+  *        2 - good name choice
+  * The return value is actually only used internally.
+  * If the result isn't zero, *name is set to the chosen name.
+  */
  static int
  FigureColnameInternal(Node *node, char **name)
  {
Index: src/backend/parser/parse_utilcmd.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/parser/parse_utilcmd.c,v
retrieving revision 2.33
diff -c -r2.33 parse_utilcmd.c
*** src/backend/parser/parse_utilcmd.c    20 Dec 2009 18:28:14 -0000    2.33
--- src/backend/parser/parse_utilcmd.c    21 Dec 2009 02:47:05 -0000
***************
*** 48,53 ****
--- 48,54 ----
  #include "parser/parse_clause.h"
  #include "parser/parse_expr.h"
  #include "parser/parse_relation.h"
+ #include "parser/parse_target.h"
  #include "parser/parse_type.h"
  #include "parser/parse_utilcmd.h"
  #include "parser/parser.h"
***************
*** 750,756 ****
                  int                colno;

                  comment = GetComment(parent_index_oid, RelationRelationId, 0);
!
                  if (comment != NULL)
                  {
                      /*
--- 751,757 ----
                  int                colno;

                  comment = GetComment(parent_index_oid, RelationRelationId, 0);
!
                  if (comment != NULL)
                  {
                      /*
***************
*** 827,858 ****
   * chooseIndexName
   *
   * Set name for unnamed index. See also the same logic in DefineIndex.
   */
  static char *
  chooseIndexName(const RangeVar *relation, IndexStmt *index_stmt)
  {
      Oid    namespaceId;
!
      namespaceId = RangeVarGetCreationNamespace(relation);
      if (index_stmt->primary)
      {
!         /* no need for column list with pkey */
!         return ChooseRelationName(relation->relname, NULL,
!                                   "pkey", namespaceId);
      }
      else if (index_stmt->excludeOpNames != NIL)
      {
!         IndexElem  *iparam = (IndexElem *) linitial(index_stmt->indexParams);
!
!         return ChooseRelationName(relation->relname, iparam->name,
!                                   "exclusion", namespaceId);
      }
      else
      {
!         IndexElem  *iparam = (IndexElem *) linitial(index_stmt->indexParams);
!
!         return ChooseRelationName(relation->relname, iparam->name,
!                                   "key", namespaceId);
      }
  }

--- 828,872 ----
   * chooseIndexName
   *
   * Set name for unnamed index. See also the same logic in DefineIndex.
+  *
+  * XXX this is inherently broken because the indexes aren't created
+  * immediately, so we fail to resolve conflicts when the same name is
+  * derived for multiple indexes.
   */
  static char *
  chooseIndexName(const RangeVar *relation, IndexStmt *index_stmt)
  {
      Oid    namespaceId;
!
      namespaceId = RangeVarGetCreationNamespace(relation);
      if (index_stmt->primary)
      {
!         /* the pkey name does not depend on the specific column(s) */
!         return ChooseRelationName(relation->relname,
!                                   NULL,
!                                   "pkey",
!                                   namespaceId);
      }
      else if (index_stmt->excludeOpNames != NIL)
      {
!         return ChooseRelationName(relation->relname,
!                                   ChooseIndexNameAddition(index_stmt->indexParams),
!                                   "exclusion",
!                                   namespaceId);
!     }
!     else if (index_stmt->isconstraint)
!     {
!         return ChooseRelationName(relation->relname,
!                                   ChooseIndexNameAddition(index_stmt->indexParams),
!                                   "key",
!                                   namespaceId);
      }
      else
      {
!         return ChooseRelationName(relation->relname,
!                                   ChooseIndexNameAddition(index_stmt->indexParams),
!                                   "idx",
!                                   namespaceId);
      }
  }

***************
*** 1039,1045 ****
              keycoltype = get_atttype(indrelid, attnum);

              iparam->name = attname;
!             iparam->expr = NULL;
          }
          else
          {
--- 1053,1060 ----
              keycoltype = get_atttype(indrelid, attnum);

              iparam->name = attname;
!             iparam->iexpr = NULL;
!             iparam->iexprname = NULL;
          }
          else
          {
***************
*** 1055,1061 ****
              change_varattnos_of_a_node(indexkey, attmap);

              iparam->name = NULL;
!             iparam->expr = indexkey;

              keycoltype = exprType(indexkey);
          }
--- 1070,1082 ----
              change_varattnos_of_a_node(indexkey, attmap);

              iparam->name = NULL;
!             iparam->iexpr = indexkey;
!
!             /*
!              * Unfortunately, we have no good way to extract a name from
!              * the already-transformed index expression; so punt.
!              */
!             iparam->iexprname = NULL;

              keycoltype = exprType(indexkey);
          }
***************
*** 1452,1458 ****
          /* OK, add it to the index definition */
          iparam = makeNode(IndexElem);
          iparam->name = pstrdup(key);
!         iparam->expr = NULL;
          iparam->opclass = NIL;
          iparam->ordering = SORTBY_DEFAULT;
          iparam->nulls_ordering = SORTBY_NULLS_DEFAULT;
--- 1473,1480 ----
          /* OK, add it to the index definition */
          iparam = makeNode(IndexElem);
          iparam->name = pstrdup(key);
!         iparam->iexpr = NULL;
!         iparam->iexprname = NULL;
          iparam->opclass = NIL;
          iparam->ordering = SORTBY_DEFAULT;
          iparam->nulls_ordering = SORTBY_NULLS_DEFAULT;
***************
*** 1579,1594 ****
      {
          IndexElem  *ielem = (IndexElem *) lfirst(l);

!         if (ielem->expr)
          {
!             ielem->expr = transformExpr(pstate, ielem->expr);

              /*
               * We check only that the result type is legitimate; this is for
               * consistency with what transformWhereClause() checks for the
               * predicate.  DefineIndex() will make more checks.
               */
!             if (expression_returns_set(ielem->expr))
                  ereport(ERROR,
                          (errcode(ERRCODE_DATATYPE_MISMATCH),
                           errmsg("index expression cannot return a set")));
--- 1601,1620 ----
      {
          IndexElem  *ielem = (IndexElem *) lfirst(l);

!         if (ielem->iexpr)
          {
!             /* Extract index column name before transforming expr */
!             ielem->iexprname = FigureIndexColname(ielem->iexpr);
!
!             /* Now do parse transformation of the expression */
!             ielem->iexpr = transformExpr(pstate, ielem->iexpr);

              /*
               * We check only that the result type is legitimate; this is for
               * consistency with what transformWhereClause() checks for the
               * predicate.  DefineIndex() will make more checks.
               */
!             if (expression_returns_set(ielem->iexpr))
                  ereport(ERROR,
                          (errcode(ERRCODE_DATATYPE_MISMATCH),
                           errmsg("index expression cannot return a set")));
Index: src/include/commands/defrem.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/commands/defrem.h,v
retrieving revision 1.98
diff -c -r1.98 defrem.h
*** src/include/commands/defrem.h    7 Dec 2009 05:22:23 -0000    1.98
--- src/include/commands/defrem.h    21 Dec 2009 02:47:05 -0000
***************
*** 45,50 ****
--- 45,51 ----
                 const char *label);
  extern char *ChooseRelationName(const char *name1, const char *name2,
                     const char *label, Oid namespaceid);
+ extern char *ChooseIndexNameAddition(List *indexElems);
  extern Oid    GetDefaultOpClass(Oid type_id, Oid am_id);

  /* commands/functioncmds.c */
Index: src/include/nodes/parsenodes.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.419
diff -c -r1.419 parsenodes.h
*** src/include/nodes/parsenodes.h    15 Dec 2009 17:57:47 -0000    1.419
--- src/include/nodes/parsenodes.h    21 Dec 2009 02:47:06 -0000
***************
*** 493,506 ****
   * IndexElem - index parameters (used in CREATE INDEX)
   *
   * For a plain index attribute, 'name' is the name of the table column to
!  * index, and 'expr' is NULL.  For an index expression, 'name' is NULL and
!  * 'expr' is the expression tree.
   */
  typedef struct IndexElem
  {
      NodeTag        type;
      char       *name;            /* name of attribute to index, or NULL */
!     Node       *expr;            /* expression to index, or NULL */
      List       *opclass;        /* name of desired opclass; NIL = default */
      SortByDir    ordering;        /* ASC/DESC/default */
      SortByNulls nulls_ordering; /* FIRST/LAST/default */
--- 493,512 ----
   * IndexElem - index parameters (used in CREATE INDEX)
   *
   * For a plain index attribute, 'name' is the name of the table column to
!  * index, and 'iexpr' is NULL.  For an index expression, 'name' is NULL and
!  * 'iexpr' is the expression tree.
!  *
!  * 'iexprname' is a name extracted from 'iexpr', or NULL for none/default.
!  * We need this field because the FigureColname machinery only works on
!  * not-yet-transformed expression trees, so we can't get it from iexpr
!  * after parse transformation.
   */
  typedef struct IndexElem
  {
      NodeTag        type;
      char       *name;            /* name of attribute to index, or NULL */
!     Node       *iexpr;            /* expression to index, or NULL */
!     char       *iexprname;        /* name extracted from iexpr, or NULL */
      List       *opclass;        /* name of desired opclass; NIL = default */
      SortByDir    ordering;        /* ASC/DESC/default */
      SortByNulls nulls_ordering; /* FIRST/LAST/default */
Index: src/include/parser/parse_target.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/parser/parse_target.h,v
retrieving revision 1.44
diff -c -r1.44 parse_target.h
*** src/include/parser/parse_target.h    1 Jan 2009 17:24:00 -0000    1.44
--- src/include/parser/parse_target.h    21 Dec 2009 02:47:06 -0000
***************
*** 37,41 ****
--- 37,42 ----
  extern TupleDesc expandRecordVariable(ParseState *pstate, Var *var,
                       int levelsup);
  extern char *FigureColname(Node *node);
+ extern char *FigureIndexColname(Node *node);

  #endif   /* PARSE_TARGET_H */
*** src/test/regress/expected/constraints.out    Sun Dec 20 21:40:15 2009
--- src/test/regress/results/constraints.out    Sun Dec 20 21:40:54 2009
***************
*** 359,370 ****
  DROP TABLE UNIQUE_TBL;
  CREATE TABLE UNIQUE_TBL (i int, t text,
      UNIQUE(i,t));
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "unique_tbl_i_key" for table "unique_tbl"
  INSERT INTO UNIQUE_TBL VALUES (1, 'one');
  INSERT INTO UNIQUE_TBL VALUES (2, 'two');
  INSERT INTO UNIQUE_TBL VALUES (1, 'three');
  INSERT INTO UNIQUE_TBL VALUES (1, 'one');
! ERROR:  duplicate key value violates unique constraint "unique_tbl_i_key"
  DETAIL:  Key (i, t)=(1, one) already exists.
  INSERT INTO UNIQUE_TBL VALUES (5, 'one');
  INSERT INTO UNIQUE_TBL (t) VALUES ('six');
--- 359,370 ----
  DROP TABLE UNIQUE_TBL;
  CREATE TABLE UNIQUE_TBL (i int, t text,
      UNIQUE(i,t));
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "unique_tbl_i_t_key" for table "unique_tbl"
  INSERT INTO UNIQUE_TBL VALUES (1, 'one');
  INSERT INTO UNIQUE_TBL VALUES (2, 'two');
  INSERT INTO UNIQUE_TBL VALUES (1, 'three');
  INSERT INTO UNIQUE_TBL VALUES (1, 'one');
! ERROR:  duplicate key value violates unique constraint "unique_tbl_i_t_key"
  DETAIL:  Key (i, t)=(1, one) already exists.
  INSERT INTO UNIQUE_TBL VALUES (5, 'one');
  INSERT INTO UNIQUE_TBL (t) VALUES ('six');
***************
*** 523,529 ****
      (c1 WITH &&, (c2::circle) WITH ~=)
      WHERE (circle_center(c1) <> '(0,0)')
  );
! NOTICE:  CREATE TABLE / EXCLUDE will create implicit index "circles_c1_exclusion" for table "circles"
  -- these should succeed because they don't match the index predicate
  INSERT INTO circles VALUES('<(0,0), 5>', '<(0,0), 5>');
  INSERT INTO circles VALUES('<(0,0), 5>', '<(0,0), 5>');
--- 523,529 ----
      (c1 WITH &&, (c2::circle) WITH ~=)
      WHERE (circle_center(c1) <> '(0,0)')
  );
! NOTICE:  CREATE TABLE / EXCLUDE will create implicit index "circles_c1_c2_exclusion" for table "circles"
  -- these should succeed because they don't match the index predicate
  INSERT INTO circles VALUES('<(0,0), 5>', '<(0,0), 5>');
  INSERT INTO circles VALUES('<(0,0), 5>', '<(0,0), 5>');
***************
*** 531,537 ****
  INSERT INTO circles VALUES('<(10,10), 10>', '<(0,0), 5>');
  -- fail, overlaps
  INSERT INTO circles VALUES('<(20,20), 10>', '<(0,0), 5>');
! ERROR:  conflicting key value violates exclusion constraint "circles_c1_exclusion"
  DETAIL:  Key (c1, (c2::circle))=(<(20,20),10>, <(0,0),5>) conflicts with existing key (c1,
(c2::circle))=(<(10,10),10>,<(0,0),5>). 
  -- succeed because c1 doesn't overlap
  INSERT INTO circles VALUES('<(20,20), 1>', '<(0,0), 5>');
--- 531,537 ----
  INSERT INTO circles VALUES('<(10,10), 10>', '<(0,0), 5>');
  -- fail, overlaps
  INSERT INTO circles VALUES('<(20,20), 10>', '<(0,0), 5>');
! ERROR:  conflicting key value violates exclusion constraint "circles_c1_c2_exclusion"
  DETAIL:  Key (c1, (c2::circle))=(<(20,20),10>, <(0,0),5>) conflicts with existing key (c1,
(c2::circle))=(<(10,10),10>,<(0,0),5>). 
  -- succeed because c1 doesn't overlap
  INSERT INTO circles VALUES('<(20,20), 1>', '<(0,0), 5>');
***************
*** 540,547 ****
  -- should fail on existing data without the WHERE clause
  ALTER TABLE circles ADD EXCLUDE USING gist
    (c1 WITH &&, (c2::circle) WITH ~=);
! NOTICE:  ALTER TABLE / ADD EXCLUDE will create implicit index "circles_c1_exclusion1" for table "circles"
! ERROR:  could not create exclusion constraint "circles_c1_exclusion1"
  DETAIL:  Key (c1, (c2::circle))=(<(0,0),5>, <(0,0),5>) conflicts with key (c1, (c2::circle))=(<(0,0),5>, <(0,0),5>).
  DROP TABLE circles;
  -- Check deferred exclusion constraint
--- 540,547 ----
  -- should fail on existing data without the WHERE clause
  ALTER TABLE circles ADD EXCLUDE USING gist
    (c1 WITH &&, (c2::circle) WITH ~=);
! NOTICE:  ALTER TABLE / ADD EXCLUDE will create implicit index "circles_c1_c2_exclusion1" for table "circles"
! ERROR:  could not create exclusion constraint "circles_c1_c2_exclusion1"
  DETAIL:  Key (c1, (c2::circle))=(<(0,0),5>, <(0,0),5>) conflicts with key (c1, (c2::circle))=(<(0,0),5>, <(0,0),5>).
  DROP TABLE circles;
  -- Check deferred exclusion constraint

======================================================================

*** src/test/regress/expected/inherit.out    Mon Oct 12 15:49:24 2009
--- src/test/regress/results/inherit.out    Sun Dec 20 21:41:09 2009
***************
*** 1033,1051 ****
   b      | text |           | extended | B
  Indexes:
      "t_all_pkey" PRIMARY KEY, btree (a)
!     "t_all_b_key" btree (b)
!     "t_all_key" btree ((a || b))
  Check constraints:
      "t1_a_check" CHECK (length(a) > 2)
  Has OIDs: no

  SELECT c.relname, objsubid, description FROM pg_description, pg_index i, pg_class c WHERE classoid =
'pg_class'::regclassAND objoid = i.indexrelid AND c.oid = i.indexrelid AND i.indrelid = 't_all'::regclass ORDER BY
c.relname,objsubid; 
!    relname   | objsubid |     description
! -------------+----------+---------------------
!  t_all_b_key |        0 | index b_key
!  t_all_key   |        1 | index column fnidx
!  t_all_pkey  |        0 | index pkey
!  t_all_pkey  |        1 | index column pkey.a
  (4 rows)

  CREATE TABLE inh_error1 () INHERITS (t1, t4);
--- 1033,1051 ----
   b      | text |           | extended | B
  Indexes:
      "t_all_pkey" PRIMARY KEY, btree (a)
!     "t_all_b_idx" btree (b)
!     "t_all_expr_idx" btree ((a || b))
  Check constraints:
      "t1_a_check" CHECK (length(a) > 2)
  Has OIDs: no

  SELECT c.relname, objsubid, description FROM pg_description, pg_index i, pg_class c WHERE classoid =
'pg_class'::regclassAND objoid = i.indexrelid AND c.oid = i.indexrelid AND i.indrelid = 't_all'::regclass ORDER BY
c.relname,objsubid; 
!     relname     | objsubid |     description
! ----------------+----------+---------------------
!  t_all_b_idx    |        0 | index b_key
!  t_all_expr_idx |        1 | index column fnidx
!  t_all_pkey     |        0 | index pkey
!  t_all_pkey     |        1 | index column pkey.a
  (4 rows)

  CREATE TABLE inh_error1 () INHERITS (t1, t4);

======================================================================

*** src/test/regress/expected/foreign_key.out    Sun Nov 22 00:20:41 2009
--- src/test/regress/results/foreign_key.out    Sun Dec 20 21:41:46 2009
***************
*** 736,742 ****
  DROP TABLE PKTABLE;
  -- Test for referencing column number smaller than referenced constraint
  CREATE TABLE PKTABLE (ptest1 int, ptest2 int, UNIQUE(ptest1, ptest2));
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_ptest1_key" for table "pktable"
  CREATE TABLE FKTABLE_FAIL1 (ftest1 int REFERENCES pktable(ptest1));
  ERROR:  there is no unique constraint matching given keys for referenced table "pktable"
  DROP TABLE FKTABLE_FAIL1;
--- 736,742 ----
  DROP TABLE PKTABLE;
  -- Test for referencing column number smaller than referenced constraint
  CREATE TABLE PKTABLE (ptest1 int, ptest2 int, UNIQUE(ptest1, ptest2));
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_ptest1_ptest2_key" for table "pktable"
  CREATE TABLE FKTABLE_FAIL1 (ftest1 int REFERENCES pktable(ptest1));
  ERROR:  there is no unique constraint matching given keys for referenced table "pktable"
  DROP TABLE FKTABLE_FAIL1;
***************
*** 860,866 ****
  create table pktable_base (base1 int not null);
  create table pktable (ptest1 int, primary key(base1), unique(base1, ptest1)) inherits (pktable_base);
  NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable"
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_base1_key" for table "pktable"
  create table fktable (ftest1 int references pktable(base1));
  -- now some ins, upd, del
  insert into pktable(base1) values (1);
--- 860,866 ----
  create table pktable_base (base1 int not null);
  create table pktable (ptest1 int, primary key(base1), unique(base1, ptest1)) inherits (pktable_base);
  NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable"
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_base1_ptest1_key" for table "pktable"
  create table fktable (ftest1 int references pktable(base1));
  -- now some ins, upd, del
  insert into pktable(base1) values (1);
***************
*** 1098,1104 ****
  NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable"
  NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_id2_key" for table "pktable"
  NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_id3_key" for table "pktable"
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_id1_key" for table "pktable"
  CREATE TEMP TABLE fktable (
          x1      INT4 REFERENCES pktable(id1),
          x2      VARCHAR(4) REFERENCES pktable(id2),
--- 1098,1104 ----
  NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pktable_pkey" for table "pktable"
  NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_id2_key" for table "pktable"
  NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_id3_key" for table "pktable"
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "pktable_id1_id2_id3_key" for table "pktable"
  CREATE TEMP TABLE fktable (
          x1      INT4 REFERENCES pktable(id1),
          x2      VARCHAR(4) REFERENCES pktable(id2),

======================================================================

*** src/test/regress/expected/alter_table.out    Sat Aug  1 16:59:17 2009
--- src/test/regress/results/alter_table.out    Sun Dec 20 21:42:47 2009
***************
*** 159,165 ****
  NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "tmp2_pkey" for table "tmp2"
  CREATE TABLE tmp3 (a int, b int);
  CREATE TABLE tmp4 (a int, b int, unique(a,b));
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "tmp4_a_key" for table "tmp4"
  CREATE TABLE tmp5 (a int, b int);
  -- Insert rows into tmp2 (pktable)
  INSERT INTO tmp2 values (1);
--- 159,165 ----
  NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "tmp2_pkey" for table "tmp2"
  CREATE TABLE tmp3 (a int, b int);
  CREATE TABLE tmp4 (a int, b int, unique(a,b));
! NOTICE:  CREATE TABLE / UNIQUE will create implicit index "tmp4_a_b_key" for table "tmp4"
  CREATE TABLE tmp5 (a int, b int);
  -- Insert rows into tmp2 (pktable)
  INSERT INTO tmp2 values (1);

======================================================================


Re: Possible patch for better index name choosing

From
Robert Haas
Date:
On Sun, Dec 20, 2009 at 10:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Attached is a WIP patch for addressing the problems mentioned in this
> thread:
> http://archives.postgresql.org/pgsql-hackers/2009-12/msg01764.php
>
> The main things that it does are (1) consider all index columns, not
> just the first one as formerly; and (2) try to generate a usable name
> for index expression columns, rather than just ignoring them which was
> the effective behavior formerly.

I'm not really sure there's any point to this.  Anyone who cares about
giving their index an intelligible name should manually assign one.
If they don't bother doing that, I don't really see why we should
worry about it either.  If anything, it seems like we should err on
the side of simplicity, since some users (or even applications) might
attempt to identify or predict automatically generated names.

> A different consideration is whether it's really a good idea to be
> messing with default index names at all.  As illustrated in the attached
> regression diffs, this does impact the error messages returned to
> applications for unique-index failures.  I don't think this is a serious
> problem across a major version update, but maybe someone thinks
> differently.

Maybe I'll reserve final judgement pending further discussion, but my
first reaction is to say it's not worth the risk.  Probably this
shouldn't be an issue for a well-designed application, but the world
is full of badly-written code.  We shouldn't throw up barriers (even
relatively trivial ones) to updating applications unless we get
something out of it, and I'm not convinced that's the case here.

...Robert


Re: Possible patch for better index name choosing

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Dec 20, 2009 at 10:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Attached is a WIP patch for addressing the problems mentioned in this
>> thread:
>> http://archives.postgresql.org/pgsql-hackers/2009-12/msg01764.php

> I'm not really sure there's any point to this.  Anyone who cares about
> giving their index an intelligible name should manually assign one.
> If they don't bother doing that, I don't really see why we should
> worry about it either.

Mainly because we historically *have* put some work into it, and it
would be inconsistent to not pay attention to the point now as we extend
the set of possible index-building constraints further.  In particular
we're going to see a lot of exclusion constraints named foo_exclusionN
if we don't expend any effort on it now.  I also claim that this is
necessary infrastructure if we are going to accept Peter's proposal of
allowing CREATE INDEX without an explicit index name.  That is really
dependent on the assumption that the system will expend more than no
effort on picking useful names.

> Maybe I'll reserve final judgement pending further discussion, but my
> first reaction is to say it's not worth the risk.  Probably this
> shouldn't be an issue for a well-designed application, but the world
> is full of badly-written code.  We shouldn't throw up barriers (even
> relatively trivial ones) to updating applications unless we get
> something out of it, and I'm not convinced that's the case here.

Well, we could tamp down the risks considerably if we undid my point
(1), namely to still consider only the first index column when
generating a name.  I am not really happy with that answer though.
I could turn your first point back on you: if an app is concerned about
the exact names assigned to indexes, why isn't it specifying them?

It's worth noting that pg_dump does preserve index names, so this isn't
going to be an issue in any case for existing apps that dump and reload
their databases.  AFAICS the only case where it would actually create a
compatibility issue is if an existing app creates multi-column UNIQUE
(non-PKEY) constraints on-the-fly, without a constraint name, and
depends on the generated name being the same as before.
        regards, tom lane


Re: Possible patch for better index name choosing

From
Robert Haas
Date:
On Mon, Dec 21, 2009 at 12:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Sun, Dec 20, 2009 at 10:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Attached is a WIP patch for addressing the problems mentioned in this
>>> thread:
>>> http://archives.postgresql.org/pgsql-hackers/2009-12/msg01764.php
>
>> I'm not really sure there's any point to this.  Anyone who cares about
>> giving their index an intelligible name should manually assign one.
>> If they don't bother doing that, I don't really see why we should
>> worry about it either.
>
> Mainly because we historically *have* put some work into it, and it
> would be inconsistent to not pay attention to the point now as we extend
> the set of possible index-building constraints further.  In particular
> we're going to see a lot of exclusion constraints named foo_exclusionN
> if we don't expend any effort on it now.

Maybe that's worth fixing and maybe it isn't.  My first reaction is
"so what"?  In all likelihood, you're going to have to look at the
index definition to see what the thing does anyway.

> I also claim that this is
> necessary infrastructure if we are going to accept Peter's proposal of
> allowing CREATE INDEX without an explicit index name.  That is really
> dependent on the assumption that the system will expend more than no
> effort on picking useful names.

That's a point to consider, though perhaps if they aren't specifying a
name it means they don't care that much.

>> Maybe I'll reserve final judgement pending further discussion, but my
>> first reaction is to say it's not worth the risk.  Probably this
>> shouldn't be an issue for a well-designed application, but the world
>> is full of badly-written code.  We shouldn't throw up barriers (even
>> relatively trivial ones) to updating applications unless we get
>> something out of it, and I'm not convinced that's the case here.
>
> Well, we could tamp down the risks considerably if we undid my point
> (1), namely to still consider only the first index column when
> generating a name.  I am not really happy with that answer though.
> I could turn your first point back on you: if an app is concerned about
> the exact names assigned to indexes, why isn't it specifying them?
>
> It's worth noting that pg_dump does preserve index names, so this isn't
> going to be an issue in any case for existing apps that dump and reload
> their databases.  AFAICS the only case where it would actually create a
> compatibility issue is if an existing app creates multi-column UNIQUE
> (non-PKEY) constraints on-the-fly, without a constraint name, and
> depends on the generated name being the same as before.

Right.  Imagine, for example, a poorly written initialization script
for an app.  Existing instances that are dumped and reloaded will be
OK, but new instances might not come out as expected.

I don't think that what you're proposing here is completely stupid;
I'm just wondering if it's not an ultimately somewhat pointless
activity.  I'm not convinced that it's possible or sensible to try to
stringify all the things that people put in their index definitions,
or that we're going to be able to do it well enough to really add any
value.    Perhaps I should RTFP before sticking my neck out too far,
but... will you serialize EXCLUDE (a =), EXCLUDE (a &&), and EXCLUDE
(a <some other operator>) differently?  And if so, do you expect the
user to be able to reconstruct what the constraint is doing by looking
at the serialized version?  It seems like something reasonably sane
can be done when the definition uses mostly column names and
functions, but operators seem like more of a problem.  I think mostly
people are going to see the constraint name that got violated and then
run \d on the table and look for it.  foo_exclusion3 may not be very
informative, but it's easy to remember for long enough to find it in
the \d output, whereas something long and hairy may not be.

...Robert


Re: Possible patch for better index name choosing

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> Perhaps I should RTFP before sticking my neck out too far,
> but... will you serialize EXCLUDE (a =), EXCLUDE (a &&), and EXCLUDE
> (a <some other operator>) differently?

No, and I'm not proposing to expose ASC/DESC/NULLS FIRST/LAST or
nondefault opclasses (to say nothing of non-btree AMs) or index
predicates either.  The proposed patch is to my mind just a logical
extension of what we have always done --- namely, to pay attention
to index column names --- to some new cases that were never exposed
before.

We could certainly make it pay attention to all that stuff, but I have
the same feeling you do that it wouldn't produce readable results.
And it would make any compatibility issues a lot worse.
        regards, tom lane


Re: Possible patch for better index name choosing

From
Peter Eisentraut
Date:
On mån, 2009-12-21 at 00:03 -0500, Tom Lane wrote:
> Well, we could tamp down the risks considerably if we undid my point
> (1), namely to still consider only the first index column when
> generating a name.

I think putting all the column names into the index names instead of
only the first is a significant improvement that should be kept.  If we
can't do it properly in some cases, we should punt in some obvious way,
not pretend to do the correct thing but actually omit some bits.



Re: Possible patch for better index name choosing

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> On mån, 2009-12-21 at 00:03 -0500, Tom Lane wrote:
>> Well, we could tamp down the risks considerably if we undid my point
>> (1), namely to still consider only the first index column when
>> generating a name.

> I think putting all the column names into the index names instead of
> only the first is a significant improvement that should be kept.

Yeah, I think so too.  It's well worth any risk of application
incompatibility --- we make much bigger changes in every major
release without blinking.
        regards, tom lane


Re: Possible patch for better index name choosing

From
Tom Lane
Date:
I wrote:
> Attached is a WIP patch for addressing the problems mentioned in this
> thread:
> http://archives.postgresql.org/pgsql-hackers/2009-12/msg01764.php
> ...
> There is one thing that is not terribly nice about the behavior, which
> is that CREATE TABLE LIKE INCLUDING INDEXES is unable to generate smart
> names for expression indexes;
> ...
> The reason for this is that the patch depends on FigureColname which
> works on untransformed parse trees, and we don't have access to such
> a tree when copying an existing index.  There seem to be three possible
> responses to that:
> ...
> 3. Implement a separate FigureIndexColname function that works as much
> like FigureColname as it can, but takes a transformed parse tree.

I fooled around with this solution and decided that it is a lot messier
than it's worth.

In the first place, we can't make a FigureColname-like function that
just takes a transformed tree: there is no way to interpret Vars without
some context.  You need at least a table OID, and more than that if
you'd like to handle cases like multiple-relation expressions or
non-RELATION RTEs.  For the case at hand of index expressions, a table
OID would be enough, but that doesn't leave much room for imagining the
function could be used for anything else in future.  Worse, for the
problematic case (CREATE TABLE LIKE) we actually do not have a table OID
because the target table doesn't exist yet.  We could finesse that by
passing the source table's OID instead, but that seems pretty klugy
itself.

In the second place, the number of "corner cases" where we'd generate
output different from FigureColname is much greater than I realized.
As an example, if foo is a type name then foo(x) and x::foo produce
the same parsed tree, but FigureColname will treat them differently.

Seeing that CREATE TABLE LIKE doesn't try to reproduce the source table's
index names anyway, I'm inclined to just go with the patch as-is and not
try to make it handle this one case nicely.
        regards, tom lane