From: Tom Lane <tgl@sss.pgh.pa.us>
To: jian he <jian.universality@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: bug when apply fast default mechanism for adding new column over domain with default value
Date: Mon, 03 Mar 2025 11:18:46 -0500
Message-ID: <3664075.1741018726@sss.pgh.pa.us> (raw)
In-Reply-To: <CACJufxE9Ey=M_gpDpXFwur5g+CcXv-ZM0aSZPc+SCk0MqngstA@mail.gmail.com>
References: <CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kQgL+TnVoX5LOyCQ@mail.gmail.com>
<2901599.1740811424@sss.pgh.pa.us>
<CACJufxG97GWGC3kczph04JjLHKGDMH4oVou1_yXKHxFSqAHwBQ@mail.gmail.com>
<3086966.1740873261@sss.pgh.pa.us>
<CACJufxFQkAwZ-VpFX4ZYHqi0PPL4RMfB4Zp=8At=oF-zgBX4yA@mail.gmail.com>
<3519325.1740953319@sss.pgh.pa.us>
<CACJufxE9Ey=M_gpDpXFwur5g+CcXv-ZM0aSZPc+SCk0MqngstA@mail.gmail.com>
jian he <jian.universality@gmail.com> writes:
> within ATExecAddColumn, we can
> if (!missingIsNull)
> StoreAttrMissingVal(rel, attribute->attnum, missingval, missingIsNull);
> to save some cycles?
Probably not really worth it: surely that's going to be a very
infrequent edge case. We already eliminated cases with a simple
null-Constant default, so we'd only get here with a more complex
expression that evaluates to null.
> + /* ... and store it. If it's null, nothing is stored. */
> + StoreAttrMissingVal(rel, attribute->attnum,
> + missingval, missingIsNull);
> + has_missing = !missingIsNull;
> + FreeExecutorState(estate);
> do we need pfree missingval?
I don't see the point. It's not like this code leaks nothing else,
and we shouldn't be running in a long-lived memory context.
> maybe here add comments mentioning that the identity column needs table rewrite.
> since we removed ``tab->rewrite |= AT_REWRITE_DEFAULT_VAL;``
> in the if (colDef->identity) branch.
We don't need that because it's not a special case anymore: the
NextValueExpr will correctly be seen as a volatile default.
The existing code needs that hack because it creates the
NextValueExpr after the point at which expression volatility
is checked, but that's just poor design.
> looking at DefineRelation comments:
> * We can set the atthasdef flags now in the tuple descriptor; this just
> * saves StoreAttrDefault from having to do an immediate update of the
> * pg_attribute rows.
> this seems not right?
> DefineRelation->heap_create_with_catalog->heap_create->RelationBuildLocalRelation->CreateTupleDescCopy
> don't copy atthasdef.
> RelationBuildLocalRelation later didn't touch atthasdef.
> populate_compact_attribute didn't touch atthasdef.
> so StoreAttrDefault has to update that pg_attribute row.
You are right: that code has no effect, and if it did have an
effect it would be wrong in the case where somebody writes
"DEFAULT NULL" explicitly. We'd end with atthasdef set and
nothing in pg_attrdef, which would upset various places later.
So we should remove that comment and also the adjustments of
atthasdef. That seems like an independent patch though.
regards, tom lane
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-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, jian.universality@gmail.com
Subject: Re: bug when apply fast default mechanism for adding new column over domain with default value
In-Reply-To: <3664075.1741018726@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