On Tue, Aug 10, 2021 at 1:40 PM Greg Nancarrow <
gregn4422@gmail.com> wrote:
>
> On Fri, Aug 6, 2021 at 6:32 PM vignesh C <
vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v19 patch has the fixes for the comments.
> >
>
> Some more review comments, this time for the v19 patch:
>
>
> (1) In patch v19-0002, there's still a couple of instances where it
> says "publication type" instead of "publication kind".
Modified
> (2) src/backend/catalog/pg_publication.c
>
> "This should only be used for normal publications."
>
> What exactly is meant by that - what type is considered normal? Maybe
> that comment should be more specific.
Modified
> (3) src/backend/catalog/pg_publication.c
> GetSchemaPublications
>
> Function header says "Gets list of publication oids for publications
> marked as FOR SCHEMA."
>
> Shouldn't it say something like: "Gets the list of FOR SCHEMA
> publication oids associated with a specified schema oid." or something
> like that?
> (since the function accepts a schemaid parameter)
Modfified
> (4) src/backend/commands/publicationcmds.c
>
> In AlterPublicationSchemas(), I notice that the two error cases
> "cannot be added to or dropped ..." don't check stmt->action for
> DEFELEM_ADD/DEFELEM_DROP.
> Is that OK? (i.e. should these cases error out if stmt->action is not
> DEFELEM_ADD/DEFELEM_DROP?)
> Also, I assume that the else part (not DEFELEM_ADD/DEFELEM_DROP) is
> the "Set" case? Maybe a comment should be added to the top of the else
> part.
The error message should also include set, I have modified the error message accordingly.
> (5) src/backend/commands/publicationcmds.c
> Typo (same in 2 places): "automaically" -> "automatically"
>
> + * will be released automaically at the end of create publication
>
> See functions:
> (i) CreatePublication
> (ii) AlterPublicationSchemas
Modified.
> (6) src/backend/commands/publicationcmds.c
> LockSchemaList
>
> Function header says "are locked in ShareUpdateExclusiveLock mode" but
> then code calls LockDatabaseObject using "AccessShareLock".
Modified.
Thanks for the comments, these issues are fixed in the v20 patch attached at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com
Regards,
Vignesh
