Re: Added schema level support for publication.

On Mon, Oct 4, 2021 at 5:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, Oct 3, 2021 at 11:25 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > On Sat, Oct 2, 2021 at 1:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> >
> > > 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);
> >
>
> But we don't need to get the partitioned tables for the invalidations,
> see the corresponding case for tables. So, I am not sure why you have
> used two scans to the system table for such scenarios?

The second loop is required to get the 'p' relkind relations like in
the below case:
create schema sch1;
create schema sch2;
create table sch1.p1 (a int) partition by list (a);
create table sch2.c1 partition of sch1.p1 for values in (1);
create table sch2.p2(a int) partition by list (a);
create table sch1.c2 partition of sch2.p2 for values in (1);
create publication pub1 for all tables in schema sch2;
The first loop will give us sch2.c1 relation and the second loop will
get sch2.p2, sch1.c2, this is required for schema publication as the
schema can have both r relkind and p relkind tables and all of them
need to be invalidated. This is not required in case of table
publication as we can get the required relations from that particular
table.

> Few additional comments on
> v36-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN:
> =========================================================================
> 1.
> @@ -3961,21 +3965,25 @@ getPublications(Archive *fout, int *numPublications)
> appendPQExpBuffer(query,
> "SELECT p.tableoid, p.oid, p.pubname, "
> "(%s p.pubowner) AS rolname, "
> - "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
> p.pubtruncate, p.pubviaroot "
> + "p.puballtables, p.pubinsert, p.pubupdate, "
> + "p.pubdelete, p.pubtruncate, p.pubviaroot "
> "FROM pg_publication p",
> username_subquery);
> else if (fout->remoteVersion >= 110000)
> appendPQExpBuffer(query,
> "SELECT p.tableoid, p.oid, p.pubname, "
> "(%s p.pubowner) AS rolname, "
> - "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete,
> p.pubtruncate, false AS pubviaroot "
> + "p.puballtables, p.pubinsert, p.pubupdate, "
> + "p.pubdelete, p.pubtruncate, false AS pubviaroot "
> "FROM pg_publication p",
> username_subquery);
> else
> appendPQExpBuffer(query,
> "SELECT p.tableoid, p.oid, p.pubname, "
> "(%s p.pubowner) AS rolname, "
> - "p.puballtables, p.pubinsert, p.pubupdate, p.pubdelete, false AS
> pubtruncate, false AS pubviaroot "
> + "p.puballtables, p.pubinsert, p.pubupdate, "
> + "p.pubdelete, false AS pubtruncate, "
> + "false AS pubviaroot "
> "FROM pg_publication p",
> username_subquery);
>
> Is there a reason to change this part of the code?

It is not required, I have removed it.

> 2.
> @@ -257,6 +257,9 @@ getSchemaData(Archive *fout, int *numTablesPtr)
> pg_log_info("reading publication membership");
> getPublicationTables(fout, tblinfo, numTables);
>
> + pg_log_info("reading publication tables in schemas");
> + getPublicationNamespaces(fout, nspinfo, numNamespaces);
>
> I think for the above change, the first should be changed to "reading
> publication membership of tables" and the second one should be changed
> to "reading publication membership of schemas".

Modified

> 3. The function names getPublicationNamespaces and
> dumpPublicationSchema are not in sync. Let's name the second one as
> dumpPublicationNamespace.

Modified

> 4. It is not clear to me why the patch has introduced a new component
> type object DUMP_COMPONENT_PUBSCHEMA. In particular, in the below code
> if we are already setting DUMP_COMPONENT_ALL, how the additional
> setting of DUMP_COMPONENT_PUBSCHEMA helps?
>
> @@ -1631,9 +1631,13 @@ selectDumpableNamespace(NamespaceInfo *nsinfo,
> Archive *fout)
> if (nsinfo->nspowner == ROLE_PG_DATABASE_OWNER)
> nsinfo->dobj.dump &= ~DUMP_COMPONENT_DEFINITION;
> nsinfo->dobj.dump_contains = DUMP_COMPONENT_ALL;
> + nsinfo->dobj.dump |= DUMP_COMPONENT_PUBSCHEMA;
> }
> else
> + {
> nsinfo->dobj.dump_contains = nsinfo->dobj.dump = DUMP_COMPONENT_ALL;
> + nsinfo->dobj.dump |= DUMP_COMPONENT_PUBSCHEMA;
> + }

I have removed DUMP_COMPONENT_PUBSCHEMA and used DUMP_COMPONENT_NONE
to identify the publications that should be dumped.

Attached v37 patch has the changes for the same.

Regards,
Vignesh