Re: Unnecessary checks for new rows by some RI trigger functions?

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Feb 20, 2019 at 9:27 AM Antonin Houska <ah(at)cybertec(dot)at> wrote:
> >> However I find it confusing that the trigger functions pass detectNewRows=true
> >> even if they only execute SELECT statement.
>
> > I don't quite see what those two things have to do with each other,
> > but I might be missing something.
>
> Me too.

Sure, that was the problem! I was thinking about the crosscheck snapshot so
much that I missed the fact that snapshot handling is not the only purpose of
the detectNewRows argument. What I considered a problem can be fixed this way,
if it's worth:

diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index e1aa3d0044..993e778875 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -233,7 +233,7 @@ static bool ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
HeapTuple old_tuple, HeapTuple new_tuple,
- bool detectNewRows, int expect_OK);
+ bool detectNewRows, bool select_only, int expect_OK);
static void ri_ExtractValues(Relation rel, HeapTuple tup,
const RI_ConstraintInfo *riinfo, bool rel_is_pk,
Datum *vals, char *nulls);
@@ -439,7 +439,7 @@ RI_FKey_check(TriggerData *trigdata)
ri_PerformCheck(riinfo, &qkey, qplan,
fk_rel, pk_rel,
NULL, new_row,
- false,
+ false, true,
SPI_OK_SELECT);

if (SPI_finish() != SPI_OK_FINISH)
@@ -575,6 +575,7 @@ ri_Check_Pk_Match(Relation pk_rel, Relation fk_rel,
fk_rel, pk_rel,
old_row, NULL,
true, /* treat like update */
+ true,
SPI_OK_SELECT);

if (SPI_finish() != SPI_OK_FINISH)
@@ -803,6 +804,7 @@ ri_restrict(TriggerData *trigdata, bool is_no_action)
fk_rel, pk_rel,
old_row, NULL,
true, /* must detect new rows */
+ true,
SPI_OK_SELECT);

if (SPI_finish() != SPI_OK_FINISH)
@@ -943,6 +945,7 @@ RI_FKey_cascade_del(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
old_row, NULL,
true, /* must detect new rows */
+ false,
SPI_OK_DELETE);

if (SPI_finish() != SPI_OK_FINISH)
@@ -1099,6 +1102,7 @@ RI_FKey_cascade_upd(PG_FUNCTION_ARGS)
fk_rel, pk_rel,
old_row, new_row,
true, /* must detect new rows */
+ false,
SPI_OK_UPDATE);

if (SPI_finish() != SPI_OK_FINISH)
@@ -1286,6 +1290,7 @@ ri_setnull(TriggerData *trigdata)
fk_rel, pk_rel,
old_row, NULL,
true, /* must detect new rows */
+ false,
SPI_OK_UPDATE);

if (SPI_finish() != SPI_OK_FINISH)
@@ -1473,6 +1478,7 @@ ri_setdefault(TriggerData *trigdata)
fk_rel, pk_rel,
old_row, NULL,
true, /* must detect new rows */
+ false,
SPI_OK_UPDATE);

if (SPI_finish() != SPI_OK_FINISH)
@@ -2356,7 +2362,7 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
RI_QueryKey *qkey, SPIPlanPtr qplan,
Relation fk_rel, Relation pk_rel,
HeapTuple old_tuple, HeapTuple new_tuple,
- bool detectNewRows, int expect_OK)
+ bool detectNewRows, bool select_only, int expect_OK)
{
Relation query_rel,
source_rel;
@@ -2423,17 +2429,20 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
* that SPI_execute_snapshot will register the snapshots, so we don't need
* to bother here.
*/
+ test_snapshot = InvalidSnapshot;
+ crosscheck_snapshot = InvalidSnapshot;
if (IsolationUsesXactSnapshot() && detectNewRows)
{
CommandCounterIncrement(); /* be sure all my own work is visible */
+
test_snapshot = GetLatestSnapshot();
- crosscheck_snapshot = GetTransactionSnapshot();
- }
- else
- {
- /* the default SPI behavior is okay */
- test_snapshot = InvalidSnapshot;
- crosscheck_snapshot = InvalidSnapshot;
+
+ /*
+ * crosscheck_snapshot is actually used only for UPDATE / DELETE
+ * queries.
+ */
+ if (!select_only)
+ crosscheck_snapshot = GetTransactionSnapshot();
}

/*

> It's quite easy to demonstrate that the second part of Antonin's
> proposed patch (ie, don't check for new rows in the FK table during
> ri_restrict) is wrong:
>
> regression=# create table p(f1 int primary key);
> CREATE TABLE
> regression=# create table f(f1 int references p on delete no action deferrable initially deferred);
> CREATE TABLE
> regression=# insert into p values (1);
> INSERT 0 1
> regression=# begin transaction isolation level serializable ;
> BEGIN
> regression=# table f;
> f1
> ----
> (0 rows)
>
> regression=# delete from p where f1=1;
> DELETE 1
>
> -- now, in another session:
>
> regression=# insert into f values (1);
> INSERT 0 1
>
> -- next, back in first session:
>
> regression=# commit;
> ERROR: update or delete on table "p" violates foreign key constraint "f_f1_fkey" on table "f"
> DETAIL: Key (f1)=(1) is still referenced from table "f".
>
> With the patch, the first session's commit succeeds, and we're left
> with a violated FK constraint.

When I was running this example, the other session got blocked until the first
(serializable) transaction committed. To achieve this ERROR (or FK violated
due to my patch proposal) I had to run the other session's INSERT before the
first session's DELETE. But I think I understand the problem.

Thanks for the detailed analysis.

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