From 2d10306f7a596d1f2ad8c70d01b427600a29c622 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Tue, 27 Aug 2024 18:36:33 +0300 Subject: [PATCH] Remove support for pageserver <-> compute protocol version 1 (#8774) Protocol version 2 has been the default for a while now, and we no longer have any computes running in production that used protocol version 1. This completes the migration by removing support for v1 in both the pageserver and the compute. See issue #6211. --- libs/pageserver_api/src/models.rs | 41 ++++-------------- pageserver/src/metrics.rs | 1 - pageserver/src/page_service.rs | 34 +-------------- pgxn/neon/libpagestore.c | 5 +-- pgxn/neon/pagestore_client.h | 5 +-- pgxn/neon/pagestore_smgr.c | 47 ++------------------- test_runner/regress/test_auth.py | 2 +- test_runner/regress/test_read_validation.py | 10 ----- 8 files changed, 17 insertions(+), 128 deletions(-) diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 4cab56771b8e..d39ac7570772 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -1063,7 +1063,7 @@ impl TryFrom for PagestreamBeMessageTag { } } -// In the V2 protocol version, a GetPage request contains two LSN values: +// A GetPage request contains two LSN values: // // request_lsn: Get the page version at this point in time. Lsn::Max is a special value that means // "get the latest version present". It's used by the primary server, which knows that no one else @@ -1076,7 +1076,7 @@ impl TryFrom for PagestreamBeMessageTag { // passing an earlier LSN can speed up the request, by allowing the pageserver to process the // request without waiting for 'request_lsn' to arrive. // -// The legacy V1 interface contained only one LSN, and a boolean 'latest' flag. The V1 interface was +// The now-defunct V1 interface contained only one LSN, and a boolean 'latest' flag. The V1 interface was // sufficient for the primary; the 'lsn' was equivalent to the 'not_modified_since' value, and // 'latest' was set to true. The V2 interface was added because there was no correct way for a // standby to request a page at a particular non-latest LSN, and also include the @@ -1084,15 +1084,11 @@ impl TryFrom for PagestreamBeMessageTag { // request, if the standby knows that the page hasn't been modified since, and risk getting an error // if that LSN has fallen behind the GC horizon, or requesting the current replay LSN, which could // require the pageserver unnecessarily to wait for the WAL to arrive up to that point. The new V2 -// interface allows sending both LSNs, and let the pageserver do the right thing. There is no +// interface allows sending both LSNs, and let the pageserver do the right thing. There was no // difference in the responses between V1 and V2. // -// The Request structs below reflect the V2 interface. If V1 is used, the parse function -// maps the old format requests to the new format. -// #[derive(Clone, Copy)] pub enum PagestreamProtocolVersion { - V1, V2, } @@ -1231,36 +1227,17 @@ impl PagestreamFeMessage { bytes.into() } - pub fn parse( - body: &mut R, - protocol_version: PagestreamProtocolVersion, - ) -> anyhow::Result { + pub fn parse(body: &mut R) -> anyhow::Result { // these correspond to the NeonMessageTag enum in pagestore_client.h // // TODO: consider using protobuf or serde bincode for less error prone // serialization. let msg_tag = body.read_u8()?; - let (request_lsn, not_modified_since) = match protocol_version { - PagestreamProtocolVersion::V2 => ( - Lsn::from(body.read_u64::()?), - Lsn::from(body.read_u64::()?), - ), - PagestreamProtocolVersion::V1 => { - // In the old protocol, each message starts with a boolean 'latest' flag, - // followed by 'lsn'. Convert that to the two LSNs, 'request_lsn' and - // 'not_modified_since', used in the new protocol version. - let latest = body.read_u8()? != 0; - let request_lsn = Lsn::from(body.read_u64::()?); - if latest { - (Lsn::MAX, request_lsn) // get latest version - } else { - (request_lsn, request_lsn) // get version at specified LSN - } - } - }; + // these two fields are the same for every request type + let request_lsn = Lsn::from(body.read_u64::()?); + let not_modified_since = Lsn::from(body.read_u64::()?); - // The rest of the messages are the same between V1 and V2 match msg_tag { 0 => Ok(PagestreamFeMessage::Exists(PagestreamExistsRequest { request_lsn, @@ -1468,9 +1445,7 @@ mod tests { ]; for msg in messages { let bytes = msg.serialize(); - let reconstructed = - PagestreamFeMessage::parse(&mut bytes.reader(), PagestreamProtocolVersion::V2) - .unwrap(); + let reconstructed = PagestreamFeMessage::parse(&mut bytes.reader()).unwrap(); assert!(msg == reconstructed); } } diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index 1f8634df939e..c4011d593c93 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -1552,7 +1552,6 @@ pub(crate) static LIVE_CONNECTIONS: Lazy = Lazy::new(|| { #[derive(Clone, Copy, enum_map::Enum, IntoStaticStr)] pub(crate) enum ComputeCommandKind { PageStreamV2, - PageStream, Basebackup, Fullbackup, LeaseLsn, diff --git a/pageserver/src/page_service.rs b/pageserver/src/page_service.rs index 81294291a93f..cb1ab70147ab 100644 --- a/pageserver/src/page_service.rs +++ b/pageserver/src/page_service.rs @@ -557,7 +557,7 @@ impl PageServerHandler { pgb: &mut PostgresBackend, tenant_id: TenantId, timeline_id: TimelineId, - protocol_version: PagestreamProtocolVersion, + _protocol_version: PagestreamProtocolVersion, ctx: RequestContext, ) -> Result<(), QueryError> where @@ -601,8 +601,7 @@ impl PageServerHandler { fail::fail_point!("ps::handle-pagerequest-message"); // parse request - let neon_fe_msg = - PagestreamFeMessage::parse(&mut copy_data_bytes.reader(), protocol_version)?; + let neon_fe_msg = PagestreamFeMessage::parse(&mut copy_data_bytes.reader())?; // invoke handler function let (handler_result, span) = match neon_fe_msg { @@ -1275,35 +1274,6 @@ where ctx, ) .await?; - } else if let Some(params) = parts.strip_prefix(&["pagestream"]) { - if params.len() != 2 { - return Err(QueryError::Other(anyhow::anyhow!( - "invalid param number for pagestream command" - ))); - } - let tenant_id = TenantId::from_str(params[0]) - .with_context(|| format!("Failed to parse tenant id from {}", params[0]))?; - let timeline_id = TimelineId::from_str(params[1]) - .with_context(|| format!("Failed to parse timeline id from {}", params[1]))?; - - tracing::Span::current() - .record("tenant_id", field::display(tenant_id)) - .record("timeline_id", field::display(timeline_id)); - - self.check_permission(Some(tenant_id))?; - - COMPUTE_COMMANDS_COUNTERS - .for_command(ComputeCommandKind::PageStream) - .inc(); - - self.handle_pagerequests( - pgb, - tenant_id, - timeline_id, - PagestreamProtocolVersion::V1, - ctx, - ) - .await?; } else if let Some(params) = parts.strip_prefix(&["basebackup"]) { if params.len() < 2 { return Err(QueryError::Other(anyhow::anyhow!( diff --git a/pgxn/neon/libpagestore.c b/pgxn/neon/libpagestore.c index 73a001b6ba72..5126c26c5d13 100644 --- a/pgxn/neon/libpagestore.c +++ b/pgxn/neon/libpagestore.c @@ -550,9 +550,6 @@ pageserver_connect(shardno_t shard_no, int elevel) case 2: pagestream_query = psprintf("pagestream_v2 %s %s", neon_tenant, neon_timeline); break; - case 1: - pagestream_query = psprintf("pagestream %s %s", neon_tenant, neon_timeline); - break; default: elog(ERROR, "unexpected neon_protocol_version %d", neon_protocol_version); } @@ -1063,7 +1060,7 @@ pg_init_libpagestore(void) NULL, &neon_protocol_version, 2, /* use protocol version 2 */ - 1, /* min */ + 2, /* min */ 2, /* max */ PGC_SU_BACKEND, 0, /* no flags required */ diff --git a/pgxn/neon/pagestore_client.h b/pgxn/neon/pagestore_client.h index 8951e6607b24..1f196d016c3f 100644 --- a/pgxn/neon/pagestore_client.h +++ b/pgxn/neon/pagestore_client.h @@ -87,9 +87,8 @@ typedef enum { * can skip traversing through recent layers which we know to not contain any * versions for the requested page. * - * These structs describe the V2 of these requests. The old V1 protocol contained - * just one LSN and a boolean 'latest' flag. If the neon_protocol_version GUC is - * set to 1, we will convert these to the V1 requests before sending. + * These structs describe the V2 of these requests. (The old now-defunct V1 + * protocol contained just one LSN and a boolean 'latest' flag.) */ typedef struct { diff --git a/pgxn/neon/pagestore_smgr.c b/pgxn/neon/pagestore_smgr.c index 8edaf65639f7..7f39c7d02603 100644 --- a/pgxn/neon/pagestore_smgr.c +++ b/pgxn/neon/pagestore_smgr.c @@ -1001,51 +1001,10 @@ nm_pack_request(NeonRequest *msg) initStringInfo(&s); - if (neon_protocol_version >= 2) - { - pq_sendbyte(&s, msg->tag); - pq_sendint64(&s, msg->lsn); - pq_sendint64(&s, msg->not_modified_since); - } - else - { - bool latest; - XLogRecPtr lsn; - - /* - * In primary, we always request the latest page version. - */ - if (!RecoveryInProgress()) - { - latest = true; - lsn = msg->not_modified_since; - } - else - { - /* - * In the protocol V1, we cannot represent that we want to read - * page at LSN X, and we know that it hasn't been modified since - * Y. We can either use 'not_modified_lsn' as the request LSN, and - * risk getting an error if that LSN is too old and has already - * fallen out of the pageserver's GC horizon, or we can send - * 'request_lsn', causing the pageserver to possibly wait for the - * recent WAL to arrive unnecessarily. Or something in between. We - * choose to use the old LSN and risk GC errors, because that's - * what we've done historically. - */ - latest = false; - lsn = msg->not_modified_since; - } + pq_sendbyte(&s, msg->tag); + pq_sendint64(&s, msg->lsn); + pq_sendint64(&s, msg->not_modified_since); - pq_sendbyte(&s, msg->tag); - pq_sendbyte(&s, latest); - pq_sendint64(&s, lsn); - } - - /* - * The rest of the request messages are the same between protocol V1 and - * V2 - */ switch (messageTag(msg)) { /* pagestore_client -> pagestore */ diff --git a/test_runner/regress/test_auth.py b/test_runner/regress/test_auth.py index 7cb85e3dd1b2..780c0e1602ac 100644 --- a/test_runner/regress/test_auth.py +++ b/test_runner/regress/test_auth.py @@ -211,7 +211,7 @@ def op(): def check_pageserver(expect_success: bool, **conn_kwargs): check_connection( env.pageserver, - f"pagestream {env.initial_tenant} {env.initial_timeline}", + f"pagestream_v2 {env.initial_tenant} {env.initial_timeline}", expect_success, **conn_kwargs, ) diff --git a/test_runner/regress/test_read_validation.py b/test_runner/regress/test_read_validation.py index d128c60a99c8..1ac881553fbc 100644 --- a/test_runner/regress/test_read_validation.py +++ b/test_runner/regress/test_read_validation.py @@ -19,11 +19,6 @@ def test_read_validation(neon_simple_env: NeonEnv): endpoint = env.endpoints.create_start( "test_read_validation", - # Use protocol version 2, because the code that constructs the V1 messages - # assumes that a primary always wants to read the latest version of a page, - # and therefore doesn't work with the test functions below to read an older - # page version. - config_lines=["neon.protocol_version=2"], ) with closing(endpoint.connect()) as con: @@ -142,11 +137,6 @@ def test_read_validation_neg(neon_simple_env: NeonEnv): endpoint = env.endpoints.create_start( "test_read_validation_neg", - # Use protocol version 2, because the code that constructs the V1 messages - # assumes that a primary always wants to read the latest version of a page, - # and therefore doesn't work with the test functions below to read an older - # page version. - config_lines=["neon.protocol_version=2"], ) with closing(endpoint.connect()) as con: