Re: Generated column is not updated (Postgres 13) - Mailing list pgsql-bugs

Vitaly Ustinov <vitaly@ustinov.ca> writes:
> I would like to report the following:
> - a generated column is never updated if you pass the whole record to a
> stored procedure (an immutable function);

TBH, I think that this is insane and needs to be forbidden.  What value
are you expecting that the function would see in the column of the
whole-row var that corresponds to the generated column?  It surely
cannot be passed the value that it hasn't computed yet.

You could perhaps argue that it'd be okay to pass NULL.  The problem
with that, though, is that it'd violate the NOT NULL constraint that
exists for the generated column.  Quite aside from any confusion that
ensues at the user level, I'm afraid that that could result in C code
crashes --- there are, for example, places in tuple deforming that
assume that NOT NULL constraints are truthful.

Anyway, I tried your test case on a debug build and observed assertion
failures, which I traced to ATRewriteTable not being careful enough
to zero out the whole tuple each time through.  Conceivably we could
fix that as per the attached quick hack.  However, I think we ought
to disallow the case instead.  I observe that we already disallow
generated columns depending on each other:

regression=# create table foo (f1 int);
CREATE TABLE
regression=# alter table foo add f2 int generated always as (f1+1) stored;
ALTER TABLE
regression=# alter table foo add f3 int generated always as (f2+1) stored;
ERROR:  cannot use generated column "f2" in column generation expression
DETAIL:  A generated column cannot reference another generated column.

But a whole-row var violates this concept completely: it makes the
generated column depend, not only on every other column, but on itself
too.  Also, even if you don't mind null-for-not-yet-computed-value,
that would expose the computation order of the generated columns.

            regards, tom lane

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ebc62034d2..4178cde3b2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5699,16 +5699,6 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
                                                table_slot_callbacks(oldrel));
             newslot = MakeSingleTupleTableSlot(newTupDesc,
                                                table_slot_callbacks(newrel));
-
-            /*
-             * Set all columns in the new slot to NULL initially, to ensure
-             * columns added as part of the rewrite are initialized to NULL.
-             * That is necessary as tab->newvals will not contain an
-             * expression for columns with a NULL default, e.g. when adding a
-             * column without a default together with a column with a default
-             * requiring an actual rewrite.
-             */
-            ExecStoreAllNullTuple(newslot);
         }
         else
         {
@@ -5747,9 +5737,21 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)

             if (tab->rewrite > 0)
             {
+                /*
+                 * Set all columns in the new slot to NULL initially, to
+                 * ensure columns added as part of the rewrite are initialized
+                 * to NULL.  That is necessary as tab->newvals will not
+                 * contain an expression for columns with a NULL default, e.g.
+                 * when adding a column without a default together with a
+                 * column with a default requiring an actual rewrite.
+                 * Moreover, we must do it each time through, so that the slot
+                 * contents are sane in case any generated expression contains
+                 * a whole-row Var (which will read this same slot).
+                 */
+                ExecStoreAllNullTuple(newslot);
+
                 /* Extract data from old tuple */
                 slot_getallattrs(oldslot);
-                ExecClearTuple(newslot);

                 /* copy attributes */
                 memcpy(newslot->tts_values, oldslot->tts_values,
@@ -5782,12 +5784,12 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
                                        &newslot->tts_isnull[ex->attnum - 1]);
                 }

-                ExecStoreVirtualTuple(newslot);
-
                 /*
                  * Now, evaluate any expressions whose inputs come from the
                  * new tuple.  We assume these columns won't reference each
-                 * other, so that there's no ordering dependency.
+                 * other, so that there's no ordering dependency.  However,
+                 * because we allow whole-row references in GENERATED
+                 * expressions, that's not strictly true ...
                  */
                 econtext->ecxt_scantuple = newslot;