Horton

Re: bug when apply fast default mechanism for adding new column over domain with default value

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: jian he <jian.universality@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2025-03-01T23:54:21Z
Lists: pgsql-hackers 
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

Commits

  1. Need to do CommandCounterIncrement after StoreAttrMissingVal.

  2. Simplify some logic around setting pg_attribute.atthasdef.

  3. Remove now-dead code in StoreAttrDefault().

  4. Fix broken handling of domains in atthasmissing logic.