Thread: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Amit Langote
Date:
Per Alvaro's advice, forking this from [1].

In that thread, Tom had asked if it wouldn't be better to find a new
place to put extraUpdatedCols [2] instead of RangeTblEntry, along with
the permission-checking fields are now no longer stored in
RangeTblEntry.

In [3] of the same thread, I proposed to move it into a List of
Bitmapsets in ModifyTable, as implemented in the attached patch that I
had been posting to that thread.

The latest version of that patch is attached herewith.  I'll add this
one to the January CF too.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/CA+HiwqGjJDmUhDSfv-U2qhKJjt9ST7Xh9JXC_irsAQ1TAUsJYg@mail.gmail.com
[2] https://www.postgresql.org/message-id/3098829.1658956718%40sss.pgh.pa.us
[3] https://www.postgresql.org/message-id/CA%2BHiwqEHoLgN%3DvSsaNMaHP-%2BqYPT40-ooySyrieXZHNzbSBj0w%40mail.gmail.com

Attachment

Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Amit Langote
Date:
On Wed, Dec 7, 2022 at 8:54 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Per Alvaro's advice, forking this from [1].
>
> In that thread, Tom had asked if it wouldn't be better to find a new
> place to put extraUpdatedCols [2] instead of RangeTblEntry, along with
> the permission-checking fields are now no longer stored in
> RangeTblEntry.
>
> In [3] of the same thread, I proposed to move it into a List of
> Bitmapsets in ModifyTable, as implemented in the attached patch that I
> had been posting to that thread.
>
> The latest version of that patch is attached herewith.  I'll add this
> one to the January CF too.

Done.

https://commitfest.postgresql.org/41/4049/

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Amit Langote
Date:
On Wed, Dec 7, 2022 at 8:54 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Per Alvaro's advice, forking this from [1].
>
> In that thread, Tom had asked if it wouldn't be better to find a new
> place to put extraUpdatedCols [2] instead of RangeTblEntry, along with
> the permission-checking fields are now no longer stored in
> RangeTblEntry.
>
> In [3] of the same thread, I proposed to move it into a List of
> Bitmapsets in ModifyTable, as implemented in the attached patch that I
> had been posting to that thread.
>
> The latest version of that patch is attached herewith.

Updated to replace a list_nth() with list_nth_node() and rewrote the
commit message.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Attachment

Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
vignesh C
Date:
On Thu, 8 Dec 2022 at 08:17, Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Wed, Dec 7, 2022 at 8:54 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > Per Alvaro's advice, forking this from [1].
> >
> > In that thread, Tom had asked if it wouldn't be better to find a new
> > place to put extraUpdatedCols [2] instead of RangeTblEntry, along with
> > the permission-checking fields are now no longer stored in
> > RangeTblEntry.
> >
> > In [3] of the same thread, I proposed to move it into a List of
> > Bitmapsets in ModifyTable, as implemented in the attached patch that I
> > had been posting to that thread.
> >
> > The latest version of that patch is attached herewith.
>
> Updated to replace a list_nth() with list_nth_node() and rewrote the
> commit message.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch
./v2-0001-Add-per-result-relation-extraUpdatedCols-to-Modif.patch
...
patching file src/backend/optimizer/util/inherit.c
Hunk #2 FAILED at 50.
Hunk #9 succeeded at 926 with fuzz 2 (offset -1 lines).
1 out of 9 hunks FAILED -- saving rejects to file
src/backend/optimizer/util/inherit.c.rej

[1] - http://cfbot.cputube.org/patch_41_4049.log

Regards,
Vignesh



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Tom Lane
Date:
Amit Langote <amitlangote09@gmail.com> writes:
> Updated to replace a list_nth() with list_nth_node() and rewrote the
> commit message.

So I was working through this with intent to commit, when I realized
that the existing code it's revising is flat out broken.  You can't
simply translate a parent rel's set of dependent generated columns
to obtain the correct set for a child.  Maybe that's sufficient for
partitioned tables, but it fails miserably for general inheritance:

regression=# create table pp(f1 int);
CREATE TABLE
regression=# create table cc(f2 int generated always as (f1+1) stored) inherits(pp);
CREATE TABLE
regression=# insert into cc values(42);
INSERT 0 1
regression=# table cc;
 f1 | f2
----+----
 42 | 43
(1 row)

regression=# update pp set f1 = f1*10;
UPDATE 1
regression=# table cc;
 f1  | f2
-----+----
 420 | 43
(1 row)

So we have a long-standing existing bug to fix here.

I think what we have to do basically is repeat what fill_extraUpdatedCols
does independently for each target table.  That's not really horrible:
given the premise that we're moving this calculation into the planner,
we can have expand_single_inheritance_child run the code while we have
each target table open.  It'll require some rethinking though, and we
will need to have the set of update target columns already available
at that point.  This suggests that we want to put the updated_cols and
extraUpdatedCols fields into RelOptInfo not PlannerInfo.

            regards, tom lane



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Tom Lane
Date:
I wrote:
> I think what we have to do basically is repeat what fill_extraUpdatedCols
> does independently for each target table.  That's not really horrible:
> given the premise that we're moving this calculation into the planner,
> we can have expand_single_inheritance_child run the code while we have
> each target table open.  It'll require some rethinking though, and we
> will need to have the set of update target columns already available
> at that point.  This suggests that we want to put the updated_cols and
> extraUpdatedCols fields into RelOptInfo not PlannerInfo.

After further thought: maybe we should get radical and postpone this
work all the way to executor startup.  The downside of that is having
to do it over again on each execution of a prepared plan.  But the
upside is that when the UPDATE targets a many-partitioned table,
we would have a chance at not doing the work at all for partitions
that get pruned at runtime.  I'm not sure if that win would emerge
immediately or if we still have executor work to do to manage pruning
of the target table.  I'm also not sure that this'd be a net win
overall.  But it seems worth considering.

            regards, tom lane



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Tom Lane
Date:
I wrote:
> After further thought: maybe we should get radical and postpone this
> work all the way to executor startup.  The downside of that is having
> to do it over again on each execution of a prepared plan.  But the
> upside is that when the UPDATE targets a many-partitioned table,
> we would have a chance at not doing the work at all for partitions
> that get pruned at runtime.  I'm not sure if that win would emerge
> immediately or if we still have executor work to do to manage pruning
> of the target table.  I'm also not sure that this'd be a net win
> overall.  But it seems worth considering.

Here's a draft patch that does it like that.  This seems like a win
for more reasons than just pruning, because I was able to integrate
the calculation into runtime setup of the expressions, so that we
aren't doing an extra stringToNode() on them.

There's still a code path that does such a calculation at plan time
(get_rel_all_updated_cols), but it's only used by postgres_fdw which
has some other rather-inefficient behaviors in the same area.

I've not looked into what it'd take to back-patch this.  We can't
add a field to ResultRelInfo in released branches (cf 4b3e37993),
but we might be able to repurpose RangeTblEntry.extraUpdatedCols.

            regards, tom lane

diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index 8bcf4784eb..8c82c71675 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -1342,25 +1342,16 @@ ExecGetUpdatedCols(ResultRelInfo *relinfo, EState *estate)
 Bitmapset *
 ExecGetExtraUpdatedCols(ResultRelInfo *relinfo, EState *estate)
 {
-    if (relinfo->ri_RangeTableIndex != 0)
-    {
-        RangeTblEntry *rte = exec_rt_fetch(relinfo->ri_RangeTableIndex, estate);
+#ifdef USE_ASSERT_CHECKING
+    /* Verify that ExecInitStoredGenerated has been called if needed. */
+    Relation    rel = relinfo->ri_RelationDesc;
+    TupleDesc    tupdesc = RelationGetDescr(rel);

-        return rte->extraUpdatedCols;
-    }
-    else if (relinfo->ri_RootResultRelInfo)
-    {
-        ResultRelInfo *rootRelInfo = relinfo->ri_RootResultRelInfo;
-        RangeTblEntry *rte = exec_rt_fetch(rootRelInfo->ri_RangeTableIndex, estate);
-        TupleConversionMap *map = ExecGetRootToChildMap(relinfo, estate);
+    if (tupdesc->constr && tupdesc->constr->has_generated_stored)
+        Assert(relinfo->ri_GeneratedExprs != NULL);
+#endif

-        if (map != NULL)
-            return execute_attr_map_cols(map->attrMap, rte->extraUpdatedCols);
-        else
-            return rte->extraUpdatedCols;
-    }
-    else
-        return NULL;
+    return relinfo->ri_extraUpdatedCols;
 }

 /* Return columns being updated, including generated columns */
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 56398c399c..687a5422ea 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -54,6 +54,7 @@
 #include "foreign/fdwapi.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
+#include "optimizer/optimizer.h"
 #include "rewrite/rewriteHandler.h"
 #include "storage/bufmgr.h"
 #include "storage/lmgr.h"
@@ -352,6 +353,93 @@ ExecCheckTIDVisible(EState *estate,
     ExecClearTuple(tempSlot);
 }

+/*
+ * Initialize to compute stored generated columns for a tuple
+ *
+ * This fills the resultRelInfo's ri_GeneratedExprs and ri_extraUpdatedCols
+ * fields.  (Currently, ri_extraUpdatedCols is consulted only in UPDATE,
+ * but we might as well fill it for INSERT too.)
+ */
+static void
+ExecInitStoredGenerated(ResultRelInfo *resultRelInfo,
+                        EState *estate,
+                        CmdType cmdtype)
+{
+    Relation    rel = resultRelInfo->ri_RelationDesc;
+    TupleDesc    tupdesc = RelationGetDescr(rel);
+    int            natts = tupdesc->natts;
+    Bitmapset  *updatedCols;
+    MemoryContext oldContext;
+
+    /* Don't call twice */
+    Assert(resultRelInfo->ri_GeneratedExprs == NULL);
+
+    /* Nothing to do if no generated columns */
+    if (!(tupdesc->constr && tupdesc->constr->has_generated_stored))
+        return;
+
+    /*
+     * In an UPDATE, we can skip computing any generated columns that do not
+     * depend on any UPDATE target column.  But if there is a BEFORE ROW
+     * UPDATE trigger, we cannot skip because the trigger might change more
+     * columns.
+     */
+    if (cmdtype == CMD_UPDATE &&
+        !(rel->trigdesc && rel->trigdesc->trig_update_before_row))
+        updatedCols = ExecGetUpdatedCols(resultRelInfo, estate);
+    else
+        updatedCols = NULL;
+
+    /*
+     * Make sure these data structures are built in the per-query memory
+     * context so they'll survive throughout the query.
+     */
+    oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
+
+    resultRelInfo->ri_GeneratedExprs =
+        (ExprState **) palloc0(natts * sizeof(ExprState *));
+    resultRelInfo->ri_NumGeneratedNeeded = 0;
+
+    for (int i = 0; i < natts; i++)
+    {
+        if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED)
+        {
+            Expr       *expr;
+
+            /* Fetch the GENERATED AS expression tree */
+            expr = (Expr *) build_column_default(rel, i + 1);
+            if (expr == NULL)
+                elog(ERROR, "no generation expression found for column number %d of table \"%s\"",
+                     i + 1, RelationGetRelationName(rel));
+
+            /*
+             * If it's an update with a known set of update target columns,
+             * see if we can skip the computation.
+             */
+            if (updatedCols)
+            {
+                Bitmapset  *attrs_used = NULL;
+
+                pull_varattnos((Node *) expr, 1, &attrs_used);
+
+                if (!bms_overlap(updatedCols, attrs_used))
+                    continue;    /* need not update this column */
+            }
+
+            /* No luck, so prepare the expression for execution */
+            resultRelInfo->ri_GeneratedExprs[i] = ExecPrepareExpr(expr, estate);
+            resultRelInfo->ri_NumGeneratedNeeded++;
+
+            /* And mark this column in resultRelInfo->ri_extraUpdatedCols */
+            resultRelInfo->ri_extraUpdatedCols =
+                bms_add_member(resultRelInfo->ri_extraUpdatedCols,
+                               i + 1 - FirstLowInvalidHeapAttributeNumber);
+        }
+    }
+
+    MemoryContextSwitchTo(oldContext);
+}
+
 /*
  * Compute stored generated columns for a tuple
  */
@@ -363,58 +451,22 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
     Relation    rel = resultRelInfo->ri_RelationDesc;
     TupleDesc    tupdesc = RelationGetDescr(rel);
     int            natts = tupdesc->natts;
+    ExprContext *econtext = GetPerTupleExprContext(estate);
     MemoryContext oldContext;
     Datum       *values;
     bool       *nulls;

+    /* We should not be called unless this is true */
     Assert(tupdesc->constr && tupdesc->constr->has_generated_stored);

     /*
-     * If first time through for this result relation, build expression
-     * nodetrees for rel's stored generation expressions.  Keep them in the
-     * per-query memory context so they'll survive throughout the query.
+     * For relations named directly in the query, ExecInitStoredGenerated
+     * should have been called already; but this might not have happened yet
+     * for a partition child rel.  Also, it's convenient for outside callers
+     * to not have to call ExecInitStoredGenerated explicitly.
      */
     if (resultRelInfo->ri_GeneratedExprs == NULL)
-    {
-        oldContext = MemoryContextSwitchTo(estate->es_query_cxt);
-
-        resultRelInfo->ri_GeneratedExprs =
-            (ExprState **) palloc(natts * sizeof(ExprState *));
-        resultRelInfo->ri_NumGeneratedNeeded = 0;
-
-        for (int i = 0; i < natts; i++)
-        {
-            if (TupleDescAttr(tupdesc, i)->attgenerated == ATTRIBUTE_GENERATED_STORED)
-            {
-                Expr       *expr;
-
-                /*
-                 * If it's an update and the current column was not marked as
-                 * being updated, then we can skip the computation.  But if
-                 * there is a BEFORE ROW UPDATE trigger, we cannot skip
-                 * because the trigger might affect additional columns.
-                 */
-                if (cmdtype == CMD_UPDATE &&
-                    !(rel->trigdesc && rel->trigdesc->trig_update_before_row) &&
-                    !bms_is_member(i + 1 - FirstLowInvalidHeapAttributeNumber,
-                                   ExecGetExtraUpdatedCols(resultRelInfo, estate)))
-                {
-                    resultRelInfo->ri_GeneratedExprs[i] = NULL;
-                    continue;
-                }
-
-                expr = (Expr *) build_column_default(rel, i + 1);
-                if (expr == NULL)
-                    elog(ERROR, "no generation expression found for column number %d of table \"%s\"",
-                         i + 1, RelationGetRelationName(rel));
-
-                resultRelInfo->ri_GeneratedExprs[i] = ExecPrepareExpr(expr, estate);
-                resultRelInfo->ri_NumGeneratedNeeded++;
-            }
-        }
-
-        MemoryContextSwitchTo(oldContext);
-    }
+        ExecInitStoredGenerated(resultRelInfo, estate, cmdtype);

     /*
      * If no generated columns have been affected by this change, then skip
@@ -435,14 +487,13 @@ ExecComputeStoredGenerated(ResultRelInfo *resultRelInfo,
     {
         Form_pg_attribute attr = TupleDescAttr(tupdesc, i);

-        if (attr->attgenerated == ATTRIBUTE_GENERATED_STORED &&
-            resultRelInfo->ri_GeneratedExprs[i])
+        if (resultRelInfo->ri_GeneratedExprs[i])
         {
-            ExprContext *econtext;
             Datum        val;
             bool        isnull;

-            econtext = GetPerTupleExprContext(estate);
+            Assert(attr->attgenerated == ATTRIBUTE_GENERATED_STORED);
+
             econtext->ecxt_scantuple = slot;

             val = ExecEvalExpr(resultRelInfo->ri_GeneratedExprs[i], econtext, &isnull);
@@ -4088,6 +4139,15 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
                     elog(ERROR, "could not find junk wholerow column");
             }
         }
+
+        /*
+         * For INSERT and UPDATE, prepare to evaluate any generated columns.
+         * We must do this now, even if we never insert or update any rows,
+         * because we have to fill resultRelInfo->ri_extraUpdatedCols for
+         * possible use by the trigger machinery.
+         */
+        if (operation == CMD_INSERT || operation == CMD_UPDATE)
+            ExecInitStoredGenerated(resultRelInfo, estate, operation);
     }

     /*
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 0cb235bfda..69324d5a9a 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -561,7 +561,6 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
     WRITE_BOOL_FIELD(lateral);
     WRITE_BOOL_FIELD(inh);
     WRITE_BOOL_FIELD(inFromCl);
-    WRITE_BITMAPSET_FIELD(extraUpdatedCols);
     WRITE_NODE_FIELD(securityQuals);
 }

diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index c284b96a54..30cd7a0da6 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -537,7 +537,6 @@ _readRangeTblEntry(void)
     READ_BOOL_FIELD(lateral);
     READ_BOOL_FIELD(inh);
     READ_BOOL_FIELD(inFromCl);
-    READ_BITMAPSET_FIELD(extraUpdatedCols);
     READ_NODE_FIELD(securityQuals);

     READ_DONE();
diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c
index 6c93c22477..07a8818d6c 100644
--- a/src/backend/optimizer/util/inherit.c
+++ b/src/backend/optimizer/util/inherit.c
@@ -25,6 +25,7 @@
 #include "optimizer/inherit.h"
 #include "optimizer/optimizer.h"
 #include "optimizer/pathnode.h"
+#include "optimizer/plancat.h"
 #include "optimizer/planmain.h"
 #include "optimizer/planner.h"
 #include "optimizer/prep.h"
@@ -345,11 +346,6 @@ expand_partitioned_rtentry(PlannerInfo *root, RelOptInfo *relinfo,
         root->partColsUpdated =
             has_partition_attrs(parentrel, parent_updatedCols, NULL);

-    /*
-     * There shouldn't be any generated columns in the partition key.
-     */
-    Assert(!has_partition_attrs(parentrel, parentrte->extraUpdatedCols, NULL));
-
     /* Nothing further to do here if there are no partitions. */
     if (partdesc->nparts == 0)
         return;
@@ -566,13 +562,6 @@ expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte,
     childrte->alias = childrte->eref = makeAlias(parentrte->eref->aliasname,
                                                  child_colnames);

-    /* Translate the bitmapset of generated columns being updated. */
-    if (childOID != parentOID)
-        childrte->extraUpdatedCols = translate_col_privs(parentrte->extraUpdatedCols,
-                                                         appinfo->translated_vars);
-    else
-        childrte->extraUpdatedCols = bms_copy(parentrte->extraUpdatedCols);
-
     /*
      * Store the RTE and appinfo in the respective PlannerInfo arrays, which
      * the caller must already have allocated space for.
@@ -672,21 +661,16 @@ get_rel_all_updated_cols(PlannerInfo *root, RelOptInfo *rel)
     Assert(IS_SIMPLE_REL(rel));

     /*
-     * We obtain updatedCols and extraUpdatedCols for the query's result
-     * relation.  Then, if necessary, we map it to the column numbers of the
-     * relation for which they were requested.
+     * We obtain updatedCols for the query's result relation.  Then, if
+     * necessary, we map it to the column numbers of the relation for which
+     * they were requested.
      */
     relid = root->parse->resultRelation;
     rte = planner_rt_fetch(relid, root);
     perminfo = getRTEPermissionInfo(root->parse->rteperminfos, rte);

     updatedCols = perminfo->updatedCols;
-    extraUpdatedCols = rte->extraUpdatedCols;

-    /*
-     * For "other" rels, we must look up the root parent relation mentioned in
-     * the query, and translate the column numbers.
-     */
     if (rel->relid != relid)
     {
         RelOptInfo *top_parent_rel = find_base_rel(root, relid);
@@ -695,10 +679,15 @@ get_rel_all_updated_cols(PlannerInfo *root, RelOptInfo *rel)

         updatedCols = translate_col_privs_multilevel(root, rel, top_parent_rel,
                                                      updatedCols);
-        extraUpdatedCols = translate_col_privs_multilevel(root, rel, top_parent_rel,
-                                                          extraUpdatedCols);
     }

+    /*
+     * Now we must check to see if there are any generated columns that depend
+     * on the updatedCols, and add them to the result.
+     */
+    extraUpdatedCols = get_dependent_generated_columns(root, rel->relid,
+                                                       updatedCols);
+
     return bms_union(updatedCols, extraUpdatedCols);
 }

diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index c24bdae717..9f158f2421 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -2199,6 +2199,11 @@ has_row_triggers(PlannerInfo *root, Index rti, CmdType event)
     return result;
 }

+/*
+ * has_stored_generated_columns
+ *
+ * Does table identified by RTI have any STORED GENERATED columns?
+ */
 bool
 has_stored_generated_columns(PlannerInfo *root, Index rti)
 {
@@ -2218,6 +2223,57 @@ has_stored_generated_columns(PlannerInfo *root, Index rti)
     return result;
 }

+/*
+ * get_dependent_generated_columns
+ *
+ * Get the column numbers of any STORED GENERATED columns of the relation
+ * that depend on any column listed in target_cols.  Both the input and
+ * result bitmapsets contain column numbers offset by
+ * FirstLowInvalidHeapAttributeNumber.
+ */
+Bitmapset *
+get_dependent_generated_columns(PlannerInfo *root, Index rti,
+                                Bitmapset *target_cols)
+{
+    Bitmapset  *dependentCols = NULL;
+    RangeTblEntry *rte = planner_rt_fetch(rti, root);
+    Relation    relation;
+    TupleDesc    tupdesc;
+    TupleConstr *constr;
+
+    /* Assume we already have adequate lock */
+    relation = table_open(rte->relid, NoLock);
+
+    tupdesc = RelationGetDescr(relation);
+    constr = tupdesc->constr;
+
+    if (constr && constr->has_generated_stored)
+    {
+        for (int i = 0; i < constr->num_defval; i++)
+        {
+            AttrDefault *defval = &constr->defval[i];
+            Node       *expr;
+            Bitmapset  *attrs_used = NULL;
+
+            /* skip if not generated column */
+            if (!TupleDescAttr(tupdesc, defval->adnum - 1)->attgenerated)
+                continue;
+
+            /* identify columns this generated column depends on */
+            expr = stringToNode(defval->adbin);
+            pull_varattnos(expr, 1, &attrs_used);
+
+            if (bms_overlap(target_cols, attrs_used))
+                dependentCols = bms_add_member(dependentCols,
+                                               defval->adnum - FirstLowInvalidHeapAttributeNumber);
+        }
+    }
+
+    table_close(relation, NoLock);
+
+    return dependentCols;
+}
+
 /*
  * set_relation_partition_info
  *
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 446f84fa97..f8e8cf71eb 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -1815,7 +1815,6 @@ apply_handle_update(StringInfo s)
     LogicalRepTupleData newtup;
     bool        has_oldtup;
     TupleTableSlot *remoteslot;
-    RangeTblEntry *target_rte;
     RTEPermissionInfo *target_perminfo;
     MemoryContext oldctx;

@@ -1864,7 +1863,6 @@ apply_handle_update(StringInfo s)
      * information.  But it would for example exclude columns that only exist
      * on the subscriber, since we are not touching those.
      */
-    target_rte = list_nth(estate->es_range_table, 0);
     target_perminfo = list_nth(estate->es_rteperminfos, 0);
     for (int i = 0; i < remoteslot->tts_tupleDescriptor->natts; i++)
     {
@@ -1881,9 +1879,6 @@ apply_handle_update(StringInfo s)
         }
     }

-    /* Also populate extraUpdatedCols, in case we have generated columns */
-    fill_extraUpdatedCols(target_rte, target_perminfo, rel->localrel);
-
     /* Build the search tuple. */
     oldctx = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
     slot_store_data(remoteslot, rel,
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index ca8b543bc1..1960dad701 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1633,46 +1633,6 @@ rewriteValuesRTEToNulls(Query *parsetree, RangeTblEntry *rte)
 }


-/*
- * Record in target_rte->extraUpdatedCols the indexes of any generated columns
- * columns that depend on any columns mentioned in
- * target_perminfo->updatedCols.
- */
-void
-fill_extraUpdatedCols(RangeTblEntry *target_rte,
-                      RTEPermissionInfo *target_perminfo,
-                      Relation target_relation)
-{
-    TupleDesc    tupdesc = RelationGetDescr(target_relation);
-    TupleConstr *constr = tupdesc->constr;
-
-    target_rte->extraUpdatedCols = NULL;
-
-    if (constr && constr->has_generated_stored)
-    {
-        for (int i = 0; i < constr->num_defval; i++)
-        {
-            AttrDefault *defval = &constr->defval[i];
-            Node       *expr;
-            Bitmapset  *attrs_used = NULL;
-
-            /* skip if not generated column */
-            if (!TupleDescAttr(tupdesc, defval->adnum - 1)->attgenerated)
-                continue;
-
-            /* identify columns this generated column depends on */
-            expr = stringToNode(defval->adbin);
-            pull_varattnos(expr, 1, &attrs_used);
-
-            if (bms_overlap(target_perminfo->updatedCols, attrs_used))
-                target_rte->extraUpdatedCols =
-                    bms_add_member(target_rte->extraUpdatedCols,
-                                   defval->adnum - FirstLowInvalidHeapAttributeNumber);
-        }
-    }
-}
-
-
 /*
  * matchLocks -
  *      match the list of locks and returns the matching rules
@@ -3738,7 +3698,6 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
     {
         int            result_relation;
         RangeTblEntry *rt_entry;
-        RTEPermissionInfo *rt_perminfo;
         Relation    rt_entry_relation;
         List       *locks;
         int            product_orig_rt_length;
@@ -3751,7 +3710,6 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
         Assert(result_relation != 0);
         rt_entry = rt_fetch(result_relation, parsetree->rtable);
         Assert(rt_entry->rtekind == RTE_RELATION);
-        rt_perminfo = getRTEPermissionInfo(parsetree->rteperminfos, rt_entry);

         /*
          * We can use NoLock here since either the parser or
@@ -3843,9 +3801,6 @@ RewriteQuery(Query *parsetree, List *rewrite_events, int orig_rt_length)
                                     parsetree->override,
                                     rt_entry_relation,
                                     NULL, 0, NULL);
-
-            /* Also populate extraUpdatedCols (for generated columns) */
-            fill_extraUpdatedCols(rt_entry, rt_perminfo, rt_entry_relation);
         }
         else if (event == CMD_MERGE)
         {
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 2cd0a4f472..2e7c30b63e 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -462,6 +462,9 @@ typedef struct ResultRelInfo
      */
     AttrNumber    ri_RowIdAttNo;

+    /* If UPDATE, attribute numbers of generated columns to be updated */
+    Bitmapset  *ri_extraUpdatedCols;
+
     /* Projection to generate new tuple in an INSERT/UPDATE */
     ProjectionInfo *ri_projectNew;
     /* Slot to hold that tuple */
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 6300945734..cfeca96d53 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1153,7 +1153,6 @@ typedef struct RangeTblEntry
     bool        lateral;        /* subquery, function, or values is LATERAL? */
     bool        inh;            /* inheritance requested? */
     bool        inFromCl;        /* present in FROM clause? */
-    Bitmapset  *extraUpdatedCols;    /* generated columns being updated */
     List       *securityQuals;    /* security barrier quals to apply, if any */
 } RangeTblEntry;

@@ -1189,15 +1188,6 @@ typedef struct RangeTblEntry
  * updatedCols is also used in some other places, for example, to determine
  * which triggers to fire and in FDWs to know which changed columns they need
  * to ship off.
- *
- * Generated columns that are caused to be updated by an update to a base
- * column are listed in extraUpdatedCols.  This is not considered for
- * permission checking, but it is useful in those places that want to know the
- * full set of columns being updated as opposed to only the ones the user
- * explicitly mentioned in the query.  (There is currently no need for an
- * extraInsertedCols, but it could exist.)  Note that extraUpdatedCols is
- * populated during query rewrite, NOT in the parser, since generated columns
- * could be added after a rule has been parsed and stored.
  */
 typedef struct RTEPermissionInfo
 {
diff --git a/src/include/optimizer/plancat.h b/src/include/optimizer/plancat.h
index 21edac04ea..eb1c3ccc4b 100644
--- a/src/include/optimizer/plancat.h
+++ b/src/include/optimizer/plancat.h
@@ -74,4 +74,7 @@ extern bool has_row_triggers(PlannerInfo *root, Index rti, CmdType event);

 extern bool has_stored_generated_columns(PlannerInfo *root, Index rti);

+extern Bitmapset *get_dependent_generated_columns(PlannerInfo *root, Index rti,
+                                                  Bitmapset *target_cols);
+
 #endif                            /* PLANCAT_H */
diff --git a/src/include/rewrite/rewriteHandler.h b/src/include/rewrite/rewriteHandler.h
index 62bca7cfdf..b71e20b087 100644
--- a/src/include/rewrite/rewriteHandler.h
+++ b/src/include/rewrite/rewriteHandler.h
@@ -24,10 +24,6 @@ extern void AcquireRewriteLocks(Query *parsetree,

 extern Node *build_column_default(Relation rel, int attrno);

-extern void fill_extraUpdatedCols(RangeTblEntry *target_rte,
-                                  RTEPermissionInfo *target_perminfo,
-                                  Relation target_relation);
-
 extern Query *get_view_query(Relation view);
 extern const char *view_query_is_auto_updatable(Query *viewquery,
                                                 bool check_cols);
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index bb4190340e..1db5f9ed47 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -337,6 +337,25 @@ CREATE TABLE gtest1_2 () INHERITS (gtest0, gtesty);  -- error
 NOTICE:  merging multiple inherited definitions of column "b"
 ERROR:  inherited column "b" has a generation conflict
 DROP TABLE gtesty;
+-- test correct handling of GENERATED column that's only in child
+CREATE TABLE gtestp (f1 int);
+CREATE TABLE gtestc (f2 int GENERATED ALWAYS AS (f1+1) STORED) INHERITS(gtestp);
+INSERT INTO gtestc values(42);
+TABLE gtestc;
+ f1 | f2
+----+----
+ 42 | 43
+(1 row)
+
+UPDATE gtestp SET f1 = f1 * 10;
+TABLE gtestc;
+ f1  | f2
+-----+-----
+ 420 | 421
+(1 row)
+
+DROP TABLE gtestp CASCADE;
+NOTICE:  drop cascades to table gtestc
 -- test stored update
 CREATE TABLE gtest3 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
 INSERT INTO gtest3 (a) VALUES (1), (2), (3), (NULL);
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 378297e6ea..39eec40bce 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -149,6 +149,15 @@ CREATE TABLE gtesty (x int, b int DEFAULT 55);
 CREATE TABLE gtest1_2 () INHERITS (gtest0, gtesty);  -- error
 DROP TABLE gtesty;

+-- test correct handling of GENERATED column that's only in child
+CREATE TABLE gtestp (f1 int);
+CREATE TABLE gtestc (f2 int GENERATED ALWAYS AS (f1+1) STORED) INHERITS(gtestp);
+INSERT INTO gtestc values(42);
+TABLE gtestc;
+UPDATE gtestp SET f1 = f1 * 10;
+TABLE gtestc;
+DROP TABLE gtestp CASCADE;
+
 -- test stored update
 CREATE TABLE gtest3 (a int, b int GENERATED ALWAYS AS (a * 3) STORED);
 INSERT INTO gtest3 (a) VALUES (1), (2), (3), (NULL);

Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Amit Langote
Date:
On Thu, Jan 5, 2023 at 4:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > After further thought: maybe we should get radical and postpone this
> > work all the way to executor startup.  The downside of that is having
> > to do it over again on each execution of a prepared plan.  But the
> > upside is that when the UPDATE targets a many-partitioned table,
> > we would have a chance at not doing the work at all for partitions
> > that get pruned at runtime.  I'm not sure if that win would emerge
> > immediately or if we still have executor work to do to manage pruning
> > of the target table.  I'm also not sure that this'd be a net win
> > overall.  But it seems worth considering.
>
> Here's a draft patch that does it like that.  This seems like a win
> for more reasons than just pruning, because I was able to integrate
> the calculation into runtime setup of the expressions, so that we
> aren't doing an extra stringToNode() on them.

Thanks for the patch.  This looks pretty neat and I agree that this
seems like a net win overall.

As an aside, I wonder why AttrDefault (and other things in
RelationData that need stringToNode() done on them to put into a Query
or a plan tree) doesn't store the expression Node tree to begin with?

> There's still a code path that does such a calculation at plan time
> (get_rel_all_updated_cols), but it's only used by postgres_fdw which
> has some other rather-inefficient behaviors in the same area.
>
> I've not looked into what it'd take to back-patch this.  We can't
> add a field to ResultRelInfo in released branches (cf 4b3e37993),
> but we might be able to repurpose RangeTblEntry.extraUpdatedCols.

I think we can make that work.  Would you like me to give that a try?

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Tom Lane
Date:
Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Jan 5, 2023 at 4:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a draft patch that does it like that.  This seems like a win
>> for more reasons than just pruning, because I was able to integrate
>> the calculation into runtime setup of the expressions, so that we
>> aren't doing an extra stringToNode() on them.

> Thanks for the patch.  This looks pretty neat and I agree that this
> seems like a net win overall.

Thanks for looking.

>> I've not looked into what it'd take to back-patch this.  We can't
>> add a field to ResultRelInfo in released branches (cf 4b3e37993),
>> but we might be able to repurpose RangeTblEntry.extraUpdatedCols.

> I think we can make that work.  Would you like me to give that a try?

I'm on it already.  AFAICT, the above won't actually work because
we don't have RTEs for all ResultRelInfos (per the
"relinfo->ri_RangeTableIndex != 0" test in ExecGetExtraUpdatedCols).
Probably we need something more like what 4b3e37993 did.

            regards, tom lane



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Amit Langote
Date:
On Fri, Jan 6, 2023 at 12:28 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Jan 5, 2023 at 4:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I've not looked into what it'd take to back-patch this.  We can't
> >> add a field to ResultRelInfo in released branches (cf 4b3e37993),
> >> but we might be able to repurpose RangeTblEntry.extraUpdatedCols.
>
> > I think we can make that work.  Would you like me to give that a try?
>
> I'm on it already.

Thanks.  What you committed seems fine to me.

>  AFAICT, the above won't actually work because
> we don't have RTEs for all ResultRelInfos (per the
> "relinfo->ri_RangeTableIndex != 0" test in ExecGetExtraUpdatedCols).

Yeah, I noticed that too.  I was thinking that that wouldn't be a
problem, because it is only partitions that are execution-time routing
targets that don't have their own RTEs and using a translated copy of
the root parent's RTE's extraUpdatedCols for them as before isn't
wrong.  Note that partitions can't have generated columns that are not
present in its parent.

BTW, you wrote in the commit message:

    However, there's nothing that says a traditional-inheritance child
    can't have generated columns that aren't there in its parent, or that
    have different dependencies than are in the parent's expression.
    (At present it seems that we don't enforce that for partitioning
    either, which is likely wrong to some degree or other; but the case
    clearly needs to be handled with traditional inheritance.)

Maybe I'm missing something, but AFICS, neither
traditional-inheritance child tables nor partitions allow a generated
column with an expression that is not the same as the parent's
expression for the same generated column:

-- traditional inheritance
create table inhp (a int, b int generated always as (a+1) stored);
create table inhc (b int generated always as (a+2) stored) inherits (inhp);
NOTICE:  moving and merging column "b" with inherited definition
DETAIL:  User-specified column moved to the position of the inherited column.
ERROR:  child column "b" specifies generation expression
HINT:  Omit the generation expression in the definition of the child
table column to inherit the generation expression from the parent
table.
create table inhc (a int, b int);
alter table inhc inherit inhp;
ERROR:  column "b" in child table must be a generated column
alter table inhc drop b, add b int generated always as (a+2) stored;
alter table inhc inherit inhp;
ERROR:  column "b" in child table has a conflicting generation expression

-- partitioning
create table partp (a int, b int generated always as (a+1) stored)
partition by list (a);
create table partc partition of partp (b generated always as (a+2)
stored) for values in (1);
ERROR:  generated columns are not supported on partitions
create table partc (a int, b int);
alter table partp attach partition partc for values in (1);
ERROR:  column "b" in child table must be a generated column
alter table partc drop b, add b int generated always as (a+2) stored;
alter table partp attach partition partc for values in (1);
ERROR:  column "b" in child table has a conflicting generation expression


--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Tom Lane
Date:
Amit Langote <amitlangote09@gmail.com> writes:
> BTW, you wrote in the commit message:
>     (At present it seems that we don't enforce that for partitioning
>     either, which is likely wrong to some degree or other; but the case
>     clearly needs to be handled with traditional inheritance.)

> Maybe I'm missing something, but AFICS, neither
> traditional-inheritance child tables nor partitions allow a generated
> column with an expression that is not the same as the parent's
> expression for the same generated column:

Well, there's some large holes in that, as per my post at [1].
I'm on board with locking this down for partitioning, but we haven't
done so yet.

            regards, tom lane

[1] https://www.postgresql.org/message-id/2793383.1672944799%40sss.pgh.pa.us



Re: moving extraUpdatedCols out of RangeTblEntry (into ModifyTable)

From
Amit Langote
Date:
On Fri, Jan 6, 2023 at 3:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > BTW, you wrote in the commit message:
> >     (At present it seems that we don't enforce that for partitioning
> >     either, which is likely wrong to some degree or other; but the case
> >     clearly needs to be handled with traditional inheritance.)
>
> > Maybe I'm missing something, but AFICS, neither
> > traditional-inheritance child tables nor partitions allow a generated
> > column with an expression that is not the same as the parent's
> > expression for the same generated column:
>
> Well, there's some large holes in that, as per my post at [1].
> I'm on board with locking this down for partitioning, but we haven't
> done so yet.
>
> [1] https://www.postgresql.org/message-id/2793383.1672944799%40sss.pgh.pa.us

Ah, I had missed that.  Will check and reply there.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com