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

On Thu, Oct 7, 2021 at 5:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.

Removed it.

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

Modified

> 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')

Modified

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

There are 2 ways to print table in psql:
1) call printTableInit, printTableAddHeader, printTableAddCell,
printTableAddFooter, printTable & printTableCleanup to print the table
2) prepare the table contents and call printQuery to print the
table(which will take care of handling all of the above)
describeOneTableDetails uses 1st method
listSchemas uses 2nd method, in case of this method since table is not
initialized we cannot use printTableAddFooter. we have to prepare the
footers and set the footers.

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

Modified

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

Modified

These comments are fixed in the v38 patch attached.

Regards,
Vignesh