MIME-Version: 1.0
References: 
 <CALDaNm3=JrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh=tamA@mail.gmail.com>
 <CALj2ACVOzhs+BD+abFV2x4oKJdsDNd6SgsE7r8UjnZDCKGEckA@mail.gmail.com>
 <CAA4eK1K6Kr88d2S0zFdHRMyuoaZeNh+ktU+oigmCuD09_x_-+g@mail.gmail.com>
 <CAHut+PsvC-NezO3MJkdyEz=G1QRje2LntjwhQiEeVbmhOQuBMA@mail.gmail.com>
 <CALDaNm18VH2j8cTqfELHQ=0ZNognbGBhbHPteJenWQC6C2dueQ@mail.gmail.com>
 <CALDaNm0k_0Ccj47wzJzzPFwgQB7w=R5+Q2_nSqYrmMmjhmcRUw@mail.gmail.com>
 <CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com>
 <CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@mail.gmail.com>
 <TYCPR01MB8373C3120C2B3112001ED6F1EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ@mail.gmail.com>
 <CAHut+PtiomM+iyAZHvb2dzfsPvRru266KuBe49hKy2n2h+m_zA@mail.gmail.com>
 <CALDaNm30KDnwX4Czi29fqLb8JBkuwqjbpj9ixwNXXox574NZqQ@mail.gmail.com>
 <CALDaNm1PfKRJsEzbKpyt=v4p3bw+_SzE+LFPsMhR5X+qs+0pPw@mail.gmail.com>
 <TYCPR01MB83730A2F1D6A5303E9C1416AEDD99@TYCPR01MB8373.jpnprd01.prod.outlook.com>
 <CALDaNm0sAU4s1KTLOEWv=rYo5dQK6uFTJn_0FKj3XG1Nv4D-qw@mail.gmail.com>
 <CALDaNm3CLRa95tpas6tEj8x58MUNDShxBNoYS+P8Uq5cryoAOw@mail.gmail.com>
 <CALDaNm0EKC3o=v+F7GneGibuCULGKkBWXmNaVB4GR9HoqD066A@mail.gmail.com>
 <CALDaNm1Z1Rmqj9s6P9ZzmrVA9F_vZ_DwwhYAJmsjqmY6dS3-hA@mail.gmail.com>
 <CAB8KJ=jJGuW=ozKmXZzKDUHZ_-J2ZYGOtJo=i2cnNbSu6=KuYg@mail.gmail.com>
 <CALDaNm1mbFP8fxHU_H1Ex4cT2Aq3n8FE79tq0TO5ThvFnDUYMA@mail.gmail.com>
 <CAB8KJ=jq4RwTs8K7pokmXQwQppP2ChVJLMSAdXaxAX+c1r+mdg@mail.gmail.com>
 <CALDaNm1mJvLni8GODebKBmyegXuZ18bLoG-Pz6H1MCX=vphCYA@mail.gmail.com>
 <CALDaNm3dWZCYDih55qTNAYsjCvYXMFv=46UsDWmfCnXMt3kPCg@mail.gmail.com>
 <CALDaNm1AQZYgT0tALRrkvpP1Q+8+e7vkGCUjQ-jim1C0q3e=zA@mail.gmail.com>
 <CAA4eK1KRdAPC=5=7tQ1GW0cRwD=zaDMi+T4u_k4GxPhPY6e8BQ@mail.gmail.com>
 <OS3PR01MB5718C8BE84B862E7E0CEC29B94BD2@OS3PR01MB5718.jpnprd01.prod.outlook.com>
 <CAA4eK1KYQz7cf46_D=6VkZ4J6Y8vJ88MMi=6zm2TJXDP+V1mLg@mail.gmail.com>
 <CANhcyEXZq4mP5dNgg7u=sMPwvxA4_ZN9U92uZEuzs=0xTu+8Yg@mail.gmail.com>
 <CANhcyEXspT3v5-Tdop9uqQV2HWBvZoN5P0BxXQ6Md6Mr7GXK9A@mail.gmail.com>
 <CAHut+PuiaLOCkiAx9nPnjk6wTbPFvnm9T5svTuKbgwJwTdea8w@mail.gmail.com>
 <CANhcyEV_MePxgftHY65et1WdOAk70M0C7PZ1STPUO8PXHVB1YA@mail.gmail.com>
 <CAHut+Ps0hSNqrjv_jT1AuXxO-CrZue3ixE0jKsxVhtArMrkujQ@mail.gmail.com>
 <CANhcyEXX3viVpYcGHD_fzhf_f6CDQWr2+VBywrJf5zm_XiB4tg@mail.gmail.com>
 <CAHut+PsXP_61ZXuVOx5u9FZGK3oH4taaA59oOzgqyygZx8ezWw@mail.gmail.com>
 <CANhcyEU+aPu6iAH2cTA0cDtn3pd6c_njBONCt3FubYZoEEnm8Q@mail.gmail.com>
 <CAHut+Pv2P6dJ7hZj_fmzN+=xzjvpOpgkAJvDZg3TD2xpvmY1NQ@mail.gmail.com>
 <CANhcyEW2LK4diNeCG862DE40yQoV3VAgf59kXUq2TuR8fnw5vQ@mail.gmail.com>
 <CAHut+PuSHScrODVGCM7P53Mv1HE2N6ThzkH4+gQ1eFXVeD-OCA@mail.gmail.com>
 <CANhcyEUtYV-9ujtxLasnxN_peT+3LuZjcRx1xUECh1CCmANB8w@mail.gmail.com>
 <CAHut+PuviFA6C7qps=+kDYfe3P99as8NCjbR=SYxoi0o96ipoA@mail.gmail.com>
 <CANhcyEXkeg3sjkS3DS9yU1ckz4ozUBNZ+RmrWaRNSSVCR8RquA@mail.gmail.com>
 <CAHut+PsHavMy_KJ0MwR9J6q0BTTty54TxS-KBZc7X6tb4u7rfA@mail.gmail.com>
 <CANhcyEU=k4+0BqOu25N76g738XKUwfLGGdf8e+ssGiRKHC4RwQ@mail.gmail.com>
 <CAHut+Ptdi8txJbMc+5Sp2uB4QLGOp-rZdkBbXyhAkxim0iAhBQ@mail.gmail.com>
 <CANhcyEW+uJB_bvQLEaZCgoRTc1=i+QnrPPHxZ2=0SBSCyj9pkg@mail.gmail.com>
 <CAHut+Ptq0-tMYUOvG3yR34AvuEzR9vUH=muqV_=uEO3zCuA6rA@mail.gmail.com>
 <CALdSSPgiDj8S8POf_6kiUkVrt9-EqYmn+2ahDqpdBpyteeAz-Q@mail.gmail.com>
In-Reply-To: 
 <CALdSSPgiDj8S8POf_6kiUkVrt9-EqYmn+2ahDqpdBpyteeAz-Q@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Wed, 20 Aug 2025 14:32:38 +0530
Message-ID: 
 <CANhcyEVocpUmFspfPDgdPS7yMcNPPKQAC1GcKWwT-ZzccrCM1w@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 vignesh C <vignesh21@gmail.com>, YeXiu <1518981153@qq.com>,
	Ian Lawrence Barwick <barwick@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CANhcyEVocpUmFspfPDgdPS7yMcNPPKQAC1GcKWwT-ZzccrCM1w%40mail.gmail.com>
Precedence: bulk

Hi Kirill,

Thanks for reviewing the patch.

On Fri, 15 Aug 2025 at 11:46, Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> 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.
>
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