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: 
 <CACJufxG97GWGC3kczph04JjLHKGDMH4oVou1_yXKHxFSqAHwBQ@mail.gmail.com>
References: 
 <CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kQgL+TnVoX5LOyCQ@mail.gmail.com>
 <2901599.1740811424@sss.pgh.pa.us>
 <CACJufxG97GWGC3kczph04JjLHKGDMH4oVou1_yXKHxFSqAHwBQ@mail.gmail.com>
Comments: In-reply-to jian he <jian.universality@gmail.com>
	message dated "Sat, 01 Mar 2025 22:02:18 +0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3086965.1740873261.1@sss.pgh.pa.us>
Date: Sat, 01 Mar 2025 18:54:21 -0500
Message-ID: <3086966.1740873261@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3086966.1740873261%40sss.pgh.pa.us>
Precedence: bulk

jian he <jian.universality@gmail.com> writes:
> So I duplicated StoreAttrDefault and removed pg_attribute.atthasdef,
> pg_attrdef related code.
> and it works fine.

I think the fundamental problem here is that StoreAttrDefault
shouldn't be involved in this in the first place.  It looks like
somebody did that in the hopes of avoiding two updates of the
new pg_attribute row (one to set atthasdef and the other to fill
attmissingval), but it's just bad code structure.  We should
take that code out of StoreAttrDefault, not duplicate it, because
the assumption that the missingval is identical to what goes into
pg_attrdef is just wrong.

(We could possibly get back down to one update by moving code
in ATExecAddColumn so that we know before calling
InsertPgAttributeTuples whether the column will have a non-null
default, and then we could set atthasdef correctly in the
originally-inserted tuple.  That's an optimization though,
not part of the basic bug fix.)

Also, since the "if (RELKIND_HAS_STORAGE(relkind))" stanza
in ATExecAddColumn is already calling expression_planner,
we should be able to avoid doing that twice on the way to
discovering whether the expression is a constant.

I kind of feel that StoreAttrMissingVal belongs in heap.c;
it's got about nothing to do with pg_attrdef.

			regards, tom lane