Re: [PATCH] Rebuild CHECK constraints after generated column SET EXPRESSION

Hi,

On Wed, 13 May 2026 at 10:50, jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> Hi.
>
> In case you are wondering, I already handled the whole-row cases for
> ALTER TABLE DROP COLUMN and ALTER COLUMN SET DATA TYPE in
> https://commitfest.postgresql.org/patch/5988 and
> https://commitfest.postgresql.org/patch/6055
>
> However, I missed the ALTER COLUMN SET EXPRESSION scenario.
>
> RememberAllDependentForRebuilding with (attnum = 0) is essentially
> asking any objects depend on this relation,
> It will certainly catch many whole-row referenced dependent objects,
> however, I wouldn’t be surprised if unintended corner cases exist.
>
> CREATE TABLE r2 (a int, b int GENERATED ALWAYS AS (a * 10) STORED);
> ALTER TABLE r2 ADD CONSTRAINT cc CHECK (a IS NOT NULL);
> CREATE INDEX r2_idx ON r2 (a);
> CREATE POLICY p3 ON r2 AS PERMISSIVE USING (a IS NOT NULL);
> select classid::regclass, * from pg_depend where refobjid =
> 'r2'::regclass::oid and classid in ('pg_policy'::regclass);
>
> The examples above show that RLS policies can have two dependencies on the
> relation: one on the specific column, and another on the relation itself.
> ``RememberAllDependentForRebuilding with (attnum = 0)`` cannot cope with
> this.
>
> ATRewriteTables->finish_heap_swap->reindex_relation may reindex the
> relation, but
> AlteredTableInfo->changedIndexOids should still remember the whole-row
> Var references index objects.
>
> For ALTER COLUMN SET EXPRESSION, no need to worry about whole-row
> referenced triggers.BEFORE trigger referencing the whole-row
> (including generated column) is not allowed (see
> CreateTriggerFiringOn: ```if (!whenClause &&stmt->whenClause)```), and
> ALTER COLUMN SET EXPRESSION will work fine with AFTER
> triggersthat have whole-row reference.
>
> The attached v2 includes support for ALTER COLUMN SET EXPRESSION on columns
> referenced by whole-row indexes, check constraints, and RLS policies.
>

Thanks for picking this up, and for going much wider than I did
(indexes + policies, plus the per-expression check rather than my
"rebuild all CHECK constraints" hammer).

Patch overall looks good to me, have few small comments.

1. Typos and grammar nits

* generated_stored.sql / generated_virtual.sql (and the matching
.out files):
"-- indedx with whole-row reference need rebuild"
-> "-- index with whole-row reference needs rebuild"
"AFFTER TRIGGER" -> "AFTER TRIGGER"

* tablecmds.c: GeRelAssociatedPolicies()
The function name is missing the 't' -- should be
GetRelAssociatedPolicies() to match the rest of the file.

* var.c: "Use exprContainWholeRow to check whole-row var existsence
when in doubt." -> "existence".

* exprContainWholeRow: I think PostgreSQL naming style favours
under_score_lowercase for new functions (compare pull_varattnos,
contain_var_clause). expr_contains_wholerow_var (or similar)
would fit better?

2. The error message text:

errmsg("cannot alter generation expression of a column used in a
policy definition"),
errdetail("%s depends on column \"%s\"", ..., colName)

The DETAIL phrasing is slightly misleading. In the whole-row case
the policy doesn't depend on the column directly; it depends on the
relation, and the column happens to be part of the row value the
policy reads. Maybe:

errmsg("cannot alter generation expression of column \"%s\" of
relation \"%s\"",
colName, RelationGetRelationName(rel)),
errdetail("%s references the whole row.",
getObjectDescription(&pol_obj, false))

or similar. Saying "the whole row" in the DETAIL also gives users a
hint about how to fix it (e.g. rewrite the policy to reference
specific columns).

3. Minor: the `wholerow_idxoids` local in
RememberWholeRowDependentForRebuilding() is declared and used only
via `list_member_oid(...)`, but is never appended to anywhere in
this function. As written it's dead code?

4. Locking: GetRelAssociatedPolicies() opens pg_depend with
RowExclusiveLock, but it only reads. I think AccessShareLock should be
enough, matching the other lookup helpers in this file.

5. nit: Cosmetic: the same pull_varattnos + bms_is_member +
bms_free(expr_attrs) + reset-to-NULL pattern is repeated three
times (CHECK constraint, indexprs, indpred). A small helper
`expr_has_wholerow_var(Node *expr)` would make the function much
shorter and easier to read.

Thoughts?

Regards,
Ayush