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

Hi,


On Wed, 13 May 2026 at 10:50, jian he <jian.universality@gmail.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