From 373c7057cc73fd13f2873c353ced250ddb900700 Mon Sep 17 00:00:00 2001 From: Em Sharnoff Date: Thu, 14 Sep 2023 03:21:50 -0700 Subject: [PATCH 1/3] vm-monitor: Fix cgroup throttling (#5303) I believe this (not actual IO problems) is the cause of the "disk speed issue" that we've had for VMs recently. See e.g.: 1. https://neondb.slack.com/archives/C03H1K0PGKH/p1694287808046179?thread_ts=1694271790.580099&cid=C03H1K0PGKH 2. https://neondb.slack.com/archives/C03H1K0PGKH/p1694511932560659 The vm-informant (and now, the vm-monitor, its replacement) is supposed to gradually increase the `neon-postgres` cgroup's memory.high value, because otherwise the kernel will throttle all the processes in the cgroup. This PR fixes a bug with the vm-monitor's implementation of this behavior. --- Other references, for the vm-informant's implementation: - Original issue: neondatabase/autoscaling#44 - Original PR: neondatabase/autoscaling#223 --- libs/vm_monitor/src/cgroup.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/libs/vm_monitor/src/cgroup.rs b/libs/vm_monitor/src/cgroup.rs index 4f529d16fd..fe1c514f76 100644 --- a/libs/vm_monitor/src/cgroup.rs +++ b/libs/vm_monitor/src/cgroup.rs @@ -315,12 +315,8 @@ impl CgroupWatcher { where E: Stream>, { - // There are several actions might do when receiving a `memory.high`, - // such as freezing the cgroup, or increasing its `memory.high`. We don't - // want to do these things too often (because postgres needs to run, and - // we only have so much memory). These timers serve as rate limits for this. let mut wait_to_freeze = pin!(tokio::time::sleep(Duration::ZERO)); - let mut wait_to_increase_memory_high = pin!(tokio::time::sleep(Duration::ZERO)); + let mut last_memory_high_increase_at: Option = None; let mut events = pin!(events); // Are we waiting to be upscaled? Could be true if we request upscale due @@ -332,6 +328,8 @@ impl CgroupWatcher { upscale = upscales.recv() => { let Sequenced { seqnum, data } = upscale .context("failed to listen on upscale notification channel")?; + waiting_on_upscale = false; + last_memory_high_increase_at = None; self.last_upscale_seqnum.store(seqnum, Ordering::Release); info!(cpu = data.cpu, mem_bytes = data.mem, "received upscale"); } @@ -396,12 +394,17 @@ impl CgroupWatcher { .send(()) .await .context("failed to request upscale")?; + waiting_on_upscale = true; continue; } // Shoot, we can't freeze or and we're still waiting on upscale, // increase memory.high to reduce throttling - if wait_to_increase_memory_high.is_elapsed() { + let can_increase_memory_high = match last_memory_high_increase_at { + None => true, + Some(t) => t.elapsed() > self.config.memory_high_increase_every, + }; + if can_increase_memory_high { info!( "received memory.high event, \ but too soon to refreeze and already requested upscale \ @@ -437,12 +440,11 @@ impl CgroupWatcher { ); self.set_high_bytes(new_high) .context("failed to set memory.high")?; - wait_to_increase_memory_high - .as_mut() - .reset(Instant::now() + self.config.memory_high_increase_every) + last_memory_high_increase_at = Some(Instant::now()); + continue; } - // we can't do anything + info!("received memory.high event, but can't do anything"); } }; } From 6686ede30f4e5271c16616d6ca08a17ab307e9c8 Mon Sep 17 00:00:00 2001 From: Alexander Bayandin Date: Thu, 14 Sep 2023 16:17:50 +0100 Subject: [PATCH 2/3] Update checksum for pg_hint_plan (#5309) ## Problem The checksum for `pg_hint_plan` doesn't match: ``` sha256sum: WARNING: 1 computed checksum did NOT match ``` Ref https://github.com/neondatabase/neon/actions/runs/6185715461/job/16793609251?pr=5307 It seems that the release was retagged yesterday: https://github.com/ossc-db/pg_hint_plan/releases/tag/REL16_1_6_0 I don't see any malicious changes from 15_1.5.1: https://github.com/ossc-db/pg_hint_plan/compare/REL15_1_5_1...REL16_1_6_0, so it should be ok to update. ## Summary of changes - Update checksum for `pg_hint_plan` 16_1.6.0 --- Dockerfile.compute-node | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile.compute-node b/Dockerfile.compute-node index ecf76f97c2..d724185d5d 100644 --- a/Dockerfile.compute-node +++ b/Dockerfile.compute-node @@ -416,7 +416,7 @@ RUN case "${PG_VERSION}" in \ ;; \ "v16") \ export PG_HINT_PLAN_VERSION=16_1_6_0 \ - export PG_HINT_PLAN_CHECKSUM=ce6a8040c78012000f5da7240caf6a971401412f41d33f930f09291e6c304b99 \ + export PG_HINT_PLAN_CHECKSUM=fc85a9212e7d2819d4ae4ac75817481101833c3cfa9f0fe1f980984e12347d00 \ ;; \ *) \ echo "Export the valid PG_HINT_PLAN_VERSION variable" && exit 1 \ From a5987eebfdf1ea3cc39d9730ed9f1f092e0ebf0e Mon Sep 17 00:00:00 2001 From: Konstantin Knizhnik Date: Fri, 15 Sep 2023 10:32:25 +0300 Subject: [PATCH 3/3] References to old and new blocks were mixed in xlog_heap_update handler (#5312) ## Problem See https://neondb.slack.com/archives/C05L7D1JAUS/p1694614585955029 https://www.notion.so/neondatabase/Duplicate-key-issue-651627ce843c45188fbdcb2d30fd2178 ## Summary of changes Swap old/new block references ## Checklist before requesting a review - [ ] I have performed a self-review of my code. - [ ] If it is a core feature, I have added thorough tests. - [ ] Do we need to implement analytics? if so did you add the relevant metrics to the dashboard? - [ ] If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section. ## Checklist before merging - [ ] Do not forget to reformat commit message to not include the above checklist --------- Co-authored-by: Konstantin Knizhnik Co-authored-by: Heikki Linnakangas --- pageserver/src/walingest.rs | 16 +++++----- test_runner/regress/test_vm_bits.py | 48 +++++++++++++++++++++++------ 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/pageserver/src/walingest.rs b/pageserver/src/walingest.rs index 9192af0ee8..d5ef1f2a24 100644 --- a/pageserver/src/walingest.rs +++ b/pageserver/src/walingest.rs @@ -470,14 +470,14 @@ impl<'a> WalIngest<'a> { // we can't validate the remaining number of bytes without parsing // the tuple data. if (xlrec.flags & pg_constants::XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) != 0 { - old_heap_blkno = Some(decoded.blocks[0].blkno); + old_heap_blkno = Some(decoded.blocks.last().unwrap().blkno); } if (xlrec.flags & pg_constants::XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) != 0 { // PostgreSQL only uses XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED on a // non-HOT update where the new tuple goes to different page than // the old one. Otherwise, only XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED is // set. - new_heap_blkno = Some(decoded.blocks[1].blkno); + new_heap_blkno = Some(decoded.blocks[0].blkno); } } } else if decoded.xl_rmid == pg_constants::RM_HEAP2_ID { @@ -526,14 +526,14 @@ impl<'a> WalIngest<'a> { // we can't validate the remaining number of bytes without parsing // the tuple data. if (xlrec.flags & pg_constants::XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) != 0 { - old_heap_blkno = Some(decoded.blocks[0].blkno); + old_heap_blkno = Some(decoded.blocks.last().unwrap().blkno); } if (xlrec.flags & pg_constants::XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) != 0 { // PostgreSQL only uses XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED on a // non-HOT update where the new tuple goes to different page than // the old one. Otherwise, only XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED is // set. - new_heap_blkno = Some(decoded.blocks[1].blkno); + new_heap_blkno = Some(decoded.blocks[0].blkno); } } } else if decoded.xl_rmid == pg_constants::RM_HEAP2_ID { @@ -582,14 +582,14 @@ impl<'a> WalIngest<'a> { // we can't validate the remaining number of bytes without parsing // the tuple data. if (xlrec.flags & pg_constants::XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) != 0 { - old_heap_blkno = Some(decoded.blocks[0].blkno); + old_heap_blkno = Some(decoded.blocks.last().unwrap().blkno); } if (xlrec.flags & pg_constants::XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) != 0 { // PostgreSQL only uses XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED on a // non-HOT update where the new tuple goes to different page than // the old one. Otherwise, only XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED is // set. - new_heap_blkno = Some(decoded.blocks[1].blkno); + new_heap_blkno = Some(decoded.blocks[0].blkno); } } } else if decoded.xl_rmid == pg_constants::RM_HEAP2_ID { @@ -745,14 +745,14 @@ impl<'a> WalIngest<'a> { // we can't validate the remaining number of bytes without parsing // the tuple data. if (xlrec.flags & pg_constants::XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) != 0 { - old_heap_blkno = Some(decoded.blocks[0].blkno); + old_heap_blkno = Some(decoded.blocks.last().unwrap().blkno); } if (xlrec.flags & pg_constants::XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) != 0 { // PostgreSQL only uses XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED on a // non-HOT update where the new tuple goes to different page than // the old one. Otherwise, only XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED is // set. - new_heap_blkno = Some(decoded.blocks[1].blkno); + new_heap_blkno = Some(decoded.blocks[0].blkno); } } pg_constants::XLOG_NEON_HEAP_MULTI_INSERT => { diff --git a/test_runner/regress/test_vm_bits.py b/test_runner/regress/test_vm_bits.py index d8034b31b0..8e20a44407 100644 --- a/test_runner/regress/test_vm_bits.py +++ b/test_runner/regress/test_vm_bits.py @@ -19,18 +19,40 @@ def test_vm_bit_clear(neon_simple_env: NeonEnv): # Install extension containing function needed for test cur.execute("CREATE EXTENSION neon_test_utils") - # Create a test table and freeze it to set the VM bit. + # Create a test table for a few different scenarios and freeze it to set the VM bits. cur.execute("CREATE TABLE vmtest_delete (id integer PRIMARY KEY)") cur.execute("INSERT INTO vmtest_delete VALUES (1)") cur.execute("VACUUM FREEZE vmtest_delete") - cur.execute("CREATE TABLE vmtest_update (id integer PRIMARY KEY)") - cur.execute("INSERT INTO vmtest_update SELECT g FROM generate_series(1, 1000) g") - cur.execute("VACUUM FREEZE vmtest_update") + cur.execute("CREATE TABLE vmtest_hot_update (id integer PRIMARY KEY, filler text)") + cur.execute("INSERT INTO vmtest_hot_update VALUES (1, 'x')") + cur.execute("VACUUM FREEZE vmtest_hot_update") + + cur.execute("CREATE TABLE vmtest_cold_update (id integer PRIMARY KEY)") + cur.execute("INSERT INTO vmtest_cold_update SELECT g FROM generate_series(1, 1000) g") + cur.execute("VACUUM FREEZE vmtest_cold_update") + + cur.execute( + "CREATE TABLE vmtest_cold_update2 (id integer PRIMARY KEY, filler text) WITH (fillfactor=100)" + ) + cur.execute("INSERT INTO vmtest_cold_update2 SELECT g, '' FROM generate_series(1, 1000) g") + cur.execute("VACUUM FREEZE vmtest_cold_update2") # DELETE and UPDATE the rows. cur.execute("DELETE FROM vmtest_delete WHERE id = 1") - cur.execute("UPDATE vmtest_update SET id = 5000 WHERE id = 1") + cur.execute("UPDATE vmtest_hot_update SET filler='x' WHERE id = 1") + cur.execute("UPDATE vmtest_cold_update SET id = 5000 WHERE id = 1") + + # Clear the VM bit on the last page with an INSERT. Then clear the VM bit on + # the page where row 1 is (block 0), by doing an UPDATE. The UPDATE is a + # cold update, and the new tuple goes to the last page, which already had + # its VM bit cleared. The point is that the UPDATE *only* clears the VM bit + # on the page containing the old tuple. We had a bug where we got the old + # and new pages mixed up, and that only shows up when one of the bits is + # cleared, but not the other one. + cur.execute("INSERT INTO vmtest_cold_update2 VALUES (9999, 'x')") + # Clears the VM bit on the old page + cur.execute("UPDATE vmtest_cold_update2 SET id = 5000, filler=repeat('x', 200) WHERE id = 1") # Branch at this point, to test that later fork_at_current_lsn(env, endpoint, "test_vm_bit_clear_new", "test_vm_bit_clear") @@ -50,9 +72,13 @@ def test_vm_bit_clear(neon_simple_env: NeonEnv): """ ) - cur.execute("SELECT * FROM vmtest_delete WHERE id = 1") + cur.execute("SELECT id FROM vmtest_delete WHERE id = 1") assert cur.fetchall() == [] - cur.execute("SELECT * FROM vmtest_update WHERE id = 1") + cur.execute("SELECT id FROM vmtest_hot_update WHERE id = 1") + assert cur.fetchall() == [(1,)] + cur.execute("SELECT id FROM vmtest_cold_update WHERE id = 1") + assert cur.fetchall() == [] + cur.execute("SELECT id FROM vmtest_cold_update2 WHERE id = 1") assert cur.fetchall() == [] cur.close() @@ -77,7 +103,11 @@ def test_vm_bit_clear(neon_simple_env: NeonEnv): """ ) - cur_new.execute("SELECT * FROM vmtest_delete WHERE id = 1") + cur_new.execute("SELECT id FROM vmtest_delete WHERE id = 1") assert cur_new.fetchall() == [] - cur_new.execute("SELECT * FROM vmtest_update WHERE id = 1") + cur_new.execute("SELECT id FROM vmtest_hot_update WHERE id = 1") + assert cur_new.fetchall() == [(1,)] + cur_new.execute("SELECT id FROM vmtest_cold_update WHERE id = 1") + assert cur_new.fetchall() == [] + cur_new.execute("SELECT id FROM vmtest_cold_update2 WHERE id = 1") assert cur_new.fetchall() == []