Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pageserver: guard against WAL gaps in the interpreted protocol #10858

Merged
merged 12 commits into from
Feb 20, 2025

Conversation

VladLazar
Copy link
Contributor

@VladLazar VladLazar commented Feb 17, 2025

Problem

The interpreted SK <-> PS protocol does not guard against gaps (neither does the Vanilla one, but that's beside the point).

Summary of changes

Extend the protocol to include the start LSN of the PG WAL section from which the records were interpreted.
Validation is enabled via a config flag on the pageserver and works as follows:

Case 1: raw_wal_start_lsn is smaller than the requested LSN
There can't be gaps here, but we check that the shard received records which it hasn't seen before.

Case 2: raw_wal_start_lsn is equal to the requested LSN
This is the happy case. No gap and nothing to check

Case 3: raw_wal_start_lsn is greater than the requested LSN
This is a gap.

To make Case 3 work I had to bend the protocol a bit.
We read record chunks of WAL which aren't record aligned and feed them to the decoder.
The picture below shows a shard which subscribes at a position somewhere within Record 2.
We already have a wal reader which is below that position so we wait to catch up.
We read some wal in Read 1 (all of Record 1 and some of Record 2). The new shard doesn't
need Record 1 (it has already processed it according to the starting position), but we read
past it's starting position. When we do Read 2, we decode Record 2 and ship it off to the shard,
but the starting position of Read 2 is greater than the starting position the shard requested.
This looks like a gap.

image

To make it work, we extend the protocol to send an empty InterpretedWalRecords to shards
if the WAL the records originated from ends the requested start position. On the pageserver,
that just updates the tracking LSNs in memory (no-op really). This gives us a workaround for
the fake gap.

As a drive by, make InterpretedWalRecords::next_record_lsn mandatory in the application level definition.
It's always included.

Related: https://github.com/neondatabase/cloud/issues/23935

@VladLazar VladLazar requested a review from a team as a code owner February 17, 2025 17:18
@VladLazar VladLazar requested a review from arssher February 17, 2025 17:18
@VladLazar VladLazar marked this pull request as draft February 17, 2025 17:19
@VladLazar VladLazar removed the request for review from arssher February 17, 2025 17:22
@VladLazar VladLazar changed the title Vlad/ps check wal contiguity pageserver: guard against WAL gaps in the interpreted protocol Feb 17, 2025
@VladLazar VladLazar force-pushed the vlad/ps-check-wal-contiguity branch from d2c1547 to 8497de9 Compare February 17, 2025 17:27
Copy link

github-actions bot commented Feb 17, 2025

7557 tests run: 7184 passed, 0 failed, 373 skipped (full report)


Flaky tests (1)

Postgres 17

Code coverage* (full report)

  • functions: 32.9% (8625 of 26199 functions)
  • lines: 48.9% (72771 of 148957 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
c642939 at 2025-02-20T15:52:13.121Z :recycle:

@VladLazar VladLazar requested review from erikgrinaker and removed request for a team February 20, 2025 10:29
@VladLazar VladLazar marked this pull request as ready for review February 20, 2025 10:29
Add the start LSN of the PG WAL from which the batch of records
originated.

New pageservers can read messages from old safekeepers, the field will
be None.

Old pageserver can read messages from new safekeepers. The field is
ignored.
This field is always included with every batch of interpreted records.
It's an optional field in the proto definition, but it doesn't have to
be optional in the application level definition.
@VladLazar VladLazar force-pushed the vlad/ps-check-wal-contiguity branch from c612bd3 to 123ccac Compare February 20, 2025 10:34
@VladLazar VladLazar force-pushed the vlad/ps-check-wal-contiguity branch from 123ccac to 66e84d0 Compare February 20, 2025 11:43
@VladLazar VladLazar added this pull request to the merge queue Feb 20, 2025
Merged via the queue into main with commit 3499641 Feb 20, 2025
90 checks passed
@VladLazar VladLazar deleted the vlad/ps-check-wal-contiguity branch February 20, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants