On Tue, Aug 10, 2021 at 1:40 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > > On Fri, Aug 6, 2021 at 6:32 PM vignesh C <vignes...@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
