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". (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. (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) (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. (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 (6) src/backend/commands/publicationcmds.c LockSchemaList Function header says "are locked in ShareUpdateExclusiveLock mode" but then code calls LockDatabaseObject using "AccessShareLock". Regards, Greg Nancarrow Fujitsu Australia
