Re: BUG #18637: CREATE INDEX won't look up operator classes in search_path if PARTITION BY is specified - Mailing list pgsql-bugs

I wrote:
> So that's pretty awful: creating the partitioned index by itself is
> willing to look up the opclass in the current search path, and then
> adding a new partition will play nice with that, but creating a
> partitioned index when there's already a partition will not.
> It's got to be considered a bug that the two paths for making a
> child index behave differently.

I looked into this and found that

* When adding a partition to a pre-existing partitioned index,
we use generateClonedIndexStmt() to construct the IndexStmt
for DefineIndex to use.  This is proof against the current
problem because it will always schema-qualify the opclass
name, cf. get_opclass().

* However, if CREATE INDEX recurses to create a child partition,
it does this:

                    IndexStmt  *childStmt = copyObject(stmt);

followed by some pretty ad-hoc cleanup.  If the original
IndexStmt wasn't written in a search-path-independent fashion
then we've got trouble.

It seems clear to me that the right fix for this is to use
generateClonedIndexStmt in this code path too.  I am not
claiming that generateClonedIndexStmt is entirely free of
related issues, but to the extent it has any we'd have to
fix them anyway.  Building the IndexStmt in two fundamentally
different ways is just asking for trouble.

I hacked this up and was amused to discover that it also
fixes a TODO left over from c01eb619a: if there's a comment
for the parent index, we've been incorrectly applying it
to children too.  generateClonedIndexStmt knows better than
to fill idxcomment for the child IndexStmt, but copyObject
does not.

This could stand an additional regression test case perhaps,
but the core tests don't have a suitable opclass at hand.
Maybe we could test it in some contrib module, but that
seems a shade hacky.

Thoughts?

            regards, tom lane

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 130cebd658..e33ad81529 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -51,6 +51,7 @@
 #include "optimizer/optimizer.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_oper.h"
+#include "parser/parse_utilcmd.h"
 #include "partitioning/partdesc.h"
 #include "pgstat.h"
 #include "rewrite/rewriteManip.h"
@@ -1465,55 +1466,20 @@ DefineIndex(Oid tableId,
                  */
                 if (!found)
                 {
-                    IndexStmt  *childStmt = copyObject(stmt);
-                    bool        found_whole_row;
-                    ListCell   *lc;
+                    IndexStmt  *childStmt;
                     ObjectAddress childAddr;

                     /*
-                     * We can't use the same index name for the child index,
-                     * so clear idxname to let the recursive invocation choose
-                     * a new name.  Likewise, the existing target relation
-                     * field is wrong, and if indexOid or oldNumber are set,
-                     * they mustn't be applied to the child either.
+                     * Build an IndexStmt describing the desired child index
+                     * in the same way that we do during ATTACH PARTITION.
+                     * Notably, we rely on generateClonedIndexStmt to produce
+                     * a search-path-independent representation, which the
+                     * original IndexStmt might not be.
                      */
-                    childStmt->idxname = NULL;
-                    childStmt->relation = NULL;
-                    childStmt->indexOid = InvalidOid;
-                    childStmt->oldNumber = InvalidRelFileNumber;
-                    childStmt->oldCreateSubid = InvalidSubTransactionId;
-                    childStmt->oldFirstRelfilelocatorSubid = InvalidSubTransactionId;
-
-                    /*
-                     * Adjust any Vars (both in expressions and in the index's
-                     * WHERE clause) to match the partition's column numbering
-                     * in case it's different from the parent's.
-                     */
-                    foreach(lc, childStmt->indexParams)
-                    {
-                        IndexElem  *ielem = lfirst(lc);
-
-                        /*
-                         * If the index parameter is an expression, we must
-                         * translate it to contain child Vars.
-                         */
-                        if (ielem->expr)
-                        {
-                            ielem->expr =
-                                map_variable_attnos((Node *) ielem->expr,
-                                                    1, 0, attmap,
-                                                    InvalidOid,
-                                                    &found_whole_row);
-                            if (found_whole_row)
-                                elog(ERROR, "cannot convert whole-row table reference");
-                        }
-                    }
-                    childStmt->whereClause =
-                        map_variable_attnos(stmt->whereClause, 1, 0,
-                                            attmap,
-                                            InvalidOid, &found_whole_row);
-                    if (found_whole_row)
-                        elog(ERROR, "cannot convert whole-row table reference");
+                    childStmt = generateClonedIndexStmt(NULL,
+                                                        parentIndex,
+                                                        attmap,
+                                                        NULL);

                     /*
                      * Recurse as the starting user ID.  Callee will use that
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fb8db37623..143ae7c09c 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2215,7 +2215,6 @@ select conname, obj_description(oid, 'pg_constraint') as desc
 (3 rows)

 alter table at_partitioned alter column name type varchar(127);
--- Note: these tests currently show the wrong behavior for comments :-(
 select relname,
   c.oid = oldoid as orig_oid,
   case relfilenode
@@ -2232,9 +2231,9 @@ select relname,
 ------------------------------+----------+---------+--------------
  at_partitioned               | t        | none    |
  at_partitioned_0             | t        | own     |
- at_partitioned_0_id_name_key | f        | own     | parent index
+ at_partitioned_0_id_name_key | f        | own     |
  at_partitioned_1             | t        | own     |
- at_partitioned_1_id_name_key | f        | own     | parent index
+ at_partitioned_1_id_name_key | f        | own     |
  at_partitioned_id_name_key   | f        | none    | parent index
 (6 rows)

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index cba15ebfec..c5dd43a15c 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1496,8 +1496,6 @@ select conname, obj_description(oid, 'pg_constraint') as desc

 alter table at_partitioned alter column name type varchar(127);

--- Note: these tests currently show the wrong behavior for comments :-(
-
 select relname,
   c.oid = oldoid as orig_oid,
   case relfilenode