Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Sun, Oct 3, 2021 at 11:25 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sat, Oct 2, 2021 at 1:13 PM Amit Kapila <amit.kapila16@gmail.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?

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?

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

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

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;
+ }

-- 
With Regards,
Amit Kapila.