Skip to content

Commit

Permalink
wal_decoder: fix compact key protobuf encoding (#10074)
Browse files Browse the repository at this point in the history
## Problem

Protobuf doesn't support 128 bit integers, so we encode the keys as two
64 bit integers. Issue is that when we split the 128 bit compact key we
use signed 64 bit integers to represent the two halves. This may result
in a negative lower half when relnode is larger than `0x00800000`. When
we convert the lower half to an i128 we get a negative `CompactKey`.

## Summary of Changes

Use unsigned integers when encoding into Protobuf.

## Deployment

* Prod: We disabled the interpreted proto, so no compat concerns.
* Staging: Disable the interpreted proto, do one release, and then
release the fixed version.
We do this because a negative int32 will convert to a large uint32 value
and could give
a key in the actual pageserver space. In production we would around this
by adding new
fields to the proto and deprecating the old ones, but we can make our
lives easy here.
* Pre-prod: Same as staging
  • Loading branch information
VladLazar authored Dec 11, 2024
1 parent d7aeca2 commit 665369c
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 5 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 @@ -24,7 +24,7 @@ pub struct Key {

/// When working with large numbers of Keys in-memory, it is more efficient to handle them as i128 than as
/// a struct of fields.
#[derive(Clone, Copy, Hash, PartialEq, Eq, Ord, PartialOrd, Serialize, Deserialize)]
#[derive(Clone, Copy, Hash, PartialEq, Eq, Ord, PartialOrd, Serialize, Deserialize, Debug)]
pub struct CompactKey(i128);

/// The storage key size.
Expand Down
4 changes: 2 additions & 2 deletions libs/wal_decoder/proto/interpreted_wal.proto
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ message ValueMeta {
}

message CompactKey {
int64 high = 1;
int64 low = 2;
uint64 high = 1;
uint64 low = 2;
}

65 changes: 63 additions & 2 deletions libs/wal_decoder/src/wire_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,8 @@ impl From<ValueMeta> for proto::ValueMeta {
impl From<CompactKey> for proto::CompactKey {
fn from(value: CompactKey) -> Self {
proto::CompactKey {
high: (value.raw() >> 64) as i64,
low: value.raw() as i64,
high: (value.raw() >> 64) as u64,
low: value.raw() as u64,
}
}
}
Expand Down Expand Up @@ -354,3 +354,64 @@ impl From<proto::CompactKey> for CompactKey {
(((value.high as i128) << 64) | (value.low as i128)).into()
}
}

#[test]
fn test_compact_key_with_large_relnode() {
use pageserver_api::key::Key;

let inputs = vec![
Key {
field1: 0,
field2: 0x100,
field3: 0x200,
field4: 0,
field5: 0x10,
field6: 0x5,
},
Key {
field1: 0,
field2: 0x100,
field3: 0x200,
field4: 0x007FFFFF,
field5: 0x10,
field6: 0x5,
},
Key {
field1: 0,
field2: 0x100,
field3: 0x200,
field4: 0x00800000,
field5: 0x10,
field6: 0x5,
},
Key {
field1: 0,
field2: 0x100,
field3: 0x200,
field4: 0x00800001,
field5: 0x10,
field6: 0x5,
},
Key {
field1: 0,
field2: 0xFFFFFFFF,
field3: 0xFFFFFFFF,
field4: 0xFFFFFFFF,
field5: 0x0,
field6: 0x0,
},
];

for input in inputs {
assert!(input.is_valid_key_on_write_path());
let compact = input.to_compact();
let proto: proto::CompactKey = compact.into();
let from_proto: CompactKey = proto.into();

assert_eq!(
compact, from_proto,
"Round trip failed for key with relnode={:#x}",
input.field4
);
}
}

1 comment on commit 665369c

@github-actions
Copy link

Choose a reason for hiding this comment

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

7223 tests run: 6898 passed, 0 failed, 325 skipped (full report)


Flaky tests (8)

Postgres 17

Postgres 16

  • test_pgdata_import_smoke[8-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64
  • test_pgdata_import_smoke[None-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64
  • test_subscriber_synchronous_commit: release-arm64

Postgres 14

  • test_pgdata_import_smoke[8-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64
  • test_pgdata_import_smoke[None-1024-RelBlockSize.MULTIPLE_RELATION_SEGMENTS]: release-arm64

Code coverage* (full report)

  • functions: 31.4% (8341 of 26538 functions)
  • lines: 47.7% (65703 of 137635 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
665369c at 2024-12-11T15:24:12.856Z :recycle:

Please sign in to comment.