Thread: Skipping schema changes in publication

Skipping schema changes in publication

From

vignesh C

Date:

22 March 2022, 07:08:43

Hi,

This feature adds an option to skip changes of all tables in specified
schema while creating publication.
This feature is helpful for use cases where the user wants to
subscribe to all the changes except for the changes present in a few
schemas.
Ex:
CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
OR
ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;

A new column pnskip is added to table "pg_publication_namespace", to
maintain the schemas that the user wants to skip publishing through
the publication. Modified the output plugin (pgoutput) to skip
publishing the changes if the relation is part of skip schema
publication.
As a continuation to this, I will work on implementing skipping tables
from all tables in schema and skipping tables from all tables
publication.

Attached patch has the implementation for this.
This feature is for the pg16 version.
Thoughts?

Regards,
Vignesh

Attachment

v1-0001-Skip-publishing-the-tables-of-schema.patch

Re: Skipping schema changes in publication

From

vignesh C

Date:

26 March 2022, 14:07:26

On Tue, Mar 22, 2022 at 12:38 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> This feature adds an option to skip changes of all tables in specified
> schema while creating publication.
> This feature is helpful for use cases where the user wants to
> subscribe to all the changes except for the changes present in a few
> schemas.
> Ex:
> CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
> OR
> ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;
>
> A new column pnskip is added to table "pg_publication_namespace", to
> maintain the schemas that the user wants to skip publishing through
> the publication. Modified the output plugin (pgoutput) to skip
> publishing the changes if the relation is part of skip schema
> publication.
> As a continuation to this, I will work on implementing skipping tables
> from all tables in schema and skipping tables from all tables
> publication.
>
> Attached patch has the implementation for this.

The patch was not applying on top of HEAD because of the recent
commits, attached patch is rebased on top of HEAD.

Regards,
Vignesh

Attachment

v1-0001-Skip-publishing-the-tables-of-schema.patch

Re: Skipping schema changes in publication

From

vignesh C

Date:

12 April 2022, 06:23:29

On Sat, Mar 26, 2022 at 7:37 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, Mar 22, 2022 at 12:38 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > This feature adds an option to skip changes of all tables in specified
> > schema while creating publication.
> > This feature is helpful for use cases where the user wants to
> > subscribe to all the changes except for the changes present in a few
> > schemas.
> > Ex:
> > CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
> > OR
> > ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;
> >
> > A new column pnskip is added to table "pg_publication_namespace", to
> > maintain the schemas that the user wants to skip publishing through
> > the publication. Modified the output plugin (pgoutput) to skip
> > publishing the changes if the relation is part of skip schema
> > publication.
> > As a continuation to this, I will work on implementing skipping tables
> > from all tables in schema and skipping tables from all tables
> > publication.
> >
> > Attached patch has the implementation for this.
>
> The patch was not applying on top of HEAD because of the recent
> commits, attached patch is rebased on top of HEAD.

The patch does not apply on top of HEAD because of the recent commit,
attached patch is rebased on top of HEAD.

I have also included the implementation for skipping a few tables from
all tables publication, the 0002 patch has the implementation for the
same.
This feature is helpful for use cases where the user wants to
subscribe to all the changes except for the changes present in a few
tables.
Ex:
CREATE PUBLICATION pub1 FOR ALL TABLES SKIP TABLE t1,t2;
OR
ALTER PUBLICATION pub1 ADD SKIP  TABLE t1,t2;

Regards,
Vignesh

On Thu, Apr 14, 2022, at 10:47 AM, Peter Eisentraut wrote:

On 12.04.22 08:23, vignesh C wrote:
> I have also included the implementation for skipping a few tables from
> all tables publication, the 0002 patch has the implementation for the
> same.
> This feature is helpful for use cases where the user wants to
> subscribe to all the changes except for the changes present in a few
> tables.
> Ex:
> CREATE PUBLICATION pub1 FOR ALL TABLES SKIP TABLE t1,t2;
> OR
> ALTER PUBLICATION pub1 ADD SKIP TABLE t1,t2;

We have already allocated the "skip" terminology for skipping
transactions, which is a dynamic run-time action. We are also using the
term "skip" elsewhere to skip locked rows, which is similarly a run-time
action. I think it would be confusing to use the term SKIP for DDL
construction.

I didn't like the SKIP choice too. We already have EXCEPT for IMPORT FOREIGN

SCHEMA and if I were to suggest a keyword, it would be EXCEPT.

I would also think about this in broader terms. For example, sometimes
people want features like "all columns except these" in certain places.
The syntax for those things should be similar.

The questions are:

What kind of issues does it solve?

Do we have a workaround for it?

That said, I'm not sure this feature is worth the trouble. If this is
useful, what about "whole database except these schemas"? What about
"create this database from this template except these schemas". This
could get out of hand. I think we should encourage users to group their
object the way they want and not offer these complicated negative
selection mechanisms.

I have the same impression too. We already provide a way to:

* include individual tables;

* include all tables;

* include all tables in a certain schema.

Doesn't it cover the majority of the use cases? We don't need to cover all

possible cases in one DDL command. IMO the current grammar for CREATE

PUBLICATION is already complicated after the ALL TABLES IN SCHEMA. You are

proposing to add "ALL TABLES SKIP ALL TABLES" that sounds repetitive but it is

not; doesn't seem well-thought-out. I'm also concerned about possible gotchas

for this proposal. The first command above suggests that it skips all tables in a

certain schema. What happen if I decide to include a particular table of the

skipped schema (second command)?

ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;

ALTER PUBLICATION pub1 ADD TABLE s1.foo;

Having said that I'm not wedded to this proposal. Unless someone provides

compelling use cases for this additional syntax, I think we should leave the

publication syntax as is.

Euler Taveira

EDB https://www.enterprisedb.com/

Re: Skipping schema changes in publication

From

Amit Kapila

Date:

18 April 2022, 07:01:59

On Fri, Apr 15, 2022 at 1:26 AM Euler Taveira <euler@eulerto.com> wrote:
>
> On Thu, Apr 14, 2022, at 10:47 AM, Peter Eisentraut wrote:
>
> On 12.04.22 08:23, vignesh C wrote:
> > I have also included the implementation for skipping a few tables from
> > all tables publication, the 0002 patch has the implementation for the
> > same.
> > This feature is helpful for use cases where the user wants to
> > subscribe to all the changes except for the changes present in a few
> > tables.
> > Ex:
> > CREATE PUBLICATION pub1 FOR ALL TABLES SKIP TABLE t1,t2;
> > OR
> > ALTER PUBLICATION pub1 ADD SKIP  TABLE t1,t2;
>
> We have already allocated the "skip" terminology for skipping
> transactions, which is a dynamic run-time action.  We are also using the
> term "skip" elsewhere to skip locked rows, which is similarly a run-time
> action.  I think it would be confusing to use the term SKIP for DDL
> construction.
>
> I didn't like the SKIP choice too. We already have EXCEPT for IMPORT FOREIGN
> SCHEMA and if I were to suggest a keyword, it would be EXCEPT.
>

+1 for EXCEPT.

> I would also think about this in broader terms.  For example, sometimes
> people want features like "all columns except these" in certain places.
> The syntax for those things should be similar.
>
> The questions are:
> What kind of issues does it solve?

As far as I understand, it is for usability, otherwise, users need to
list all required columns' names even if they don't want to hide most
of the columns in the table. Consider user doesn't want to publish the
'salary' or other sensitive information of executives/employees but
would like to publish all other columns. I feel in such cases it will
be a lot of work for the user especially when the table has many
columns. I see that Oracle has a similar feature [1]. I think without
this it will be difficult for users to use this feature in some cases.

> Do we have a workaround for it?
>

I can't think of any except the user needs to manually input all
required columns. Can you think of any other workaround?

> That said, I'm not sure this feature is worth the trouble.  If this is
> useful, what about "whole database except these schemas"?  What about
> "create this database from this template except these schemas".  This
> could get out of hand.  I think we should encourage users to group their
> object the way they want and not offer these complicated negative
> selection mechanisms.
>
> I have the same impression too. We already provide a way to:
>
> * include individual tables;
> * include all tables;
> * include all tables in a certain schema.
>
> Doesn't it cover the majority of the use cases?
>

Similar to columns, the same applies to tables. Users need to manually
add all tables for a database even when she wants to avoid only a
handful of tables from the database say because they contain sensitive
information or are not required. I think we don't need to cover all
possible exceptions but a few where users can avoid some tables would
be useful. If not, what kind of alternative do users have except for
listing all columns or all tables that are required.

[1] -
https://docs.oracle.com/en/cloud/paas/goldengate-cloud/gwuad/selecting-columns.html#GUID-9A851C8B-48F7-43DF-8D98-D086BE069E20

-- 
With Regards,
Amit Kapila.

Re: Skipping schema changes in publication

From

vignesh C

Date:

18 April 2022, 09:40:46

On Thu, Apr 14, 2022 at 7:18 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 12.04.22 08:23, vignesh C wrote:
> > I have also included the implementation for skipping a few tables from
> > all tables publication, the 0002 patch has the implementation for the
> > same.
> > This feature is helpful for use cases where the user wants to
> > subscribe to all the changes except for the changes present in a few
> > tables.
> > Ex:
> > CREATE PUBLICATION pub1 FOR ALL TABLES SKIP TABLE t1,t2;
> > OR
> > ALTER PUBLICATION pub1 ADD SKIP  TABLE t1,t2;
>
> We have already allocated the "skip" terminology for skipping
> transactions, which is a dynamic run-time action.  We are also using the
> term "skip" elsewhere to skip locked rows, which is similarly a run-time
> action.  I think it would be confusing to use the term SKIP for DDL
> construction.
>
> Let's find another term like "omit", "except", etc.

+1 for Except

> I would also think about this in broader terms.  For example, sometimes
> people want features like "all columns except these" in certain places.
> The syntax for those things should be similar.
>
> That said, I'm not sure this feature is worth the trouble.  If this is
> useful, what about "whole database except these schemas"?  What about
> "create this database from this template except these schemas".  This
> could get out of hand.  I think we should encourage users to group their
> object the way they want and not offer these complicated negative
> selection mechanisms.

I thought this feature would help when there are many many tables in
the database and the user wants only certain confidential tables like
credit card information. In this case instead of specifying the whole
table list it will be better to specify "ALL TABLES EXCEPT
cred_info_tbl".
I had seen that mysql also has a similar option replicate-ignore-table
to ignore the changes on specific tables as mentioned in [1].
Similar use case exists in pg_dump too. pg_dump has an option
exclude-table that will be used for not dumping any tables that are
matching the table specified as in [2].

[1] - https://dev.mysql.com/doc/refman/5.7/en/change-replication-filter.html
[2] - https://www.postgresql.org/docs/devel/app-pgdump.html

Regards,
Vignesh

Re: Skipping schema changes in publication

From

vignesh C

Date:

21 April 2022, 03:15:07

On Mon, Apr 18, 2022 at 12:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Apr 15, 2022 at 1:26 AM Euler Taveira <euler@eulerto.com> wrote:
> >
> > On Thu, Apr 14, 2022, at 10:47 AM, Peter Eisentraut wrote:
> >
> > On 12.04.22 08:23, vignesh C wrote:
> > > I have also included the implementation for skipping a few tables from
> > > all tables publication, the 0002 patch has the implementation for the
> > > same.
> > > This feature is helpful for use cases where the user wants to
> > > subscribe to all the changes except for the changes present in a few
> > > tables.
> > > Ex:
> > > CREATE PUBLICATION pub1 FOR ALL TABLES SKIP TABLE t1,t2;
> > > OR
> > > ALTER PUBLICATION pub1 ADD SKIP  TABLE t1,t2;
> >
> > We have already allocated the "skip" terminology for skipping
> > transactions, which is a dynamic run-time action.  We are also using the
> > term "skip" elsewhere to skip locked rows, which is similarly a run-time
> > action.  I think it would be confusing to use the term SKIP for DDL
> > construction.
> >
> > I didn't like the SKIP choice too. We already have EXCEPT for IMPORT FOREIGN
> > SCHEMA and if I were to suggest a keyword, it would be EXCEPT.
> >
>
> +1 for EXCEPT.

Updated patch by changing the syntax to use EXCEPT instead of SKIP.

Regards,
Vignesh

Attachment

v2-0001-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch

Re: Skipping schema changes in publication

From

Bharath Rupireddy

Date:

22 April 2022, 16:09:24

On Tue, Mar 22, 2022 at 12:39 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> This feature adds an option to skip changes of all tables in specified
> schema while creating publication.
> This feature is helpful for use cases where the user wants to
> subscribe to all the changes except for the changes present in a few
> schemas.
> Ex:
> CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
> OR
> ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;
>
> A new column pnskip is added to table "pg_publication_namespace", to
> maintain the schemas that the user wants to skip publishing through
> the publication. Modified the output plugin (pgoutput) to skip
> publishing the changes if the relation is part of skip schema
> publication.
> As a continuation to this, I will work on implementing skipping tables
> from all tables in schema and skipping tables from all tables
> publication.
>
> Attached patch has the implementation for this.
> This feature is for the pg16 version.
> Thoughts?

The feature seems to be useful especially when there are lots of
schemas in a database. However, I don't quite like the syntax. Do we
have 'SKIP' identifier in any of the SQL statements in SQL standard?
Can we think of adding skip_schema_list as an option, something like
below?

CREATE PUBLICATION foo FOR ALL TABLES (skip_schema_list = 's1, s2');
ALTER PUBLICATION foo SET (skip_schema_list = 's1, s2'); - to set
ALTER PUBLICATION foo SET (skip_schema_list = ''); - to reset

Regards,
Bharath Rupireddy.

Re: Skipping schema changes in publication

From

Peter Smith

Date:

26 April 2022, 01:55:21

On Sat, Apr 23, 2022 at 2:09 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Mar 22, 2022 at 12:39 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > This feature adds an option to skip changes of all tables in specified
> > schema while creating publication.
> > This feature is helpful for use cases where the user wants to
> > subscribe to all the changes except for the changes present in a few
> > schemas.
> > Ex:
> > CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
> > OR
> > ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;
> >
> > A new column pnskip is added to table "pg_publication_namespace", to
> > maintain the schemas that the user wants to skip publishing through
> > the publication. Modified the output plugin (pgoutput) to skip
> > publishing the changes if the relation is part of skip schema
> > publication.
> > As a continuation to this, I will work on implementing skipping tables
> > from all tables in schema and skipping tables from all tables
> > publication.
> >
> > Attached patch has the implementation for this.
> > This feature is for the pg16 version.
> > Thoughts?
>
> The feature seems to be useful especially when there are lots of
> schemas in a database. However, I don't quite like the syntax. Do we
> have 'SKIP' identifier in any of the SQL statements in SQL standard?
> Can we think of adding skip_schema_list as an option, something like
> below?
>
> CREATE PUBLICATION foo FOR ALL TABLES (skip_schema_list = 's1, s2');
> ALTER PUBLICATION foo SET (skip_schema_list = 's1, s2'); - to set
> ALTER PUBLICATION foo SET (skip_schema_list = ''); - to reset
>

I had been wondering for some time if there was any way to introduce a
more flexible pattern matching into PUBLICATION but without bloating
the syntax. Maybe your idea to use an option for the "skip" gives a
way to do it...

For example, if we could use regex (for <schemaname>.<tablename>
patterns) for the option value then....

~~

e.g.1. Exclude certain tables:

// do NOT publish any tables of schemas s1,s2
CREATE PUBLICATION foo FOR ALL TABLES (exclude_match = '(s1\..*)|(s2\..*)');

// do NOT publish my secret tables (those called "mysecretXXX")
CREATE PUBLICATION foo FOR ALL TABLES (exclude_match = '(.*\.mysecret.*)');

~~

e.g.2. Only allow certain tables.

// ONLY publish my tables (those called "mytableXXX")
CREATE PUBLICATION foo FOR ALL TABLES (subset_match = '(.*\.mytable.*)');

// So following is equivalent to FOR ALL TABLES IN SCHEMA s1
CREATE PUBLICATION foo FOR ALL TABLES (subset_match = '(s1\..*)');

------
Kind Regards,
Peter Smith.
Fujitsu Australia

RE: Skipping schema changes in publication

From

"osumi.takamichi@fujitsu.com"

Date:

26 April 2022, 06:02:46

On Thursday, April 21, 2022 12:15 PM vignesh C <vignesh21@gmail.com> wrote:
> Updated patch by changing the syntax to use EXCEPT instead of SKIP.
Hi


This is my review comments on the v2 patch.

(1) gram.y

I think we can make a unified function that merges
preprocess_alltables_pubobj_list with check_except_in_pubobj_list.

With regard to preprocess_alltables_pubobj_list,
we don't use the 2nd argument "location" in this function.

(2) create_publication.sgml

+  <para>
+   Create a publication that publishes all changes in all the tables except for
+   the changes of <structname>users</structname> and
+   <structname>departments</structname> table;

This sentence should end ":" not ";".

(3) publication.out & publication.sql

+-- fail - can't set except table to schema  publication
+ALTER PUBLICATION testpub_forschema SET EXCEPT TABLE testpub_tbl1;

There is one unnecessary space in the comment.
Kindly change from "schema  publication" to "schema publication".

(4) pg_dump.c & describe.c

In your first email of this thread, you explained this feature
is for PG16. Don't we need additional branch for PG16 ?

@@ -6322,6 +6328,21 @@ describePublications(const char *pattern)
                        }
                }

+               if (pset.sversion >= 150000)
+               {


@@ -4162,7 +4164,7 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
        /* Collect all publication membership info. */
        if (fout->remoteVersion >= 150000)
                appendPQExpBufferStr(query,
-                                                        "SELECT tableoid, oid, prpubid, prrelid, "
+                                                        "SELECT tableoid, oid, prpubid, prrelid, prexcept,"


(5) psql-ref.sgml

+        If <literal>+</literal> is appended to the command name, the tables,
+        except tables and schemas associated with each publication are shown as
+        well.

I'm not sure if "except tables" is a good description.
I suggest "excluded tables". This applies to the entire patch,
in case if this is reasonable suggestion.


Best Regards,
    Takamichi Osumi

Re: Skipping schema changes in publication

From

vignesh C

Date:

27 April 2022, 12:50:11

On Tue, Apr 26, 2022 at 11:32 AM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Thursday, April 21, 2022 12:15 PM vignesh C <vignesh21@gmail.com> wrote:
> > Updated patch by changing the syntax to use EXCEPT instead of SKIP.
> Hi
>
>
> This is my review comments on the v2 patch.
>
> (1) gram.y
>
> I think we can make a unified function that merges
> preprocess_alltables_pubobj_list with check_except_in_pubobj_list.
>
> With regard to preprocess_alltables_pubobj_list,
> we don't use the 2nd argument "location" in this function.

Removed location and made a unified function.

> (2) create_publication.sgml
>
> +  <para>
> +   Create a publication that publishes all changes in all the tables except for
> +   the changes of <structname>users</structname> and
> +   <structname>departments</structname> table;
>
> This sentence should end ":" not ";".

Modified

> (3) publication.out & publication.sql
>
> +-- fail - can't set except table to schema  publication
> +ALTER PUBLICATION testpub_forschema SET EXCEPT TABLE testpub_tbl1;
>
> There is one unnecessary space in the comment.
> Kindly change from "schema  publication" to "schema publication".

Modified

> (4) pg_dump.c & describe.c
>
> In your first email of this thread, you explained this feature
> is for PG16. Don't we need additional branch for PG16 ?
>
> @@ -6322,6 +6328,21 @@ describePublications(const char *pattern)
>                         }
>                 }
>
> +               if (pset.sversion >= 150000)
> +               {
>
>
> @@ -4162,7 +4164,7 @@ getPublicationTables(Archive *fout, TableInfo tblinfo[], int numTables)
>         /* Collect all publication membership info. */
>         if (fout->remoteVersion >= 150000)
>                 appendPQExpBufferStr(query,
> -                                                        "SELECT tableoid, oid, prpubid, prrelid, "
> +                                                        "SELECT tableoid, oid, prpubid, prrelid, prexcept,"
>

Modified by adding a comment saying "FIXME: 150000 should be changed
to 160000 later for PG16."

> (5) psql-ref.sgml
>
> +        If <literal>+</literal> is appended to the command name, the tables,
> +        except tables and schemas associated with each publication are shown as
> +        well.
>
> I'm not sure if "except tables" is a good description.
> I suggest "excluded tables". This applies to the entire patch,
> in case if this is reasonable suggestion.

Modified it in most of the places where it was applicable. I felt the
usage was ok in a few places.

Thanks for the comments, the attached v3 patch has the changes for the same.

Regards.
Vignesh

Attachment

v3-0001-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch

RE: Skipping schema changes in publication

From

"osumi.takamichi@fujitsu.com"

Date:

28 April 2022, 11:20:52

On Wednesday, April 27, 2022 9:50 PM vignesh C <vignesh21@gmail.com> wrote:
> Thanks for the comments, the attached v3 patch has the changes for the same.
Hi

Thank you for updating the patch. Several minor comments on v3.

(1) commit message

The new syntax allows specifying schemas. For example:
CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
OR
ALTER PUBLICATION pub1 ADD EXCEPT TABLE t1,t2;

We have above sentence, but it looks better
to make the description a bit more accurate.

Kindly change
From :
"The new syntax allows specifying schemas"
To :
"The new syntax allows specifying excluded relations"

Also, kindly change "OR" to "or",
because this description is not syntax.

(2) publication_add_relation

@@ -396,6 +400,9 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
                ObjectIdGetDatum(pubid);
        values[Anum_pg_publication_rel_prrelid - 1] =
                ObjectIdGetDatum(relid);
+       values[Anum_pg_publication_rel_prexcept - 1] =
+               BoolGetDatum(pri->except);
+

        /* Add qualifications, if available */

It would be better to remove the blank line,
because with this change, we'll have two blank
lines in a row.

(3) pg_dump.h & pg_dump_sort.c

@@ -80,6 +80,7 @@ typedef enum
        DO_REFRESH_MATVIEW,
        DO_POLICY,
        DO_PUBLICATION,
+       DO_PUBLICATION_EXCEPT_REL,
        DO_PUBLICATION_REL,
        DO_PUBLICATION_TABLE_IN_SCHEMA,
        DO_SUBSCRIPTION

@@ -90,6 +90,7 @@ enum dbObjectTypePriorities
        PRIO_FK_CONSTRAINT,
        PRIO_POLICY,
        PRIO_PUBLICATION,
+       PRIO_PUBLICATION_EXCEPT_REL,
        PRIO_PUBLICATION_REL,
        PRIO_PUBLICATION_TABLE_IN_SCHEMA,
        PRIO_SUBSCRIPTION,
@@ -144,6 +145,7 @@ static const int dbObjectTypePriority[] =
        PRIO_REFRESH_MATVIEW,           /* DO_REFRESH_MATVIEW */
        PRIO_POLICY,                            /* DO_POLICY */
        PRIO_PUBLICATION,                       /* DO_PUBLICATION */
+       PRIO_PUBLICATION_EXCEPT_REL,    /* DO_PUBLICATION_EXCEPT_REL */
        PRIO_PUBLICATION_REL,           /* DO_PUBLICATION_REL */
        PRIO_PUBLICATION_TABLE_IN_SCHEMA,       /* DO_PUBLICATION_TABLE_IN_SCHEMA */
        PRIO_SUBSCRIPTION                       /* DO_SUBSCRIPTION */

How about having similar order between
pg_dump.h and pg_dump_sort.c, like
we'll add DO_PUBLICATION_EXCEPT_REL
after DO_PUBLICATION_REL in pg_dump.h ?


(4) GetAllTablesPublicationRelations

+       /*
+        * pg_publication_rel and pg_publication_namespace  will only have except
+        * tables in case of all tables publication, no need to pass except flag
+        * to get the relations.
+        */
+       List       *exceptpubtablelist = GetPublicationRelations(pubid, PUBLICATION_PART_ALL);
+

There is one unnecessary space in a comment
"...pg_publication_namespace  will only have...". Kindly remove it.

Then, how about diving the variable declaration and
the insertion of the return value of GetPublicationRelations ?
That might be aligned with other places in this file.

(5) GetTopMostAncestorInPublication


@@ -302,8 +303,9 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level
        foreach(lc, ancestors)
        {
                Oid                     ancestor = lfirst_oid(lc);
-               List       *apubids = GetRelationPublications(ancestor);
+               List       *apubids = GetRelationPublications(ancestor, false);
                List       *aschemaPubids = NIL;
+               List       *aexceptpubids;

                level++;

@@ -317,7 +319,9 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level
                else
                {
                        aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
-                       if (list_member_oid(aschemaPubids, puboid))
+                       aexceptpubids = GetRelationPublications(ancestor, true);
+                       if (list_member_oid(aschemaPubids, puboid) ||
+                               (puballtables && !list_member_oid(aexceptpubids, puboid)))
                        {
                                topmost_relid = ancestor;

It seems we forgot to call list_free for "aexceptpubids".


Best Regards,
    Takamichi Osumi

Re: Skipping schema changes in publication

From

Amit Kapila

Date:

28 April 2022, 11:31:53

On Fri, Apr 22, 2022 at 9:39 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Mar 22, 2022 at 12:39 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > This feature adds an option to skip changes of all tables in specified
> > schema while creating publication.
> > This feature is helpful for use cases where the user wants to
> > subscribe to all the changes except for the changes present in a few
> > schemas.
> > Ex:
> > CREATE PUBLICATION pub1 FOR ALL TABLES SKIP ALL TABLES IN SCHEMA s1,s2;
> > OR
> > ALTER PUBLICATION pub1 ADD SKIP ALL TABLES IN SCHEMA s1,s2;
> >
>
> The feature seems to be useful especially when there are lots of
> schemas in a database. However, I don't quite like the syntax. Do we
> have 'SKIP' identifier in any of the SQL statements in SQL standard?
>

After discussion, it seems EXCEPT is a preferred choice and the same
is used in the other existing syntax as well.

> Can we think of adding skip_schema_list as an option, something like
> below?
>
> CREATE PUBLICATION foo FOR ALL TABLES (skip_schema_list = 's1, s2');
> ALTER PUBLICATION foo SET (skip_schema_list = 's1, s2'); - to set
> ALTER PUBLICATION foo SET (skip_schema_list = ''); - to reset
>

Yeah, that is also an option but it seems it will be difficult to
extend if want to support "all columns except (c1, ..)" for the column
list feature.

The other thing to decide is for which all objects we want to support
EXCEPT clause as it may not be useful for everything as indicated by
Peter E. and Euler. We have seen that Oracle supports "all columns
except (c1, ..)" [1] and MySQL seems to support for tables [2]. I
guess we should restrict ourselves to those two cases for now and then
we can extend it later for schemas if required or people agree. Also,
we should see the syntax we choose here should be extendable.

Another idea that occurred to me today for tables this is as follows:
1. Allow to mention except during create publication ... For All Tables.
CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
2. Allow to Reset it. This new syntax will reset all objects in the
publications.
Alter Publication ... RESET;
3. Allow to add it to an existing publication
Alter Publication ... Add ALL TABLES [EXCEPT TABLE t1,t2];

I think it can be extended in a similar way for schema syntax as well.

[1] - https://dev.mysql.com/doc/refman/5.7/en/change-replication-filter.html
[2] -
https://docs.oracle.com/en/cloud/paas/goldengate-cloud/gwuad/selecting-columns.html#GUID-9A851C8B-48F7-43DF-8D98-D086BE069E20

-- 
With Regards,
Amit Kapila.

Re: Skipping schema changes in publication

From

vignesh C

Date:

29 April 2022, 11:42:59

On Thu, Apr 28, 2022 at 4:50 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Wednesday, April 27, 2022 9:50 PM vignesh C <vignesh21@gmail.com> wrote:
> > Thanks for the comments, the attached v3 patch has the changes for the same.
> Hi
>
> Thank you for updating the patch. Several minor comments on v3.
>
> (1) commit message
>
> The new syntax allows specifying schemas. For example:
> CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> OR
> ALTER PUBLICATION pub1 ADD EXCEPT TABLE t1,t2;
>
> We have above sentence, but it looks better
> to make the description a bit more accurate.
>
> Kindly change
> From :
> "The new syntax allows specifying schemas"
> To :
> "The new syntax allows specifying excluded relations"
>
> Also, kindly change "OR" to "or",
> because this description is not syntax.

Slightly reworded and modified

> (2) publication_add_relation
>
> @@ -396,6 +400,9 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
>                 ObjectIdGetDatum(pubid);
>         values[Anum_pg_publication_rel_prrelid - 1] =
>                 ObjectIdGetDatum(relid);
> +       values[Anum_pg_publication_rel_prexcept - 1] =
> +               BoolGetDatum(pri->except);
> +
>
>         /* Add qualifications, if available */
>
> It would be better to remove the blank line,
> because with this change, we'll have two blank
> lines in a row.

Modified

> (3) pg_dump.h & pg_dump_sort.c
>
> @@ -80,6 +80,7 @@ typedef enum
>         DO_REFRESH_MATVIEW,
>         DO_POLICY,
>         DO_PUBLICATION,
> +       DO_PUBLICATION_EXCEPT_REL,
>         DO_PUBLICATION_REL,
>         DO_PUBLICATION_TABLE_IN_SCHEMA,
>         DO_SUBSCRIPTION
>
> @@ -90,6 +90,7 @@ enum dbObjectTypePriorities
>         PRIO_FK_CONSTRAINT,
>         PRIO_POLICY,
>         PRIO_PUBLICATION,
> +       PRIO_PUBLICATION_EXCEPT_REL,
>         PRIO_PUBLICATION_REL,
>         PRIO_PUBLICATION_TABLE_IN_SCHEMA,
>         PRIO_SUBSCRIPTION,
> @@ -144,6 +145,7 @@ static const int dbObjectTypePriority[] =
>         PRIO_REFRESH_MATVIEW,           /* DO_REFRESH_MATVIEW */
>         PRIO_POLICY,                            /* DO_POLICY */
>         PRIO_PUBLICATION,                       /* DO_PUBLICATION */
> +       PRIO_PUBLICATION_EXCEPT_REL,    /* DO_PUBLICATION_EXCEPT_REL */
>         PRIO_PUBLICATION_REL,           /* DO_PUBLICATION_REL */
>         PRIO_PUBLICATION_TABLE_IN_SCHEMA,       /* DO_PUBLICATION_TABLE_IN_SCHEMA */
>         PRIO_SUBSCRIPTION                       /* DO_SUBSCRIPTION */
>
> How about having similar order between
> pg_dump.h and pg_dump_sort.c, like
> we'll add DO_PUBLICATION_EXCEPT_REL
> after DO_PUBLICATION_REL in pg_dump.h ?
>

Modified

> (4) GetAllTablesPublicationRelations
>
> +       /*
> +        * pg_publication_rel and pg_publication_namespace  will only have except
> +        * tables in case of all tables publication, no need to pass except flag
> +        * to get the relations.
> +        */
> +       List       *exceptpubtablelist = GetPublicationRelations(pubid, PUBLICATION_PART_ALL);
> +
>
> There is one unnecessary space in a comment
> "...pg_publication_namespace  will only have...". Kindly remove it.
>
> Then, how about diving the variable declaration and
> the insertion of the return value of GetPublicationRelations ?
> That might be aligned with other places in this file.

Modified

> (5) GetTopMostAncestorInPublication
>
>
> @@ -302,8 +303,9 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level
>         foreach(lc, ancestors)
>         {
>                 Oid                     ancestor = lfirst_oid(lc);
> -               List       *apubids = GetRelationPublications(ancestor);
> +               List       *apubids = GetRelationPublications(ancestor, false);
>                 List       *aschemaPubids = NIL;
> +               List       *aexceptpubids;
>
>                 level++;
>
> @@ -317,7 +319,9 @@ GetTopMostAncestorInPublication(Oid puboid, List *ancestors, int *ancestor_level
>                 else
>                 {
>                         aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
> -                       if (list_member_oid(aschemaPubids, puboid))
> +                       aexceptpubids = GetRelationPublications(ancestor, true);
> +                       if (list_member_oid(aschemaPubids, puboid) ||
> +                               (puballtables && !list_member_oid(aexceptpubids, puboid)))
>                         {
>                                 topmost_relid = ancestor;
>
> It seems we forgot to call list_free for "aexceptpubids".

Modified

The attached v4 patch has the changes for the same.

Regards,
Vignesh

Attachment

v4-0001-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch

Re: Skipping schema changes in publication

From

Peter Smith

Date:

03 May 2022, 08:54:33

On Thu, Apr 28, 2022 at 9:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
...
> Another idea that occurred to me today for tables this is as follows:
> 1. Allow to mention except during create publication ... For All Tables.
> CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> 2. Allow to Reset it. This new syntax will reset all objects in the
> publications.
> Alter Publication ... RESET;
> 3. Allow to add it to an existing publication
> Alter Publication ... Add ALL TABLES [EXCEPT TABLE t1,t2];
>
> I think it can be extended in a similar way for schema syntax as well.
>

Consider if the user does
CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
ALTER PUBLICATION pub1 ADD ALL TABLES EXCEPT t3,t4;

What does it mean?
e.g. Is there only one exception list that is modified? Or did the ADD
ALL TABLES override all meaning of the original list?
e.g. Are we now skipping t1,t2,t3,t4, or are we now only skipping t3,t4?

~~~

Here is a similar example, where the ADD TABLE seems confusing to me
when it intersects with a prior EXCEPT
e.g.
CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT t1,t2; // ok
ALTER PUBLICATION pub1 ADD TABLE t1; ???

What does it mean?
e.g. Does the explicit ADD TABLE override the original exception list?
e.g. Is t1 published now or should that ALTER have caused an error?

~~

It feels like there are too many tricky rules when using EXCEPT with
ALTER PUBLICATION. I guess complexities can be described in the
documentation but IMO it would be better if the ALTER syntax could be
unambiguous in the first place. So perhaps the rules should be more
restrictive (e.g. just disallow ALTER ... ADD any table that overlaps
the existing EXCEPT list ??)

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

Re: Skipping schema changes in publication

From

Amit Kapila

Date:

04 May 2022, 04:14:53

On Tue, May 3, 2022 at 2:24 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Apr 28, 2022 at 9:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> ...
> > Another idea that occurred to me today for tables this is as follows:
> > 1. Allow to mention except during create publication ... For All Tables.
> > CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> > 2. Allow to Reset it. This new syntax will reset all objects in the
> > publications.
> > Alter Publication ... RESET;
> > 3. Allow to add it to an existing publication
> > Alter Publication ... Add ALL TABLES [EXCEPT TABLE t1,t2];
> >
> > I think it can be extended in a similar way for schema syntax as well.
> >
>
> Consider if the user does
> CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> ALTER PUBLICATION pub1 ADD ALL TABLES EXCEPT t3,t4;
>
> What does it mean?
> e.g. Is there only one exception list that is modified? Or did the ADD
> ALL TABLES override all meaning of the original list?
> e.g. Are we now skipping t1,t2,t3,t4, or are we now only skipping t3,t4?
>

This won't be allowed. We won't allow changing ALL TABLES publication
unless the user first performs RESET. This is the purpose of providing
the RESET variant.

> ~~~
>
> Here is a similar example, where the ADD TABLE seems confusing to me
> when it intersects with a prior EXCEPT
> e.g.
> CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT t1,t2; // ok
> ALTER PUBLICATION pub1 ADD TABLE t1; ???
>
> What does it mean?
> e.g. Does the explicit ADD TABLE override the original exception list?
> e.g. Is t1 published now or should that ALTER have caused an error?
>

This won't be allowed either. We don't allow to Add/Drop from All
Tables publication unless the user performs a RESET. This is true even
today except that we don't have a RESET syntax.

> ~~
>
> It feels like there are too many tricky rules when using EXCEPT with
> ALTER PUBLICATION. I guess complexities can be described in the
> documentation but IMO it would be better if the ALTER syntax could be
> unambiguous in the first place.
>

Agreed.

> So perhaps the rules should be more
> restrictive (e.g. just disallow ALTER ... ADD any table that overlaps
> the existing EXCEPT list ??)
>

I think the current proposal seems to be restrictive enough to avoid
any tricky issues. Do you see any other problem?


-- 
With Regards,
Amit Kapila.

Re: Skipping schema changes in publication

From

Peter Eisentraut

Date:

04 May 2022, 13:34:54

On 14.04.22 15:47, Peter Eisentraut wrote:
> That said, I'm not sure this feature is worth the trouble.  If this is 
> useful, what about "whole database except these schemas"?  What about 
> "create this database from this template except these schemas".  This 
> could get out of hand.  I think we should encourage users to group their 
> object the way they want and not offer these complicated negative 
> selection mechanisms.

Another problem in general with this "all except these" way of 
specifying things is that you need to track negative dependencies.

For example, assume you can't add a table to a publication unless it has 
a replica identity.  Now, if you have a publication p1 that says 
includes "all tables except t1", you now have to check p1 whenever a new 
table is created, even though the new table has no direct dependency 
link with p1.  So in more general cases, you would have to check all 
existing objects to see whether their specification is in conflict with 
the new object being created.

Now publications don't actually work that way, so it's not a real 
problem right now, but similar things could work like that.  So I think 
it's worth thinking this through a bit.

Re: Skipping schema changes in publication

From

Amit Kapila

Date:

05 May 2022, 03:50:36

On Wed, May 4, 2022 at 7:05 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 14.04.22 15:47, Peter Eisentraut wrote:
> > That said, I'm not sure this feature is worth the trouble.  If this is
> > useful, what about "whole database except these schemas"?  What about
> > "create this database from this template except these schemas".  This
> > could get out of hand.  I think we should encourage users to group their
> > object the way they want and not offer these complicated negative
> > selection mechanisms.
>
> Another problem in general with this "all except these" way of
> specifying things is that you need to track negative dependencies.
>
> For example, assume you can't add a table to a publication unless it has
> a replica identity.  Now, if you have a publication p1 that says
> includes "all tables except t1", you now have to check p1 whenever a new
> table is created, even though the new table has no direct dependency
> link with p1.  So in more general cases, you would have to check all
> existing objects to see whether their specification is in conflict with
> the new object being created.
>

Yes, I think we should avoid adding such negative dependencies. We
have carefully avoided such dependencies during row filter, column
list work where we don't try to perform DDL time verification.
However, it is not clear to me how this proposal is related to this
example or in general about tracking negative dependencies? AFAIR, we
currently have such a check while changing persistence of logged table
(logged to unlogged, see ATPrepChangePersistence) where we cannot
allow changing persistence if that relation is part of some
publication. But as per my understanding, this feature shouldn't add
any such new dependencies. I agree that we have to ensure that
existing checks shouldn't break due to this feature.

> Now publications don't actually work that way, so it's not a real
> problem right now, but similar things could work like that.  So I think
> it's worth thinking this through a bit.
>

This is a good point and I agree that we should be careful to not add
some new negative dependencies unless it is really required but I
can't see how this proposal will make it more prone to such checks.

-- 
With Regards,
Amit Kapila.

Re: Skipping schema changes in publication

From

Amit Kapila

Date:

05 May 2022, 04:12:32

On Thu, May 5, 2022 at 9:20 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, May 4, 2022 at 7:05 PM Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
> >
> > On 14.04.22 15:47, Peter Eisentraut wrote:
> > > That said, I'm not sure this feature is worth the trouble.  If this is
> > > useful, what about "whole database except these schemas"?  What about
> > > "create this database from this template except these schemas".  This
> > > could get out of hand.  I think we should encourage users to group their
> > > object the way they want and not offer these complicated negative
> > > selection mechanisms.
> >
> > Another problem in general with this "all except these" way of
> > specifying things is that you need to track negative dependencies.
> >
> > For example, assume you can't add a table to a publication unless it has
> > a replica identity.  Now, if you have a publication p1 that says
> > includes "all tables except t1", you now have to check p1 whenever a new
> > table is created, even though the new table has no direct dependency
> > link with p1.  So in more general cases, you would have to check all
> > existing objects to see whether their specification is in conflict with
> > the new object being created.
> >
>
> Yes, I think we should avoid adding such negative dependencies. We
> have carefully avoided such dependencies during row filter, column
> list work where we don't try to perform DDL time verification.
> However, it is not clear to me how this proposal is related to this
> example or in general about tracking negative dependencies?
>

I mean to say that even if we have such a restriction, it would apply
to "for all tables" or other publications as well. In your example,
consider one wants to Alter a table and remove its replica identity,
we have to check whether the table is part of any publication similar
to what we are doing for relation persistence in
ATPrepChangePersistence.

> AFAIR, we
> currently have such a check while changing persistence of logged table
> (logged to unlogged, see ATPrepChangePersistence) where we cannot
> allow changing persistence if that relation is part of some
> publication. But as per my understanding, this feature shouldn't add
> any such new dependencies. I agree that we have to ensure that
> existing checks shouldn't break due to this feature.
>
> > Now publications don't actually work that way, so it's not a real
> > problem right now, but similar things could work like that.  So I think
> > it's worth thinking this through a bit.
> >
>
> This is a good point and I agree that we should be careful to not add
> some new negative dependencies unless it is really required but I
> can't see how this proposal will make it more prone to such checks.
>

-- 
With Regards,
Amit Kapila.

Re: Skipping schema changes in publication

From

Peter Smith

Date:

06 May 2022, 02:35:16

On Thu, Apr 28, 2022 at 9:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
...
>
> Another idea that occurred to me today for tables this is as follows:
> 1. Allow to mention except during create publication ... For All Tables.
> CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> 2. Allow to Reset it. This new syntax will reset all objects in the
> publications.
> Alter Publication ... RESET;
> 3. Allow to add it to an existing publication
> Alter Publication ... Add ALL TABLES [EXCEPT TABLE t1,t2];
>
> I think it can be extended in a similar way for schema syntax as well.
>

If the proposed syntax ALTER PUBLICATION ... RESET will reset all the
objects in the publication then there still seems simple way to remove
only the EXCEPT list but leave everything else intact. IIUC to clear
just the EXCEPT list would require a 2 step process - 1) ALTER ...
RESET then 2) ALTER ... ADD ALL TABLES again.

I was wondering if it might be useful to have a variation that *only*
resets the EXCEPT list, but still leaves everything else as-is?

So, instead of:
ALTER PUBLICATION pubname RESET

use a syntax something like:
ALTER PUBLICATION pubname RESET {ALL | EXCEPT}
or
ALTER PUBLICATION pubname RESET [EXCEPT]

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Skipping schema changes in publication

From

vignesh C

Date:

10 May 2022, 03:38:48

On Fri, May 6, 2022 at 8:05 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Apr 28, 2022 at 9:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> ...
> >
> > Another idea that occurred to me today for tables this is as follows:
> > 1. Allow to mention except during create publication ... For All Tables.
> > CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> > 2. Allow to Reset it. This new syntax will reset all objects in the
> > publications.
> > Alter Publication ... RESET;
> > 3. Allow to add it to an existing publication
> > Alter Publication ... Add ALL TABLES [EXCEPT TABLE t1,t2];
> >
> > I think it can be extended in a similar way for schema syntax as well.
> >
>
> If the proposed syntax ALTER PUBLICATION ... RESET will reset all the
> objects in the publication then there still seems simple way to remove
> only the EXCEPT list but leave everything else intact. IIUC to clear
> just the EXCEPT list would require a 2 step process - 1) ALTER ...
> RESET then 2) ALTER ... ADD ALL TABLES again.
>
> I was wondering if it might be useful to have a variation that *only*
> resets the EXCEPT list, but still leaves everything else as-is?
>
> So, instead of:
> ALTER PUBLICATION pubname RESET

+1 for this syntax as this syntax can be extendable to include options
like (except/all/etc) later.
Currently we can support this syntax and can be extended later based
on the requirements.

The new feature will handle the various use cases based on the
behavior given below:
-- CREATE Publication with EXCEPT TABLE syntax
CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2; -- ok
Alter Publication pub1 RESET;
-- All Tables and options are reset similar to creating publication
without any publication object and publication option (create
publication pub1)
\dRp+ pub1
Publication pub2
Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
---------+------------+---------+---------+---------+-----------+----------
vignesh | f | t | t | t | t | f
(1 row)

-- Can add except table after reset of publication
ALTER PUBLICATION pub1 Add ALL TABLES EXCEPT TABLE t1,t2; -- ok

-- Cannot add except table without reset of publication
ALTER PUBLICATION pub1 Add EXCEPT TABLE t3,t4; -- not ok, need to be reset

Alter Publication pub1 RESET;
-- Cannot add table to ALL TABLES Publication
ALTER PUBLICATION pub1 Add ALL TABLES EXCEPT TABLE t1,t2, t3, t4,
TABLE t5; -- not ok, ALL TABLES Publications does not support
including of TABLES

Alter Publication pub1 RESET;
-- Cannot add table to ALL TABLES Publication
ALTER PUBLICATION pub1 Add ALL TABLES TABLE t1,t2; -- not ok, ALL
TABLES Publications does not support including of TABLES

-- Cannot add ALL TABLES IN SCHEMA to ALL TABLES Publication
ALTER PUBLICATION pub1 Add ALL TABLES ALL TABLES IN SCHEMA sch1, sch2;
-- not ok, ALL TABLES Publications does not support including of ALL
TABLES IN SCHEMA

-- Existing syntax should work as it is
CREATE PUBLICATION pub1 FOR TABLE t1;
ALTER PUBLICATION pub1 ADD TABLE t1; -- ok, existing ALTER should work
as it is (ok without reset)
ALTER PUBLICATION pub1 ADD ALL TABLES IN SCHEMA sch1; -- ok, existing
ALTER should work as it is (ok without reset)
ALTER PUBLICATION pub1 DROP TABLE t1; -- ok, existing ALTER should
work as it is (ok without reset)
ALTER PUBLICATION pub1 DROP ALL TABLES IN SCHEMA sch1; -- ok, existing
ALTER should work as it is (ok without reset)
ALTER PUBLICATION pub1 SET TABLE t1; -- ok, existing ALTER should work
as it is (ok without reset)
ALTER PUBLICATION pub1 SET ALL TABLES IN SCHEMA sch1; -- ok, existing
ALTER should work as it is (ok without reset)

I will modify the patch to handle this.

Regards,
Vignesh

Re: Skipping schema changes in publication

From

vignesh C

Date:

12 May 2022, 04:24:39

On Tue, May 10, 2022 at 9:08 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, May 6, 2022 at 8:05 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Thu, Apr 28, 2022 at 9:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > ...
> > >
> > > Another idea that occurred to me today for tables this is as follows:
> > > 1. Allow to mention except during create publication ... For All Tables.
> > > CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> > > 2. Allow to Reset it. This new syntax will reset all objects in the
> > > publications.
> > > Alter Publication ... RESET;
> > > 3. Allow to add it to an existing publication
> > > Alter Publication ... Add ALL TABLES [EXCEPT TABLE t1,t2];
> > >
> > > I think it can be extended in a similar way for schema syntax as well.
> > >
> >
> > If the proposed syntax ALTER PUBLICATION ... RESET will reset all the
> > objects in the publication then there still seems simple way to remove
> > only the EXCEPT list but leave everything else intact. IIUC to clear
> > just the EXCEPT list would require a 2 step process - 1) ALTER ...
> > RESET then 2) ALTER ... ADD ALL TABLES again.
> >
> > I was wondering if it might be useful to have a variation that *only*
> > resets the EXCEPT list, but still leaves everything else as-is?
> >
> > So, instead of:
> > ALTER PUBLICATION pubname RESET
>
> +1 for this syntax as this syntax can be extendable to include options
> like (except/all/etc) later.
> Currently we can support this syntax and can be extended later based
> on the requirements.

The attached patch has the implementation for "ALTER PUBLICATION
pubname RESET". This command will reset the publication to default
state which includes resetting the publication options, setting ALL
TABLES option to false and dropping the relations and schemas that are
associated with the publication.

Regards,
Vignesh

Attachment

v1-0001-Add-RESET-option-to-Alter-Publication-which-will-.patch

Re: Skipping schema changes in publication

From

Peter Smith

Date:

13 May 2022, 04:07:17

On Thu, May 12, 2022 at 2:24 PM vignesh C <vignesh21@gmail.com> wrote:
>
...
> The attached patch has the implementation for "ALTER PUBLICATION
> pubname RESET". This command will reset the publication to default
> state which includes resetting the publication options, setting ALL
> TABLES option to false and dropping the relations and schemas that are
> associated with the publication.
>

Please see below my review comments for the v1-0001 (RESET) patch

======

1. Commit message

This patch adds a new RESET option to ALTER PUBLICATION which

Wording: "RESET option" -> "RESET clause"

~~~

2. doc/src/sgml/ref/alter_publication.sgml

+  <para>
+   The <literal>RESET</literal> clause will reset the publication to default
+   state which includes resetting the publication options, setting
+   <literal>ALL TABLES</literal> option to <literal>false</literal>
and drop the
+   relations and schemas that are associated with the publication.
   </para>

2a. Wording: "to default state" -> "to the default state"

2b. Wording: "and drop the relations..." -> "and dropping all relations..."

~~~

3. doc/src/sgml/ref/alter_publication.sgml

+   invoking user to be a superuser.  <literal>RESET</literal> of publication
+   requires invoking user to be a superuser. To alter the owner, you must also

Wording: "requires invoking user" -> "requires the invoking user"

~~~

4. doc/src/sgml/ref/alter_publication.sgml - Example

@@ -207,6 +220,12 @@ ALTER PUBLICATION sales_publication ADD ALL
TABLES IN SCHEMA marketing, sales;
    <structname>production_publication</structname>:
 <programlisting>
 ALTER PUBLICATION production_publication ADD TABLE users,
departments, ALL TABLES IN SCHEMA production;
+</programlisting></para>
+
+  <para>
+   Resetting the publication <structname>production_publication</structname>:
+<programlisting>
+ALTER PUBLICATION production_publication RESET;

Wording: "Resetting the publication" -> "Reset the publication"

~~~

5. src/backend/commands/publicationcmds.c

+ /* Check and reset the options */

IMO the code can just reset all these options unconditionally. I did
not see the point to check for existing option values first. I feel
the simpler code outweighs any negligible performance difference in
this case.

~~~

6. src/backend/commands/publicationcmds.c

+ /* Check and reset the options */

Somehow it seemed a pity having to hardcode all these default values
true/false in multiple places; e.g. the same is already hardcoded in
the parse_publication_options function.

To avoid multiple hard coded bools you could just call the
parse_publication_options with an empty options list. That would set
the defaults which you can then use:
values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactiondefs->insert);

Alternatively, maybe there should be #defines to use instead of having
the scattered hardcoded bool defaults:
#define PUBACTION_DEFAULT_INSERT true
#define PUBACTION_DEFAULT_UPDATE true
etc

~~~

7. src/include/nodes/parsenodes.h

@@ -4033,7 +4033,8 @@ typedef enum AlterPublicationAction
 {
  AP_AddObjects, /* add objects to publication */
  AP_DropObjects, /* remove objects from publication */
- AP_SetObjects /* set list of objects */
+ AP_SetObjects, /* set list of objects */
+ AP_ReSetPublication /* reset the publication */
 } AlterPublicationAction;

Unusual case: "AP_ReSetPublication" -> "AP_ResetPublication"

~~~

8. src/test/regress/sql/publication.sql

8a.
+-- Test for RESET PUBLICATION
SUGGESTED
+-- Tests for ALTER PUBLICATION ... RESET

8b.
+-- Verify that 'ALL TABLES' option is reset
SUGGESTED:
+-- Verify that 'ALL TABLES' flag is reset

8c.
+-- Verify that publish option and publish via root option is reset
SUGGESTED:
+-- Verify that publish options and publish_via_partition_root option are reset

8d.
+-- Verify that only superuser can execute RESET publication
SUGGESTED
+-- Verify that only superuser can reset a publication

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Re: Skipping schema changes in publication

From

vignesh C

Date:

14 May 2022, 13:32:54

On Fri, May 13, 2022 at 9:37 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, May 12, 2022 at 2:24 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> ...
> > The attached patch has the implementation for "ALTER PUBLICATION
> > pubname RESET". This command will reset the publication to default
> > state which includes resetting the publication options, setting ALL
> > TABLES option to false and dropping the relations and schemas that are
> > associated with the publication.
> >
>
> Please see below my review comments for the v1-0001 (RESET) patch
>
> ======
>
> 1. Commit message
>
> This patch adds a new RESET option to ALTER PUBLICATION which
>
> Wording: "RESET option" -> "RESET clause"

Modified

> ~~~
>
> 2. doc/src/sgml/ref/alter_publication.sgml
>
> +  <para>
> +   The <literal>RESET</literal> clause will reset the publication to default
> +   state which includes resetting the publication options, setting
> +   <literal>ALL TABLES</literal> option to <literal>false</literal>
> and drop the
> +   relations and schemas that are associated with the publication.
>    </para>
>
> 2a. Wording: "to default state" -> "to the default state"

Modified

> 2b. Wording: "and drop the relations..." -> "and dropping all relations..."

Modified

> ~~~
>
> 3. doc/src/sgml/ref/alter_publication.sgml
>
> +   invoking user to be a superuser.  <literal>RESET</literal> of publication
> +   requires invoking user to be a superuser. To alter the owner, you must also
>
> Wording: "requires invoking user" -> "requires the invoking user"

Modified

> ~~~
>
> 4. doc/src/sgml/ref/alter_publication.sgml - Example
>
> @@ -207,6 +220,12 @@ ALTER PUBLICATION sales_publication ADD ALL
> TABLES IN SCHEMA marketing, sales;
>     <structname>production_publication</structname>:
>  <programlisting>
>  ALTER PUBLICATION production_publication ADD TABLE users,
> departments, ALL TABLES IN SCHEMA production;
> +</programlisting></para>
> +
> +  <para>
> +   Resetting the publication <structname>production_publication</structname>:
> +<programlisting>
> +ALTER PUBLICATION production_publication RESET;
>
> Wording: "Resetting the publication" -> "Reset the publication"

Modified

> ~~~
>
> 5. src/backend/commands/publicationcmds.c
>
> + /* Check and reset the options */
>
> IMO the code can just reset all these options unconditionally. I did
> not see the point to check for existing option values first. I feel
> the simpler code outweighs any negligible performance difference in
> this case.

Modified

> ~~~
>
> 6. src/backend/commands/publicationcmds.c
>
> + /* Check and reset the options */
>
> Somehow it seemed a pity having to hardcode all these default values
> true/false in multiple places; e.g. the same is already hardcoded in
> the parse_publication_options function.
>
> To avoid multiple hard coded bools you could just call the
> parse_publication_options with an empty options list. That would set
> the defaults which you can then use:
> values[Anum_pg_publication_pubinsert - 1] = BoolGetDatum(pubactiondefs->insert);
>
> Alternatively, maybe there should be #defines to use instead of having
> the scattered hardcoded bool defaults:
> #define PUBACTION_DEFAULT_INSERT true
> #define PUBACTION_DEFAULT_UPDATE true
> etc

I have used #define for default value and used it in both the functions.

> ~~~
>
> 7. src/include/nodes/parsenodes.h
>
> @@ -4033,7 +4033,8 @@ typedef enum AlterPublicationAction
>  {
>   AP_AddObjects, /* add objects to publication */
>   AP_DropObjects, /* remove objects from publication */
> - AP_SetObjects /* set list of objects */
> + AP_SetObjects, /* set list of objects */
> + AP_ReSetPublication /* reset the publication */
>  } AlterPublicationAction;
>
> Unusual case: "AP_ReSetPublication" -> "AP_ResetPublication"

Modified

> ~~~
>
> 8. src/test/regress/sql/publication.sql
>
> 8a.
> +-- Test for RESET PUBLICATION
> SUGGESTED
> +-- Tests for ALTER PUBLICATION ... RESET

Modified

> 8b.
> +-- Verify that 'ALL TABLES' option is reset
> SUGGESTED:
> +-- Verify that 'ALL TABLES' flag is reset

Modified

> 8c.
> +-- Verify that publish option and publish via root option is reset
> SUGGESTED:
> +-- Verify that publish options and publish_via_partition_root option are reset

Modified

> 8d.
> +-- Verify that only superuser can execute RESET publication
> SUGGESTED
> +-- Verify that only superuser can reset a publication

Modified

Thanks for the comments, the attached v5 patch has the changes for the
same. Also I have made the changes for SKIP Table based on the new
syntax, the changes for the same are available in
v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch.

Regards,
Vignesh

On Mon, May 16, 2022 at 8:32 AM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Saturday, May 14, 2022 10:33 PM vignesh C <vignesh21@gmail.com> wrote:
> > Thanks for the comments, the attached v5 patch has the changes for the same.
> > Also I have made the changes for SKIP Table based on the new syntax, the
> > changes for the same are available in
> > v5-0002-Skip-publishing-the-tables-specified-in-EXCEPT-TA.patch.
> Hi,
>
>
> Thank you for updating the patch.
> I'll share few minor review comments on v5-0001.
>
>
> (1) doc/src/sgml/ref/alter_publication.sgml
>
> @@ -73,12 +85,13 @@ ALTER PUBLICATION <replaceable class="parameter">name</replaceable> RENAME TO <r
>     Adding a table to a publication additionally requires owning that table.
>     The <literal>ADD ALL TABLES IN SCHEMA</literal> and
>     <literal>SET ALL TABLES IN SCHEMA</literal> to a publication requires the
> -   invoking user to be a superuser.  To alter the owner, you must also be a
> -   direct or indirect member of the new owning role. The new owner must have
> -   <literal>CREATE</literal> privilege on the database.  Also, the new owner
> -   of a <literal>FOR ALL TABLES</literal> or <literal>FOR ALL TABLES IN
> -   SCHEMA</literal> publication must be a superuser. However, a superuser can
> -   change the ownership of a publication regardless of these restrictions.
> +   invoking user to be a superuser.  <literal>RESET</literal> of publication
> +   requires the invoking user to be a superuser. To alter the owner, you must
> ...
>
>
> I suggest to combine the first part of your change with one existing sentence
> before your change, to make our description concise.
>
> FROM:
> "The <literal>ADD ALL TABLES IN SCHEMA</literal> and
> <literal>SET ALL TABLES IN SCHEMA</literal> to a publication requires the
> invoking user to be a superuser.  <literal>RESET</literal> of publication
> requires the invoking user to be a superuser."
>
> TO:
> "The <literal>ADD ALL TABLES IN SCHEMA</literal>,
> <literal>SET ALL TABLES IN SCHEMA</literal> to a publication and
> <literal>RESET</literal> of publication requires the invoking user to be a superuser."

Modified

>
> (2) typo
>
> +++ b/src/backend/commands/publicationcmds.c
> @@ -53,6 +53,13 @@
>  #include "utils/syscache.h"
>  #include "utils/varlena.h"
>
> +#define PUB_ATION_INSERT_DEFAULT true
> +#define PUB_ACTION_UPDATE_DEFAULT true
>
>
> Kindly change
> FROM:
> "PUB_ATION_INSERT_DEFAULT"
> TO:
> "PUB_ACTION_INSERT_DEFAULT"

Modified

>
> (3) src/test/regress/expected/publication.out
>
> +-- Verify that only superuser can reset a publication
> +ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2;
> +SET ROLE regress_publication_user2;
> +ALTER PUBLICATION testpub_reset RESET; -- fail
>
>
> We have "-- fail" for one case in this patch.
> On the other hand, isn't better to add "-- ok" (or "-- success") for
> other successful statements,
> when we consider the entire tests description consistency ?

We generally do not mention success comments for all the success cases
as that might be an overkill. I felt it is better to keep it as it is.
Thoughts?

The attached v6 patch has the changes for the same.

Regards,
Vignesh

On Thu, May 19, 2022 at 1:49 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Below are my review comments for v6-0001.
>
> ======
>
> 1. General.
>
> The patch failed 'publication' tests in the make check phase.
>
> Please add this work to the commit-fest so that the 'cfbot' can report
> such errors sooner.

Added commitfest entry

> ~~~
>
> 2. src/backend/commands/publicationcmds.c - AlterPublicationReset
>
> +/*
> + * Reset the publication.
> + *
> + * Reset the publication options, publication relations and
> publication schemas.
> + */
> +static void
> +AlterPublicationReset(ParseState *pstate, AlterPublicationStmt *stmt,
> + Relation rel, HeapTuple tup)
>
> SUGGESTION (Make the comment similar to the sgml text instead of
> repeating "publication" 4x !)
> /*
>  * Reset the publication options, set the ALL TABLES flag to false, and
>  * drop all relations and schemas that are associated with the publication.
>  */

Modified

> ~~~
>
> 3. src/test/regress/expected/publication.out
>
> make check failed. The diff is below:
>
> @@ -1716,7 +1716,7 @@
>  -- Verify that only superuser can reset a publication
>  ALTER PUBLICATION testpub_reset OWNER TO regress_publication_user2;
>  SET ROLE regress_publication_user2;
> -ALTER PUBLICATION testpub_reset RESET; -- fail
> +ALTER PUBLICATION testpub_reset RESET; -- fail - must be superuser
>  ERROR:  must be superuser to RESET publication
>  SET ROLE regress_publication_user;
>  DROP PUBLICATION testpub_reset;

It passed for me locally because the change was present in the 002
patch. I have moved the change to 001.

The attached v7 patch has the changes for the same.
[1] - https://commitfest.postgresql.org/38/3646/

Regards,
Vignesh

On Sat, May 21, 2022 at 11:06 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, May 20, 2022 at 11:23 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Below are my review comments for v6-0002.
> >
> > ======
> >
> > 1. Commit message.
> > The psql \d family of commands to display excluded tables.
> >
> > SUGGESTION
> > The psql \d family of commands can now display excluded tables.
>
> Modified
>
> > ~~~
> >
> > 2. doc/src/sgml/ref/alter_publication.sgml
> >
> > @@ -22,6 +22,7 @@ PostgreSQL documentation
> >   <refsynopsisdiv>
> >  <synopsis>
> >  ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
> > ADD <replaceable class="parameter">publication_object</replaceable> [,
> > ...]
> > +ALTER PUBLICATION <replaceable class="parameter">name</replaceable>
> > ADD ALL TABLES [ EXCEPT [ TABLE ] exception_object [, ... ] ]
> >
> > The "exception_object" font is wrong. Should look the same as
> > "publication_object"
>
> Modified
>
> > ~~~
> >
> > 3. doc/src/sgml/ref/alter_publication.sgml - Examples
> >
> > @@ -214,6 +220,14 @@ ALTER PUBLICATION sales_publication ADD ALL
> > TABLES IN SCHEMA marketing, sales;
> >  </programlisting>
> >    </para>
> >
> > +  <para>
> > +   Alter publication <structname>production_publication</structname> to publish
> > +   all tables except <structname>users</structname> and
> > +   <structname>departments</structname> tables:
> > +<programlisting>
> > +ALTER PUBLICATION production_publication ADD ALL TABLES EXCEPT TABLE
> > users, departments;
> > +</programlisting></para>
> >
> > Consider using "EXCEPT" instead of "EXCEPT TABLE" because that will
> > show TABLE keyword is optional.
>
> Modified
>
> > ~~~
> >
> > 4. doc/src/sgml/ref/create_publication.sgml
> >
> > An SGML tag error caused building the docs to fail. My fix was
> > previously reported [1].
>
> Modified
>
> > ~~~
> >
> > 5. doc/src/sgml/ref/create_publication.sgml
> >
> > @@ -22,7 +22,7 @@ PostgreSQL documentation
> >   <refsynopsisdiv>
> >  <synopsis>
> >  CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
> > -    [ FOR ALL TABLES
> > +    [ FOR ALL TABLES [ EXCEPT [ TABLE ] exception_object [, ... ] ]
> >
> > The "exception_object" font is wrong. Should look the same as
> > "publication_object"
>
> Modified
>
> > ~~~
> >
> > 6. doc/src/sgml/ref/create_publication.sgml - Examples
> >
> > @@ -351,6 +366,15 @@ CREATE PUBLICATION production_publication FOR
> > TABLE users, departments, ALL TABL
> >  CREATE PUBLICATION sales_publication FOR ALL TABLES IN SCHEMA marketing, sales;
> >  </programlisting></para>
> >
> > +  <para>
> > +   Create a publication that publishes all changes in all the tables except for
> > +   the changes of <structname>users</structname> and
> > +   <structname>departments</structname> table:
> > +<programlisting>
> > +CREATE PUBLICATION mypublication FOR ALL TABLE EXCEPT TABLE users, departments;
> > +</programlisting>
> > +  </para>
> > +
> >
> > 6a.
> > Typo: "FOR ALL TABLE" -> "FOR ALL TABLES"
>
> Modified
>
> > 6b.
> > Consider using "EXCEPT" instead of "EXCEPT TABLE" because that will
> > show TABLE keyword is optional.
>
> Modified
>
> > ~~~
> >
> > 7. src/backend/catalog/pg_publication.c - GetTopMostAncestorInPublication
> >
> > @@ -316,18 +316,25 @@ GetTopMostAncestorInPublication(Oid puboid, List
> > *ancestors, int *ancestor_level
> >   }
> >   else
> >   {
> > - aschemaPubids = GetSchemaPublications(get_rel_namespace(ancestor));
> > - if (list_member_oid(aschemaPubids, puboid))
> > + List    *aschemapubids = NIL;
> > + List    *aexceptpubids = NIL;
> > +
> > + aschemapubids = GetSchemaPublications(get_rel_namespace(ancestor));
> > + aexceptpubids = GetRelationPublications(ancestor, true);
> > + if (list_member_oid(aschemapubids, puboid) ||
> > + (puballtables && !list_member_oid(aexceptpubids, puboid)))
> >   {
> >
> > You could re-write this as multiple conditions instead of one. That
> > could avoid always assigning the 'aexceptpubids', so it might be a
> > more efficient way to write this logic.
>
> Modified
>
> > ~~~
> >
> > 8. src/backend/catalog/pg_publication.c - CheckPublicationDefValues
> >
> > +/*
> > + * Check if the publication has default values
> > + *
> > + * Check the following:
> > + * Publication is having default options
> > + *  Publication is not associated with relations
> > + *  Publication is not associated with schemas
> > + *  Publication is not set with "FOR ALL TABLES"
> > + */
> > +static bool
> > +CheckPublicationDefValues(HeapTuple tup)
> >
> > 8a.
> > Remove the tab. Replace with spaces.
>
> Modified
>
> > 8b.
> > It might be better if this comment order is the same as the logic order.
> > e.g.
> >
> > * Check the following:
> > *  Publication is not set with "FOR ALL TABLES"
> > *  Publication is having default options
> > *  Publication is not associated with schemas
> > *  Publication is not associated with relations
>
> Modified
>
> > ~~~
> >
> > 9. src/backend/catalog/pg_publication.c - AlterPublicationSetAllTables
> >
> > +/*
> > + * Reset the publication.
> > + *
> > + * Reset the publication options, publication relations and
> > publication schemas.
> > + */
> > +static void
> > +AlterPublicationSetAllTables(Relation rel, HeapTuple tup)
> >
> > The function comment and the function name do not seem to match here;
> > something looks like a cut/paste error ??
>
> Modified
>
> > ~~~
> >
> > 10. src/backend/catalog/pg_publication.c - AlterPublicationSetAllTables
> >
> > + /* set all tables option */
> > + values[Anum_pg_publication_puballtables - 1] = BoolGetDatum(true);
> > + replaces[Anum_pg_publication_puballtables - 1] = true;
> >
> > SUGGEST (comment)
> > /* set all ALL TABLES flag */
>
> Modified
>
> > ~~~
> >
> > 11. src/backend/catalog/pg_publication.c - AlterPublication
> >
> > @@ -1501,6 +1579,20 @@ AlterPublication(ParseState *pstate,
> > AlterPublicationStmt *stmt)
> >   aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION,
> >      stmt->pubname);
> >
> > + if (stmt->for_all_tables)
> > + {
> > + bool isdefault = CheckPublicationDefValues(tup);
> > +
> > + if (!isdefault)
> > + ereport(ERROR,
> > + errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > + errmsg("Setting ALL TABLES requires publication \"%s\" to have
> > default values",
> > +    stmt->pubname),
> > + errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
> >
> > The errmsg should start with a lowercase letter.
>
> Modified
>
> > ~~~
> >
> > 12. src/backend/catalog/pg_publication.c - AlterPublication
> >
> > @@ -1501,6 +1579,20 @@ AlterPublication(ParseState *pstate,
> > AlterPublicationStmt *stmt)
> >   aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_PUBLICATION,
> >      stmt->pubname);
> >
> > + if (stmt->for_all_tables)
> > + {
> > + bool isdefault = CheckPublicationDefValues(tup);
> > +
> > + if (!isdefault)
> > + ereport(ERROR,
> > + errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > + errmsg("Setting ALL TABLES requires publication \"%s\" to have
> > default values",
> > +    stmt->pubname),
> > + errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
> >
> > Example test:
> >
> > postgres=# create table t1(a int);
> > CREATE TABLE
> > postgres=# create publication p1 for table t1;
> > CREATE PUBLICATION
> > postgres=# alter publication p1 add all tables except t1;
> > 2022-05-20 14:34:49.301 AEST [21802] ERROR:  Setting ALL TABLES
> > requires publication "p1" to have default values
> > 2022-05-20 14:34:49.301 AEST [21802] HINT:  Use ALTER PUBLICATION ...
> > RESET to reset the publication
> > 2022-05-20 14:34:49.301 AEST [21802] STATEMENT:  alter publication p1
> > add all tables except t1;
> > ERROR:  Setting ALL TABLES requires publication "p1" to have default values
> > HINT:  Use ALTER PUBLICATION ... RESET to reset the publication
> > postgres=# alter publication p1 set all tables except t1;
> >
> > That error message does not quite match what the user was doing.
> > Firstly, they were adding the ALL TABLES, not setting it. Secondly,
> > all the values of the publication were already defaults (only there
> > was an existing table t1 in the publication). Maybe some minor changes
> > to the message wording can be a better reflect what the user is doing
> > here.
>
> Modified
>
> > ~~~
> >
> > 13. src/backend/parser/gram.y
> >
> > @@ -10410,7 +10411,7 @@ AlterOwnerStmt: ALTER AGGREGATE
> > aggregate_with_argtypes OWNER TO RoleSpec
> >   *
> >   * CREATE PUBLICATION name [WITH options]
> >   *
> > - * CREATE PUBLICATION FOR ALL TABLES [WITH options]
> > + * CREATE PUBLICATION FOR ALL TABLES [EXCEPT TABLE table [, ...]]
> > [WITH options]
> >
> > Comment should show the "TABLE" keyword is optional
>
> Modified
>
> > ~~~
> >
> > 14. src/bin/pg_dump/pg_dump.c - dumpPublicationTable
> >
> > @@ -4332,6 +4380,7 @@ dumpPublicationTable(Archive *fout, const
> > PublicationRelInfo *pubrinfo)
> >
> >   appendPQExpBuffer(query, "ALTER PUBLICATION %s ADD TABLE ONLY",
> >     fmtId(pubinfo->dobj.name));
> > +
> >   appendPQExpBuffer(query, " %s",
> >     fmtQualifiedDumpable(tbinfo));
> >
> > This additional whitespace seems unrelated to this patch
>
> Modified
>
> > ~~~
> >
> > 15. src/include/nodes/parsenodes.h
> >
> > 15a.
> > @@ -3999,6 +3999,7 @@ typedef struct PublicationTable
> >   RangeVar   *relation; /* relation to be published */
> >   Node    *whereClause; /* qualifications */
> >   List    *columns; /* List of columns in a publication table */
> > + bool except; /* except relation */
> >  } PublicationTable;
> >
> > Maybe the comment should be more like similar ones:
> > /* exclude the relation */
>
> Modified
>
> > 15b.
> > @@ -4007,6 +4008,7 @@ typedef struct PublicationTable
> >  typedef enum PublicationObjSpecType
> >  {
> >   PUBLICATIONOBJ_TABLE, /* A table */
> > + PUBLICATIONOBJ_EXCEPT_TABLE, /* An Except table */
> >   PUBLICATIONOBJ_TABLES_IN_SCHEMA, /* All tables in schema */
> >   PUBLICATIONOBJ_TABLES_IN_CUR_SCHEMA, /* All tables in first element of
> >
> > Maybe the comment should be more like:
> > /* A table to be excluded */
>
> Modified
>
> > ~~~
> >
> > 16. src/test/regress/sql/publication.sql
> >
> > I did not see any test cases using EXCEPT when the optional TABLE
> > keyword is omitted.
>
> Added a test
>
> Thanks for the comments, the v7 patch attached at [1] has the changes
> for the same.
> [1] -
https://www.postgresql.org/message-id/CALDaNm3EpX3%2BRu%3DSNaYi%3DUW5ZLE6nNhGRHZ7a8-fXPZ_-gLdxQ%40mail.gmail.com

Attached v7 patch which fixes the buildfarm warning for an unused
warning in release mode as in  [1].
[1] - https://cirrus-ci.com/task/6220288017825792

Regards,
Vignesh

On Thu, May 26, 2022 at 7:04 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Monday, May 23, 2022 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> > Attached v7 patch which fixes the buildfarm warning for an unused warning in
> > release mode as in  [1].
> Hi, thank you for the patches.
>
>
> I'll share several review comments.
>
> For v7-0001.
>
> (1) I'll suggest some minor rewording.
>
> +  <para>
> +   The <literal>RESET</literal> clause will reset the publication to the
> +   default state which includes resetting the publication options, setting
> +   <literal>ALL TABLES</literal> flag to <literal>false</literal> and
> +   dropping all relations and schemas that are associated with the publication.
>
> My suggestion is
> "The RESET clause will reset the publication to the
> default state. It resets the publication operations,
> sets ALL TABLES flag to false and drops all relations
> and schemas associated with the publication."

I felt the existing looks better. I would prefer to keep it that way.

> (2) typo and rewording
>
> +/*
> + * Reset the publication.
> + *
> + * Reset the publication options, setting ALL TABLES flag to false and drop
> + * all relations and schemas that are associated with the publication.
> + */
>
> The "setting" in this sentence should be "set".
>
> How about changing like below ?
> FROM:
> "Reset the publication options, setting ALL TABLES flag to false and drop
> all relations and schemas that are associated with the publication."
> TO:
> "Reset the publication operations, set ALL TABLES flag to false and drop
> all relations and schemas associated with the publication."

 I felt the existing looks better. I would prefer to keep it that way.

> (3) AlterPublicationReset
>
> Do we need to call CacheInvalidateRelcacheAll() or
> InvalidatePublicationRels() at the end of
> AlterPublicationReset() like AlterPublicationOptions() ?

CacheInvalidateRelcacheAll should be called if we change all tables
from true to false, else the cache will not be invalidated. Modified

>
> For v7-0002.
>
> (4)
>
> +       if (stmt->for_all_tables)
> +       {
> +               bool            isdefault = CheckPublicationDefValues(tup);
> +
> +               if (!isdefault)
> +                       ereport(ERROR,
> +                                       errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +                                       errmsg("adding ALL TABLES requires the publication to have default
publicationoptions, no tables/....
 
> +                                       errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
>
>
> The errmsg string has three messages for user and is a bit long
> (we have two sentences there connected by 'and').
> Can't we make it concise and split it into a couple of lines for code readability ?
>
> I'll suggest a change below.
> FROM:
> "adding ALL TABLES requires the publication to have default publication options, no tables/schemas associated and ALL
TABLESflag should not be set"
 
> TO:
> "adding ALL TABLES requires the publication defined not for ALL TABLES"
> "to have default publish actions without any associated tables/schemas"

Added errdetail and split it

> (5) typo
>
>    <varlistentry>
> +    <term><literal>EXCEPT TABLE</literal></term>
> +    <listitem>
> +     <para>
> +      This clause specifies a list of tables to exclude from the publication.
> +      It can only be used with <literal>FOR ALL TABLES</literal>.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> +
>
> Kindly change
> FROM:
> This clause specifies a list of tables to exclude from the publication.
> TO:
> This clause specifies a list of tables to be excluded from the publication.
> or
> This clause specifies a list of tables excluded from the publication.

Modified

> (6) Minor suggestion for an expression change
>
>        Marks the publication as one that replicates changes for all tables in
> -      the database, including tables created in the future.
> +      the database, including tables created in the future. If
> +      <literal>EXCEPT TABLE</literal> is specified, then exclude replicating
> +      the changes for the specified tables.
>
>
> I'll suggest a minor rewording.
> FROM:
> ...exclude replicating the changes for the specified tables
> TO:
> ...exclude replication changes for the specified tables

I felt the existing is better.

> (7)
> (7-1)
>
> +/*
> + * Check if the publication has default values
> + *
> + * Check the following:
> + * a) Publication is not set with "FOR ALL TABLES"
> + * b) Publication is having default options
> + * c) Publication is not associated with schemas
> + * d) Publication is not associated with relations
> + */
> +static bool
> +CheckPublicationDefValues(HeapTuple tup)
>
>
> I think this header comment can be improved.
> FROM:
> Check the following:
> TO:
> Returns true if the publication satisfies all the following conditions:

Modified

> (7-2)
>
> b) should be changed as well
> FROM:
> Publication is having default options
> TO:
> Publication has the default publish operations

Changed it to "Publication is having default publication parameter values"

Thanks for the comments, the attached v8 patch has the changes for the same.

Regards,
Vignesh

On Fri, Jun 3, 2022 at 3:37 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the attached v8 patch has the changes for the same.
>

AFAICS, the summary of this proposal is that we want to support
exclude of certain objects from publication with two kinds of
variants. The first variant is to add support to exclude specific
tables from ALL TABLES PUBLICATION. Without this feature, users need
to manually add all tables for a database even when she wants to avoid
only a handful of tables from the database say because they contain
sensitive information or are not required. We have seen that other
database like MySQL also provides similar feature [1] (See
REPLICATE_WILD_IGNORE_TABLE). The proposed syntax for this is as
follows:

CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
or
ALTER PUBLICATION pub1 ADD ALL TABLES EXCEPT TABLE t1,t2;

This will allow us to publish all the tables in the current database
except t1 and t2. Now, I see that pg_dump has a similar option
provided by switch --exclude-table but that allows tables matching
patterns which is not the case here. I am not sure if we need a
similar variant here.

Then users will be allowed to reset the publication by:
ALTER PUBLICATION pub1 RESET;

This will reset the publication to the default state which includes
resetting the publication parameters, setting the ALL TABLES flag to
false, and dropping the relations and schemas that are associated with
the publication. I don't know if we want to go further with allowing
to RESET specific parameters and if so which parameters and what would
its syntax be?

The second variant is to add support to exclude certain columns of a
table while publishing a particular table. Currently, users need to
list all required columns' names even if they don't want to hide most
of the columns in the table (for example Create Publication pub For
Table t1 (c1, c2)). Consider user doesn't want to publish the 'salary'
or other sensitive information of executives/employees but would like
to publish all other columns. I feel in such cases it will be a lot of
work for the user especially when the table has many columns. I see
that Oracle has a similar feature [2]. I think without this it will be
difficult for users to use this feature in some cases. The patch for
this is not proposed but I would imagine syntax for it to be something
like "Create Publication pub For Table t1 Except (c3)" and similar
variants for Alter Publication.

Have I missed anything?

Thoughts on the proposal/syntax would be appreciated?

[1] - https://dev.mysql.com/doc/refman/5.7/en/change-replication-filter.html
[2] -
https://docs.oracle.com/en/cloud/paas/goldengate-cloud/gwuad/selecting-columns.html#GUID-9A851C8B-48F7-43DF-8D98-D086BE069E20

--
With Regards,
Amit Kapila.

RE: Skipping schema changes in publication

From

"houzj.fnst@fujitsu.com"

Date:

14 June 2022, 03:40:42

On Wednesday, June 8, 2022 7:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Fri, Jun 3, 2022 at 3:37 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Thanks for the comments, the attached v8 patch has the changes for the
> same.
> >
> 
> AFAICS, the summary of this proposal is that we want to support
> exclude of certain objects from publication with two kinds of
> variants. The first variant is to add support to exclude specific
> tables from ALL TABLES PUBLICATION. Without this feature, users need
> to manually add all tables for a database even when she wants to avoid
> only a handful of tables from the database say because they contain
> sensitive information or are not required. We have seen that other
> database like MySQL also provides similar feature [1] (See
> REPLICATE_WILD_IGNORE_TABLE). The proposed syntax for this is as
> follows:
> 
> CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> or
> ALTER PUBLICATION pub1 ADD ALL TABLES EXCEPT TABLE t1,t2;
> 
> This will allow us to publish all the tables in the current database
> except t1 and t2. Now, I see that pg_dump has a similar option
> provided by switch --exclude-table but that allows tables matching
> patterns which is not the case here. I am not sure if we need a
> similar variant here.
> 
> Then users will be allowed to reset the publication by:
> ALTER PUBLICATION pub1 RESET;
> 
> This will reset the publication to the default state which includes
> resetting the publication parameters, setting the ALL TABLES flag to
> false, and dropping the relations and schemas that are associated with
> the publication. I don't know if we want to go further with allowing
> to RESET specific parameters and if so which parameters and what would
> its syntax be?
> 
> The second variant is to add support to exclude certain columns of a
> table while publishing a particular table. Currently, users need to
> list all required columns' names even if they don't want to hide most
> of the columns in the table (for example Create Publication pub For
> Table t1 (c1, c2)). Consider user doesn't want to publish the 'salary'
> or other sensitive information of executives/employees but would like
> to publish all other columns. I feel in such cases it will be a lot of
> work for the user especially when the table has many columns. I see
> that Oracle has a similar feature [2]. I think without this it will be
> difficult for users to use this feature in some cases. The patch for
> this is not proposed but I would imagine syntax for it to be something
> like "Create Publication pub For Table t1 Except (c3)" and similar
> variants for Alter Publication.

I think the feature to exclude certain columns of a table would be useful.

In some production scenarios, we usually do not want to replicate
sensitive fields(column) in the table. Although we already can achieve
this by specify all replicated columns in the list[1], but that seems a
hard work when the table has hundreds of columns.

[1]
CREATE TABLE test(a int, b int, c int,..., sensitive text);
CRAETE PUBLICATION pub FOR TABLE test(a,b,c,...);

In addition, it's not easy to maintain the column list like above. Because
we sometimes need to add new fields or delete fields due to business
needs. Every time we add a column(or delete a column in column list), we
need to update the column list.

If we support Except:
CRAETE PUBLICATION pub FOR TABLE test EXCEPT (sensitive);

We don't need to update the column list in most cases.

Thanks for "hametan" for providing the use case off-list.

Best regards,
Hou zj

Re: Skipping schema changes in publication

From

Amit Kapila

Date:

16 June 2022, 04:04:54

On Tue, Jun 14, 2022 at 9:10 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, June 8, 2022 7:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jun 3, 2022 at 3:37 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > Thanks for the comments, the attached v8 patch has the changes for the
> > same.
> > >
> >
> > AFAICS, the summary of this proposal is that we want to support
> > exclude of certain objects from publication with two kinds of
> > variants. The first variant is to add support to exclude specific
> > tables from ALL TABLES PUBLICATION. Without this feature, users need
> > to manually add all tables for a database even when she wants to avoid
> > only a handful of tables from the database say because they contain
> > sensitive information or are not required. We have seen that other
> > database like MySQL also provides similar feature [1] (See
> > REPLICATE_WILD_IGNORE_TABLE). The proposed syntax for this is as
> > follows:
> >
> > CREATE PUBLICATION pub1 FOR ALL TABLES EXCEPT TABLE t1,t2;
> > or
> > ALTER PUBLICATION pub1 ADD ALL TABLES EXCEPT TABLE t1,t2;
> >
> > This will allow us to publish all the tables in the current database
> > except t1 and t2. Now, I see that pg_dump has a similar option
> > provided by switch --exclude-table but that allows tables matching
> > patterns which is not the case here. I am not sure if we need a
> > similar variant here.
> >
> > Then users will be allowed to reset the publication by:
> > ALTER PUBLICATION pub1 RESET;
> >
> > This will reset the publication to the default state which includes
> > resetting the publication parameters, setting the ALL TABLES flag to
> > false, and dropping the relations and schemas that are associated with
> > the publication. I don't know if we want to go further with allowing
> > to RESET specific parameters and if so which parameters and what would
> > its syntax be?
> >
> > The second variant is to add support to exclude certain columns of a
> > table while publishing a particular table. Currently, users need to
> > list all required columns' names even if they don't want to hide most
> > of the columns in the table (for example Create Publication pub For
> > Table t1 (c1, c2)). Consider user doesn't want to publish the 'salary'
> > or other sensitive information of executives/employees but would like
> > to publish all other columns. I feel in such cases it will be a lot of
> > work for the user especially when the table has many columns. I see
> > that Oracle has a similar feature [2]. I think without this it will be
> > difficult for users to use this feature in some cases. The patch for
> > this is not proposed but I would imagine syntax for it to be something
> > like "Create Publication pub For Table t1 Except (c3)" and similar
> > variants for Alter Publication.
>
> I think the feature to exclude certain columns of a table would be useful.
>
> In some production scenarios, we usually do not want to replicate
> sensitive fields(column) in the table. Although we already can achieve
> this by specify all replicated columns in the list[1], but that seems a
> hard work when the table has hundreds of columns.
>
> [1]
> CREATE TABLE test(a int, b int, c int,..., sensitive text);
> CRAETE PUBLICATION pub FOR TABLE test(a,b,c,...);
>
> In addition, it's not easy to maintain the column list like above. Because
> we sometimes need to add new fields or delete fields due to business
> needs. Every time we add a column(or delete a column in column list), we
> need to update the column list.
>
> If we support Except:
> CRAETE PUBLICATION pub FOR TABLE test EXCEPT (sensitive);
>
> We don't need to update the column list in most cases.
>

Right, this is a valid point and I think it makes sense for me to
support such a feature for column list and also to exclude a
particular table(s) from the ALL TABLES publication.

Peter E., Euler, and others, do you have any objections to supporting
the above-mentioned two cases?

-- 
With Regards,
Amit Kapila.

Re: Skipping schema changes in publication

From

vignesh C

Date:

08 August 2022, 07:16:39

On Fri, Jun 3, 2022 at 3:36 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, May 26, 2022 at 7:04 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Monday, May 23, 2022 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> > > Attached v7 patch which fixes the buildfarm warning for an unused warning in
> > > release mode as in  [1].
> > Hi, thank you for the patches.
> >
> >
> > I'll share several review comments.
> >
> > For v7-0001.
> >
> > (1) I'll suggest some minor rewording.
> >
> > +  <para>
> > +   The <literal>RESET</literal> clause will reset the publication to the
> > +   default state which includes resetting the publication options, setting
> > +   <literal>ALL TABLES</literal> flag to <literal>false</literal> and
> > +   dropping all relations and schemas that are associated with the publication.
> >
> > My suggestion is
> > "The RESET clause will reset the publication to the
> > default state. It resets the publication operations,
> > sets ALL TABLES flag to false and drops all relations
> > and schemas associated with the publication."
>
> I felt the existing looks better. I would prefer to keep it that way.
>
> > (2) typo and rewording
> >
> > +/*
> > + * Reset the publication.
> > + *
> > + * Reset the publication options, setting ALL TABLES flag to false and drop
> > + * all relations and schemas that are associated with the publication.
> > + */
> >
> > The "setting" in this sentence should be "set".
> >
> > How about changing like below ?
> > FROM:
> > "Reset the publication options, setting ALL TABLES flag to false and drop
> > all relations and schemas that are associated with the publication."
> > TO:
> > "Reset the publication operations, set ALL TABLES flag to false and drop
> > all relations and schemas associated with the publication."
>
>  I felt the existing looks better. I would prefer to keep it that way.
>
> > (3) AlterPublicationReset
> >
> > Do we need to call CacheInvalidateRelcacheAll() or
> > InvalidatePublicationRels() at the end of
> > AlterPublicationReset() like AlterPublicationOptions() ?
>
> CacheInvalidateRelcacheAll should be called if we change all tables
> from true to false, else the cache will not be invalidated. Modified
>
> >
> > For v7-0002.
> >
> > (4)
> >
> > +       if (stmt->for_all_tables)
> > +       {
> > +               bool            isdefault = CheckPublicationDefValues(tup);
> > +
> > +               if (!isdefault)
> > +                       ereport(ERROR,
> > +                                       errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > +                                       errmsg("adding ALL TABLES requires the publication to have default
publicationoptions, no tables/....
 
> > +                                       errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
> >
> >
> > The errmsg string has three messages for user and is a bit long
> > (we have two sentences there connected by 'and').
> > Can't we make it concise and split it into a couple of lines for code readability ?
> >
> > I'll suggest a change below.
> > FROM:
> > "adding ALL TABLES requires the publication to have default publication options, no tables/schemas associated and
ALLTABLES flag should not be set"
 
> > TO:
> > "adding ALL TABLES requires the publication defined not for ALL TABLES"
> > "to have default publish actions without any associated tables/schemas"
>
> Added errdetail and split it
>
> > (5) typo
> >
> >    <varlistentry>
> > +    <term><literal>EXCEPT TABLE</literal></term>
> > +    <listitem>
> > +     <para>
> > +      This clause specifies a list of tables to exclude from the publication.
> > +      It can only be used with <literal>FOR ALL TABLES</literal>.
> > +     </para>
> > +    </listitem>
> > +   </varlistentry>
> > +
> >
> > Kindly change
> > FROM:
> > This clause specifies a list of tables to exclude from the publication.
> > TO:
> > This clause specifies a list of tables to be excluded from the publication.
> > or
> > This clause specifies a list of tables excluded from the publication.
>
> Modified
>
> > (6) Minor suggestion for an expression change
> >
> >        Marks the publication as one that replicates changes for all tables in
> > -      the database, including tables created in the future.
> > +      the database, including tables created in the future. If
> > +      <literal>EXCEPT TABLE</literal> is specified, then exclude replicating
> > +      the changes for the specified tables.
> >
> >
> > I'll suggest a minor rewording.
> > FROM:
> > ...exclude replicating the changes for the specified tables
> > TO:
> > ...exclude replication changes for the specified tables
>
> I felt the existing is better.
>
> > (7)
> > (7-1)
> >
> > +/*
> > + * Check if the publication has default values
> > + *
> > + * Check the following:
> > + * a) Publication is not set with "FOR ALL TABLES"
> > + * b) Publication is having default options
> > + * c) Publication is not associated with schemas
> > + * d) Publication is not associated with relations
> > + */
> > +static bool
> > +CheckPublicationDefValues(HeapTuple tup)
> >
> >
> > I think this header comment can be improved.
> > FROM:
> > Check the following:
> > TO:
> > Returns true if the publication satisfies all the following conditions:
>
> Modified
>
> > (7-2)
> >
> > b) should be changed as well
> > FROM:
> > Publication is having default options
> > TO:
> > Publication has the default publish operations
>
> Changed it to "Publication is having default publication parameter values"
>
> Thanks for the comments, the attached v8 patch has the changes for the same.

The patch needed to be rebased on top of HEAD because of commit
"0c20dd33db1607d6a85ffce24238c1e55e384b49", attached a rebased v8
version for the changes of the same.

Regards,
Vignesh

Attachment

Re: Skipping schema changes in publication

From

vignesh C

Date:

08 August 2022, 09:23:28

On Mon, Aug 8, 2022 at 12:46 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, Jun 3, 2022 at 3:36 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Thu, May 26, 2022 at 7:04 PM osumi.takamichi@fujitsu.com
> > <osumi.takamichi@fujitsu.com> wrote:
> > >
> > > On Monday, May 23, 2022 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > Attached v7 patch which fixes the buildfarm warning for an unused warning in
> > > > release mode as in  [1].
> > > Hi, thank you for the patches.
> > >
> > >
> > > I'll share several review comments.
> > >
> > > For v7-0001.
> > >
> > > (1) I'll suggest some minor rewording.
> > >
> > > +  <para>
> > > +   The <literal>RESET</literal> clause will reset the publication to the
> > > +   default state which includes resetting the publication options, setting
> > > +   <literal>ALL TABLES</literal> flag to <literal>false</literal> and
> > > +   dropping all relations and schemas that are associated with the publication.
> > >
> > > My suggestion is
> > > "The RESET clause will reset the publication to the
> > > default state. It resets the publication operations,
> > > sets ALL TABLES flag to false and drops all relations
> > > and schemas associated with the publication."
> >
> > I felt the existing looks better. I would prefer to keep it that way.
> >
> > > (2) typo and rewording
> > >
> > > +/*
> > > + * Reset the publication.
> > > + *
> > > + * Reset the publication options, setting ALL TABLES flag to false and drop
> > > + * all relations and schemas that are associated with the publication.
> > > + */
> > >
> > > The "setting" in this sentence should be "set".
> > >
> > > How about changing like below ?
> > > FROM:
> > > "Reset the publication options, setting ALL TABLES flag to false and drop
> > > all relations and schemas that are associated with the publication."
> > > TO:
> > > "Reset the publication operations, set ALL TABLES flag to false and drop
> > > all relations and schemas associated with the publication."
> >
> >  I felt the existing looks better. I would prefer to keep it that way.
> >
> > > (3) AlterPublicationReset
> > >
> > > Do we need to call CacheInvalidateRelcacheAll() or
> > > InvalidatePublicationRels() at the end of
> > > AlterPublicationReset() like AlterPublicationOptions() ?
> >
> > CacheInvalidateRelcacheAll should be called if we change all tables
> > from true to false, else the cache will not be invalidated. Modified
> >
> > >
> > > For v7-0002.
> > >
> > > (4)
> > >
> > > +       if (stmt->for_all_tables)
> > > +       {
> > > +               bool            isdefault = CheckPublicationDefValues(tup);
> > > +
> > > +               if (!isdefault)
> > > +                       ereport(ERROR,
> > > +                                       errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > +                                       errmsg("adding ALL TABLES requires the publication to have default
publicationoptions, no tables/....
 
> > > +                                       errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
> > >
> > >
> > > The errmsg string has three messages for user and is a bit long
> > > (we have two sentences there connected by 'and').
> > > Can't we make it concise and split it into a couple of lines for code readability ?
> > >
> > > I'll suggest a change below.
> > > FROM:
> > > "adding ALL TABLES requires the publication to have default publication options, no tables/schemas associated and
ALLTABLES flag should not be set"
 
> > > TO:
> > > "adding ALL TABLES requires the publication defined not for ALL TABLES"
> > > "to have default publish actions without any associated tables/schemas"
> >
> > Added errdetail and split it
> >
> > > (5) typo
> > >
> > >    <varlistentry>
> > > +    <term><literal>EXCEPT TABLE</literal></term>
> > > +    <listitem>
> > > +     <para>
> > > +      This clause specifies a list of tables to exclude from the publication.
> > > +      It can only be used with <literal>FOR ALL TABLES</literal>.
> > > +     </para>
> > > +    </listitem>
> > > +   </varlistentry>
> > > +
> > >
> > > Kindly change
> > > FROM:
> > > This clause specifies a list of tables to exclude from the publication.
> > > TO:
> > > This clause specifies a list of tables to be excluded from the publication.
> > > or
> > > This clause specifies a list of tables excluded from the publication.
> >
> > Modified
> >
> > > (6) Minor suggestion for an expression change
> > >
> > >        Marks the publication as one that replicates changes for all tables in
> > > -      the database, including tables created in the future.
> > > +      the database, including tables created in the future. If
> > > +      <literal>EXCEPT TABLE</literal> is specified, then exclude replicating
> > > +      the changes for the specified tables.
> > >
> > >
> > > I'll suggest a minor rewording.
> > > FROM:
> > > ...exclude replicating the changes for the specified tables
> > > TO:
> > > ...exclude replication changes for the specified tables
> >
> > I felt the existing is better.
> >
> > > (7)
> > > (7-1)
> > >
> > > +/*
> > > + * Check if the publication has default values
> > > + *
> > > + * Check the following:
> > > + * a) Publication is not set with "FOR ALL TABLES"
> > > + * b) Publication is having default options
> > > + * c) Publication is not associated with schemas
> > > + * d) Publication is not associated with relations
> > > + */
> > > +static bool
> > > +CheckPublicationDefValues(HeapTuple tup)
> > >
> > >
> > > I think this header comment can be improved.
> > > FROM:
> > > Check the following:
> > > TO:
> > > Returns true if the publication satisfies all the following conditions:
> >
> > Modified
> >
> > > (7-2)
> > >
> > > b) should be changed as well
> > > FROM:
> > > Publication is having default options
> > > TO:
> > > Publication has the default publish operations
> >
> > Changed it to "Publication is having default publication parameter values"
> >
> > Thanks for the comments, the attached v8 patch has the changes for the same.
>
> The patch needed to be rebased on top of HEAD because of commit
> "0c20dd33db1607d6a85ffce24238c1e55e384b49", attached a rebased v8
> version for the changes of the same.

I had missed attaching one of the changes that was present locally.
The updated patch has the changes for the same.

Regards,
Vignesh

On Mon, Aug 8, 2022 at 2:53 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, Aug 8, 2022 at 12:46 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, Jun 3, 2022 at 3:36 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 7:04 PM osumi.takamichi@fujitsu.com
> > > <osumi.takamichi@fujitsu.com> wrote:
> > > >
> > > > On Monday, May 23, 2022 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > Attached v7 patch which fixes the buildfarm warning for an unused warning in
> > > > > release mode as in  [1].
> > > > Hi, thank you for the patches.
> > > >
> > > >
> > > > I'll share several review comments.
> > > >
> > > > For v7-0001.
> > > >
> > > > (1) I'll suggest some minor rewording.
> > > >
> > > > +  <para>
> > > > +   The <literal>RESET</literal> clause will reset the publication to the
> > > > +   default state which includes resetting the publication options, setting
> > > > +   <literal>ALL TABLES</literal> flag to <literal>false</literal> and
> > > > +   dropping all relations and schemas that are associated with the publication.
> > > >
> > > > My suggestion is
> > > > "The RESET clause will reset the publication to the
> > > > default state. It resets the publication operations,
> > > > sets ALL TABLES flag to false and drops all relations
> > > > and schemas associated with the publication."
> > >
> > > I felt the existing looks better. I would prefer to keep it that way.
> > >
> > > > (2) typo and rewording
> > > >
> > > > +/*
> > > > + * Reset the publication.
> > > > + *
> > > > + * Reset the publication options, setting ALL TABLES flag to false and drop
> > > > + * all relations and schemas that are associated with the publication.
> > > > + */
> > > >
> > > > The "setting" in this sentence should be "set".
> > > >
> > > > How about changing like below ?
> > > > FROM:
> > > > "Reset the publication options, setting ALL TABLES flag to false and drop
> > > > all relations and schemas that are associated with the publication."
> > > > TO:
> > > > "Reset the publication operations, set ALL TABLES flag to false and drop
> > > > all relations and schemas associated with the publication."
> > >
> > >  I felt the existing looks better. I would prefer to keep it that way.
> > >
> > > > (3) AlterPublicationReset
> > > >
> > > > Do we need to call CacheInvalidateRelcacheAll() or
> > > > InvalidatePublicationRels() at the end of
> > > > AlterPublicationReset() like AlterPublicationOptions() ?
> > >
> > > CacheInvalidateRelcacheAll should be called if we change all tables
> > > from true to false, else the cache will not be invalidated. Modified
> > >
> > > >
> > > > For v7-0002.
> > > >
> > > > (4)
> > > >
> > > > +       if (stmt->for_all_tables)
> > > > +       {
> > > > +               bool            isdefault = CheckPublicationDefValues(tup);
> > > > +
> > > > +               if (!isdefault)
> > > > +                       ereport(ERROR,
> > > > +                                       errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > > +                                       errmsg("adding ALL TABLES requires the publication to have default
publicationoptions, no tables/....
 
> > > > +                                       errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
> > > >
> > > >
> > > > The errmsg string has three messages for user and is a bit long
> > > > (we have two sentences there connected by 'and').
> > > > Can't we make it concise and split it into a couple of lines for code readability ?
> > > >
> > > > I'll suggest a change below.
> > > > FROM:
> > > > "adding ALL TABLES requires the publication to have default publication options, no tables/schemas associated
andALL TABLES flag should not be set"
 
> > > > TO:
> > > > "adding ALL TABLES requires the publication defined not for ALL TABLES"
> > > > "to have default publish actions without any associated tables/schemas"
> > >
> > > Added errdetail and split it
> > >
> > > > (5) typo
> > > >
> > > >    <varlistentry>
> > > > +    <term><literal>EXCEPT TABLE</literal></term>
> > > > +    <listitem>
> > > > +     <para>
> > > > +      This clause specifies a list of tables to exclude from the publication.
> > > > +      It can only be used with <literal>FOR ALL TABLES</literal>.
> > > > +     </para>
> > > > +    </listitem>
> > > > +   </varlistentry>
> > > > +
> > > >
> > > > Kindly change
> > > > FROM:
> > > > This clause specifies a list of tables to exclude from the publication.
> > > > TO:
> > > > This clause specifies a list of tables to be excluded from the publication.
> > > > or
> > > > This clause specifies a list of tables excluded from the publication.
> > >
> > > Modified
> > >
> > > > (6) Minor suggestion for an expression change
> > > >
> > > >        Marks the publication as one that replicates changes for all tables in
> > > > -      the database, including tables created in the future.
> > > > +      the database, including tables created in the future. If
> > > > +      <literal>EXCEPT TABLE</literal> is specified, then exclude replicating
> > > > +      the changes for the specified tables.
> > > >
> > > >
> > > > I'll suggest a minor rewording.
> > > > FROM:
> > > > ...exclude replicating the changes for the specified tables
> > > > TO:
> > > > ...exclude replication changes for the specified tables
> > >
> > > I felt the existing is better.
> > >
> > > > (7)
> > > > (7-1)
> > > >
> > > > +/*
> > > > + * Check if the publication has default values
> > > > + *
> > > > + * Check the following:
> > > > + * a) Publication is not set with "FOR ALL TABLES"
> > > > + * b) Publication is having default options
> > > > + * c) Publication is not associated with schemas
> > > > + * d) Publication is not associated with relations
> > > > + */
> > > > +static bool
> > > > +CheckPublicationDefValues(HeapTuple tup)
> > > >
> > > >
> > > > I think this header comment can be improved.
> > > > FROM:
> > > > Check the following:
> > > > TO:
> > > > Returns true if the publication satisfies all the following conditions:
> > >
> > > Modified
> > >
> > > > (7-2)
> > > >
> > > > b) should be changed as well
> > > > FROM:
> > > > Publication is having default options
> > > > TO:
> > > > Publication has the default publish operations
> > >
> > > Changed it to "Publication is having default publication parameter values"
> > >
> > > Thanks for the comments, the attached v8 patch has the changes for the same.
> >
> > The patch needed to be rebased on top of HEAD because of commit
> > "0c20dd33db1607d6a85ffce24238c1e55e384b49", attached a rebased v8
> > version for the changes of the same.
>
> I had missed attaching one of the changes that was present locally.
> The updated patch has the changes for the same.

The patch needed to be rebased on top of HEAD because of a recent
commit. The updated v8 patch has the changes for the same.

Regards,
Vignesh

Attachment

Re: Skipping schema changes in publication

From

Ian Lawrence Barwick

Date:

04 November 2022, 02:49:46

2022年8月19日(金) 2:41 vignesh C <vignesh21@gmail.com>:
>
> On Mon, Aug 8, 2022 at 2:53 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Mon, Aug 8, 2022 at 12:46 PM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Fri, Jun 3, 2022 at 3:36 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > On Thu, May 26, 2022 at 7:04 PM osumi.takamichi@fujitsu.com
> > > > <osumi.takamichi@fujitsu.com> wrote:
> > > > >
> > > > > On Monday, May 23, 2022 2:13 PM vignesh C <vignesh21@gmail.com> wrote:
> > > > > > Attached v7 patch which fixes the buildfarm warning for an unused warning in
> > > > > > release mode as in  [1].
> > > > > Hi, thank you for the patches.
> > > > >
> > > > >
> > > > > I'll share several review comments.
> > > > >
> > > > > For v7-0001.
> > > > >
> > > > > (1) I'll suggest some minor rewording.
> > > > >
> > > > > +  <para>
> > > > > +   The <literal>RESET</literal> clause will reset the publication to the
> > > > > +   default state which includes resetting the publication options, setting
> > > > > +   <literal>ALL TABLES</literal> flag to <literal>false</literal> and
> > > > > +   dropping all relations and schemas that are associated with the publication.
> > > > >
> > > > > My suggestion is
> > > > > "The RESET clause will reset the publication to the
> > > > > default state. It resets the publication operations,
> > > > > sets ALL TABLES flag to false and drops all relations
> > > > > and schemas associated with the publication."
> > > >
> > > > I felt the existing looks better. I would prefer to keep it that way.
> > > >
> > > > > (2) typo and rewording
> > > > >
> > > > > +/*
> > > > > + * Reset the publication.
> > > > > + *
> > > > > + * Reset the publication options, setting ALL TABLES flag to false and drop
> > > > > + * all relations and schemas that are associated with the publication.
> > > > > + */
> > > > >
> > > > > The "setting" in this sentence should be "set".
> > > > >
> > > > > How about changing like below ?
> > > > > FROM:
> > > > > "Reset the publication options, setting ALL TABLES flag to false and drop
> > > > > all relations and schemas that are associated with the publication."
> > > > > TO:
> > > > > "Reset the publication operations, set ALL TABLES flag to false and drop
> > > > > all relations and schemas associated with the publication."
> > > >
> > > >  I felt the existing looks better. I would prefer to keep it that way.
> > > >
> > > > > (3) AlterPublicationReset
> > > > >
> > > > > Do we need to call CacheInvalidateRelcacheAll() or
> > > > > InvalidatePublicationRels() at the end of
> > > > > AlterPublicationReset() like AlterPublicationOptions() ?
> > > >
> > > > CacheInvalidateRelcacheAll should be called if we change all tables
> > > > from true to false, else the cache will not be invalidated. Modified
> > > >
> > > > >
> > > > > For v7-0002.
> > > > >
> > > > > (4)
> > > > >
> > > > > +       if (stmt->for_all_tables)
> > > > > +       {
> > > > > +               bool            isdefault = CheckPublicationDefValues(tup);
> > > > > +
> > > > > +               if (!isdefault)
> > > > > +                       ereport(ERROR,
> > > > > +                                       errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> > > > > +                                       errmsg("adding ALL TABLES requires the publication to have default
publicationoptions, no tables/.... 
> > > > > +                                       errhint("Use ALTER PUBLICATION ... RESET to reset the publication"));
> > > > >
> > > > >
> > > > > The errmsg string has three messages for user and is a bit long
> > > > > (we have two sentences there connected by 'and').
> > > > > Can't we make it concise and split it into a couple of lines for code readability ?
> > > > >
> > > > > I'll suggest a change below.
> > > > > FROM:
> > > > > "adding ALL TABLES requires the publication to have default publication options, no tables/schemas associated
andALL TABLES flag should not be set" 
> > > > > TO:
> > > > > "adding ALL TABLES requires the publication defined not for ALL TABLES"
> > > > > "to have default publish actions without any associated tables/schemas"
> > > >
> > > > Added errdetail and split it
> > > >
> > > > > (5) typo
> > > > >
> > > > >    <varlistentry>
> > > > > +    <term><literal>EXCEPT TABLE</literal></term>
> > > > > +    <listitem>
> > > > > +     <para>
> > > > > +      This clause specifies a list of tables to exclude from the publication.
> > > > > +      It can only be used with <literal>FOR ALL TABLES</literal>.
> > > > > +     </para>
> > > > > +    </listitem>
> > > > > +   </varlistentry>
> > > > > +
> > > > >
> > > > > Kindly change
> > > > > FROM:
> > > > > This clause specifies a list of tables to exclude from the publication.
> > > > > TO:
> > > > > This clause specifies a list of tables to be excluded from the publication.
> > > > > or
> > > > > This clause specifies a list of tables excluded from the publication.
> > > >
> > > > Modified
> > > >
> > > > > (6) Minor suggestion for an expression change
> > > > >
> > > > >        Marks the publication as one that replicates changes for all tables in
> > > > > -      the database, including tables created in the future.
> > > > > +      the database, including tables created in the future. If
> > > > > +      <literal>EXCEPT TABLE</literal> is specified, then exclude replicating
> > > > > +      the changes for the specified tables.
> > > > >
> > > > >
> > > > > I'll suggest a minor rewording.
> > > > > FROM:
> > > > > ...exclude replicating the changes for the specified tables
> > > > > TO:
> > > > > ...exclude replication changes for the specified tables
> > > >
> > > > I felt the existing is better.
> > > >
> > > > > (7)
> > > > > (7-1)
> > > > >
> > > > > +/*
> > > > > + * Check if the publication has default values
> > > > > + *
> > > > > + * Check the following:
> > > > > + * a) Publication is not set with "FOR ALL TABLES"
> > > > > + * b) Publication is having default options
> > > > > + * c) Publication is not associated with schemas
> > > > > + * d) Publication is not associated with relations
> > > > > + */
> > > > > +static bool
> > > > > +CheckPublicationDefValues(HeapTuple tup)
> > > > >
> > > > >
> > > > > I think this header comment can be improved.
> > > > > FROM:
> > > > > Check the following:
> > > > > TO:
> > > > > Returns true if the publication satisfies all the following conditions:
> > > >
> > > > Modified
> > > >
> > > > > (7-2)
> > > > >
> > > > > b) should be changed as well
> > > > > FROM:
> > > > > Publication is having default options
> > > > > TO:
> > > > > Publication has the default publish operations
> > > >
> > > > Changed it to "Publication is having default publication parameter values"
> > > >
> > > > Thanks for the comments, the attached v8 patch has the changes for the same.
> > >
> > > The patch needed to be rebased on top of HEAD because of commit
> > > "0c20dd33db1607d6a85ffce24238c1e55e384b49", attached a rebased v8
> > > version for the changes of the same.
> >
> > I had missed attaching one of the changes that was present locally.
> > The updated patch has the changes for the same.
>
> The patch needed to be rebased on top of HEAD because of a recent
> commit. The updated v8 patch has the changes for the same.

Hi

cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is
currently underway, this would be an excellent time to update the patch.

[1] http://cfbot.cputube.org/patch_40_3646.log

Thanks

Ian Barwick

Re: Skipping schema changes in publication

From

vignesh C

Date:

07 November 2022, 13:39:41

On Fri, 4 Nov 2022 at 08:19, Ian Lawrence Barwick <barwick@gmail.com> wrote:
>
> Hi
>
> cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is
> currently underway, this would be an excellent time to update the patch.
>
> [1] http://cfbot.cputube.org/patch_40_3646.log

Here is an updated patch which is rebased on top of HEAD.

Regards,
Vignesh

Attachment

Re: Skipping schema changes in publication

From

Ian Lawrence Barwick

Date:

16 November 2022, 04:04:18

2022年11月7日(月) 22:39 vignesh C <vignesh21@gmail.com>:
>
> On Fri, 4 Nov 2022 at 08:19, Ian Lawrence Barwick <barwick@gmail.com> wrote:
> >
> > Hi
> >
> > cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is
> > currently underway, this would be an excellent time to update the patch.
> >
> > [1] http://cfbot.cputube.org/patch_40_3646.log
>
> Here is an updated patch which is rebased on top of HEAD.

Thanks for the updated patch.

While reviewing the patch backlog, we have determined that this patch adds
one or more TAP tests but has not added the test to the "meson.build" file.

To do this, locate the relevant "meson.build" file for each test and add it
in the 'tests' dictionary, which will look something like this:

  'tap': {
    'tests': [
      't/001_basic.pl',
    ],
  },

For some additional details please see this Wiki article:

  https://wiki.postgresql.org/wiki/Meson_for_patch_authors

For more information on the meson build system for PostgreSQL see:

  https://wiki.postgresql.org/wiki/Meson

Regards

Ian Barwick

Re: Skipping schema changes in publication

From

vignesh C

Date:

16 November 2022, 10:05:31

On Wed, 16 Nov 2022 at 09:34, Ian Lawrence Barwick <barwick@gmail.com> wrote:
>
> 2022年11月7日(月) 22:39 vignesh C <vignesh21@gmail.com>:
> >
> > On Fri, 4 Nov 2022 at 08:19, Ian Lawrence Barwick <barwick@gmail.com> wrote:
> > >
> > > Hi
> > >
> > > cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is
> > > currently underway, this would be an excellent time to update the patch.
> > >
> > > [1] http://cfbot.cputube.org/patch_40_3646.log
> >
> > Here is an updated patch which is rebased on top of HEAD.
>
> Thanks for the updated patch.
>
> While reviewing the patch backlog, we have determined that this patch adds
> one or more TAP tests but has not added the test to the "meson.build" file.

Thanks, I have updated the meson.build to include the TAP test. The
attached patch has the changes for the same.

Regards,
Vignesh

Attachment

Re: Skipping schema changes in publication

From

vignesh C

Date:

20 January 2023, 10:00:54

On Wed, 16 Nov 2022 at 15:35, vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 16 Nov 2022 at 09:34, Ian Lawrence Barwick <barwick@gmail.com> wrote:
> >
> > 2022年11月7日(月) 22:39 vignesh C <vignesh21@gmail.com>:
> > >
> > > On Fri, 4 Nov 2022 at 08:19, Ian Lawrence Barwick <barwick@gmail.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > cfbot reports the patch no longer applies [1]. As CommitFest 2022-11 is
> > > > currently underway, this would be an excellent time to update the patch.
> > > >
> > > > [1] http://cfbot.cputube.org/patch_40_3646.log
> > >
> > > Here is an updated patch which is rebased on top of HEAD.
> >
> > Thanks for the updated patch.
> >
> > While reviewing the patch backlog, we have determined that this patch adds
> > one or more TAP tests but has not added the test to the "meson.build" file.
>
> Thanks, I have updated the meson.build to include the TAP test. The
> attached patch has the changes for the same.

The patch was not applying on top of HEAD, attached a rebased version.

Regards,
Vignesh

On Thu, 17 Apr 2025 at 09:12, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Apr 16, 2025 at 8:22 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Thu, Apr 10, 2025 at 7:25 PM Amit Kapila wrote:
> > >
> > > On Tue, Jan 9, 2024 at 12:02 PM vignesh C <vignesh21@gmail.com> wrote:
> > > >
> > > > As I did not see much interest from others, I'm withdrawing this patch
> > > > for now. But if there is any interest others in future, I would be
> > > > more than happy to work on this feature.
> > > >
> > >
> > > Just FYI, I noticed a use case for this patch in email [1]. Users would like to
> > > replicate all except a few columns having sensitive information. The challenge
> > > with current column list features is that adding new tables to columns would
> > > lead users to change the respective publications as well.
> > >
> > > [1] -
> > > https://www.postgresql.org/message-id/tencent_DCDF626FCD4A556C51BE
> > > 270FDC3047540208%40qq.com
> >
> > BTW, I noticed that debezium, an open source distributed platform for change
> > data capture that replies on logical decoding, also support specifying the
> > column exclusion list[1]. So, this indicates that there could be some use cases
> > for this feature.
> >
>
> Thanks for sharing the link. I see that they support both the include
> and exclude lists for columns and tables.
>

Hi Hackers,

I see there is some interest in the functionality added by this patch.
I have rebased the patches in [1]. I saw a new column 'pubgencols' was
added in pg_publication in PG 18. So, I have modified v11-0001 to
RESET this as well.
I am also working on creating a patch to exclude columns in
publication as per suggestion in [2].

[1]: https://www.postgresql.org/message-id/CALDaNm3dWZCYDih55qTNAYsjCvYXMFv%3D46UsDWmfCnXMt3kPCg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1KRdAPC%3D5%3D7tQ1GW0cRwD%3DzaDMi%2BT4u_k4GxPhPY6e8BQ%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

Attachment

Re: Skipping schema changes in publication

From

Shlok Kyal

Date:

19 June, 09:41:48

On Wed, 18 Jun 2025 at 06:34, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Jun 17, 2025 at 5:41 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> ...
> > I have attached a patch support excluding columns for publication.
> >
> > I have added a syntax: "FOR TABLE table_name EXCEPT (c1, c2, ..)"
> > It can be used with CREATE or ALTER PUBLICATION.
> >
> > v12-0003 patch contains the changes for the same.
> >
>
> Hi Shlok,
>
> I was interested in your new EXCEPT (col-list) so I had a quick look
> at your patch v12-0003 (only looked at the documentation).
>
> Below are some comments:
>
> ======
>
> 1. Chapter 29.5 "Column Lists".
>
> I think new EXCEPT syntax needs a mention here as well.
>
Added

> ======
>
> doc/src/sgml/catalogs.sgml
>
> 2.
> +      <para>
> +       This is an array of values that indicates which table columns are
> +       excluded from the publication.  For example, a value of
> +       <literal>1 3</literal> would mean that the columns except the first and
> +       the third columns are published.
> +       A null value indicates that no columns are excluded from being
> published.
> +      </para></entry>
>
> The sentence "A null value indicates that no columns are excluded from
> being published" seems kind of confusing, because if the user has a
> "normal" column-list  although nothing was being *explicitly* excluded
> (using EXCEPT), any columns not named are *implicitly* excluded from
> being published.
>
I have removed this line.

> ~
>
> 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.

> ======
>
> doc/src/sgml/ref/alter_publication.sgml
>
> 4. ALTER PUBLICATION syntax
>
> The syntax is currently written as:
> TABLE [ ONLY ] table_name [ * ] { [ [ ( column_name [, ... ] ) ] | [
> EXCEPT ( column_name [, ... ] ) ] ] } [ WHERE ( expression ) ] [, ...
> ]
>
> Can't this be more simply written as:
> TABLE [ ONLY ] table_name [ * ] [ [ EXCEPT ] ( column_name [, ... ] )
> ] [ WHERE ( expression ) ] [, ... ]
>
> ~~~
Fixed

>
> 5.
> +  <para>
> +   Alter publication <structname>mypublication</structname> to add table
> +   <structname>users</structname> except column
> +   <structname>security_pin</structname>:
> +<programlisting>
> +ALTER PUBLICATION production_publication ADD TABLE users EXCEPT (security_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?

> ======
> doc/src/sgml/ref/create_publication.sgml
>
> 6. CREATE PUBLICATION syntax
>
> The syntax is currently written as:
> TABLE [ ONLY ] table_name [ * ] { [ [ ( column_name [, ... ] ) ] | [
> EXCEPT ( column_name [, ... ] ) ] ] } [ WHERE ( expression ) ] [, ...
> ]
>
> Can't this be more simply written as:
> TABLE [ ONLY ] table_name [ * ] [ [ EXCEPT ] ( column_name [, ... ] )
> ] [ WHERE ( expression ) ] [, ... ]
>
> ~~~
Fixed

>
> 7.
> +     <para>
> +      When a column list is specified with EXCEPT, the named columns are not
> +      replicated. The excluded column list cannot contain generated
> columns. The
> +      column list and excluded column list cannot be specified together.
> +      Specifying a column list has no effect on <literal>TRUNCATE</literal>
> +      commands.
> +     </para>
>
> IMO you don't need to say "The column list and excluded column list
> cannot be specified together." because AFAIK the syntax makes that
> impossible to do anyhow.
>
Removed this line

> ~~~
>
> 8.
> +  <para>
> +   Create a publication that publishes all changes for table
> <structname>users</structname>
> +   except changes for columns <structname>security_pin</structname>:
> +<programlisting>
> +CREATE PUBLICATION users_safe FOR TABLE users EXCEPT (security_pin);
> +</programlisting>
> +  </para>
>
> 8a.
> Same review comment as previously -- Those tags don't seem correct.
> e.g. "users" and "security_pin" are not <structname> (???).
> Again, are all the other existing tags also wrong? Maybe a new thread
> needed to address these?
>
> ~
Same as point 5.
I also feel this should be addressed in a new thread.

> 8b.
> Plural?  /except changes for columns/except changes for column/
Fixed

Also in this patch I added displaying "EXCEPT (column_list)" for \dRp+
and \d table_name psql commands.

Thanks and Regards,
Shlok Kyal

On Fri, 20 Jun 2025 at 09:28, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Jun 19, 2025 at 4:42 PM Shlok Kyal <shlok.kyal.oss@gmail.com> 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 ignored)
> null       t                          all cols are published (flag is ignored)
>

I agree with your point and also it would be a better approach. In
patch 0001 an column 'prexcept' was added in pg_publication_rel. We
use that only for publication with all tables. I have reused this
column for patch 0003. If publication is not for all tables and the
'prexcept' flag is true, it implies that the columns in 'prattrs' are
to be excluded from being published. I have included the changes for
it in v14-0003 patch.

> > > 5.
> > > +  <para>
> > > +   Alter publication <structname>mypublication</structname> to add table
> > > +   <structname>users</structname> except column
> > > +   <structname>security_pin</structname>:
> > > +<programlisting>
> > > +ALTER PUBLICATION production_publication ADD TABLE users EXCEPT (security_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.
Thanks for starting a thread for it.

>
> //////////
>
> Below are a few more review comments for v13-0003
>
> ======
> Commit message
>
> 1.
> Typo /THe/The/
>
> ~~~
Fixed

> 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)".
>
Fixed

> ~~~
>
> 3.
> Typo /family of command/family of commands/
>
> ======
> 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.
>
> ======
> src/backend/catalog/pg_publication.c
>
> 5.
> +/*
> + * Returns true if the relation has exluded column list associated with the
> + * 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/
>
updated the comment according to latest implementation

> ~~~
>
> 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=stored 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.
>
I have removed this restriction. Now we can specify stored generated
columns in EXCEPT (column_list) when we use the
'publish_generated_columns' flag.

> ======
> 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.
>
> ======
Fixed

Thanks and Regards,
Shlok Kyal

On Thu, 26 Jun 2025 at 09:06, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shlok.
>
> Below are some review comments for v14-0003
>
> ======
> 1. GENERAL
>
> Since the new syntax uses EXCEPT, then, in my opinion, you should try
> to use that same term where possible when describing things. I
> understand it is hard to do this in text and I agree often it makes
> more sense to say "exclude" columns etc, but OTOH in the code there
> are lots of places where you could have named vars/params differently:
> e.g. 'except_collist' instead of 'exclude_collist' might have been
> better.
>
Fixed the variable names.

> ======
> Commit message
>
> 2.
> Column list specifed with EXCEPT is stored in column "prattrs" in table
> "pg_publication_rel" and also column "prexcept" is set to "true", to maintain
> the column list that user wants to exclude from the publication.
>
> ~
>
> That paragraph could do with some rewording. For example, AFAIK,
> "prattrs" is for all column lists -- not just except col-lists, but
> the way it is described here sounds different.
>
> Also, /specifed/specified/
>
Reworded the paragraph

> ======
> doc/src/sgml/catalogs.sgml
>
> 3. (52.42. pg_publication_rel)
>
>        <para>
> -       True if the relation must be excluded
> +       True if the relation or column list must be excluded. If publication is
> +       created <literal>FOR ALL TABLES</literal> and it is specified as true,
> +       the relation should be excluded. Else if it is true the columns in
> +       <literal>prattrs</literal> should be excluded from being published.
>        </para></entry>
>
> I felt this could be expressed more simply without mentioning anything
> about FOR ALL TABLES.
>
> SUGGESTION
> True if the column list or relation must be excluded from publication.
> If a column list is specified in <literal>prattrs</literal>, then
> exclude only those columns. If <literal>prattrs</literal> is NULL,
> then exclude the entire relation.
>
Fixed

> ======
> doc/src/sgml/logical-replication.sgml
>
> 4. (29.5. Column Lists)
>
>    <para>
> -   Each publication can optionally specify which columns of each table are
> -   replicated to subscribers. The table on the subscriber side must have at
> -   least all the columns that are published. If no column list is specified,
> -   then all columns on the publisher are replicated.
> +   Each publication can optionally specify which columns of each
> table should be
> +   replicated or excluded from replication. On the subscriber side, the table
> +   must include at least all the columns that are published. If no column list
> +   is provided, all columns from the publisher are replicated by default.
>     See <xref linkend="sql-createpublication"/> for details on the syntax.
>    </para>
>
> I felt this patch may have changed too much text. IMO, you only needed
> to say "... are replicated or excluded from replication.". The other
> changes did not seem necessary.
>
> ~~~
Fixed

> 5.
>    <para>
> -   If no column list is specified, any columns added to the table later are
> -   automatically replicated. This means that having a column list which names
> -   all columns is not the same as having no column list at all.
> +   If no column list or a column list with EXCEPT is specified, any columns
> +   added to the table later are automatically replicated. This means
> that having
> +   a column list which names all columns is not the same as having no
> +   column list at all. If an column list is specified, any columns added to the
> +   table later are automatically replicated.
>    </para>
>
> 5a.
> "This means that having a column list which names all columns is not
> the same as having no column list at all." -- That note does not make
> sense when you say EXCEPT. I think some rewording is needed here.
>
Fixed

> ~
>
> 5b.
> "If an column list is specified, any columns added to the table later
> are automatically replicated.".
>
> This made no sense -- some words missing?
>
This change was done by mistake. Removed it.

> ~~~
>
> 6.
>     Generated columns can also be specified in a column list. This allows
>     generated columns to be published, regardless of the publication parameter
>     <link linkend="sql-createpublication-params-with-publish-generated-columns">
> -   <literal>publish_generated_columns</literal></link>. See
> -   <xref linkend="logical-replication-gencols"/> for details.
> +   <literal>publish_generated_columns</literal></link>. Generated columns can
> +   be included in column list specified with EXCEPT clause if publication
> +   parameter
> +   <link linkend="sql-createpublication-params-with-publish-generated-columns">
> +   <literal>publish_generated_columns</literal></link> is not set to
> +   <literal>none</literal>. Specified generated columns will not be published.
> +   See <xref linkend="logical-replication-gencols"/> for details.
>    </para>
>
> I am not so sure about this. It seemed overly strict to me.
>
> Why can't it simply say:
> "Generated columns can also be specified in a column list. This allows
> generated columns to be published or excluded, regardless of the
> publication parameter..."
>
> Specifically, I don't know why you need to say:
> Generated columns can be included in column list specified with EXCEPT
> clause if publication parameter publish_generated_columns is not set
> to none. Specified generated columns will not be published.
>
> IIUC, then EXCEPT (gencol1, gencol2) is saying to exclude the named
> cols. So if param is "stored", then the named cols will be excluded.
> OTOH, if param is "none" then all generated cols will be excluded
> anyway, so why not just allow the EXCEPT (gencol,gencol2) here as
> well, because the result will be the same.
>
>
I have removed this change. And allowed specifying generated columns
in EXCEPT column list as well irrespective of value of
‘publish_generated_columns’.

> ~~~
>
> 7. (29.5.1. Examples)
>
>     <para>
> -    Create a table <literal>t1</literal> to be used in the following example.
> +    Create tables <literal>t1</literal>, <literal>t2</literal> to be
> used in the
> +    following example.
>
> /Create tables t1, t2/Create tables t1 and t2/
>
Fixed

> ~~~
>
> 8.
>     <para>
>      Create a publication <literal>p1</literal>. A column list is defined for
> -    table <literal>t1</literal> to reduce the number of columns that will be
> -    replicated. Notice that the order of column names in the column list does
> -    not matter.
> +    table <literal>t1</literal> and a column list is defined for table
> +    <literal>t2</literal> with EXCEPT clause to reduce the number of
> columns that will be
> +    replicated. Notice that the order of column names in the column
> lists does not matter.
>
> BEFORE
> A column list is defined for table t1 and a column list is defined for
> table t2...
>
> SUGGESTION (added comma, etc.)
> A column list is defined for table t1, and another column list is
> defined for table t2...
>
Fixed

> ~~~
>
> 9.
> The final example still says:
> "Only data from the column list of publication p1 is replicated."
>
> That doesn't seem quite appropriate now that you also have an EXCEPT
> column list.
>
> SUGGESTION:
> Only data specified by the column lists of publication p1 is replicated.
>
Fixed

> ======
> doc/src/sgml/ref/create_publication.sgml
>
> 10.
> +     <para>
> +      When a column list is specified with EXCEPT, the named columns are not
> +      replicated. Specifying a column list has no effect on
> +      <literal>TRUNCATE</literal> commands.
> +     </para>
>
> I felt that to be clearer the preceding paragraph should be changed as follows:
>
> /When a column list is specified, only the named columns are
> replicated./When a column list without EXCEPT is specified, only the
> named columns are replicated./
>
Fixed

> ~~~
>
> 11. CREATE PUBLICATION (NOTES section)
>
> 11a.
> The NOTES talk about replica identity columns -- should you mention EXCEPT here?
>
Added notes for EXCEPT

> ~
>
> 11b.
> The NOTES talk about generated columns -- should you mention EXCEPT here?
>
I felt it is not needed.

> ======
> src/backend/catalog/pg_publication.c
>
> 12. check_and_fetch_column_list
>
> + if (!isnull)
> + except = DatumGetBool(cfdatum);
> +
> + *except_columns = except && !pub->alltables;
>
> AFAICT, you can Assert(!pub->alltables) because you already checked
> that earlier up front.
> So you don't need 'except' var either. Just assign *except_cols up
> front and then overwrite it later if true.
>
> SUGGESTION:
>
> *except_cols = false;
>
> if (pub->alltables)
>   return false;
> ...
> if (!isnull)
>  *except_cols = DatumGetBool(cfdatum);
>
Fixed

> ~~~
>
> 13. publication_add_relation
>
>   /* Validate and translate column names into a Bitmapset of attnums. */
> - attnums = pub_collist_validate(pri->relation, pri->columns);
> + attnums = pub_collist_validate(pri->relation, pri->columns,
> +    pri->except && !pub->alltables,
> +    pub->pubgencols_type);
>
>
> I am wondering why we are even calling a function to validate column
> lists if pub->alltables was true. AFAIK, that combination of
> column-lists and FOR ALL TABLES is not even possible, so the code
> seems strange.
>
Fixed

> ~~~
>
> 14. pub_exclude_collist_validate
> .
> + /*
> + * Check if column list specified with EXCEPT have any stored
> + * generated column and 'publish_generated_columns' is not set to
> + * 'stored'.
> + */
> + if (except_columns &&
> + TupleDescAttr(tupdesc, attnum - 1)->attgenerated ==
> ATTRIBUTE_GENERATED_STORED &&
> + pubgencols_type != PUBLISH_GENCOLS_STORED)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
> + errmsg("cannot use stored generated column \"%s\" in publication
> column list specified with EXCEPT when \"%s\" set to \"%s\"",
> +    colname, "publish_generated_columns", "stored"));
>
> As mentioned in the above DOCS comments, I was having doubts about why
> we have this error.
>
> If the parameter says "none", then generated columns will not be
> replicated, so why should we care if the user also says
> EXCEPT(gencol1,gencol2). Either way, the result will be the same; the
> generated column will not be published.
>
Removed this restriction.

> ~~~
>
> 15. GetRelationPublications
>
>   {
>   HeapTuple tup = &pubrellist->members[i]->tuple;
>   Oid pubid = ((Form_pg_publication_rel) GETSTRUCT(tup))->prpubid;
> + HeapTuple pubtup = SearchSysCache1(PUBLICATIONOID, ObjectIdGetDatum(pubid));
> + bool is_table_excluded = ((Form_pg_publication)
> GETSTRUCT(pubtup))->puballtables &&
> + ((Form_pg_publication_rel) GETSTRUCT(tup))->prexcept;
>
> - if (except_flag == ((Form_pg_publication_rel) GETSTRUCT(tup))->prexcept)
> + if (except_flag == is_table_excluded)
>   result = lappend_oid(result, pubid);
> +
> + ReleaseS
>
>
> I'm not 100% sure you need the additional 'pubtup'... Can't you just
> look at the "prattrs" field to see if a column-list was specified? If
> "prattrs" is null and "prexcept" is true, isn't that the same
> combination as what you are looking for here?
>
Yes, we can use this combination as well. Fixed it in latest patch.

> ~~~
>
> 16. pg_get_publication_tables
>
> + columnsDatum = SysCacheGetAttr(PUBLICATIONRELMAP, pubtuple,
> +    Anum_pg_publication_rel_prattrs,
> +    &(nulls[2]));
> +
> + /* if column list is specified with EXCEPT */
> + if (!pub->alltables && except)
> + columns = pub_collist_to_bitmapset(NULL, columnsDatum, NULL);
> + else
> + values[2] = columnsDatum;
>
> 16a.
> Something seems fishy here. Isn't there a pathway where you missed
> assigning value[2] to anything?
>
Modified this change.

> ~
>
> 16b.
> Also, I feel there should be some other boolean variable used here
> instead of checking bot (!pub->alltables && except) in multiple
> places.
>
Fixed
>
> ======
> src/backend/replication/pgoutput/pgoutput.c
>
> 17. RelationSyncEntry
> +
> + /* Indicate if no column is included in the publication */
> + bool no_cols_published;
>
> Maybe this can have a more explanatory comment to explain why it is needed?
>
Fixed

> ~~~
>
> 18. check_and_init_gencol
>
> + bool found = false;
> + bool except_columns = false;
> +
> + found = check_and_fetch_column_list(pub, entry->publish_as_relid, NULL,
> + NULL, &except_columns);
> +
>   /*
>   * The column list takes precedence over the
>   * 'publish_generated_columns' parameter. Those will be checked later,
> - * see pgoutput_column_list_init.
> + * see pgoutput_column_list_init. But when a column list is specified
> + * with EXCEPT, it should be checked.
>   */
> - if (check_and_fetch_column_list(pub, entry->publish_as_relid, NULL, NULL))
> + if (found && !except_columns)
>   continue;
>
> The variable 'found' seems a poor name; how about 'has_column_list' or similar?
>
Fixed

> ~~~
>
> 19. pgoutput_change
>
> + /*
> + * If all columns of a table is present in column list specified with
> + * EXCEPT, skip publishing the changes.
> + */
> + if (relentry->no_cols_published)
> + return;
>
> /is present/are present/
>
fixed

> ======
> src/bin/pg_dump/pg_dump.c
>
> 20. getPublicationTables
>
> + if (strcmp(prexcept, "t") == 0 && PQgetisnull(res, i, i_prattrs))
>   pubrinfo[j].dobj.objType = DO_PUBLICATION_EXCEPT_REL;
> + else
> + pubrinfo[j].dobj.objType = DO_PUBLICATION_REL;
>
>   pubrinfo[j].dobj.catId.tableoid =
>   atooid(PQgetvalue(res, i, i_tableoid));
> @@ -4797,6 +4797,7 @@ getPublicationTables(Archive *fout, TableInfo
> tblinfo[], int numTables)
>   pubrinfo[j].pubrelqual = NULL;
>   else
>   pubrinfo[j].pubrelqual = pg_strdup(PQgetvalue(res, i, i_prrelqual));
> + pubrinfo[j].pubexcept = (strcmp(prexcept, "t") == 0);
>
>
> Why not assign pubrinfo[j].pubexcept earlier so you don't have to
> repeat the strcmp?
>
Fixed

> ~~~
>
> 21.
> - if (strcmp(prexcept, "t") == 0)
> + if (strcmp(prexcept, "t") == 0 && PQgetisnull(res, i, i_prattrs))
>   simple_ptr_list_append(&exceptinfo, &pubrinfo[j]);
>
> Why not assign pubrinfo[j].pubexcept earlier so you don't have to
> repeat the strcmp? Same also for the PQgetisnull(res, i,
> i_prattrs))...
>
Fixed

> ~~~
>
> 22. dumpPublicationTable
>
>   if (pubrinfo->pubrattrs)
> - appendPQExpBuffer(query, " (%s)", pubrinfo->pubrattrs);
> + {
> + if (pubrinfo->pubexcept)
> + appendPQExpBuffer(query, " EXCEPT (%s)", pubrinfo->pubrattrs);
> + else
> + appendPQExpBuffer(query, " (%s)", pubrinfo->pubrattrs);
> + }
>
> SUGGESTION
> {
>   if (pubrinfo->pubexcept)
>     appendPQExpBuffer(query, " EXCEPT");
>
>   appendPQExpBuffer(query, " (%s)", pubrinfo->pubrattrs);
> }
Fixed

I have addressed the comments shared by you and shared the updated v15
patch set here.

Thanks and Regards,
Shlok Kyal

On Fri, Jun 27, 2025 at 3:44 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 26 Jun 2025 at 15:27, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Tue, Jun 24, 2025 at 9:48 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> > >  I have included the changes for
> > > it in v14-0003 patch.
> > >
> > Thanks for the patches. I have reviewed patch001 alone, please find
> > few comments:
> >
> > 1)
> > +  <para>
> > +   The <literal>RESET</literal> clause will reset the publication to the
> > +   default state which includes resetting the publication parameters, setting
> > +   <literal>ALL TABLES</literal> flag to <literal>false</literal> and
> > +   dropping all relations and schemas that are associated with the
> > +   publication.
> >    </para>
> >
> > It is misleading, as far as I have understood, we do not drop the
> > tables or schemas associated with the pub; we just remove those from
> > the publication's object list. See previous doc:
> > "The ADD and DROP clauses will add and remove one or more
> > tables/schemas from the publication"
> >
> > Perhaps we want to say the same thing when we speak about the 'drop'
> > aspect of RESET.
> I have updated the document.
>
> > 2)
> > AlterPublicationReset():
> >
> > + if (!OidIsValid(prid))
> > + ereport(ERROR,
> > + (errcode(ERRCODE_UNDEFINED_OBJECT),
> > + errmsg("relation \"%s\" is not part of the publication",
> > + get_rel_name(relid))));
> >
> > Can you please help me understand which scenario will give this error?
> >
> > Another question is do we really need this error? IIUC, we generally
> > give errors if a user has explicitly called out a name of an object
> > and that object is not found. Example:
> >
> > postgres=# alter publication pubnew drop table t1,tab2;
> > ERROR:  relation "t1" is not part of the publication
> >
> > While in a few other cases, we pass missing_okay as true and do not
> > give errors. Please see other callers of performDeletion in
> > publicationcmds.c itself. There we have usage of missing_okay=true. I
> > have not researched myself, but please analyze the cases where
> > missing_okay is passed as true to figure out if those match our RESET
> > case. Try to reproduce if possible and then take a call.
> I thought about the above point and I also think this check is not
> required. Also, the function was calling PublicationDropSchemas with
> missing_ok as false. I have changed it to be true.
>

Okay. Is there a reason for not using PublicationDropTables() here? We
have rewritten similar code in the Reset flow.

> > 3)
> > +ALTER PUBLICATION testpub_reset ADD ALL TABLES IN SCHEMA public;
> > +ERROR:  syntax error at or near "ALL"
> > +LINE 1: ALTER PUBLICATION testpub_reset ADD ALL TABLES IN SCHEMA pub...
> >
> > There is a problem in syntax, I think the intention of testcase was to
> > run this query successfully.
>
> I have fixed it.
>
> Thanks Shveta for reviewing the patch. I have addressed the comments
> and posted an updated version v15 in [1].

Thanks for the patches. My review is in progress but please find few
comments on 002:

1)
where exception_object is:
    [ ONLY ] table_name [ * ]

We have the above in CREATE and ALTER pub docs, but we do not explain
ONLY with EXCEPT. We do have an explanation of ONLY under 'FOR TABLE'.
But since 'FOR TABLE' and 'EXCEPT' do not go together, it is somewhat
difficult to connect the dots and find the information ONLY in the
context of EXCEPT. We shall have ONLY explained for EXCEPT as well. Or
we can have ONLY defined in a way that both 'FOR TABLE' and 'EXCEPT'
can refer to it.

2)
We get tab-completion options in this command:
postgres=# create publication pub5 for TABLE tab1 W
WHERE (  WITH (

Similarly in this command:
create publication pub5 for TABLES IN SCHEMA s1

But once we have 'EXCEPT TABLE', we do not get further tab-completion
option like WITH(...)
create publication pub5 for ALL TABLES EXCEPT TABLE tab1

3)
During tab-expansion, 'EXCEPT TABLE' and  'WITH (' in the below
command looks like they are connecting words. Can the gap be increased
similar to tab-expansion of next command shown below:

postgres=# create publication pub4 for ALL TABLES
EXCEPT TABLE  WITH (

postgres=# create publication pub4 for
ALL TABLES        TABLE             TABLES IN SCHEMA

4)
alter_publication.sgml.orig is a left-over in patch002.

thanks
Shveta