Re: Skipping schema changes in publication - Mailing list pgsql-hackers

Hi

On Fri, 15 Aug 2025 at 05:53, Peter Smith <smithpb2250@gmail.com> wrote:

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

People are not generally excited about refactoring code they did not
change. This makes patch to have more review cycles, and less probable
to actually being committed. If we are really wedded with this change,
this could be a separate thread.


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

+ 1

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

For me that's equal. lets see what other people think


-- 
Best regards,
Kirill Reshke