Re: Added schema level support for publication.

On Tue, Aug 10, 2021 at 1:40 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
>
> On Fri, Aug 6, 2021 at 6:32 PM vignesh C <vignesh21(at)gmail(dot)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