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>
In-Reply-To: 
 <CANhcyEV_MePxgftHY65et1WdOAk70M0C7PZ1STPUO8PXHVB1YA@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Fri, 20 Jun 2025 13:57:39 +1000
Message-ID: 
 <CAHut+Ps0hSNqrjv_jT1AuXxO-CrZue3ixE0jKsxVhtArMrkujQ@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: 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"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAHut%2BPs0hSNqrjv_jT1AuXxO-CrZue3ixE0jKsxVhtArMrkujQ%40mail.gmail.com>
Precedence: bulk

On Thu, Jun 19, 2025 at 4:42=E2=80=AFPM Shlok Kyal <shlok.kyal.oss@gmail.co=
m> wrote:
...
> > 3.
> > TBH, I was wondering why a new catalog attribute was necessary...
> >
> > Can't you simply re-use the existing attribute "prattrs" attribute.
> > e.g. let's just define negative means exclude.
> >
> > e.g. a value of 1 3 means only the 1st and 3rd columns are published
> > e.g. a value of -1 -3 means all columns except 1st and 3rd columns are =
published
> > e.g. a value of null mean all columns are published
> >
> > (mixes of negative and positive will not be possible)
> >
>
> Currently I have added a new attribute 'prexcludeattrs' in
> pg_publication_rel table. I used this approach because it will be
> easier for user to get the exclude column list, in code no extra
> processing is required to get the exclude column list.
>
> For an approach to use negative numbers for exclude columns. I see an
> advantage that we do not need to introduce a new column for
> pg_publication_rel. But in code, each time we want to get a column
> list or exclude column list we need an extra processing of 'prattrs'
> columns. Also I don't see any existing catalog table using a negative
> attribute for column list.
>
> Based on above observations, I feel that the current is better.
>
> Please correct me if I missed an advantage for the approach you suggested=
.
>

OK. Maybe using negative numbers was a bridge too far...

But IMO it is not good to have 2 separate attributes for the lists.
Doing so implies they can coexist, but that is not true. I felt there
are not really 2 "kinds" of columns list anyway -- there is just a
"column list" which defines columns that are either included or
excluded from the publication determined by EXCEPT.

Having  dual lists gets weird/confusing to describe them -- you end up
continually having to refer to the other one to clarify behaviour.

e.g. Does 'prattrs' value NULL mean publish everything? Well, no...
that depends if there is a non null 'prexcludeattrs'
e.g. Does 'prexcludeattrs' value NULL mean publish everything? Well,
no... that depends if there is a non null 'prattrs'

Furthermore, all the code is doubling up referring to "column list"
and "exclude column list"  -- code / docs / comments / error messages.
There are quite a lot of places the patch touches that I thought were
not really needed if you don't have 2 different kinds of column-lists.

To summarise, I felt it would be better to just keep the existing
'prattrs' as the one-and-only column list, but add another BOOLEAN
attribute to flag whether 'prattrs' columns should be included or
excluded.

prattrs;   prattrs_exclude;  Means
--------------------------------------------
1 2 3     f                          only cols 1,2,3 will be published
4 5 6     t                          only cols 4,5,6 will NOT be published
null       f                          all cols are published (flag is ignor=
ed)
null       t                          all cols are published (flag is ignor=
ed)

> > 5.
> > +  <para>
> > +   Alter publication <structname>mypublication</structname> to add tab=
le
> > +   <structname>users</structname> except column
> > +   <structname>security_pin</structname>:
> > +<programlisting>
> > +ALTER PUBLICATION production_publication ADD TABLE users EXCEPT (secur=
ity_pin);
> >
> > Those tags don't seem correct. e.g. "users" and "security_pin" are not
> > <structname> (???).
> >
> > Perhaps, every other example here is wrong too and you just copied
> > them? Anyway, something here looks wrong to me.
> >
> I saw different documents and usage of tags seems not well defined.
> For example for table we are using tags in document
> create_publication.sgml, update.sgml <structname> is used, in document
> table.sgml, advanced.sgml <classname> is used, and in
> logical-replication.sgml <literal>  is used. Similarly for column
> names <structname>, <structfield> or <literal> are used in different
> parts of the document.
>
> I kept the changed tag to <structfield> for the column for this patch.
> Do you have any suggestions?

No, for this patch I think it is best that you just follow nearby code
(as you are already doing). I plan to raise another thread to ask what
are the guidelines for this  sort of markup which is currently used
inconsistently in different places.

//////////

Below are a few more review comments for v13-0003

=3D=3D=3D=3D=3D=3D
Commit message

1.
Typo /THe/The/

~~~

2.
The new syntax allows specifying excluded column list when creating or
altering a publication. For example:
CREATE PUBLICATION pubname FOR TABLE tabname EXCEPT (exclude_column_list)
or
ALTER PUBLICATION pubname ADD TABLE tabname EXCEPT (exclude_column_list)

~

I felt since you say these "For example:" it would be better to give
real examples.
e.g. say "(col1,col2,col3)" instead of "(exclude_column_list)".

~~~

3.
Typo /family of command/family of commands/

=3D=3D=3D=3D=3D=3D
doc/src/sgml/logical-replication.sgml

4.
I am not sure that it was a good idea to be making a new term called
an "exclude column list"... because in introduces a new concept of
something that sounds like it is a different kind of list, and now you
have to keep referring everywhere to both to "column list" versus
"exclude column list". All the doubling up add more complication I
think.

IMO really there is just a "column list". Whether that list is for
exclusion or not just depends on the presence of EXCEPT. So I felt
maybe all places mentioning "exclude column list" could be rephrased.

=3D=3D=3D=3D=3D=3D
src/backend/catalog/pg_publication.c

5.
+/*
+ * Returns true if the relation has exluded column list associated with th=
e
+ * publication, false otherwise.
+ *
+ * If a exclude column list is found, the corresponding bitmap is returned
+ * through the cols parameter, if provided. The bitmap is constructed
within the
+ * given memory context (mcxt).
+ */
+

Typo /exluded column/an excluded column/
Typo /exclude column list/excluded column list/

~~~

6.
+/*
+ * pub_exclude_collist_validate
+ * Process and validate the 'excluded columns' list and ensure the columns
+ * are all valid to exclude from publication.  Checks for and raises an
+ * ERROR for any unknown columns, system columns, duplicate columns, or
+ * generated columns.
+ *

Why can't you exclude generated columns?

e.g. Maybe PUBLICATION says publish_generated_columns=3Dstored and there
are 100s of such columns, but the user just wants to exclude one of
them. Why say they cannot do that? Hmm. Perhaps this is being already
handled elsewhere, in which case this comment still seems misleading.

=3D=3D=3D=3D=3D=3D
src/backend/commands/publicationcmds.c

7.
+ * With REPLICA IDENTITY FULL, no column list and no excluded column
+ * list is allowed.

Really, just "no column list is allowed." same as it said before.

=3D=3D=3D=3D=3D=3D
Kind Regards,
Peter Smith.
Fujitsu Australia