Date: Wed, 23 Apr 2025 16:25:41 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Nisha Moond <nisha.moond412@gmail.com>
Cc: Euler Taveira <euler@eulerto.com>,
	Michael Paquier <michael@paquier.xyz>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	"Jonathan S. Katz" <jkatz@postgresql.org>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Large expressions in indexes can't be stored (non-TOASTable)
Message-ID: <aAla1fg-1vPeXNck@nathan>
References: 
 <CAA4eK1LeMM8royHj5UyMBDbspD7+_70AJkV4Uji122hmkn7jOw@mail.gmail.com>
 <Z_WGI4cKoTjhUXrs@nathan>
 <4c2a6100-738e-48cc-8a11-2d85f95ae0ed@app.fastmail.com>
 <Z_X1jfsMrBBNUx4n@paquier.xyz>
 <Z_bFDNzcqWxatpL-@nathan>
 <18a111db-9f31-449c-a14b-61779a69f344@app.fastmail.com>
 <Z_mHvqVyGjEZhzl7@nathan>
 <aAZqiKRhUv7-fSpQ@nathan>
 <CABdArM6j03AAX5A2B9TPCZrY1xr+eFpGpzfdOBAww7mZnniUFg@mail.gmail.com>
 <aAfP_bitjVC1bcNV@nathan>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="JsS/Y9YpAk9vOtU8"
Content-Disposition: inline
In-Reply-To: <aAfP_bitjVC1bcNV@nathan>
Archived-At: <https://www.postgresql.org/message-id/aAla1fg-1vPeXNck%40nathan>
Precedence: bulk


--JsS/Y9YpAk9vOtU8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Apr 22, 2025 at 12:21:01PM -0500, Nathan Bossart wrote:
> On Tue, Apr 22, 2025 at 05:17:17PM +0530, Nisha Moond wrote:
>> 5) As Euler pointed out, logical replication already has a built-in
>> restriction for internally assigned origin names, limiting them to
>> NAMEDATALEN. Given that, only the SQL function
>> pg_replication_origin_create() is capable of creating longer origin
>> names. Would it make sense to consider moving the check into
>> pg_replication_origin_create() itself, since it seems redundant for
>> all other callers?
>> That said, if there's a possibility of future callers needing this
>> restriction at a lower level, it may be reasonable to leave it as is.
> 
> pg_replication_origin_create() might be a better place.  After all, that's
> where we're enforcing the name doesn't start with "pg_" and, for testing,
> _does_ start with "regress_".  Plus, it seems highly unlikely that any
> other callers of replorigin_create() will ever provide names longer than
> 512 bytes.

Here is a new version of the patch with this change.

-- 
nathan

--JsS/Y9YpAk9vOtU8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v4-0001-Remove-pg_replication_origin-s-TOAST-table.patch"

From 1ca6f9c2a4a3c9917bb4846a77d0b7eaf936b8b8 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 23 Apr 2025 16:21:22 -0500
Subject: [PATCH v4 1/1] Remove pg_replication_origin's TOAST table.

Reviewed-by: Michael Paquier <michael@paquier.xyz>
Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
Reviewed-by: Euler Taveira <euler@eulerto.com>
Reviewed-by: Nisha Moond <nisha.moond412@gmail.com>
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     | 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 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..e9629350cb0 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -1296,6 +1296,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..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..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..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)


--JsS/Y9YpAk9vOtU8--