From 2c10224c9afb2a6d3b26baf3b03742a1d564e083 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki@zenith.tech>
Date: Thu, 2 Sep 2021 22:01:46 +0300
Subject: [PATCH] Partial fix for issue with extending relation with a gap.

This should fix the sporadic regression test failures we've been seeing
lately with "no base img found" errors.

This fixes the common case, but one corner case is still not handled:
If a relation is extended across a segment boundary, leaving a gap block
in the segment preceding the segment containing the target block, the
preceding segment will not be padded with zeros correctly. This adds
a test case for that, but it's commented out.

See github issue https://github.com/zenithdb/zenith/issues/500
---
 .../src/layered_repository/inmemory_layer.rs  | 31 +++++++++++++++++++
 pageserver/src/repository.rs                  | 31 +++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/pageserver/src/layered_repository/inmemory_layer.rs b/pageserver/src/layered_repository/inmemory_layer.rs
index e1082748fd..476b53ffc4 100644
--- a/pageserver/src/layered_repository/inmemory_layer.rs
+++ b/pageserver/src/layered_repository/inmemory_layer.rs
@@ -7,6 +7,7 @@ use crate::layered_repository::storage_layer::{
     Layer, PageReconstructData, PageReconstructResult, PageVersion, SegmentTag, RELISH_SEG_SIZE,
 };
 use crate::layered_repository::LayeredTimeline;
+use crate::layered_repository::ZERO_PAGE;
 use crate::layered_repository::{DeltaLayer, ImageLayer};
 use crate::repository::WALRecord;
 use crate::PageServerConf;
@@ -364,6 +365,36 @@ impl InMemoryLayer {
                     newsize,
                     lsn
                 );
+
+                // If we are extending the relation by more than one page, initialize the "gap"
+                // with zeros
+                //
+                // XXX: What if the caller initializes the gap with subsequent call with same LSN?
+                // I don't think that can happen currently, but that is highly dependent on how
+                // PostgreSQL writes its WAL records and there's no guarantee of it. If it does
+                // happen, we would hit the "page version already exists" warning above on the
+                // subsequent call to initialize the gap page.
+                let gapstart = self.seg.segno * RELISH_SEG_SIZE + oldsize;
+                for gapblknum in gapstart..blknum {
+                    let zeropv = PageVersion {
+                        page_image: Some(ZERO_PAGE.clone()),
+                        record: None,
+                    };
+                    println!(
+                        "filling gap blk {} with zeros for write of {}",
+                        gapblknum, blknum
+                    );
+                    let old = inner.page_versions.insert((gapblknum, lsn), zeropv);
+                    // We already had an entry for this LSN. That's odd..
+
+                    if old.is_some() {
+                        warn!(
+                            "Page version of rel {} blk {} at {} already exists",
+                            self.seg.rel, blknum, lsn
+                        );
+                    }
+                }
+
                 inner.segsizes.insert(lsn, newsize);
             }
         }
diff --git a/pageserver/src/repository.rs b/pageserver/src/repository.rs
index 8ef8c5d9d0..4634f1a192 100644
--- a/pageserver/src/repository.rs
+++ b/pageserver/src/repository.rs
@@ -358,6 +358,37 @@ mod tests {
             TEST_IMG("foo blk 2 at 5")
         );
 
+        // Truncate to zero length
+        tline.put_truncation(TESTREL_A, Lsn(0x60), 0)?;
+        tline.advance_last_record_lsn(Lsn(0x60));
+        assert_eq!(tline.get_relish_size(TESTREL_A, Lsn(0x60))?.unwrap(), 0);
+
+        // Extend from 0 to 2 blocks, leaving a gap
+        tline.put_page_image(TESTREL_A, 1, Lsn(0x70), TEST_IMG("foo blk 1"))?;
+        tline.advance_last_record_lsn(Lsn(0x70));
+        assert_eq!(tline.get_relish_size(TESTREL_A, Lsn(0x70))?.unwrap(), 2);
+        assert_eq!(tline.get_page_at_lsn(TESTREL_A, 0, Lsn(0x70))?, ZERO_PAGE);
+        assert_eq!(
+            tline.get_page_at_lsn(TESTREL_A, 1, Lsn(0x70))?,
+            TEST_IMG("foo blk 1")
+        );
+
+        // Extend a lot more, leaving a big gap that spans across segments
+        // FIXME: This is currently broken, see https://github.com/zenithdb/zenith/issues/500
+        /*
+        tline.put_page_image(TESTREL_A, 1500, Lsn(0x80), TEST_IMG("foo blk 1500"))?;
+        tline.advance_last_record_lsn(Lsn(0x80));
+        assert_eq!(tline.get_relish_size(TESTREL_A, Lsn(0x80))?.unwrap(), 1501);
+        for blk in 2..1500 {
+            assert_eq!(
+                tline.get_page_at_lsn(TESTREL_A, blk, Lsn(0x80))?,
+                ZERO_PAGE);
+        }
+        assert_eq!(
+            tline.get_page_at_lsn(TESTREL_A, 1500, Lsn(0x80))?,
+            TEST_IMG("foo blk 1500"));
+         */
+
         Ok(())
     }