Skip to content

Commit

Permalink
is_rel_block_key: exclude the relsize key
Browse files Browse the repository at this point in the history
Before this PR, `is_rel_block_key` returns true for the blknum `0xffffffff`,
which is a blknum that's actually never written by Postgres, but used by
Neon Pageserver to store the relsize.

Quoting @MMeent:

> PostgreSQL can't extend the relation beyond size of 0xFFFFFFFF blocks,
> so block number 0xFFFFFFFE is the last valid block number.

This PR changes the definition of the function to exclude blknum 0xffffffff.

My motivation for doing this change is to fix the `pagebench` getpage
benchmark, which uses `is_rel_block_key` to filter the keyspace for
valid pages to request from page_service.
fixes #6210

I checked other users of the function.

The first one is `key_is_shard0`, which already had added an exemption
for 0xffffffff. So, there's no functional change with this PR.

The second one is `DatadirModification::flush`[^1]. With this PR,
`.flush()` will skip the relsize key, whereas it didn't
before. This means we will pile up all the relsize key-value pairs `(Key,u32)`
in `DatadirModification::pending_updates` until `.commit()` is called.

The only place I can think of where that would be a problem is if we
import from a full basebackup, and don't `.commit()` regularly,
like we currently don't do in `import_basebackup_from_tar`.
It exposes us to input-controlled allocations.
However, that was already the case for the other keys that are skipped,
so, one can argue that this change is not making the situation much
worse.

[^1]: That type's `flush()` and `commit()` methods are terribly named, but,
      that's for another time
  • Loading branch information
problame committed Jan 3, 2024
1 parent fb518ae commit 9f8ff6a
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 7 deletions.
2 changes: 1 addition & 1 deletion libs/pageserver_api/src/key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ impl Key {
}

pub fn is_rel_block_key(key: &Key) -> bool {
key.field1 == 0x00 && key.field4 != 0
key.field1 == 0x00 && key.field4 != 0 && key.field6 != 0xffffffff
}

impl std::str::FromStr for Key {
Expand Down
7 changes: 1 addition & 6 deletions libs/pageserver_api/src/shard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -515,12 +515,7 @@ fn key_is_shard0(key: &Key) -> bool {
// relation pages are distributed to shards other than shard zero. Everything else gets
// stored on shard 0. This guarantees that shard 0 can independently serve basebackup
// requests, and any request other than those for particular blocks in relations.
//
// In this condition:
// - is_rel_block_key includes only relations, i.e. excludes SLRU data and
// all metadata.
// - field6 is set to -1 for relation size pages.
!(is_rel_block_key(key) && key.field6 != 0xffffffff)
!is_rel_block_key(key)
}

/// Provide the same result as the function in postgres `hashfn.h` with the same name
Expand Down

0 comments on commit 9f8ff6a

Please sign in to comment.