Обсуждение: Skipping schema changes in publication

Поиск

Список

Период

Сортировка

Skipping schema changes in publication

От

vignesh C

Дата:

22 марта 2022 г., 10: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

Вложения

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

Re: Skipping schema changes in publication

От

vignesh C

Дата:

26 марта 2022 г., 17: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

Вложения

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

Re: Skipping schema changes in publication

От

vignesh C

Дата:

12 апреля 2022 г., 09: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

От

Amit Kapila

Дата:

18 апреля 2022 г., 10: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

От

vignesh C

Дата:

18 апреля 2022 г., 12: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

От

vignesh C

Дата:

21 апреля 2022 г., 06: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

Вложения

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

Re: Skipping schema changes in publication

От

Bharath Rupireddy

Дата:

22 апреля 2022 г., 19: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

От

Peter Smith

Дата:

26 апреля 2022 г., 04: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

От

"osumi.takamichi@fujitsu.com"

Дата:

26 апреля 2022 г., 09: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

От

vignesh C

Дата:

27 апреля 2022 г., 15: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

Вложения

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

RE: Skipping schema changes in publication

От

"osumi.takamichi@fujitsu.com"

Дата:

28 апреля 2022 г., 14: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

От

Amit Kapila

Дата:

28 апреля 2022 г., 14: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

От

vignesh C

Дата:

29 апреля 2022 г., 14: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

Вложения

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

Re: Skipping schema changes in publication

От

Peter Smith

Дата:

03 мая 2022 г., 11: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

От

Amit Kapila

Дата:

04 мая 2022 г., 07: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

От

Peter Eisentraut

Дата:

04 мая 2022 г., 16: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

От

Amit Kapila

Дата:

05 мая 2022 г., 06: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

От

Amit Kapila

Дата:

05 мая 2022 г., 07: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

От

Peter Smith

Дата:

06 мая 2022 г., 05: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

От

vignesh C

Дата:

10 мая 2022 г., 06: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

От

vignesh C

Дата:

12 мая 2022 г., 07: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

Вложения

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

Re: Skipping schema changes in publication

От

Peter Smith

Дата:

13 мая 2022 г., 07: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

От

vignesh C

Дата:

14 мая 2022 г., 16: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

От

"houzj.fnst@fujitsu.com"

Дата:

14 июня 2022 г., 06: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

От

Amit Kapila

Дата:

16 июня 2022 г., 07: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

От

vignesh C

Дата:

08 августа 2022 г., 10: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

Вложения

Re: Skipping schema changes in publication

От

vignesh C

Дата:

08 августа 2022 г., 12: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

Вложения

Re: Skipping schema changes in publication

От

Ian Lawrence Barwick

Дата:

04 ноября 2022 г., 05: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

От

vignesh C

Дата:

07 ноября 2022 г., 16: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

Вложения

Re: Skipping schema changes in publication

От

Ian Lawrence Barwick

Дата:

16 ноября 2022 г., 07: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

От

vignesh C

Дата:

16 ноября 2022 г., 13: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

Вложения

Re: Skipping schema changes in publication

От

vignesh C

Дата:

20 января 2023 г., 13: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

Вложения

Re: Skipping schema changes in publication

От

vignesh C

Дата:

09 января, 09:32:04

On Fri, 20 Jan 2023 at 15:30, vignesh C <vignesh21@gmail.com> wrote:
>
> 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.

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.

Regards,
Vignesh

Вход в личный кабинет

Восстановление пароля

Подтверждение аккаунта

Изменение пароля

Обсуждение: Skipping schema changes in publication

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения