Re: Added schema level support for publication.

On Sat, Oct 2, 2021 at 1:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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.

I have merged these changes into the main patch.

> 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.

I have removed unique list changes from GetPubPartitionOptionRelations
and handled it in GetSchemaPublicationRelations.

> 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?

In the first table scan we are getting all the ordinary tables present
in the schema. In the second table scan we will get all the
partitioned table present in the schema and the relations will be
added based on pub_partopt. I felt if we have the check we will not
get the relations in the following case:
create schema sch1;
create schema sch2;
create table sch1.p (a int) partition by list (a);
create table sch2.c1 partition of sch1.p for values in (1);

> 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.

Removed this change.

Attached v36 patch has the changes for the same.

Regards,
Vignesh