Hi Henson,
Thanks for the patch. I agree with the analysis: since
c6be1f43ab0, find_expr_references_walker() records a dependency
on each target column of an INSERT or UPDATE query, but a MERGE
keeps the target lists of its INSERT and UPDATE actions in
mergeActionList rather than in the query's targetList, so they
were never looked at -- an oversight in 7103ebb7aae, which
added MERGE. Recording the dependencies for the MERGE action
target lists, in the same walker and via the same helper, is
the right layer for the fix.
I used AI assistance for: the builds, test
runs and transcripts below were run with it. And I have
checked the outputs myself.
What I ran (current master 72b9498df9a; your CFBot commits
fd56cd9 and 3f549c7 cherry-pick cleanly onto it):
Bug reproduced on unpatched master, using your reproducer
(the target table given one extra, unassigned column):
ALTER COLUMN a TYPE and ALTER COLUMN b TYPE both succeed,
and so do DROP COLUMN a and DROP COLUMN b;
pg_get_functiondef then shows
THEN UPDATE SET "........pg.dropped.2........" = s.v
THEN INSERT (k, "........pg.dropped.3........")
and, after ALTER COLUMN a TYPE bigint, calling the function
fails with "table row type and query-specified row type do
not match". The rule with a MERGE in WITH behaves the same:
the drops succeed and pg_get_ruledef shows the same
pg.dropped placeholders.
With both patches, on objects created after the fix:
ALTER COLUMN a TYPE / ALTER COLUMN b TYPE:
ERROR: cannot alter type of a column used by a function
or procedure; DETAIL: function f() depends on column "a"
(and likewise "b")
DROP COLUMN a / DROP COLUMN b:
ERROR: cannot drop column ... because other objects
depend on it, with the matching DETAIL
DROP COLUMN on the unassigned column still succeeds,
pg_get_functiondef stays intact, and the function still
executes. The rule case fails the same way (worded "used
by a view or rule", naming the rule), its DROP on the
unassigned column succeeds, and pg_get_ruledef stays
intact. ALTER TYPE on the INSERT-assigned column is not in
the added tests; I probed it separately and it is blocked
too, for both the function and the rule.
Pre-existing objects, as you state, only gain the
dependencies when re-created: a function created on
unpatched master in the same database still allows the drop
after the patched build is installed, and its definition
still shows the placeholder. That matches the trade-off
accepted for stored definitions before, but it is worth a
committer's eye.
Scope: I searched src/backend for other places that record
target-column dependencies for stored query trees;
find_expr_references_walker() is the only one. The other
mergeActionList consumers are rewrite, executor and
deparse code, not dependency recording. DELETE and DO
NOTHING actions have no target list and are correctly
skipped, and because the walk covers every action in
mergeActionList, the WHEN ... BY SOURCE variants are
covered by construction.
Tests: meson setup + regress suites on the patched build
pass (Ok: 4, Fail: 0). With the tests applied but
dependency.c reverted to master, exactly create_function_sql
and rules fail, and the diffs are only the new MERGE
sections -- the dependency errors disappear and the
pg.dropped placeholders return -- so the tests discriminate
the fix. installcheck-world against an installed build
returned rc 0: 54 suites, 658 tests, all passed. TAP tests
were not enabled in that build, so the TAP suites did not
run.
Two notes for the committer rather than the author: the
backpatch extent (you propose 15, and the fix commit says
Backpatch-through: 15; the separate 16/15 version's
OCLASS_CLASS point in your mail needs the care you flag),
and the commit messages have no Discussion: trailer.
Verdict: Ready for Committer.
Regards,
Prashant
