On Tue, Jul 27, 2021 at 5:11 AM Greg Nancarrow <gregn4...@gmail.com> wrote:
>
> On Mon, Jul 26, 2021 at 3:21 PM vignesh C <vignes...@gmail.com> wrote:
> >
> > Thanks for the comment, this is modified in the v15 patch attached.
> >
>
> I have several minor review comments.
>
> (1) src/backend/catalog/objectaddress.c
> Should start comment sentences with an uppercase letter, for consistency.
>
> + /* fetch publication name and schema oid from input list */
>
> I also notice that some 1-sentence comments end with "." (full-stop)
> and others don't. It seems to alternate all over the place, and so is
> quite noticeable.
> Unfortunately, it already seems to be like this in much of the code
> that this patch patches.
> Ideally (at least my personal preference is) 1-sentence comments
> should not end with a ".".
Modified.
> (2) src/backend/catalog/pg_publication.c
> errdetail message
>
> I think the following should say "Temporary schemas ..." (since the
> existing error message for tables says "System tables cannot be added
> to publications.").
>
> + errdetail("Temporary schema cannot be added to publications.")));
>
Modified.
> (3) src/backend/commands/publicationcmds.c
> PublicationAddTables
>
> I think that the Assert below is not correct (i.e. not restrictive
enough).
> Although the condition passes, it is allowing, for example,
> stmt->for_all_tables==true if stmt->schemas==NIL, and that doesn't
> seem to be correct.
> I suggest the following change:
>
> BEFORE:
> + Assert(!stmt || !stmt->for_all_tables || !stmt->schemas);
> AFTER:
> + Assert(!stmt || (!stmt->for_all_tables && !stmt->schemas));
>
Modified.
> (4) src/backend/commands/publicationcmds.c
> PublicationAddSchemas
>
> Similarly, I think that the Assert below is not restrictive enough,
> and think it should be changed:
>
> BEFORE:
> + Assert(!stmt || !stmt->for_all_tables || !stmt->tables);
> AFTER:
> + Assert(!stmt || (!stmt->for_all_tables && !stmt->tables));
>
Modified.
> (5) src/bin/pg_dump/common.c
>
> Spelling mistake.
>
> BEFORE:
> + pg_log_info("reading publciation schemas");
> AFTER:
> + pg_log_info("reading publication schemas");
Modified.
Thanks for the comments, the comments are fixed in the v16 patch attached
at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm2LgV5XcLF80rJ60NwnjKpZj%3D%3DLxJpO4W2TG2G5XmUtDA%40mail.gmail.com
Regards,
Vignesh
