Thread: BUG #18637: CREATE INDEX won't look up operator classes in search_path if PARTITION BY is specified

The following bug has been logged on the website:

Bug reference:      18637
Logged by:          CREATE INDEX won't look up operator classes in search_path if PARTITION BY
is specified
Email address:      usamoi@outlook.com
PostgreSQL version: 17.0
Operating system:   Linux 6.10
Description:

This SQL works in PostgreSQL 16.4 but not in PostgreSQL 17.0. Here
`vector_l2_ops` is an operator class in schema `vectors`.
```
SET search_path TO public, vectors;
CREATE TABLE items (val vector(3), category_id int) PARTITION BY
LIST(category_id);
CREATE TABLE id_123 PARTITION OF items FOR VALUES IN (1, 2, 3);
CREATE TABLE id_456 PARTITION OF items FOR VALUES IN (4, 5, 6);
CREATE TABLE id_789 PARTITION OF items FOR VALUES IN (7, 8, 9);
CREATE INDEX ON items USING vectors (val vector_l2_ops);
```
The error message is: `ERROR:  operator class "vector_l2_ops" does not exist
for access method "vectors"`

If I do not use PARTITION BY and CREATE TABLE OF, it works and the message
is `CREATE INDEX`.

After debugging, I find that `DefineIndex` would call `RestrictSearchPath`
in `indexcmds.c:1234` and `indexcmds.c:1334` before entering recursion, so
that the nested `DefineIndex` would be only able to look up operator classes
in `pg_catalog, pg_temp`.

Since the behavior between `CREATE INDEX` and `CREATE INDEX PARTITION BY` is
different, it should be a bug.

On Sat, 28 Sept 2024 at 02:59, PG Bug reporting form
<noreply@postgresql.org> wrote:
> After debugging, I find that `DefineIndex` would call `RestrictSearchPath`
> in `indexcmds.c:1234` and `indexcmds.c:1334` before entering recursion, so
> that the nested `DefineIndex` would be only able to look up operator classes
> in `pg_catalog, pg_temp`.
>
> Since the behavior between `CREATE INDEX` and `CREATE INDEX PARTITION BY` is
> different, it should be a bug.

There's an item in the release notes [1] which should be expended to
mention this as an incompatibility, namely:

"Change functions to use a safe search_path during maintenance
operations (Jeff Davis) §

This prevents maintenance operations (ANALYZE, CLUSTER, REFRESH
MATERIALIZED VIEW, REINDEX, or VACUUM) from performing unsafe access.
Functions used by expression indexes and materialized views that need
to reference non-default schemas must specify a search path during
function creation."

That doesn't quite mention CREATE INDEX. There's a discussion about
fixing that by adding CREATE INDEX to the list of commands. See [2].

I think you can now safely assume that you'll need to provide the
schema name for the opclass you've specified.

Also please see the CREATE INDEX documentation [3] where it says:

"While CREATE INDEX is running, the search_path is temporarily changed
to pg_catalog, pg_temp."

David

[1] https://www.postgresql.org/docs/release/17.0/
[2] https://www.postgresql.org/message-id/20240926141921.57d0b430fa53ac4389344847%40sraoss.co.jp
[3] https://www.postgresql.org/docs/17/sql-createindex.html

David Rowley <dgrowleyml@gmail.com> writes:
> There's an item in the release notes [1] which should be expended to
> mention this as an incompatibility, namely:

> "Change functions to use a safe search_path during maintenance
> operations (Jeff Davis) §

I have not dug into the code, but I think this may actually be a
bug; not because of the change of search path, but because it looks
like the opclass is probably being looked up more than once during
the command, with different search paths.  That has security
implications, and not good ones.  We should fix this so that the
opclass is looked up exactly once, and passed down to the partitions
by OID not name.

            regards, tom lane

I wrote:
> I have not dug into the code, but I think this may actually be a
> bug; not because of the change of search path, but because it looks
> like the opclass is probably being looked up more than once during
> the command, with different search paths.  That has security
> implications, and not good ones.  We should fix this so that the
> opclass is looked up exactly once, and passed down to the partitions
> by OID not name.

Here's an example that doesn't rely on any outside extension:

regression=# create extension cube;
CREATE EXTENSION
regression=# create table items(category_id int, val cube) partition by list(category_id);
CREATE TABLE
regression=# CREATE INDEX ON items (val cube_ops);
CREATE INDEX
regression=# CREATE TABLE id_123 PARTITION OF items FOR VALUES IN (1, 2);
CREATE TABLE
regression=# CREATE INDEX items_idx_2 ON items (val cube_ops);
ERROR:  operator class "cube_ops" does not exist for access method "btree"

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.

As far as I understand, the change of search path is only supposed to
affect user-defined code within the expressions of an expression index.
Operands of the command itself should consistently be interpreted in
the current search path.

            regards, tom lane

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

On Thu, Oct 03, 2024 at 02:17:15PM -0400, Tom Lane wrote:
> 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?

For contrib/citext/sql/create_index_acl.sql, I settled on testing in contrib.
I valued the test fidelity of a real opclass more than I disliked the use of
contrib.  The distinction between the various parts of check-world is blurry
to non-hackers, so I expect it won't bother them.

Noah Misch <noah@leadboat.com> writes:
> On Thu, Oct 03, 2024 at 02:17:15PM -0400, Tom Lane wrote:
>> 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.

> For contrib/citext/sql/create_index_acl.sql, I settled on testing in contrib.
> I valued the test fidelity of a real opclass more than I disliked the use of
> contrib.  The distinction between the various parts of check-world is blurry
> to non-hackers, so I expect it won't bother them.

Agreed.  Pushed it with a test added to contrib/seg, just to be
different.

            regards, tom lane