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
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>
Comments: In-reply-to jian he <jian.universality@gmail.com>
	message dated "Mon, 03 Mar 2025 16:45:21 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3664074.1741018726.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 03 Mar 2025 11:18:46 -0500
Message-ID: <3664075.1741018726@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3664075.1741018726%40sss.pgh.pa.us>
Precedence: bulk

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 =3D !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 |=3D 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 t=
he
>      * pg_attribute rows.
> this seems not right?
> DefineRelation->heap_create_with_catalog->heap_create->RelationBuildLoca=
lRelation->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