On Mon, 15 Jun 2026 at 14:29, Peter Smith <[email protected]> wrote: > > Some review comments for v50-0004. > > ====== > src/bin/pg_dump/pg_dump.c > > dumpSubscription: > > 1. > + * We skip the default value ('log') to match the handling of other > + * default subscription options. > + */ > > There is only one "default", so I think this should say "...other > subscription options", not "...other default subscription options".
modified > ====== > src/include/catalog/pg_subscription.h > > 2. > +#define CONFLICT_LOG_RELATION_NAME_FMT "pg_conflict_log_%u" > + > > 2a. > This format string (and first usage of it) really be in one of the > earlier patches. This was implemented in this patch, as prior to it there was only a single instance of its usage. > ~ > > 2b. > Add a comment to explain that the %u is the subscription oid. Added it > ~ > > 2c. > Is this the correct header file for this? IMO it is CLT specific, so > belongs in conflict.h. I should be kept in pg_subscription.h, as it is also required by the frontend. These changes for the same are available in the v51 version patch posted at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm2r_5hmz22Gsio%3DOABvA%3Dc_cr8avv%3Dy19Epb0vKMo%2BQJQ%40mail.gmail.com Regards, Vignesh
