On Mon, Apr 28, 2025 at 06:19:19PM -0400, Tom Lane wrote: > LGTM other than that nit.
Thanks. I'll stash this away for July unless someone wants to argue that it's fair game for v18. IMHO this isn't nearly urgent enough for that, and the bugs will continue to exist on older major versions regardless. -- nathan
>From 4c015dd4ccc09f193eccb5d447d01d6f5c378397 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nat...@postgresql.org> Date: Tue, 29 Apr 2025 12:19:15 -0500 Subject: [PATCH v7 1/1] Remove pg_replication_origin's TOAST table. A few places that access this catalog do not set up an active snapshot before potentially accessing its TOAST table, which is unsafe. However, roname (the replication origin name) is the only varlena column, so this is only ever a problem if the name requires out-of-line storage, which seems unlikely. This commit removes its TOAST table to avoid needing to set up a snapshot, and it establishes a length restriction of 512 bytes for replication origin names. Even without this restriction, names that would require out-of-line storage would fail with a "row is too big" error; the additional length restriction just provides a more specific error message. Reviewed-by: Michael Paquier <mich...@paquier.xyz> Reviewed-by: Amit Kapila <amit.kapil...@gmail.com> Reviewed-by: Euler Taveira <eu...@eulerto.com> Reviewed-by: Nisha Moond <nisha.moond...@gmail.com> Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> Discussion: https://postgr.es/m/ZvMSUPOqUU-VNADN%40nathan --- doc/src/sgml/func.sgml | 1 + src/backend/catalog/catalog.c | 2 -- src/backend/replication/logical/origin.c | 24 ++++++++++++++++++++ src/include/catalog/pg_replication_origin.h | 2 -- src/include/replication/origin.h | 7 ++++++ src/test/regress/expected/misc_functions.out | 4 ++++ src/test/regress/expected/misc_sanity.out | 8 ++++++- src/test/regress/sql/misc_functions.sql | 3 +++ src/test/regress/sql/misc_sanity.sql | 5 ++++ 9 files changed, 51 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 574a544d9fa..593e45495be 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -29941,6 +29941,7 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset <para> Creates a replication origin with the given external name, and returns the internal ID assigned to it. + The name must be no longer than 512 bytes. </para></entry> </row> diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 60000bd0bc7..59caae8f1bc 100644 --- a/src/backend/catalog/catalog.c +++ b/src/backend/catalog/catalog.c @@ -346,8 +346,6 @@ IsSharedRelation(Oid relationId) relationId == PgDbRoleSettingToastIndex || relationId == PgParameterAclToastTable || relationId == PgParameterAclToastIndex || - relationId == PgReplicationOriginToastTable || - relationId == PgReplicationOriginToastIndex || relationId == PgShdescriptionToastTable || relationId == PgShdescriptionToastIndex || relationId == PgShseclabelToastTable || diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c index 6583dd497da..9e5a09e63e1 100644 --- a/src/backend/replication/logical/origin.c +++ b/src/backend/replication/logical/origin.c @@ -287,6 +287,18 @@ replorigin_create(const char *roname) rel = table_open(ReplicationOriginRelationId, ExclusiveLock); + /* + * We want to be able to access pg_replication_origin without setting up a + * snapshot. To make that safe, it needs to not have a TOAST table, since + * TOASTed data cannot be fetched without a snapshot. As of this writing, + * its TOAST table would only be used for replication origin names, which + * we avoid with a reasonable length restriction. If you add a TOAST + * table to this catalog, be sure to set up a snapshot everywhere it might + * be needed. For more information, see + * https://postgr.es/m/ZvMSUPOqUU-VNADN%40nathan. + */ + Assert(!OidIsValid(rel->rd_rel->reltoastrelid)); + for (roident = InvalidOid + 1; roident < PG_UINT16_MAX; roident++) { bool nulls[Natts_pg_replication_origin]; @@ -1296,6 +1308,18 @@ pg_replication_origin_create(PG_FUNCTION_ARGS) elog(WARNING, "replication origins created by regression test cases should have names starting with \"regress_\""); #endif + /* + * To avoid needing a TOAST table for pg_replication_origin, we restrict + * replication origin names to 512 bytes. This should be more than enough + * for all practical use. + */ + if (strlen(name) > MAX_RONAME_LEN) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("replication origin name is too long"), + errdetail("Replication origin names must be no longer than %d bytes.", + MAX_RONAME_LEN))); + roident = replorigin_create(name); pfree(name); diff --git a/src/include/catalog/pg_replication_origin.h b/src/include/catalog/pg_replication_origin.h index deb43065fe9..7ade8bbda39 100644 --- a/src/include/catalog/pg_replication_origin.h +++ b/src/include/catalog/pg_replication_origin.h @@ -54,8 +54,6 @@ CATALOG(pg_replication_origin,6000,ReplicationOriginRelationId) BKI_SHARED_RELAT typedef FormData_pg_replication_origin *Form_pg_replication_origin; -DECLARE_TOAST_WITH_MACRO(pg_replication_origin, 4181, 4182, PgReplicationOriginToastTable, PgReplicationOriginToastIndex); - DECLARE_UNIQUE_INDEX_PKEY(pg_replication_origin_roiident_index, 6001, ReplicationOriginIdentIndex, pg_replication_origin, btree(roident oid_ops)); DECLARE_UNIQUE_INDEX(pg_replication_origin_roname_index, 6002, ReplicationOriginNameIndex, pg_replication_origin, btree(roname text_ops)); diff --git a/src/include/replication/origin.h b/src/include/replication/origin.h index 9cb2248fa9f..29724c0dcae 100644 --- a/src/include/replication/origin.h +++ b/src/include/replication/origin.h @@ -33,6 +33,13 @@ typedef struct xl_replorigin_drop #define InvalidRepOriginId 0 #define DoNotReplicateId PG_UINT16_MAX +/* + * To avoid needing a TOAST table for pg_replication_origin, we restrict + * replication origin names to 512 bytes. This should be more than enough for + * all practical use. + */ +#define MAX_RONAME_LEN 512 + extern PGDLLIMPORT RepOriginId replorigin_session_origin; extern PGDLLIMPORT XLogRecPtr replorigin_session_origin_lsn; extern PGDLLIMPORT TimestampTz replorigin_session_origin_timestamp; diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index d3f5d16a672..3de32c3c7ef 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -914,3 +914,7 @@ SELECT test_relpath(); (1 row) +-- pg_replication_origin.roname length restriction +SELECT pg_replication_origin_create('regress_' || repeat('a', 505)); +ERROR: replication origin name is too long +DETAIL: Replication origin names must be no longer than 512 bytes. diff --git a/src/test/regress/expected/misc_sanity.out b/src/test/regress/expected/misc_sanity.out index b032a3f4761..21297fc6164 100644 --- a/src/test/regress/expected/misc_sanity.out +++ b/src/test/regress/expected/misc_sanity.out @@ -42,6 +42,11 @@ WHERE refclassid = 0 OR refobjid = 0 OR -- as user data by pg_upgrade, which would cause failures. -- 3. pg_authid, since its toast table cannot be accessed when it would be -- needed, i.e., during authentication before we've selected a database. +-- 4. pg_replication_origin, since we want to be able to access that catalog +-- without setting up a snapshot. To make that safe, it needs to not have a +-- toast table, since toasted data cannot be fetched without a snapshot. As of +-- this writing, its toast table would only be used for replication origin +-- names, which we can avoid with a reasonable length restriction. SELECT relname, attname, atttypid::regtype FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid WHERE c.oid < 16384 AND @@ -61,7 +66,8 @@ ORDER BY 1, 2; pg_class | relpartbound | pg_node_tree pg_largeobject | data | bytea pg_largeobject_metadata | lomacl | aclitem[] -(10 rows) + pg_replication_origin | roname | text +(11 rows) -- system catalogs without primary keys -- diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index aaebb298330..ea72a62dab7 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -411,3 +411,6 @@ CREATE FUNCTION test_relpath() AS :'regresslib' LANGUAGE C; SELECT test_relpath(); + +-- pg_replication_origin.roname length restriction +SELECT pg_replication_origin_create('regress_' || repeat('a', 505)); diff --git a/src/test/regress/sql/misc_sanity.sql b/src/test/regress/sql/misc_sanity.sql index 135793871b4..8ff898843e9 100644 --- a/src/test/regress/sql/misc_sanity.sql +++ b/src/test/regress/sql/misc_sanity.sql @@ -45,6 +45,11 @@ WHERE refclassid = 0 OR refobjid = 0 OR -- as user data by pg_upgrade, which would cause failures. -- 3. pg_authid, since its toast table cannot be accessed when it would be -- needed, i.e., during authentication before we've selected a database. +-- 4. pg_replication_origin, since we want to be able to access that catalog +-- without setting up a snapshot. To make that safe, it needs to not have a +-- toast table, since toasted data cannot be fetched without a snapshot. As of +-- this writing, its toast table would only be used for replication origin +-- names, which we can avoid with a reasonable length restriction. SELECT relname, attname, atttypid::regtype FROM pg_class c JOIN pg_attribute a ON c.oid = attrelid -- 2.39.5 (Apple Git-154)
