hio: Relax rules for calling GetVisibilityMapPins()

GetVisibilityMapPins() insisted on the buffer1/buffer2 being in a specific
order. This required checks at the callsite. As a subsequent patch will add
another callsite, move related logic into GetVisibilityMapPins().

Discussion: https://postgr.es/m/20230403190030[email protected]

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 7479212d4e02739520d89f83ac6b93447564c9e1..db810ec45ea10ee92bd97544775ea391d8208461 100644 (file)
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -131,9 +131,9 @@ ReadBufferBI(Relation relation, BlockNumber targetBlock,
  * For each heap page which is all-visible, acquire a pin on the appropriate
  * visibility map page, if we haven't already got one.
  *
- * buffer2 may be InvalidBuffer, if only one buffer is involved.  buffer1
- * must not be InvalidBuffer.  If both buffers are specified, block1 must
- * be less than block2.
+ * To avoid complexity in the callers, either buffer1 or buffer2 may be
+ * InvalidBuffer if only one buffer is involved. For the same reason, block2
+ * may be smaller than block1.
  */
 static void
 GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
@@ -143,6 +143,26 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
    bool        need_to_pin_buffer1;
    bool        need_to_pin_buffer2;
 
+   /*
+    * Swap buffers around to handle case of a single block/buffer, and to
+    * handle if lock ordering rules require to lock block2 first.
+    */
+   if (!BufferIsValid(buffer1) ||
+       (BufferIsValid(buffer2) && block1 > block2))
+   {
+       Buffer      tmpbuf = buffer1;
+       Buffer     *tmpvmbuf = vmbuffer1;
+       BlockNumber tmpblock = block1;
+
+       buffer1 = buffer2;
+       vmbuffer1 = vmbuffer2;
+       block1 = block2;
+
+       buffer2 = tmpbuf;
+       vmbuffer2 = tmpvmbuf;
+       block2 = tmpblock;
+   }
+
    Assert(BufferIsValid(buffer1));
    Assert(buffer2 == InvalidBuffer || block1 <= block2);
 
@@ -502,14 +522,9 @@ loop:
         * done a bit of extra work for no gain, but there's no real harm
         * done.
         */
-       if (otherBuffer == InvalidBuffer || targetBlock <= otherBlock)
-           GetVisibilityMapPins(relation, buffer, otherBuffer,
-                                targetBlock, otherBlock, vmbuffer,
-                                vmbuffer_other);
-       else
-           GetVisibilityMapPins(relation, otherBuffer, buffer,
-                                otherBlock, targetBlock, vmbuffer_other,
-                                vmbuffer);
+       GetVisibilityMapPins(relation, buffer, otherBuffer,
+                            targetBlock, otherBlock, vmbuffer,
+                            vmbuffer_other);
 
        /*
         * Now we can check to see if there's enough free space here. If so,