public inbox for [email protected]
help / color / mirror / Atom feedFrom: Shlok Kyal <[email protected]>
To: Kirill Reshke <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: vignesh C <[email protected]>
Cc: YeXiu <[email protected]>
Cc: Ian Lawrence Barwick <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Skipping schema changes in publication
Date: Wed, 20 Aug 2025 14:32:38 +0530
Message-ID: <CANhcyEVocpUmFspfPDgdPS7yMcNPPKQAC1GcKWwT-ZzccrCM1w@mail.gmail.com> (raw)
In-Reply-To: <CALdSSPgiDj8S8POf_6kiUkVrt9-EqYmn+2ahDqpdBpyteeAz-Q@mail.gmail.com>
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>
Hi Kirill,
Thanks for reviewing the patch.
On Fri, 15 Aug 2025 at 11:46, Kirill Reshke <[email protected]> wrote:
>
> Hi
>
> On Fri, 15 Aug 2025 at 05:53, Peter Smith <[email protected]> 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.g...
[2] : https://www.postgresql.org/message-id/CANhcyEUEMWSkTfGc7Q3B%2BUiOzSiOmOGLgK-%2BC5DXwtCGOnDBhg%40mail...
Thanks,
Shlok Kyal
view thread (377+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Skipping schema changes in publication
In-Reply-To: <CANhcyEVocpUmFspfPDgdPS7yMcNPPKQAC1GcKWwT-ZzccrCM1w@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox