Re: let ALTER TABLE DROP COLUMN drop whole-row referenced object

On Mon, Jan 19, 2026 at 2:56 AM Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>
> Hi!
> I did take a look at v7.
>
> In 0001:
>
> > + indscan = systable_beginscan(pg_index, IndexIndrelidIndexId, true,
> > + NULL, 1, &skey);
> > + while (HeapTupleIsValid(htup = systable_getnext(indscan)))
> > + {
> > + Form_pg_index index = (Form_pg_index) GETSTRUCT(htup);
> > +
> > + /* add index's OID to result list */
> > + indexlist = lappend_oid(indexlist, index->indexrelid);
> > + }
> > + systable_endscan(indscan);
> > +
> > + table_close(pg_index, AccessShareLock);
> > +
> > + foreach_oid(indexoid, indexlist)
> > + {
>
> Hmm, why is this not just a one cycle? Also, not sure how many
> relations can be returned by pg_index scan. Maybe it is worth adding
> CHECK_FOR_INTERRUPTS() here?
>

I refactored this part, I realized that v7 has some duplicated code in
recordWholeRowDependencyOnOrError.
After refactoring, CHECK_FOR_INTERRUPTS is not needed, I think.

On Mon, Jan 19, 2026 at 6:00 PM 金 <jinbinge(at)126(dot)com> wrote:
>
> Hello,
>
> I did take a look at v7-0001 and v7-0002.
>
> In v7-0002:
>
> 1.
> > + pull_varattnos(expr, PRS2_OLD_VARNO, &expr_attrs);
> > + pull_varattnos(expr, PRS2_NEW_VARNO, &expr_attrs);
>
> The function "pull_varattnos" was called twice, with different varno parameters each time (first using PRS2_NEW_VARNO, then using PRS2_OLD_VARNO).
> I think it would be better to use PRS2_NEW_VARNO | PRS2_OLD_VARNO in a single call.
> pull_varattnos(expr, PRS2_NEW_VARNO | PRS2_OLD_VARNO, &expr_attrs);
>

We are checking if the trig->tgqual expression referenced Var->varno
equals PRS2_NEW_VARNO or PRS2_NEW_VARNO.
PRS2_NEW_VARNO | PRS2_OLD_VARNO is equal to 3,
So, I don't think it will work, see pull_varattnos_walker.

> 2.
>
> > + if (trig->tgqual != NULL)
>
> Should we first check if TRIGGER_FOR_ROW(trig->tgtype) before processing trig->tgqual are more appropriate?

I see.
However, unconditionally checking the trigger's WHEN clause whole-row
references seems better, IMHO.

> 3.
> Another issue is the Bitmapset set allocated by pull_varattnos will never be released. This will cause a memory leak.
> Please add the statement bms_free(expr_attrs); after each usage within the loops in recordWholeRowDependencyOnOrError.
>
> In v7-0001：
>
> As in v7-0002, the Bitmapset returned by pull_varattnos is never released.
> Please add the statement bms_free(expr_attrs);
>

Yech, In this case, we need ``bms_free(expr_attrs);`` because in the function
recordWholeRowDependencyOnOrError, we use the variable `expr_attrs` constantly.

--
jian
https://www.enterprisedb.com/