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
