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();