Re: [HACKERS] WAL logging problem in 9.4.3? - Mailing list pgsql-hackers

I should have replied this first.

At Sun, 17 Nov 2019 20:54:34 -0800, Noah Misch <noah@leadboat.com> wrote in 
> On Tue, Nov 05, 2019 at 02:53:35PM -0800, Noah Misch wrote:
> > I started pre-commit editing on 2019-10-28, and comment+README updates have
> > been the largest part of that.  I'll check my edits against the things you
> > list here, and I'll share on-list before committing.  I've now marked the CF
> > entry Ready for Committer.
> 
> Having dedicated many days to that, I am attaching v24nm.  I know of two
> remaining defects:
> 
> === Defect 1: gistGetFakeLSN()
> 
> When I modified pg_regress.c to use wal_level=minimal for all suites,
> src/test/isolation/specs/predicate-gist.spec failed the assertion in
> gistGetFakeLSN().  One could reproduce the problem just by running this
> sequence in psql:
> 
>           begin;
>           create table gist_point_tbl(id int4, p point);
>           create index gist_pointidx on gist_point_tbl using gist(p);
>           insert into gist_point_tbl (id, p)
>           select g, point(g*10, g*10) from generate_series(1, 1000) g;
> 
> I've included a wrong-in-general hack to make the test pass.  I see two main
> options for fixing this:
> 
> (a) Introduce an empty WAL record that reserves an LSN and has no other
> effect.  Make GiST use that for permanent relations that are skipping WAL.
> Further optimizations are possible.  For example, we could use a backend-local
> counter (like the one gistGetFakeLSN() uses for temp relations) until the
> counter is greater a recent real LSN.  That optimization is probably too
> clever, though it would make the new WAL record almost never appear.
> 
> (b) Exempt GiST from most WAL skipping.  GiST index build could still skip
> WAL, but it would do its own smgrimmedsync() in addition to the one done at
> commit.  Regular GiST mutations would test RELPERSISTENCE_PERMANENT instead of
> RelationNeedsWal(), and we'd need some hack for index_copy_data() and possibly
> other AM-independent code that skips WAL.
> 
> Overall, I like the cleanliness of (a).  The main argument for (b) is that it
> ensures we have all the features to opt-out of WAL skipping, which could be
> useful for out-of-tree index access methods.  (I think we currently have the
> features for a tableam to do so, but not for an indexam to do so.)  Overall, I
> lean toward (a).  Any other ideas or preferences?

I don't like (b), too.

What we need there is any sequential numbers for page LSN but really
compatible with real LSN. Couldn't we use GetXLogInsertRecPtr() in the
case?  Or, I'm not sure but I suppose that nothing happenes when
UNLOGGED GiST index gets turned into LOGGED one.

Rewriting table like SET LOGGED will work but not realistic.

> === Defect 2: repetitive work when syncing many relations
> 
> For deleting relfilenodes, smgrDoPendingDeletes() collects a list for
> smgrdounlinkall() to pass to DropRelFileNodesAllBuffers(), which is
> sophisticated about optimizing the shared buffers scan.  Commit 279628a
> introduced that, in 2013.  I think smgrDoPendingSyncs() should do likewise, to
> further reduce the chance of causing performance regressions.  (One could,
> however, work around the problem by raising wal_skip_threshold.)  Kyotaro, if
> you agree, could you modify v24nm to implement that?

Seems reasonable. Please wait a minite.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/gist/gistutil.c b/src/backend/access/gist/gistutil.c
index 66c52d6dd6..387b1f7d18 100644
--- a/src/backend/access/gist/gistutil.c
+++ b/src/backend/access/gist/gistutil.c
@@ -1017,8 +1017,7 @@ gistGetFakeLSN(Relation rel)
      * XXX before commit fix this.  This is not correct for
      * RELPERSISTENCE_PERMANENT, but it suffices to make tests pass.
      */
-    if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP
-        || rel->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
+    if (rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
     {
         /*
          * Temporary relations are only accessible in our session, so a simple
@@ -1026,6 +1025,15 @@ gistGetFakeLSN(Relation rel)
          */
         return counter++;
     }
+    else if (rel->rd_rel->relpersistence == RELPERSISTENCE_PERMANENT)
+    {
+        /*
+         * Even though we are skipping WAL-logging of a permanent relations,
+         * the LSN must be a real one because WAL-logging starts after commit.
+         */
+        Assert(!RelationNeedsWAL(rel));
+        return GetXLogInsertRecPtr();
+    }
     else
     {
         /*