Re: [HACKERS] WAL logging problem in 9.4.3?

Hello.

At Mon, 16 Jul 2018 16:14:09 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in
<20180716201409.2qfcneo4qkdwjvpv@alvherre.pgsql>
> On 2018-Jul-12, Heikki Linnakangas wrote:
> 
> > > > Thanks for the pointer.  My tap test has been covering two out of
> > > > the three scenarios you have in your script.  I have been able to
> > > > convert the extra as the attached, and I have added as well an
> > > > extra test with TRUNCATE triggers.  So it seems to me that we want
> > > > to disable the optimization if any type of trigger are defined on
> > > > the relation copied to as it could be possible that these triggers
> > > > work on the blocks copied as well, for any BEFORE/AFTER and
> > > > STATEMENT/ROW triggers.  What do you think?
> > > 
> > > Yeah, this seems like the only sane approach.
> > 
> > Doesn't have to be a trigger, could be a CHECK constraint, datatype
> > input function, etc. Admittedly, having a datatype input function that
> > inserts to the table is worth a "huh?", but I'm feeling very confident
> > that we can catch all such cases, and some of them might even be
> > sensible.
> 
> A counterexample could be a a JSON compresion scheme that uses a catalog
> for a dictionary of keys.  Hasn't this been described already?  Also not
> completely out of the question for GIS data, I think (Not sure if
> PostGIS does this already.)

In the third case, IIUC, disabling bulk-insertion after any
WAL-logging insertion happend seems to work. The attached diff to
v2 patch makes the three TAP tests pass. It uses relcache to
store the last insertion XID but it will not be invalidated
during a COPY operation.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 72395a50b8..e5c651b498 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2509,6 +2509,18 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
 
     MarkBufferDirty(buffer);
 
+    /*
+     * Bulk insertion is not safe after a WAL-logging insertion in the same
+     * transaction. We don't start bulk insertion under inhibitin conditions
+     * but we also need to cancel WAL-skipping in the case where WAL-logging
+     * insertion happens during a bulk insertion. This happens by anything
+     * that can insert a tuple during bulk insertion such like triggers,
+     * constraints or type conversions. We need not worry about relcache flush
+     * happening while a bulk insertion is running.
+     */
+    if (relation->last_logged_insert_xid == xid)
+        options &= ~HEAP_INSERT_SKIP_WAL;
+
     /* XLOG stuff */
     if (!(options & HEAP_INSERT_SKIP_WAL) && RelationNeedsWAL(relation))
     {
@@ -2582,6 +2594,12 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
         recptr = XLogInsert(RM_HEAP_ID, info);
 
         PageSetLSN(page, recptr);
+
+        /*
+         * If this happens during a bulk insertion, stop WAL skipping for the
+         * rest of the current command.
+         */
+        relation->last_logged_insert_xid = xid;
     }
 
     END_CRIT_SECTION();
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 7674369613..7b9a7af2d2 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2416,10 +2416,8 @@ CopyFrom(CopyState cstate)
     {
         hi_options |= HEAP_INSERT_SKIP_FSM;
 
-        if (!XLogIsNeeded() &&
-            cstate->rel->trigdesc == NULL &&
-            RelationGetNumberOfBlocks(cstate->rel) == 0)
-            hi_options |= HEAP_INSERT_SKIP_WAL;
+        if (!XLogIsNeeded() && RelationGetNumberOfBlocks(cstate->rel) == 0)
+             hi_options |= HEAP_INSERT_SKIP_WAL;
     }
 
     /*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 30a956822f..34a692a497 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1575,6 +1575,9 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
         {
             /* Immediate, non-rollbackable truncation is OK */
             heap_truncate_one_rel(rel);
+
+            /* Allow bulk-insert */
+            rel->last_logged_insert_xid = InvalidTransactionId;
         }
         else
         {
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 6125421d39..99fb7e1dd8 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1243,6 +1243,8 @@ RelationBuildDesc(Oid targetRelId, bool insertIt)
     /* It's fully valid */
     relation->rd_isvalid = true;
 
+    relation->last_logged_insert_xid = InvalidTransactionId;
+
     return relation;
 }
 
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index c97f9d1b43..6ee575ad14 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -188,6 +188,9 @@ typedef struct RelationData
 
     /* use "struct" here to avoid needing to include pgstat.h: */
     struct PgStat_TableStatus *pgstat_info; /* statistics collection area */
+
+    /* XID of the last transaction on which WAL-logged insertion happened */
+    TransactionId        last_logged_insert_xid;
 } RelationData;