On Wed, Apr 09, 2025 at 08:54:21PM -0300, Euler Taveira wrote:
> LGTM. I have a few suggestions.
Thanks for reviewing.
> + /*
> + * 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)
> + ereport(ERROR,
>
> I wouldn't duplicate the comment. Instead, I would keep it only in origin.h.
Hm. I agree that duplicating the comment isn't great, but I'm also not
wild about forcing readers to jump to the macro definition to figure out
why there is a length restriction.
> + errdetail("Repilcation origin names must be no longer than %d bytes.",
> + MAX_RONAME_LEN)));
>
> s/Repilcation/Replication/
Fixed.
> +#define MAX_RONAME_LEN (512)
>
> It is just a cosmetic suggestion but I usually use parentheses when it is an
> expression but don't include it for a single number.
We use both styles, but the no-parentheses style does seem to be preferred.
$ grep -E "^#define\s[A-Z_]+\s\([0-9]+\)$" src/* -rI | wc -l
23
$ grep -E "^#define\s[A-Z_]+\s[0-9]+$" src/* -rI | wc -l
861
> Should we document this maximum length?
I've added a note.
--
nathan
