Sanitize some WAL-logging buffer handling in GIN and GiST code

As transam's README documents, the general order of actions recommended
when WAL-logging a buffer is to unlock and unpin buffers after leaving a
critical section.  This pattern was not being followed by some code
paths of GIN and GiST, adjusted in this commit, where buffers were
either unlocked or unpinned inside a critical section.  Based on my
analysis of each code path updated here, there is no reason to not
follow the recommended unlocking/unpin pattern done outside of a
critical section.

These inconsistencies are rather old, coming mainly from ecaa4708e5dd
and ff301d6e690b.  The guidelines in the README predate these commits,
being introduced in 6d61cdec0761.

Author: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/CALdSSPgBPnpNNzxv0Y+_GNFzW6PmzRZYh+_hpf06Y1N2zLhZaQ@mail.gmail.com

diff --git a/src/backend/access/gin/gindatapage.c b/src/backend/access/gin/gindatapage.c
index 436e54f2066ab2ba6a8bae523679521d39d36c66..c5d7db28077daee4b31cf5d3e2ae0efd9279f3ac 100644 (file)
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -1854,10 +1854,10 @@ createPostingTree(Relation index, ItemPointerData *items, uint32 nitems,
                PageSetLSN(page, recptr);
        }
 
-       UnlockReleaseBuffer(buffer);
-
        END_CRIT_SECTION();
 
+       UnlockReleaseBuffer(buffer);
+
        /* During index build, count the newly-added data page */
        if (buildStats)
                buildStats->nDataPages++;

diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index 7a6b177977b748b894e123dd0618a82a98554f0f..f50848eb65a813bdb4f90272a58b5735425e61bd 100644 (file)
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -134,10 +134,10 @@ writeListPage(Relation index, Buffer buffer,
        /* get free space before releasing buffer */
        freesize = PageGetExactFreeSpace(page);
 
-       UnlockReleaseBuffer(buffer);
-
        END_CRIT_SECTION();
 
+       UnlockReleaseBuffer(buffer);
+
        return freesize;
 }
 
@@ -459,10 +459,10 @@ ginHeapTupleFastInsert(GinState *ginstate, GinTupleCollector *collector)
        if (metadata->nPendingPages * GIN_PAGE_FREESIZE > cleanupSize * (Size) 1024)
                needCleanup = true;
 
-       UnlockReleaseBuffer(metabuffer);
-
        END_CRIT_SECTION();
 
+       UnlockReleaseBuffer(metabuffer);
+
        /*
         * Since it could contend with concurrent cleanup process we cleanup
         * pending list not forcibly.
@@ -659,11 +659,11 @@ shiftList(Relation index, Buffer metabuffer, BlockNumber newHead,
                        }
                }
 
+               END_CRIT_SECTION();
+
                for (i = 0; i < data.ndeleted; i++)
                        UnlockReleaseBuffer(buffers[i]);
 
-               END_CRIT_SECTION();
-
                for (i = 0; fill_fsm && i < data.ndeleted; i++)
                        RecordFreeIndexPage(index, freespace[i]);
 

diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index d205093e21dc723a9277b6c42919ae4799431fa7..ff927279cc39a4d78cf0afcb40ee763593a475be 100644 (file)
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -663,9 +663,9 @@ ginUpdateStats(Relation index, const GinStatsData *stats, bool is_build)
                PageSetLSN(metapage, recptr);
        }
 
-       UnlockReleaseBuffer(metabuffer);
-
        END_CRIT_SECTION();
+
+       UnlockReleaseBuffer(metabuffer);
 }
 
 /*

diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c
index 11a6674a10b0d698e70012006cd98f9dda25445b..c9f143f6c31b5db148c69b824d4f31b15aca2c3d 100644 (file)
--- a/src/backend/access/gin/ginvacuum.c
+++ b/src/backend/access/gin/ginvacuum.c
@@ -224,12 +224,12 @@ ginDeletePage(GinVacuumState *gvs, BlockNumber deleteBlkno, BlockNumber leftBlkn
                PageSetLSN(BufferGetPage(lBuffer), recptr);
        }
 
+       END_CRIT_SECTION();
+
        ReleaseBuffer(pBuffer);
        ReleaseBuffer(lBuffer);
        ReleaseBuffer(dBuffer);
 
-       END_CRIT_SECTION();
-
        gvs->result->pages_newly_deleted++;
        gvs->result->pages_deleted++;
 }
@@ -654,8 +654,8 @@ ginbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats,
                        PageRestoreTempPage(resPage, page);
                        MarkBufferDirty(buffer);
                        xlogVacuumPage(gvs.index, buffer);
-                       UnlockReleaseBuffer(buffer);
                        END_CRIT_SECTION();
+                       UnlockReleaseBuffer(buffer);
                }
                else
                {

diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index d3acaa636c3ed448445e85e6c998a33ee64200e8..a9a1678acc97a04141136e9762d37419f61852f3 100644 (file)
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -1355,11 +1355,12 @@ log_newpage_range(Relation rel, ForkNumber forknum,
                recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI);
 
                for (i = 0; i < nbufs; i++)
-               {
                        PageSetLSN(BufferGetPage(bufpack[i]), recptr);
-                       UnlockReleaseBuffer(bufpack[i]);
-               }
+
                END_CRIT_SECTION();
+
+               for (i = 0; i < nbufs; i++)
+                       UnlockReleaseBuffer(bufpack[i]);
        }
 }