Re: RFC: Logging plan of the running query - Mailing list pgsql-hackers

At Tue, 1 Feb 2022 23:11:03 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> >> One thing that really bothers me about commit e2c79e14 is that
> >> LockPage() is called, not LockBuffer(). GIN had no LockPage() calls
> >> before that commit, and is now the only code in the entire system that
> >> calls LockPage()/ConditionalLockPage() (the hash am no longer uses
> >> page heavyweight locks following recent work there).
> > I agree to the discussion.  Can't we use other mechanism here to get
> > rid of the Lockpage()?
> 
> I have no good idea for that yet, but I agree it's better to get rid
> of page level lock.

It's my turn?

The page lock is used to hold-off simultaneous cleanups on the same
index.  ShareUpdateExclusive lock on the index relation works that
way. In that path it seems like we are always holding a RowExclusive
lock, so it seems to me we can use ShareUpdateExclusive for our
purpose.

There might be a false blocking case when another backend is holding a
conflicting lock on the index.  They are, Share, ShareRowExclusive,
Exclusive and AccessExclusive.  The last three cases don't seem worth
discussion.  I'm not sure about Share and Share Row cases. AFAICS
Share lock is taken on an index in ATExecReplicaIdentity, and there no
use of ShareRowExclusive lock on an index. It's no use discussing about
explicit locking.

So aren't we able to use ShareUpdateExclusive lock for that?

In the attached patch, ginInsertCleanup has an extra check for such
stronger locks not being held.  At least "make check" doesn't cause
the extra assertion to fire.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index 7409fdc165..1af9a69abb 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -791,20 +791,29 @@ ginInsertCleanup(GinState *ginstate, bool full_clean,
     bool        fsm_vac = false;
     Size        workMemory;
 
-    /*
+    /*r
      * We would like to prevent concurrent cleanup process. For that we will
      * lock metapage in exclusive mode using LockPage() call. Nobody other
      * will use that lock for metapage, so we keep possibility of concurrent
      * insertion into pending list
      */
 
+    /*
+     * we use ShareUpdateExclusive lock on this relation to hold-off concurrent
+     * cleanup
+     */
+    Assert(!CheckRelationLockedByMe(index, ShareUpdateExclusiveLock, false));
+
+    /* tentative debug-purpose assertion for stronger locks */
+    Assert(!CheckRelationLockedByMe(index, ShareLock, true));
+    
     if (forceCleanup)
     {
         /*
          * We are called from [auto]vacuum/analyze or gin_clean_pending_list()
          * and we would like to wait concurrent cleanup to finish.
          */
-        LockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock);
+        LockRelation(index, ShareUpdateExclusiveLock);
         workMemory =
             (IsAutoVacuumWorkerProcess() && autovacuum_work_mem != -1) ?
             autovacuum_work_mem : maintenance_work_mem;
@@ -816,7 +825,7 @@ ginInsertCleanup(GinState *ginstate, bool full_clean,
          * just exit in hope that concurrent process will clean up pending
          * list.
          */
-        if (!ConditionalLockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock))
+        if (!ConditionalLockRelation(index, ShareUpdateExclusiveLock))
             return;
         workMemory = work_mem;
     }
@@ -830,7 +839,7 @@ ginInsertCleanup(GinState *ginstate, bool full_clean,
     {
         /* Nothing to do */
         UnlockReleaseBuffer(metabuffer);
-        UnlockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock);
+        UnlockRelation(index, ShareUpdateExclusiveLock);
         return;
     }
 
@@ -1002,7 +1011,7 @@ ginInsertCleanup(GinState *ginstate, bool full_clean,
         page = BufferGetPage(buffer);
     }
 
-    UnlockPage(index, GIN_METAPAGE_BLKNO, ExclusiveLock);
+    UnlockRelation(index, ShareUpdateExclusiveLock);
     ReleaseBuffer(metabuffer);
 
     /*