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

On Mon, Mar 3, 2025 at 6:08 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> jian he <jian(dot)universality(at)gmail(dot)com> writes:
> > On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I think the fundamental problem here is that StoreAttrDefault
> >> shouldn't be involved in this in the first place.
>
> > i've split missingval related code into StoreAttrMissingVal.
>
> Hm, this does nothing to improve the modularity of the affected
> code; if anything it's worse than before. (Fools around for
> awhile...) I had something more like the attached in mind.
> The idea here is to centralize the control of whether we are
> storing a missingval or doing a table rewrite in ATExecAddColumn.
> StoreAttrMissingVal has nothing to do with pg_attrdef nor does
> StoreAttrDefault have anything to do with attmissingval.
>

your patch looks very good!!!

within ATExecAddColumn, we can
if (!missingIsNull)
StoreAttrMissingVal(rel, attribute->attnum, missingval, missingIsNull);
to save some cycles?

+ /* ... and store it. If it's null, nothing is stored. */
+ StoreAttrMissingVal(rel, attribute->attnum,
+ missingval, missingIsNull);
+ has_missing = !missingIsNull;
+ FreeExecutorState(estate);
do we need pfree missingval?

+ else
+ {
+ /*
+ * Failed to use missing mode. We have to do a table rewrite
+ * to install the value --- unless it's a virtual generated
+ * column.
+ */
+ if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
+ tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+ }
maybe here add comments mentioning that the identity column needs table rewrite.
since we removed ``tab->rewrite |= AT_REWRITE_DEFAULT_VAL;``
in the if (colDef->identity) branch.

StoreAttrDefault is at the end of
ATExecAlterColumnType, ATExecCookedColumnDefault, StoreConstraints
these function later will call CommandCounterIncrement
I also checked AddRelationNewConstraints surrounding code.
overall i think StoreAttrDefault doesn't have CommandCounterIncrement
should be fine.

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 the
* pg_attribute rows.
this seems not right?
DefineRelation->heap_create_with_catalog->heap_create->RelationBuildLocalRelation->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.

somehow related, if you look at AddRelationNewConstraints.
```
foreach_ptr(RawColumnDefault, colDef, newColDefaults)
{
defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal);
cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
cooked->contype = CONSTR_DEFAULT;
cooked->conoid = defOid;
}
maybe we can minor change comments in struct CookedConstraint.conoid.
We can mention: if contype is CONSTR_DEFAULT then it is pg_attrdef.oid
for the default expression.