Rework redundant code in subtrans.c

When this code was written the duplicity didn't matter, but with all the
SLRU-bank stuff we just added, it has become excessive.  Turn it into a
simpler loop with no code duplication.  Also add a test so that this
code becomes covered.

Discussion: https://postgr.es/m/202403041517.3a35jw53os65@alvherre.pgsql

diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51b5626e3f2cb41a4d31d578b272cd5b..50bb1d8cfc5f0950fd797bb3f84e40c792e38f90 100644 (file)
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
    FullTransactionId nextXid;
    int64       startPage;
    int64       endPage;
-   LWLock     *prevlock;
+   LWLock     *prevlock = NULL;
    LWLock     *lock;
 
    /*
@@ -324,42 +324,27 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
    nextXid = TransamVariables->nextXid;
    endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
 
-   prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
-   LWLockAcquire(prevlock, LW_EXCLUSIVE);
-   while (startPage != endPage)
+   for (;;)
    {
        lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
-       /*
-        * Check if we need to acquire the lock on the new bank then release
-        * the lock on the old bank and acquire on the new bank.
-        */
        if (prevlock != lock)
        {
-           LWLockRelease(prevlock);
+           if (prevlock)
+               LWLockRelease(prevlock);
            LWLockAcquire(lock, LW_EXCLUSIVE);
            prevlock = lock;
        }
 
        (void) ZeroSUBTRANSPage(startPage);
+       if (startPage == endPage)
+           break;
+
        startPage++;
        /* must account for wraparound */
        if (startPage > TransactionIdToPage(MaxTransactionId))
            startPage = 0;
    }
 
-   lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
-   /*
-    * Check if we need to acquire the lock on the new bank then release the
-    * lock on the old bank and acquire on the new bank.
-    */
-   if (prevlock != lock)
-   {
-       LWLockRelease(prevlock);
-       LWLockAcquire(lock, LW_EXCLUSIVE);
-   }
-   (void) ZeroSUBTRANSPage(startPage);
    LWLockRelease(lock);
 }
 

diff --git a/src/test/recovery/t/009_twophase.pl b/src/test/recovery/t/009_twophase.pl
index a3cdcf3240823ceadfe1006bf065137f96074345..701f9cc20f8411dc49e456dcc8dc3075cf78eb24 100644 (file)
--- a/src/test/recovery/t/009_twophase.pl
+++ b/src/test/recovery/t/009_twophase.pl
@@ -45,6 +45,10 @@ $node_london->backup('london_backup');
 my $node_paris = PostgreSQL::Test::Cluster->new('paris');
 $node_paris->init_from_backup($node_london, 'london_backup',
    has_streaming => 1);
+$node_paris->append_conf(
+   'postgresql.conf', qq(
+   subtransaction_buffers = 32
+));
 $node_paris->start;
 
 # Switch to synchronous replication in both directions
@@ -481,4 +485,42 @@ is( $psql_out,
    qq{27|issued to paris},
    "Check expected t_009_tbl2 data on standby");
 
+
+# Exercise the 2PC recovery code in StartupSUBTRANS, which is concerned with
+# ensuring that enough pg_subtrans pages exist on disk to cover the range of
+# prepared transactions at server start time.  There's not much we can verify
+# directly, but let's at least get the code to run.
+$cur_standby->stop();
+configure_and_reload($cur_primary, "synchronous_standby_names = ''");
+
+$cur_primary->safe_psql('postgres', "CHECKPOINT");
+
+my $start_lsn =
+  $cur_primary->safe_psql('postgres', 'select pg_current_wal_insert_lsn()');
+$cur_primary->safe_psql('postgres',
+   "CREATE TABLE test(); BEGIN; CREATE TABLE test1(); PREPARE TRANSACTION 'foo';"
+);
+my $osubtrans = $cur_primary->safe_psql('postgres',
+   "select 'pg_subtrans/'||f, s.size from pg_ls_dir('pg_subtrans') f, pg_stat_file('pg_subtrans/'||f) s"
+);
+$cur_primary->pgbench(
+   '--no-vacuum --client=5 --transactions=1000',
+   0,
+   [],
+   [],
+   'pgbench run to cause pg_subtrans traffic',
+   {
+       '009_twophase.pgb' => 'insert into test default values'
+   });
+# StartupSUBTRANS is exercised with a wide range of visible XIDs in this
+# stop/start sequence, because we left a prepared transaction open above.
+# Also, setting subtransaction_buffers to 32 above causes to switch SLRU
+# bank, for additional code coverage.
+$cur_primary->stop;
+$cur_primary->start;
+my $nsubtrans = $cur_primary->safe_psql('postgres',
+   "select 'pg_subtrans/'||f, s.size from pg_ls_dir('pg_subtrans') f, pg_stat_file('pg_subtrans/'||f) s"
+);
+isnt($osubtrans, $nsubtrans, "contents of pg_subtrans/ have changed");
+
 done_testing();