Re: Large expressions in indexes can't be stored (non-TOASTable)

On Mon, Apr 21, 2025 at 9:26 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> Apparently replication origins in tests are supposed to be prefixed with
> "regress_". Here is a new patch with that fixed.
>

Hi Nathan,
Thanks for the patch! I tested it for functionality and here are a few comments:

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?
~~~

Below are a few cosmetic suggestions you might consider.
2)
<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>

Would it make sense to make the phrasing a bit more formal, perhaps
something like:
“The name must not exceed 512 bytes.”?

3) For the code comments -
+ /*
+ * 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)

a) /bytes. This/bytes. This/ (there is an extra space before “This”)
b) /all practical use/all practical uses/
c) Both (a) and (b) above, also apply to the comment above the macro
“MAX_RONAME_LEN”.

4) The "Detail" part of the error message could be made a bit more
formal and precise -

Current:
DETAIL: Replication origin names must be no longer than 512 bytes.

Suggestion:
DETAIL: Replication origin names must not exceed 512 bytes.

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.

--
Thanks,
Nisha