Date: Tue, 22 Apr 2025 12:21:01 -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: <aAfP_bitjVC1bcNV@nathan>
References: <Z-_sixINC08J_SQB@nathan>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CABdArM6j03AAX5A2B9TPCZrY1xr+eFpGpzfdOBAww7mZnniUFg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/aAfP_bitjVC1bcNV%40nathan>
Precedence: bulk

On Tue, Apr 22, 2025 at 05:17:17PM +0530, Nisha Moond wrote:
> Thanks for the patch! I tested it for functionality and here are a few comments:

Thank you for reviewing.

> 1) While testing, I noticed an unexpected behavior with the new 512
> byte length restriction in place. Since there´s no explicit
> restriction on the column roname´s type, it´s possible to insert an
> origin name exceeding 512 bytes. For instance, can use the COPY
> command -- similar to how pg_dump might dump and restore catalog
> tables.
> 
> For example:
>   postgres=# COPY pg_catalog.pg_replication_origin (roident, roname) FROM stdin;
>   Enter data to be copied followed by a newline.
>   End with a backslash and a period on a line by itself, or an EOF signal.
>   >> 44   regress_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa... [>512 bytes string]
>   >> \.
>   COPY 1
> 
> Once inserted, I was able to use  the pg_replication_origin_xxx
> functions with this origin name without any errors.
> Not sure how common pg_dump/restore of catalog tables is in real
> systems, but should we still consider if this behavior is acceptable?

I'm personally not too worried about this.  The limit is primarily there to
provide a nicer ERROR than "row is too big", which AFAICT is the worst-case
scenario if you go to the trouble of setting allow_system_table_mods and
modifying shared catalogs directly.

> 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.

-- 
nathan