From 457e3a3ebc35b35c589532164395b1d98a4fd2a4 Mon Sep 17 00:00:00 2001 From: Konstantin Knizhnik Date: Fri, 21 Jul 2023 21:20:53 +0300 Subject: [PATCH] Mx offset bug (#4775) Fix mx_offset_to_flags_offset() function Fixes issue #4774 Postgres `MXOffsetToFlagsOffset` was not correctly converted to Rust because cast to u16 is done before division by modulo. It is possible only if divider is power of two. Add a small rust unit test to check that the function produces same results as the PostgreSQL macro, and extend the existing python test to cover this bug. Co-authored-by: Konstantin Knizhnik Co-authored-by: Heikki Linnakangas --- libs/postgres_ffi/src/nonrelfile_utils.rs | 44 +++++++++++++++++++++-- test_runner/regress/test_multixact.py | 21 ++++++++--- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/libs/postgres_ffi/src/nonrelfile_utils.rs b/libs/postgres_ffi/src/nonrelfile_utils.rs index 5acf90be70..e3e7133b94 100644 --- a/libs/postgres_ffi/src/nonrelfile_utils.rs +++ b/libs/postgres_ffi/src/nonrelfile_utils.rs @@ -57,9 +57,9 @@ pub fn slru_may_delete_clogsegment(segpage: u32, cutoff_page: u32) -> bool { // Multixact utils pub fn mx_offset_to_flags_offset(xid: MultiXactId) -> usize { - ((xid / pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP as u32) as u16 - % pg_constants::MULTIXACT_MEMBERGROUPS_PER_PAGE - * pg_constants::MULTIXACT_MEMBERGROUP_SIZE) as usize + ((xid / pg_constants::MULTIXACT_MEMBERS_PER_MEMBERGROUP as u32) + % pg_constants::MULTIXACT_MEMBERGROUPS_PER_PAGE as u32 + * pg_constants::MULTIXACT_MEMBERGROUP_SIZE as u32) as usize } pub fn mx_offset_to_flags_bitshift(xid: MultiXactId) -> u16 { @@ -81,3 +81,41 @@ fn mx_offset_to_member_page(xid: u32) -> u32 { pub fn mx_offset_to_member_segment(xid: u32) -> i32 { (mx_offset_to_member_page(xid) / pg_constants::SLRU_PAGES_PER_SEGMENT) as i32 } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_multixid_calc() { + // Check that the mx_offset_* functions produce the same values as the + // corresponding PostgreSQL C macros (MXOffsetTo*). These test values + // were generated by calling the PostgreSQL macros with a little C + // program. + assert_eq!(mx_offset_to_member_segment(0), 0); + assert_eq!(mx_offset_to_member_page(0), 0); + assert_eq!(mx_offset_to_flags_offset(0), 0); + assert_eq!(mx_offset_to_flags_bitshift(0), 0); + assert_eq!(mx_offset_to_member_offset(0), 4); + assert_eq!(mx_offset_to_member_segment(1), 0); + assert_eq!(mx_offset_to_member_page(1), 0); + assert_eq!(mx_offset_to_flags_offset(1), 0); + assert_eq!(mx_offset_to_flags_bitshift(1), 8); + assert_eq!(mx_offset_to_member_offset(1), 8); + assert_eq!(mx_offset_to_member_segment(123456789), 2358); + assert_eq!(mx_offset_to_member_page(123456789), 75462); + assert_eq!(mx_offset_to_flags_offset(123456789), 4780); + assert_eq!(mx_offset_to_flags_bitshift(123456789), 8); + assert_eq!(mx_offset_to_member_offset(123456789), 4788); + assert_eq!(mx_offset_to_member_segment(u32::MAX - 1), 82040); + assert_eq!(mx_offset_to_member_page(u32::MAX - 1), 2625285); + assert_eq!(mx_offset_to_flags_offset(u32::MAX - 1), 5160); + assert_eq!(mx_offset_to_flags_bitshift(u32::MAX - 1), 16); + assert_eq!(mx_offset_to_member_offset(u32::MAX - 1), 5172); + assert_eq!(mx_offset_to_member_segment(u32::MAX), 82040); + assert_eq!(mx_offset_to_member_page(u32::MAX), 2625285); + assert_eq!(mx_offset_to_flags_offset(u32::MAX), 5160); + assert_eq!(mx_offset_to_flags_bitshift(u32::MAX), 24); + assert_eq!(mx_offset_to_member_offset(u32::MAX), 5176); + } +} diff --git a/test_runner/regress/test_multixact.py b/test_runner/regress/test_multixact.py index 78635576f1..9db463dc4a 100644 --- a/test_runner/regress/test_multixact.py +++ b/test_runner/regress/test_multixact.py @@ -8,6 +8,10 @@ from fixtures.utils import query_scalar # Now this test is very minimalistic - # it only checks next_multixact_id field in restored pg_control, # since we don't have functions to check multixact internals. +# We do check that the datadir contents exported from the +# pageserver match what the running PostgreSQL produced. This +# is enough to verify that the WAL records are handled correctly +# in the pageserver. # def test_multixact(neon_simple_env: NeonEnv, test_output_dir): env = neon_simple_env @@ -18,8 +22,8 @@ def test_multixact(neon_simple_env: NeonEnv, test_output_dir): cur = endpoint.connect().cursor() cur.execute( """ - CREATE TABLE t1(i int primary key); - INSERT INTO t1 select * from generate_series(1, 100); + CREATE TABLE t1(i int primary key, n_updated int); + INSERT INTO t1 select g, 0 from generate_series(1, 50) g; """ ) @@ -29,6 +33,7 @@ def test_multixact(neon_simple_env: NeonEnv, test_output_dir): # Lock entries using parallel connections in a round-robin fashion. nclients = 20 + update_every = 97 connections = [] for _ in range(nclients): # Do not turn on autocommit. We want to hold the key-share locks. @@ -36,14 +41,20 @@ def test_multixact(neon_simple_env: NeonEnv, test_output_dir): connections.append(conn) # On each iteration, we commit the previous transaction on a connection, - # and issue antoher select. Each SELECT generates a new multixact that + # and issue another select. Each SELECT generates a new multixact that # includes the new XID, and the XIDs of all the other parallel transactions. # This generates enough traffic on both multixact offsets and members SLRUs # to cross page boundaries. - for i in range(5000): + for i in range(20000): conn = connections[i % nclients] conn.commit() - conn.cursor().execute("select * from t1 for key share") + + # Perform some non-key UPDATEs too, to exercise different multixact + # member statuses. + if i % update_every == 0: + conn.cursor().execute(f"update t1 set n_updated = n_updated + 1 where i = {i % 50}") + else: + conn.cursor().execute("select * from t1 for key share") # We have multixacts now. We can close the connections. for c in connections: