Add tests, refactor the code changes

Add a python test case that fails without the code fix. In a debug
build, it triggers a rust panic in the pageserver, because integer
wraparound is checked in debug builds. In a release build, the
nextMulti wraparound happens to works, even though it performs a
non-wraparound-aware comparison: "xlrec.mid >=
self.checkpoint.nextMulti". That comparison is wrong, but as long as
we see the the multixd creation records for every multi-xid, the
record with `xlrec.mid == 0xffffffff`, it will cause nextMulti to wrap
around to 0, and from there on the comparison works again. Fiddly and
incorrect, but happens to work...

nextMulti is not so lucky, however, because it won't visit every
integer, so it won't usually be called with the exact value of
0xffffffff.

Move the code to perform the comparisons to a separate function,
update_next_multixid(), similar to the update_next_xid() function for
regular XIDs. Add unit tests for it. This made it easier to convince
myself that it's actually doing the right thing.

One notable code change: we no longer skip multixid 0. I confirmed
that vanilla Postgres also sets nextMulti to 0 at wraparound, it's
just skipped in GetNewMultiXactId():

    postgres=# update tt set i = i; checkpoint; select next_multixact_id, next_multi_offset, next_xid from pg_control_checkpoint ();
    UPDATE 1
    CHECKPOINT
     next_multixact_id | next_multi_offset | next_xid
    -------------------+-------------------+----------
            4294967294 |        4294967048 | 0:1012
    (1 row)

    postgres=# update tt set i = i; checkpoint; select next_multixact_id, next_multi_offset, next_xid from pg_control_checkpoint ();
    UPDATE 1
    CHECKPOINT
     next_multixact_id | next_multi_offset | next_xid
    -------------------+-------------------+----------
            4294967295 |        4294967050 | 0:1013
    (1 row)

    postgres=# update tt set i = i; checkpoint; select next_multixact_id, next_multi_offset, next_xid from pg_control_checkpoint ();
    UPDATE 1
    CHECKPOINT
     next_multixact_id | next_multi_offset | next_xid
    -------------------+-------------------+----------
                     0 |        4294967052 | 0:1014
    (1 row)

    postgres=# update tt set i = i; checkpoint; select next_multixact_id, next_multi_offset, next_xid from pg_control_checkpoint ();
    UPDATE 1
    CHECKPOINT
     next_multixact_id | next_multi_offset | next_xid
    -------------------+-------------------+----------
                     2 |        4294967054 | 0:1015
    (1 row)

libs/postgres_ffi/src/xlog_utils.rs

@@ -356,6 +356,28 @@ impl CheckPoint {
         }
         false
     }
+
+    /// Advance next multi-XID/offset to those given in arguments.
+    ///
+    /// It's important that this handles wraparound correctly. This should match the
+    /// MultiXactAdvanceNextMXact() logic in PostgreSQL's xlog_redo() function.
+    ///
+    /// Returns 'true' if the Checkpoint was updated.
+    pub fn update_next_multixid(&mut self, multi_xid: u32, multi_offset: u32) -> bool {
+        let mut modified = false;
+
+        if multi_xid.wrapping_sub(self.nextMulti) as i32 > 0 {
+            self.nextMulti = multi_xid;
+            modified = true;
+        }
+
+        if multi_offset.wrapping_sub(self.nextMultiOffset) as i32 > 0 {
+            self.nextMultiOffset = multi_offset;
+            modified = true;
+        }
+
+        modified
+    }
 }
 
 /// Generate new, empty WAL segment, with correct block headers at the first

libs/postgres_ffi/wal_craft/src/xlog_utils_test.rs

@@ -202,6 +202,53 @@ pub fn test_update_next_xid() {
     assert_eq!(checkpoint.nextXid.value, 2048);
 }
 
+#[test]
+pub fn test_update_next_multixid() {
+    let checkpoint_buf = [0u8; std::mem::size_of::<CheckPoint>()];
+    let mut checkpoint = CheckPoint::decode(&checkpoint_buf).unwrap();
+
+    // simple case
+    checkpoint.nextMulti = 20;
+    checkpoint.nextMultiOffset = 20;
+    checkpoint.update_next_multixid(1000, 2000);
+    assert_eq!(checkpoint.nextMulti, 1000);
+    assert_eq!(checkpoint.nextMultiOffset, 2000);
+
+    // No change
+    checkpoint.update_next_multixid(500, 900);
+    assert_eq!(checkpoint.nextMulti, 1000);
+    assert_eq!(checkpoint.nextMultiOffset, 2000);
+
+    // Close to wraparound, but not wrapped around yet
+    checkpoint.nextMulti = 0xffff0000;
+    checkpoint.nextMultiOffset = 0xfffe0000;
+    checkpoint.update_next_multixid(0xffff00ff, 0xfffe00ff);
+    assert_eq!(checkpoint.nextMulti, 0xffff00ff);
+    assert_eq!(checkpoint.nextMultiOffset, 0xfffe00ff);
+
+    // Wraparound
+    checkpoint.update_next_multixid(1, 900);
+    assert_eq!(checkpoint.nextMulti, 1);
+    assert_eq!(checkpoint.nextMultiOffset, 900);
+
+    // Wraparound nextMulti to 0.
+    //
+    // It's a bit surprising that nextMulti can be 0, because that's a special value
+    // (InvalidMultiXactId). However, that's how Postgres does it at multi-xid wraparound:
+    // nextMulti wraps around to 0, but then when the next multi-xid is assigned, it skips
+    // the 0 and the next multi-xid actually assigned is 1.
+    checkpoint.nextMulti = 0xffff0000;
+    checkpoint.nextMultiOffset = 0xfffe0000;
+    checkpoint.update_next_multixid(0, 0xfffe00ff);
+    assert_eq!(checkpoint.nextMulti, 0);
+    assert_eq!(checkpoint.nextMultiOffset, 0xfffe00ff);
+
+    // Wraparound nextMultiOffset to 0
+    checkpoint.update_next_multixid(0, 0);
+    assert_eq!(checkpoint.nextMulti, 0);
+    assert_eq!(checkpoint.nextMultiOffset, 0);
+}
+
 #[test]
 pub fn test_encode_logical_message() {
     let expected = [

pageserver/src/walingest.rs

@@ -1384,21 +1384,28 @@ impl WalIngest {
             // Note: The multixact members can wrap around, even within one WAL record.
             offset = offset.wrapping_add(n_this_page as u32);
         }
-        // Advance `nextMulti` in wraparound-aware way. This should match the MultiXactAdvanceNextMXact()
-        // logic in PostgreSQL's xlog_redo() function.
-        if xlrec.mid.wrapping_sub(self.checkpoint.nextMulti) as i32 >= 0 {
-            let next_mid =
-                std::cmp::max(xlrec.mid.wrapping_add(1), pg_constants::FIRST_MULTIXACT_ID);
-            self.checkpoint.nextMulti = next_mid;
-            self.checkpoint_modified = true;
-        }
-        if (xlrec.moff.wrapping_add(xlrec.nmembers)).wrapping_sub(self.checkpoint.nextMultiOffset)
-            as i32
-            >= 0
-        {
-            self.checkpoint.nextMultiOffset = xlrec.moff.wrapping_add(xlrec.nmembers);
+        let next_offset = offset;
+        assert!(xlrec.moff.wrapping_add(xlrec.nmembers) == next_offset);
+
+        // Update next-multi-xid and next-offset
+        //
+        // NB: In PostgreSQL, the next-multi-xid stored in the control file is allowed to
+        // go to 0, and it's fixed up by skipping to FirstMultiXactId in functions that
+        // read it, like GetNewMultiXactId(). This is different from how nextXid is
+        // incremented! nextXid skips over < FirstNormalTransactionId when the the value
+        // is stored, so it's never 0 in a checkpoint.
+        //
+        // I don't know why it's done that way, it seems less error-prone to skip over 0
+        // over 0 when the value is stored rather than when it's read. But let's do it the
+        // same way here.
+        let next_multi_xid = xlrec.mid.wrapping_add(1);
+
+        if self.checkpoint.update_next_multixid(next_multi_xid, next_offset) {
             self.checkpoint_modified = true;
         }
+
+        // Also update the next-xid with the highest member. According to the comments in
+        // multixact_redo(), this shouldn't be necessary, but let's do the same here.
         let max_mbr_xid = xlrec.members.iter().fold(None, |acc, mbr| {
             if let Some(max_xid) = acc {
                 if mbr.xid.wrapping_sub(max_xid) as i32 > 0 {