Date: Tue, 29 Apr 2025 12:39:58 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Nisha Moond <nisha.moond412@gmail.com>,
	Euler Taveira <euler@eulerto.com>,
	Michael Paquier <michael@paquier.xyz>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	"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: <aBEO7vzq1jw9L_Pv@nathan>
References: <aAZqiKRhUv7-fSpQ@nathan>
 <CABdArM6j03AAX5A2B9TPCZrY1xr+eFpGpzfdOBAww7mZnniUFg@mail.gmail.com>
 <aAfP_bitjVC1bcNV@nathan>
 <aAla1fg-1vPeXNck@nathan>
 <1503201.1745444021@sss.pgh.pa.us>
 <aAlfU06XTChyIGBD@nathan>
 <aA_zMYasoUwNrDx1@nathan>
 <905635.1745876108@sss.pgh.pa.us>
 <aA_4S5ZBaI_Qje2s@nathan>
 <910625.1745878759@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="K9JbhwB5gisUXW7l"
Content-Disposition: inline
In-Reply-To: <910625.1745878759@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/aBEO7vzq1jw9L_Pv%40nathan>
Precedence: bulk


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

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

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

From 4c015dd4ccc09f193eccb5d447d01d6f5c378397 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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 <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>
Reviewed-by: Tom Lane <tgl@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)


--K9JbhwB5gisUXW7l--