Fix problems when a plain-inheritance parent table is excluded.
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 24 Oct 2023 18:48:28 +0000 (14:48 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 24 Oct 2023 18:48:33 +0000 (14:48 -0400)
When an UPDATE/DELETE/MERGE's target table is an old-style
inheritance tree, it's possible for the parent to get excluded
from the plan while some children are not.  (I believe this is
only possible if we can prove that a CHECK ... NO INHERIT
constraint on the parent contradicts the query WHERE clause,
so it's a very unusual case.)  In such a case, ExecInitModifyTable
mistakenly concluded that the first surviving child is the target
table, leading to at least two bugs:

1. The wrong table's statement-level triggers would get fired.

2. In v16 and up, it was possible to fail with "invalid perminfoindex
0 in RTE with relid nnnn" due to the child RTE not having permissions
data included in the query plan.  This was hard to reproduce reliably
because it did not occur unless the update triggered some non-HOT
index updates.

In v14 and up, this is easy to fix by defining ModifyTable.rootRelation
to be the parent RTE in plain inheritance as well as partitioned cases.

While the wrong-triggers bug also appears in older branches, the
relevant code in both the planner and executor is quite a bit
different, so it would take a good deal of effort to develop and
test a suitable patch.  Given the lack of field complaints about the
trigger issue, I'll desist for now.  (Patching v11 for this seems
unwise anyway, given that it will have no more releases after next
month.)

Per bug #18147 from Hans Buschmann.

Amit Langote and Tom Lane

Discussion: https://postgr.es/m/18147-6fc796538913ee88@postgresql.org

src/backend/executor/nodeModifyTable.c
src/backend/optimizer/plan/planner.c
src/backend/optimizer/util/pathnode.c
src/include/nodes/pathnodes.h
src/include/nodes/plannodes.h
src/test/regress/expected/inherit.out
src/test/regress/sql/inherit.sql

index f62d28ac6084a7e04bf89e6c82f46394891d3099..299c2c75be8f596079bc63618ad59b677036a395 100644 (file)
@@ -3966,10 +3966,10 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
         *   must be converted, and
         * - the root partitioned table used for tuple routing.
         *
-        * If it's a partitioned table, the root partition doesn't appear
-        * elsewhere in the plan and its RT index is given explicitly in
-        * node->rootRelation.  Otherwise (i.e. table inheritance) the target
-        * relation is the first relation in the node->resultRelations list.
+        * If it's a partitioned or inherited table, the root partition or
+        * appendrel RTE doesn't appear elsewhere in the plan and its RT index is
+        * given explicitly in node->rootRelation.  Otherwise, the target relation
+        * is the sole relation in the node->resultRelations list.
         *----------
         */
        if (node->rootRelation > 0)
@@ -3980,6 +3980,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
        }
        else
        {
+               Assert(list_length(node->resultRelations) == 1);
                mtstate->rootResultRelInfo = mtstate->resultRelInfo;
                ExecInitResultRelation(estate, mtstate->resultRelInfo,
                                                           linitial_int(node->resultRelations));
index aafef24cf14ecbdbbbdee18daafed6a816a9d7e1..a8cea5efe1415bdeb231a967cee6d0b36a933218 100644 (file)
@@ -1800,6 +1800,9 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
                                                                                                                   parse->resultRelation);
                                int                     resultRelation = -1;
 
+                               /* Pass the root result rel forward to the executor. */
+                               rootRelation = parse->resultRelation;
+
                                /* Add only leaf children to ModifyTable. */
                                while ((resultRelation = bms_next_member(root->leaf_result_relids,
                                                                                                                 resultRelation)) >= 0)
@@ -1923,6 +1926,7 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
                        else
                        {
                                /* Single-relation INSERT/UPDATE/DELETE/MERGE. */
+                               rootRelation = 0;       /* there's no separate root rel */
                                resultRelations = list_make1_int(parse->resultRelation);
                                if (parse->commandType == CMD_UPDATE)
                                        updateColnosLists = list_make1(root->update_colnos);
@@ -1934,16 +1938,6 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
                                        mergeActionLists = list_make1(parse->mergeActionList);
                        }
 
-                       /*
-                        * If target is a partition root table, we need to mark the
-                        * ModifyTable node appropriately for that.
-                        */
-                       if (rt_fetch(parse->resultRelation, parse->rtable)->relkind ==
-                               RELKIND_PARTITIONED_TABLE)
-                               rootRelation = parse->resultRelation;
-                       else
-                               rootRelation = 0;
-
                        /*
                         * If there was a FOR [KEY] UPDATE/SHARE clause, the LockRows node
                         * will have dealt with fetching non-locked marked rows, else we
index b65323532beaa1202cfde39670a5d873c241960c..bc8757f9d720444afbe99351e64415ada87699cf 100644 (file)
@@ -3663,7 +3663,7 @@ create_lockrows_path(PlannerInfo *root, RelOptInfo *rel,
  * 'operation' is the operation type
  * 'canSetTag' is true if we set the command tag/es_processed
  * 'nominalRelation' is the parent RT index for use of EXPLAIN
- * 'rootRelation' is the partitioned table root RT index, or 0 if none
+ * 'rootRelation' is the partitioned/inherited table root RTI, or 0 if none
  * 'partColsUpdated' is true if any partitioning columns are being updated,
  *             either from the target relation or a descendent partitioned table.
  * 'resultRelations' is an integer list of actual RT indexes of target rel(s)
index 5702fbba60c3acc6771213067ce4dce74a80798c..2027265b315aa5db4bb756083b9ca40a1d17f7fb 100644 (file)
@@ -2344,7 +2344,7 @@ typedef struct ModifyTablePath
        CmdType         operation;              /* INSERT, UPDATE, DELETE, or MERGE */
        bool            canSetTag;              /* do we set the command tag/es_processed? */
        Index           nominalRelation;        /* Parent RT index for use of EXPLAIN */
-       Index           rootRelation;   /* Root RT index, if target is partitioned */
+       Index           rootRelation;   /* Root RT index, if partitioned/inherited */
        bool            partColsUpdated;        /* some part key in hierarchy updated? */
        List       *resultRelations;    /* integer list of RT indexes */
        List       *updateColnosLists;  /* per-target-table update_colnos lists */
index 8cafbf3f8a4394d78e9a38917ad532f22427e44b..d64fe6a328ba32dcd902abe81de760dca73822ed 100644 (file)
@@ -216,11 +216,12 @@ typedef struct ProjectSet
  *             Apply rows produced by outer plan to result table(s),
  *             by inserting, updating, or deleting.
  *
- * If the originally named target table is a partitioned table, both
- * nominalRelation and rootRelation contain the RT index of the partition
- * root, which is not otherwise mentioned in the plan.  Otherwise rootRelation
- * is zero.  However, nominalRelation will always be set, as it's the rel that
- * EXPLAIN should claim is the INSERT/UPDATE/DELETE/MERGE target.
+ * If the originally named target table is a partitioned table or inheritance
+ * tree, both nominalRelation and rootRelation contain the RT index of the
+ * partition root or appendrel RTE, which is not otherwise mentioned in the
+ * plan.  Otherwise rootRelation is zero.  However, nominalRelation will
+ * always be set, as it's the rel that EXPLAIN should claim is the
+ * INSERT/UPDATE/DELETE/MERGE target.
  *
  * Note that rowMarks and epqParam are presumed to be valid for all the
  * table(s); they can't contain any info that varies across tables.
@@ -232,7 +233,7 @@ typedef struct ModifyTable
        CmdType         operation;              /* INSERT, UPDATE, DELETE, or MERGE */
        bool            canSetTag;              /* do we set the command tag/es_processed? */
        Index           nominalRelation;        /* Parent RT index for use of EXPLAIN */
-       Index           rootRelation;   /* Root RT index, if target is partitioned */
+       Index           rootRelation;   /* Root RT index, if partitioned/inherited */
        bool            partColsUpdated;        /* some part key in hierarchy updated? */
        List       *resultRelations;    /* integer list of RT indexes */
        List       *updateColnosLists;  /* per-target-table update_colnos lists */
index 08d93884d8722e1b104a8bc1a6bdec23c0cecb22..0f1aa831f643251bdfb3390df31ed04ec79d369b 100644 (file)
@@ -539,6 +539,33 @@ CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a);
 INSERT INTO z VALUES (NULL, 'text'); -- should fail
 ERROR:  null value in column "aa" of relation "z" violates not-null constraint
 DETAIL:  Failing row contains (null, text).
+-- Check inherited UPDATE with first child excluded
+create table some_tab (f1 int, f2 int, f3 int, check (f1 < 10) no inherit);
+create table some_tab_child () inherits(some_tab);
+insert into some_tab_child select i, i+1, 0 from generate_series(1,1000) i;
+create index on some_tab_child(f1, f2);
+-- while at it, also check that statement-level triggers fire
+create function some_tab_stmt_trig_func() returns trigger as
+$$begin raise notice 'updating some_tab'; return NULL; end;$$
+language plpgsql;
+create trigger some_tab_stmt_trig
+  before update on some_tab execute function some_tab_stmt_trig_func();
+explain (costs off)
+update some_tab set f3 = 11 where f1 = 12 and f2 = 13;
+                                     QUERY PLAN                                     
+------------------------------------------------------------------------------------
+ Update on some_tab
+   Update on some_tab_child some_tab_1
+   ->  Result
+         ->  Index Scan using some_tab_child_f1_f2_idx on some_tab_child some_tab_1
+               Index Cond: ((f1 = 12) AND (f2 = 13))
+(5 rows)
+
+update some_tab set f3 = 11 where f1 = 12 and f2 = 13;
+NOTICE:  updating some_tab
+drop table some_tab cascade;
+NOTICE:  drop cascades to table some_tab_child
+drop function some_tab_stmt_trig_func();
 -- Check inherited UPDATE with all children excluded
 create table some_tab (a int, b int);
 create table some_tab_child () inherits (some_tab);
index 3d57c7ee950ca1b05990e4dee4c7916ff86a9ee9..f7b4062ce51464f1209c845e0d26149e23b107a3 100644 (file)
@@ -97,6 +97,25 @@ SELECT relname, d.* FROM ONLY d, pg_class where d.tableoid = pg_class.oid;
 CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a);
 INSERT INTO z VALUES (NULL, 'text'); -- should fail
 
+-- Check inherited UPDATE with first child excluded
+create table some_tab (f1 int, f2 int, f3 int, check (f1 < 10) no inherit);
+create table some_tab_child () inherits(some_tab);
+insert into some_tab_child select i, i+1, 0 from generate_series(1,1000) i;
+create index on some_tab_child(f1, f2);
+-- while at it, also check that statement-level triggers fire
+create function some_tab_stmt_trig_func() returns trigger as
+$$begin raise notice 'updating some_tab'; return NULL; end;$$
+language plpgsql;
+create trigger some_tab_stmt_trig
+  before update on some_tab execute function some_tab_stmt_trig_func();
+
+explain (costs off)
+update some_tab set f3 = 11 where f1 = 12 and f2 = 13;
+update some_tab set f3 = 11 where f1 = 12 and f2 = 13;
+
+drop table some_tab cascade;
+drop function some_tab_stmt_trig_func();
+
 -- Check inherited UPDATE with all children excluded
 create table some_tab (a int, b int);
 create table some_tab_child () inherits (some_tab);