Re: Unnecessary checks for new rows by some RI trigger functions? - Mailing list pgsql-hackers

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Feb 20, 2019 at 9:27 AM Antonin Houska <ah@cybertec.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