From: Tom Lane <tgl@sss.pgh.pa.us>
To: Vitaly Ustinov <vitaly@ustinov.ca>
Cc: pgsql-bugs@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Subject: Re: Generated column is not updated (Postgres 13)
Date: Wed, 19 May 2021 18:03:21 -0400
Message-ID: <3513055.1621461801@sss.pgh.pa.us> (raw)
In-Reply-To: <CAM_DEiWR2DPT6U4xb-Ehigozzd3n3G37ZB1+867zbsEVtYoJww@mail.gmail.com>
References: <CAM_DEiWR2DPT6U4xb-Ehigozzd3n3G37ZB1+867zbsEVtYoJww@mail.gmail.com>
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
Attachments:hack-to-stop-crash-with-self-referential-generated-column.patch (text/x-diff, 2.4K) downloaddiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 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;
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, vitaly@ustinov.ca, peter.eisentraut@enterprisedb.com
Subject: Re: Generated column is not updated (Postgres 13)
In-Reply-To: <3513055.1621461801@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox