Re: bug when apply fast default mechanism for adding new column over domain with default value - Mailing list pgsql-hackers

On Mon, Mar 3, 2025 at 6:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <tgl@sss.pgh.pa.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.