Skip to content

Commit

Permalink
Do not assign page LSN to new (uninitialized) page in ClearVisibility…
Browse files Browse the repository at this point in the history
…MapFlags redo handler (#9287)

## Problem

https://neondb.slack.com/archives/C04DGM6SMTM/p1727872045252899

See #9240

## Summary of changes

Add `!page_is_new` check before assigning page lsn.

## 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 <[email protected]>
  • Loading branch information
knizhnik and Konstantin Knizhnik authored Nov 1, 2024
1 parent 3c16bd6 commit 8ac523d
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions pageserver/src/walredo/apply_neon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ pub(crate) fn apply_in_neon(
let map = &mut page[pg_constants::MAXALIGN_SIZE_OF_PAGE_HEADER_DATA..];

map[map_byte as usize] &= !(flags << map_offset);
postgres_ffi::page_set_lsn(page, lsn);
// The page should never be empty, but we're checking it anyway as a precaution, so that if it is empty for some reason anyway, we don't make matters worse by setting the LSN on it.
if !postgres_ffi::page_is_new(page) {
postgres_ffi::page_set_lsn(page, lsn);
}
}

// Repeat for 'old_heap_blkno', if any
Expand All @@ -81,7 +84,10 @@ pub(crate) fn apply_in_neon(
let map = &mut page[pg_constants::MAXALIGN_SIZE_OF_PAGE_HEADER_DATA..];

map[map_byte as usize] &= !(flags << map_offset);
postgres_ffi::page_set_lsn(page, lsn);
// The page should never be empty, but we're checking it anyway as a precaution, so that if it is empty for some reason anyway, we don't make matters worse by setting the LSN on it.
if !postgres_ffi::page_is_new(page) {
postgres_ffi::page_set_lsn(page, lsn);
}
}
}
// Non-relational WAL records are handled here, with custom code that has the
Expand Down

1 comment on commit 8ac523d

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5328 tests run: 5105 passed, 1 failed, 222 skipped (full report)


Failures on Postgres 17

# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_timeline_size[debug-pg17]"
Flaky tests (1)

Postgres 17

Test coverage report is not available

The comment gets automatically updated with the latest test results
8ac523d at 2024-11-01T19:17:58.447Z :recycle:

Please sign in to comment.