On Mon, Dec 6, 2021 at 11:46 AM Michael Paquier <mich...@paquier.xyz> wrote: > > On Fri, Dec 03, 2021 at 05:20:35PM +0000, Bossart, Nathan wrote: > > On 12/2/21, 11:57 PM, "tanghy.f...@fujitsu.com" <tanghy.f...@fujitsu.com> > > wrote: > > > Thanks for your patch. > > > I tested it and it fixed this problem as expected. It also passed "make > > > check-world". > > > > +1, the patch looks good to me, too. My only other suggestion would > > be to move IsSchemaPublication() to pg_publication.c > > There is more to that, no? It seems to me that anything that opens > PublicationNamespaceRelationId should be in pg_publication.c, so that > would include RemovePublicationSchemaById(). >
It is currently similar to RemovePublicationById, RemovePublicationRelById, etc. which are also in publicationcmds.c. > If you do that, > GetSchemaPublicationRelations() could be local to pg_publication.c. > > + tup = systable_getnext(scan); > + if (HeapTupleIsValid(tup)) > + result = true; > This can be written as just "result = HeapTupleIsValid(tup)". Anyway, > this code also means that once we drop the schema this publication > won't be considered anymore as a schema publication, meaning that it > also makes this code weaker to actual cache lookup failures? > How, can you be a bit more specific? > I find > the semantics around pg_publication_namespace is bit weird because of > that, and inconsistent with the existing > puballtables/pg_publication_rel. > What do you mean by inconsistent with puballtables/pg_publication_rel? -- With Regards, Amit Kapila.
