Hi,
On Wed, 13 May 2026 at 10:50, jian he <[email protected]> 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
