Unnecessary checks for new rows by some RI trigger functions?

When reviewing a related patch [1], I spent some time thinking about the
"detectNewRows" argument of ri_PerformCheck(). My understanding is that it
should (via the es_crosscheck_snapshot field of EState) prevent the executor
from accidentally deleting PK value that another transaction (whose data
changes the trigger's transaction does not see) might need.

However I find it confusing that the trigger functions pass detectNewRows=true
even if they only execute SELECT statement. I think that in these cases the
trigger functions 1) detect themselves that another transaction inserted
row(s) referencing the PK whose deletion is being checked, 2) do not try to
delete the PK anyway. Furthermore (AFAICS), only heap_update() and
heap_delete() functions receive the crosscheck snapshot, so ri_PerformCheck()
does not have to pass the crosscheck snapshot to SPI_execute_snapshot() for
SELECT queries.

Following is patch proposal. Is anything wrong about that?

diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index e1aa3d0044..e235ad9cd0 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -574,7 +574,7 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
result = ri_PerformCheck(riinfo, &qkey, qplan,
fk_rel, pk_rel,
old_row, NULL,
- true, /* treat like update */
+ false,
SPI_OK_SELECT);

if (SPI_finish() != SPI_OK_FINISH)
@@ -802,7 +802,7 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
ri_PerformCheck(riinfo, &qkey, qplan,
fk_rel, pk_rel,
old_row, NULL,
- true, /* must detect new rows */
+ false,
SPI_OK_SELECT);

if (SPI_finish() != SPI_OK_FINISH)

[1] https://commitfest.postgresql.org/22/1975/

--
Antonin Houska
https://www.cybertec-postgresql.com