On Mon, Dec 6, 2021 at 11:46 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Dec 03, 2021 at 05:20:35PM +0000, Bossart, Nathan wrote:
> > On 12/2/21, 11:57 PM, "tanghy.fnst@fujitsu.com" <tanghy.fnst@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.