public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Shlok Kyal <[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: Fri, 15 Aug 2025 10:53:10 +1000
Message-ID: <CAHut+Ptq0-tMYUOvG3yR34AvuEzR9vUH=muqV_=uEO3zCuA6rA@mail.gmail.com> (raw)
In-Reply-To: <CANhcyEW+uJB_bvQLEaZCgoRTc1=i+QnrPPHxZ2=0SBSCyj9pkg@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>
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.
~~~
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...
~~~
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")
*---------------------------------------------------
*/
~~~
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)?
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
[application/octet-stream] PS_addFooterToPublicationOrTableDesc.diff (4.8K, 2-PS_addFooterToPublicationOrTableDesc.diff)
download | inline diff:
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 6b72745..482cca6 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1561,49 +1561,69 @@ describeTableDetails(const char *pattern, bool verbose, bool showSystem)
}
/*
- * If is_tbl_desc is true add footer to table description else add footer to
- * publication description.
+ * Add a footer to a publication description or a table description.
+ *
+ * 'is_pub_desc' - true for a pub desc; false for a table desc
+ * 'as_schema' - true if the pub_desc only shows schemas, otherwise false
*/
static bool
-addFooterToPublicationOrTableDesc(PQExpBuffer buf, const char *footermsg,
- bool as_schema, printTableContent *const cont,
- bool is_tbl_desc)
+addFooterToPublicationOrTableDesc(PQExpBuffer buf,
+ printTableContent *const cont,
+ const char *footermsg,
+ bool is_pub_desc, bool as_schema)
{
PGresult *res;
- int count = 0;
- int i = 0;
- int col = is_tbl_desc ? 0 : 1;
+ int count;
+ int col = is_pub_desc ? 1 : 0;
res = PSQLexec(buf->data);
if (!res)
return false;
- else
- count = PQntuples(res);
+ count = PQntuples(res);
if (count > 0)
printTableAddFooter(cont, footermsg);
- /*---------------------------------------------------
- * 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")
- *---------------------------------------------------
+ /*--------------------------------------------------------------
+ * 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")
+ *--------------------------------------------------------------
*/
- for (i = 0; i < count; i++)
+ for (int i = 0; i < count; i++)
{
- if (as_schema)
- printfPQExpBuffer(buf, " \"%s\"", PQgetvalue(res, i, 0));
- else
+ printfPQExpBuffer(buf, " "); /* indent */
+
+ /* Footers entries for a publication description or a table description */
+ if (is_pub_desc)
{
- if (is_tbl_desc)
- printfPQExpBuffer(buf, " \"%s\"", PQgetvalue(res, i, col));
+ if (as_schema)
+ {
+ /* Schemas of the publication... */
+ appendPQExpBuffer(buf, "\"%s\"", PQgetvalue(res, i, 0));
+ }
else
- printfPQExpBuffer(buf, " \"%s.%s\"", PQgetvalue(res, i, 0),
+ {
+ /* Tables of the publication... */
+ appendPQExpBuffer(buf, "\"%s.%s\"", PQgetvalue(res, i, 0),
PQgetvalue(res, i, col));
+ }
+ }
+ else
+ {
+ /* Publications of the table... */
+ appendPQExpBuffer(buf, "\"%s\"", PQgetvalue(res, i, col));
+ }
+ /* Common footer output for column list and/or row filter */
+ if (!as_schema)
+ {
if (!PQgetisnull(res, i, col + 2))
{
if (strcmp(PQgetvalue(res, i, col + 3), "t") == 0)
@@ -3192,7 +3212,7 @@ describeOneTableDetails(const char *schemaname,
oid, oid);
}
- if (!addFooterToPublicationOrTableDesc(&buf, _("Publications:"), false, &cont, true))
+ if (!addFooterToPublicationOrTableDesc(&buf, &cont, _("Publications:"), false, false))
goto error_return;
}
@@ -6755,7 +6775,7 @@ describePublications(const char *pattern)
" AND pr.prpubid = '%s'\n", pubid);
appendPQExpBuffer(&buf, "ORDER BY 1,2");
- if (!addFooterToPublicationOrTableDesc(&buf, _("Tables:"), false, &cont, false))
+ if (!addFooterToPublicationOrTableDesc(&buf, &cont, _("Tables:"), true, false))
goto error_return;
if (pset.sversion >= 150000)
@@ -6767,8 +6787,8 @@ describePublications(const char *pattern)
" JOIN pg_catalog.pg_publication_namespace pn ON n.oid = pn.pnnspid\n"
"WHERE pn.pnpubid = '%s'\n"
"ORDER BY 1", pubid);
- if (!addFooterToPublicationOrTableDesc(&buf, _("Tables from schemas:"),
- true, &cont, false))
+ if (!addFooterToPublicationOrTableDesc(&buf, &cont,
+ _("Tables from schemas:"), true, true))
goto error_return;
}
}
@@ -6784,8 +6804,8 @@ describePublications(const char *pattern)
"WHERE pr.prpubid = '%s'\n"
" AND pr.prexcept\n"
"ORDER BY 1", pubid);
- if (!addFooterToPublicationOrTableDesc(&buf, _("Except tables:"),
- true, &cont, false))
+ if (!addFooterToPublicationOrTableDesc(&buf, &cont,
+ _("Except tables:"), true, true))
goto error_return;
}
}
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]
Subject: Re: Skipping schema changes in publication
In-Reply-To: <CAHut+Ptq0-tMYUOvG3yR34AvuEzR9vUH=muqV_=uEO3zCuA6rA@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