On Wed, Apr 09, 2025 at 08:54:21PM -0300, Euler Taveira wrote:
> LGTM. I have a few suggestions.
Thanks for reviewing.
> + /*
> + * 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(roname) > MAX_RONAME_LEN)
> + ereport(ERROR,
>
> I wouldn't duplicate the comment. Instead, I would keep it only in origin.h.
Hm. I agree that duplicating the comment isn't great, but I'm also not
wild about forcing readers to jump to the macro definition to figure out
why there is a length restriction.
> + errdetail("Repilcation origin names must be no longer than
> %d bytes.",
> + MAX_RONAME_LEN)));
>
> s/Repilcation/Replication/
Fixed.
> +#define MAX_RONAME_LEN (512)
>
> It is just a cosmetic suggestion but I usually use parentheses when it is an
> expression but don't include it for a single number.
We use both styles, but the no-parentheses style does seem to be preferred.
$ grep -E "^#define\s[A-Z_]+\s\([0-9]+\)$" src/* -rI | wc -l
23
$ grep -E "^#define\s[A-Z_]+\s[0-9]+$" src/* -rI | wc -l
861
> Should we document this maximum length?
I've added a note.
--
nathan
>From 6762a3786b76379c82ccfa4c9da1812e928179b5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email protected]>
Date: Wed, 9 Apr 2025 14:00:31 -0500
Subject: [PATCH v2 1/1] Remove pg_replication_origin's TOAST table.
---
doc/src/sgml/func.sgml | 1 +
src/backend/catalog/catalog.c | 2 --
src/backend/replication/logical/origin.c | 12 ++++++++++++
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 | 6 +++++-
src/test/regress/sql/misc_functions.sql | 3 +++
src/test/regress/sql/misc_sanity.sql | 3 +++
9 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1c5cfee25d1..a9c5c855524 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 a6edf614606..a7eb60dd2d2 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -315,8 +315,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..c17c0348c6e 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -264,6 +264,18 @@ replorigin_create(const char *roname)
SysScanDesc scan;
ScanKeyData key;
+ /*
+ * 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(roname) > 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)));
+
roname_d = CStringGetTextDatum(roname);
Assert(IsTransactionState());
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..d28b69e50ad 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(repeat('0123456789abcdef', 33));
+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..0cb58311329 100644
--- a/src/test/regress/expected/misc_sanity.out
+++ b/src/test/regress/expected/misc_sanity.out
@@ -42,6 +42,9 @@ 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 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 +64,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..7122ff6133d 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(repeat('0123456789abcdef', 33));
diff --git a/src/test/regress/sql/misc_sanity.sql
b/src/test/regress/sql/misc_sanity.sql
index 135793871b4..545b2b48989 100644
--- a/src/test/regress/sql/misc_sanity.sql
+++ b/src/test/regress/sql/misc_sanity.sql
@@ -45,6 +45,9 @@ 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 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)
