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.
