Re: Skipping schema changes in publication

Hi Kirill,

Thanks for reviewing the patch.

On Fri, 15 Aug 2025 at 11:46, Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>
> Hi
>
> On Fri, 15 Aug 2025 at 05:53, Peter Smith <smithpb2250(at)gmail(dot)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.
>
I also feel that we should create a new thread for the same. I have
created a new thread. 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
> > (???).
> >
> > ~~~
> >
> > 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
>
Included these changes in the latest patch [2].

> >
> > 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
>
For now I have used the version shared by Peter. I felt it was more descriptive.

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

Thanks,
Shlok Kyal