Re: Skipping schema changes in publication

Hi Shlok,

Here are some review comments for v20-0003.

======
src/backend/commands/publicationcmds.c

AlterPublicationTables:

1.
bool isnull = true;
- Datum whereClauseDatum;
- Datum columnListDatum;
+ Datum datum;

I know you did not write the code, but that "isnull = true" is
redundant, and seems kind of misleading because it will always be
re-assigned before it is used.

~~~

2.
/* Load the WHERE clause for this table. */
- whereClauseDatum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple,
- Anum_pg_publication_rel_prqual,
- &isnull);
+ datum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple,
+ Anum_pg_publication_rel_prqual,
+ &isnull);
if (!isnull)
- oldrelwhereclause = stringToNode(TextDatumGetCString(whereClauseDatum));
+ oldrelwhereclause = stringToNode(TextDatumGetCString(datum));

/* Transform the int2vector column list to a bitmap. */
- columnListDatum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple,
- Anum_pg_publication_rel_prattrs,
- &isnull);
+ datum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple,
+ Anum_pg_publication_rel_prattrs,
+ &isnull);
+
+ if (!isnull)
+ oldcolumns = pub_collist_to_bitmapset(NULL, datum, NULL);
+
+ /* Load the prexcept flag for this table. */
+ datum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple,
+ Anum_pg_publication_rel_prexcept,
+ &isnull);

if (!isnull)
- oldcolumns = pub_collist_to_bitmapset(NULL, columnListDatum, NULL);
+ oldexcept = DatumGetBool(datum);

Use consistent spacing. Either do or don't (I prefer don't) put a
blank line between the pairs of "datum =" and "if (!isnull)". Avoid
having a mixture.

======
src/bin/psql/describe.c

addFooterToPublicationOrTableDesc:

3.
+/*
+ * If is_tbl_desc is true add footer to table description else add footer to
+ * publication description.
+ */
+static bool
+addFooterToPublicationOrTableDesc(PQExpBuffer buf, const char *footermsg,
+ bool as_schema, printTableContent *const cont,
+ bool is_tbl_desc)

3a.
Since you are changing this anyway, I think it would be better to keep
those boolean params together (at the end).

~

3b.
It seems a bit mixed up calling this addFooterToPublicationOrTableDesc
but having the variable 'is_tbl_desc', because it seems more natural
to me to read left to right, so the logical order of everything here
should be pub desc then table desc. In other words, use boolean
'is_pub_desc' instead of 'is_tbl_desc'. Also, I think that 'as_schema'
thing is kind of a *subset* of the publication description, so it
makes more sense for that to come last too.

e.g.
CURRENT
addFooterToPublicationOrTableDesc(buf, footermsg, as_schema, cont, is_tbl_desc)
SUGGESTION
addFooterToPublicationOrTableDesc(buf, cont, footermsg, is_pub_desc, as_schema)

~

3c
While you are changing things, maybe also consider changing that
'as_schema' name because I did not understand what "as" means. Perhaps
rename like 'pub_schemas', or 'only_show_schemas' or something better
(???).

~~~

4.
+ PGresult *res;
+ int count = 0;
+ int i = 0;
+ int col = is_tbl_desc ? 0 : 1;
+
+ res = PSQLexec(buf->data);
+ if (!res)
+ return false;
+ else
+ count = PQntuples(res);
+

4a.
Assignment count = 0 is redundant.

~

4b.
Remove the 'i' declaration here. Declare it in the "for" loop later.

~

4c.
The "else" is not required. If 'res' was not good, you already returned.

~~~

5.
+ if (as_schema)
+ printfPQExpBuffer(buf, " \"%s\"", PQgetvalue(res, i, 0));
+ else
+ {
+ if (is_tbl_desc)
+ printfPQExpBuffer(buf, " \"%s\"", PQgetvalue(res, i, col));
+ else
+ printfPQExpBuffer(buf, " \"%s.%s\"", PQgetvalue(res, i, 0),
+ PQgetvalue(res, i, col));

This function is basically either (a) a footer for a table description
or (b) a footer for a publication description. And that all hinges on
the boolean 'is_tbl_desc'. Therefore, it seems more natural for the
main condition to be "if (is_tbl_desc)" here.

This turned everything inside out. PSA: a top-up patch to show a way
to do this. Perhaps my implementation is a bit verbose, but OTOH it
seems easier to understand. Anyway, see what you think...

~~~

6.
+ /*---------------------------------------------------
+ * Publication/ table description columns:
+ * [0]: schema name (nspname)
+ * [col]: table name (relname) / publication name (pubname)
+ * [col + 1]: row filter expression (prqual), may be NULL
+ * [col + 2]: column list (comma-separated), may be NULL
+ * [col + 3]: except flag ("t" if EXCEPT, else "f")
+ *---------------------------------------------------

I've modified this comment slightly so I could understand it better.
See if you agree.

SUGGESTION
/*---------------------------------------------------
* Description columns:
* PUB TBL
* [0] - : schema name (nspname)
* [col] - : table name (relname)
* - [col] : publication name (pubname)
* [col+1] [col+1]: row filter expression (prqual), may be NULL
* [col+2] [col+1]: column list (comma-separated), may be NULL
* [col+3] [col+1]: except flag ("t" if EXCEPT, else "f")
*---------------------------------------------------
*/

~~~

describeOneTableDetails:

7.
+ else if (pset.sversion >= 150000)
+ {
+ printfPQExpBuffer(&buf,
+ "SELECT pubname\n"
+ " , NULL\n"
+ " , NULL\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\n"
+ "WHERE pc.oid ='%s' and pg_catalog.pg_relation_is_publishable('%s')\n"
+ "UNION\n"
+ "SELECT pubname\n"
+ " , pg_get_expr(pr.prqual, c.oid)\n"
+ " , (CASE WHEN pr.prattrs IS NOT NULL THEN\n"
+ " (SELECT string_agg(attname, ', ')\n"
+ " FROM pg_catalog.generate_series(0,
pg_catalog.array_upper(pr.prattrs::pg_catalog.int2[], 1)) s,\n"
+ " pg_catalog.pg_attribute\n"
+ " WHERE attrelid = pr.prrelid AND attnum = prattrs[s])\n"
+ " ELSE NULL END) "
+ "FROM pg_catalog.pg_publication p\n"
+ " JOIN pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
+ " JOIN pg_catalog.pg_class c ON c.oid = pr.prrelid\n"
+ "WHERE pr.prrelid = '%s'\n"
+ "UNION\n"
+ "SELECT pubname\n"
+ " , NULL\n"
+ " , NULL\n"
+ "FROM pg_catalog.pg_publication p\n"
+ "WHERE p.puballtables AND pg_catalog.pg_relation_is_publishable('%s')\n"
+ "ORDER BY 1;",
+ oid, oid, oid, oid);

AFAICT, that >= 150000 code seems to have added another UNION at the
end that was not previously there. What's that about? How is that
related to EXCEPT (column-list)?

======
Kind Regards,
Peter Smith.
Fujitsu Australia