Move code from ExecutorCheckPerms into ExecutorStart#789
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #789 +/- ##
==========================================
+ Coverage 87.93% 88.04% +0.10%
==========================================
Files 3 3
Lines 1318 1313 -5
Branches 186 185 -1
==========================================
- Hits 1159 1156 -3
+ Misses 77 76 -1
+ Partials 82 81 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
artemgavrilov
approved these changes
Jul 14, 2026
The data we get in the ExecutorCheckPerms hook is also available in the QueryDesc object we get in the other executor hooks so doing the relation name lookup in a separate hook only makes the code more complicated for no gain. It might make even more sense to move this lookup to the ExecutorEnd hook but that risks changing the behavior as we cannot do catalog lookups when the transaction has been rolled back. It is unclear if we need to support that, but that kind of change should be its own commit.
b668dc9 to
53b3df0
Compare
dutow
reviewed
Jul 14, 2026
dutow
left a comment
Contributor
There was a problem hiding this comment.
This introduced a bug (or two, depending on how count it): the utility path doesn't update relations and num_relations
-- Observed results for the trailing COPY:
-- base (main) : {public.logtbl} (correct — COPY's own table)
-- patched (pr789) : {public.sensitive_audit} (WRONG — from the prior errored query)
CREATE EXTENSION IF NOT EXISTS pg_stat_monitor;
CREATE TABLE sensitive_audit(id int);
INSERT INTO sensitive_audit VALUES (1);
CREATE TABLE logtbl(id int);
SELECT pg_stat_monitor_reset();
-- Tracked query touching sensitive_audit, errors during ExecutorRun:
SELECT id, 1/(id-1) FROM sensitive_audit; -- division by zero
-- Unrelated utility statement stored via the ProcessUtility path:
COPY logtbl TO '/tmp/pgsm_stale.csv' CSV;
SELECT substring(query for 45) AS query, relations
FROM pg_stat_monitor
WHERE query ILIKE '%COPY logtbl%' OR query ILIKE '%sensitive%'
ORDER BY query;and
-- Observed results:
-- base (main) patched (pr789)
-- SELECT ... FROM parent {public.parent} {public.parent} (unchanged)
-- INSERT INTO child (has FK) {} (clobbered/empty) {public.child} (FIXED)
-- UPDATE child (has FK) {} (clobbered/empty) {public.child} (FIXED)
-- COPY parent TO ... {public.parent} {} (REGRESSION)
CREATE EXTENSION IF NOT EXISTS pg_stat_monitor;
CREATE TABLE parent(id int primary key);
CREATE TABLE child(id int primary key, pid int references parent(id));
INSERT INTO parent VALUES (1),(2),(3);
SELECT pg_stat_monitor_reset();
SELECT * FROM parent WHERE id = 1;
INSERT INTO child VALUES (10, 1);
COPY parent TO '/tmp/pgsm_copy_test.csv' CSV;
UPDATE child SET pid = 2 WHERE id = 10;
SELECT substring(query for 42) AS query, relations
FROM pg_stat_monitor
WHERE query NOT ILIKE '%pg_stat_monitor%' AND query NOT ILIKE '%CREATE%'
ORDER BY query;| } | ||
|
|
||
| /* | ||
| * Save names of relations involved in the qeury. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The data we get in the
ExecutorCheckPermshook is also available in theQueryDescobject we get in the other executor hooks so doing the relation name lookup in a separate hook only makes the code more complicated for no gain.It might make even more sense to move this lookup to the
ExecutorEndhook but that risks changing the behavior as we cannot do catalog lookups when the transaction has been rolled back. It is unclear if we need to support that, but that kind of change should be its own commit.PG-0