Re: Added schema level support for publication.

On Thu, Sep 30, 2021 at 3:39 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> The suggested change works, I have modified it in the attached patch.
>

I have reviewed the latest version and made a number of changes to the
0001 patch. The changes are in v1-0001-Changes-by-Amit. It includes
(a) Changed preprocess_pubobj_list() to make the code easy to
understand, (b) the handling of few variables was missing in equal
function, (c) the ordering of functions, and a few parameters were not
matching .c and .h files, (d) added/edited multiple comments and other
cosmetic changes.

Apart from that, I have few other comments:
1. It seems you have started using unique list variants in
GetPubPartitionOptionRelations because one of its caller
GetSchemaPublicationRelations need it. I think the unique variants are
costlier, so isn't it better to use it where it is required? I think
it would be good to use in GetPubPartitionOptionRelations, if most of
its caller requires the same.

2. In GetSchemaPublicationRelations(), I think we need to perform a
second scan using RELKIND_PARTITIONED_TABLE only if we
publish_via_root (aka pub_partopt is PUBLICATION_PART_ROOT). This is
what we are doing in GetAllTablesPublicationRelations. Is there a
reason to be different here?

3.
@@ -538,7 +788,7 @@ RemovePublicationById(Oid pubid)
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for publication %u", pubid);

- pubform = (Form_pg_publication)GETSTRUCT(tup);
+ pubform = (Form_pg_publication) GETSTRUCT(tup);

We don't need the above change for this patch. I think this may be due
pgindent but we can do this separately rather than as part of this
patch.

--
With Regards,
Amit Kapila.