Horton

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

jian he <jian.universality@gmail.com>

View thread View on postgr.es
From: jian he <jian.universality@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2025-03-02T09:50:50Z
Lists: pgsql-hackers

Attachments

On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 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.
>

in StoreAttrDefault,
i've split missingval related code into StoreAttrMissingVal.

> 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.
done.


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

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.