On Thu, May 19, 2022 at 1:49 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Below are my review comments for v6-0001.
>
> ======
>
> 1. General.
>
> The patch failed 'publication' tests in the make check phase.
>
> Please add this work to the commit-fest so that the 'cfbot' can report
> such errors sooner.
Added commitfest entry
> ~~~
>
> 2. src/backend/commands/publicationcmds.c - AlterPublicationReset
>
> +/*
> + * Reset the publication.
> + *
> + * Reset the publication options, publication relations and
> publication schemas.
> + */
> +static void
> +AlterPublicationReset(ParseState *pstate, AlterPublicationStmt *stmt,
> + Relation rel, HeapTuple tup)
>
> SUGGESTION (Make the comment similar to the sgml text instead of
> repeating "publication" 4x !)
> /*
> * Reset the publication options, set the ALL TABLES flag to false, and
> * drop all relations and schemas that are associated with the publication.
> */
Modified
> ~~~
>
> 3. src/test/regress/expected/publication.out
>
> make check failed. The diff is below:
>
> @@ -1716,7 +1716,7 @@
> -- Verify that only superuser can reset a publication
> ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2;
> SET ROLE regress_publication_user2;
> -ALTER PUBLICATION testpub_reset RESET; -- fail
> +ALTER PUBLICATION testpub_reset RESET; -- fail - must be superuser
> ERROR: must be superuser to RESET publication
> SET ROLE regress_publication_user;
> DROP PUBLICATION testpub_reset;
It passed for me locally because the change was present in the 002
patch. I have moved the change to 001.
The attached v7 patch has the changes for the same.
[1] - https://commitfest.postgresql.org/38/3646/
Regards,
Vignesh