Re: Skipping schema changes in publication

On Fri, 15 Aug 2025 at 06:23, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.
>
Since this is part of already existing code, I think this should be a
new thread. I have created a new thread for this. See [1].

> ~~~
>
> 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
> (???).
>
I have used pub_schemas.
> ~~~
>
> 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...
>
I have also used the patch with minor changes.

> ~~~
>
> 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")
> *---------------------------------------------------
> */
>
> ~~~
>
I have used the suggested description with some modifications.

> 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)?
>
This patch does not add any new code to >= 150000. It is the same as
HEAD. This diff appears because of changes in 0002 patchset. In patch
0002, I did not create a separate full query for >= 190000 due to
small changes.

I have addressed the rest of the comments and added the changes in the
latest v21 patchset.

[1]: https://www.postgresql.org/message-id/CANhcyEXHiCbk2q8%3Dbq3boQDyc8ac9fjgK-kkp5PdTYLcAOq80Q%40mail.gmail.com

Thanks,
Shlok Kyal