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

On Wed, Oct 6, 2021 at 11:12 AM vignesh C <vignesh21@gmail.com> wrote:
>
> Attached v37 patch has the changes for the same.
>

Few comments:
==============
v37-0001-Added-schema-level-support-for-publication
1.
+ *
+ * The first scan will get all the 'r' relkind tables for the specified schema,
+ * iterate the 'r' relkind tables and prepare a list of:
+ * 1) non partition table if pub_partopt is PUBLICATION_PART_ROOT
+ * 2) partition table and non partition table if pub_partopt is
+ *    PUBLICATION_PART_LEAF.
+ *
+ * The second scan will get all the 'p'' relkind tables for the  specified
+ * schema, iterate the 'p' relkind tables and prepare a list of:
+ * 1) partition table's child relations if pub_partopt is PUBLICATION_PART_LEAF
+ * 2) partition table if pub_partopt is PUBLICATION_PART_ROOT.

I think these comments are redundant and not sure if they are
completely correct. We don't need these as the actual code explains
these conditions better. The earlier part of these comments is
sufficient.

v37-0002-Client-side-changes-to-support-FOR-ALL-TABLES-IN
2.
+ * selectDumpablePublicationObject: policy-setting subroutine
+ * Mark a publication as to be dumped or not
  *
- * Publication tables have schemas, but those are ignored in decision making,
+ * Publications have schemas, but those are ignored in decision making,
  * because publications are only dumped when we are dumping everything.
  */

Change the above comment lines:
a. "Mark a publication as to be dumped or not" to "Mark a publication
object as to be dumped or not".

b. "Publications have schemas, but those are ignored in decision
making, .." to "A publication can have schemas and tables which have
schemas, but those are ignored in decision making, .."

3.
+/*
+ * dumpPublicationNamespace
+ *   dump the definition of the given publication tables in schema mapping
+ */

Can we change the comment to: "dump the definition of the given
publication schema mapping"? IT is easier to read and understand.

4.
+/*
+ * The PublicationSchemaInfo struct is used to represent publication tables
+ * in schema mapping.
+ */
+typedef struct _PublicationSchemaInfo
+{
+ DumpableObject dobj;
+ NamespaceInfo *pubschema;
+ PublicationInfo *publication;
+} PublicationSchemaInfo;

Can we change the comment similar to the comment change in point 3?
Also, let's keep PublicationInfo * before NamespaceInfo * just to be
consistent with the existing structure PublicationRelInfo?

5.
+ printfPQExpBuffer(&buf,
+   "SELECT p.pubname\n"
+   "FROM pg_catalog.pg_publication p\n"
+   " JOIN pg_catalog.pg_publication_namespace pn ON p.oid = pn.pnpubid\n"
+   " JOIN pg_catalog.pg_class pc ON pc.relnamespace = pn.pnnspid AND
pc.oid = '%s'\n"

I think this part of the query can be improved in multiple ways: (a)
pc.oid = '%s' should be part of WHERE clause not join condition, (b)
for pubname, no need to use alias name, it can be directly referred as
pubname, (c) you can check if the relation is publishable. So, the
formed query would look like:

SELECT p.pubname FROM pg_catalog.pg_publication p JOIN
pg_catalog.pg_publication_namespace pn ON p.oid = pn.pnpubid JOIN
pg_catalog.pg_class pc ON pc.relnamespace = pn.pnnspid  WHERE pc.oid =
'%s' and pg_catalog.pg_relation_is_publishable('%s')

6.
listSchemas()
{
..
+ if (pub_schema_tuples > 0)
+ {
+ /*
+ * Allocate memory for footers. Size of footers will be 1 (for
+ * storing "Publications:" string) + Schema count +  1 (for
+ * storing NULL).
+ */
+ footers = (char **) palloc((1 + pub_schema_tuples + 1) * sizeof(char *));
+ footers[0] = pstrdup(_("Publications:"));
+
+ /* Might be an empty set - that's ok */
+ for (i = 0; i < pub_schema_tuples; i++)
+ {
+ printfPQExpBuffer(&buf, "    \"%s\"",
+   PQgetvalue(result, i, 0));
+
+ footers[i + 1] = pstrdup(buf.data);
+ }
+
+ footers[i + 1] = NULL;
+ myopt.footers = footers;
+ }
..
}

Is there a reason of not using printTableAddFooter() here similar to
how we use it to print Publications in describeOneTableDetails()?

7.
describePublications()
{
..
+ /* Get the schemas for the specified publication */
+ printfPQExpBuffer(&buf,
+   "SELECT n.nspname\n"
+   "FROM pg_catalog.pg_namespace n,\n"
+   "     pg_catalog.pg_publication_namespace pn\n"
+   "WHERE n.oid = pn.pnnspid\n"
+   "  AND pn.pnpubid = '%s'\n"
+   "ORDER BY 1", pubid);
+ if (!addFooterToPublicationDesc(&buf, "Tables from schemas:",
+ true, &cont))
+ goto error_return;
..
}

Shouldn't we try to get schemas only when pset.sversion >= 150000?

8.
+addFooterToPublicationDesc(PQExpBuffer buf, char *footermsg,
+    bool singlecol, printTableContent *cont)
+{
..
+ termPQExpBuffer(buf);
..
}

It seems this buffer is freed at the caller's site, if so, no need to
free it here.

-- 
With Regards,
Amit Kapila.