MIME-Version: 1.0
References: 
 <CALDaNm3=JrucjhiiwsYQw5-PGtBHFONa6F7hhWCXMsGvh=tamA@mail.gmail.com>
 <CALj2ACVOzhs+BD+abFV2x4oKJdsDNd6SgsE7r8UjnZDCKGEckA@mail.gmail.com>
 <CAA4eK1K6Kr88d2S0zFdHRMyuoaZeNh+ktU+oigmCuD09_x_-+g@mail.gmail.com>
 <CAHut+PsvC-NezO3MJkdyEz=G1QRje2LntjwhQiEeVbmhOQuBMA@mail.gmail.com>
 <CALDaNm18VH2j8cTqfELHQ=0ZNognbGBhbHPteJenWQC6C2dueQ@mail.gmail.com>
 <CALDaNm0k_0Ccj47wzJzzPFwgQB7w=R5+Q2_nSqYrmMmjhmcRUw@mail.gmail.com>
 <CAHut+Pv_0DwyWoGQNMF+G2AGqMuJTzWQKRtmxaC+=zLTPL-Zkw@mail.gmail.com>
 <CALDaNm2-GJt2HsYTkLqQ=ecm=R-vOBw1=aM_d2EiYbz39x_cTQ@mail.gmail.com>
 <TYCPR01MB83737C28187A6E0BADAE98F0EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <TYCPR01MB83737C28187A6E0BADAE98F0EDCF9@TYCPR01MB8373.jpnprd01.prod.outlook.com>
From: vignesh C <vignesh21@gmail.com>
Date: Wed, 18 May 2022 23:24:02 +0530
Message-ID: 
 <CALDaNm3XiwZKFVkfQ+Q0GobWkyMb3ygKqghWQWuDj7ZxR9rUMw@mail.gmail.com>
Subject: Re: Skipping schema changes in publication
To: "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>
Cc: Peter Smith <smithpb2250@gmail.com>,
 Amit Kapila <amit.kapila16@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3XiwZKFVkfQ%2BQ0GobWkyMb3ygKqghWQWuDj7ZxR9rUMw%40mail.gmail.com>
Precedence: bulk

On Mon, May 16, 2022 at 2:00 PM 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,
>
>
>
> Several comments on v5-0002.
>
> (1) One unnecessary space before "except_pub_obj_list" syntax definition
>
> + except_pub_obj_list:  ExceptPublicationObjSpec
> +                                       { $$ = list_make1($1); }
> +                       | except_pub_obj_list ',' ExceptPublicationObjSpec
> +                                       { $$ = lappend($1, $3); }
> +                       |  /*EMPTY*/                                                            { $$ = NULL; }
> +       ;
> +
>
> From above part, kindly change
> FROM:
> " except_pub_obj_list:  ExceptPublicationObjSpec"
> TO:
> "except_pub_obj_list:  ExceptPublicationObjSpec"
>

Modified

> (2) doc/src/sgml/ref/create_publication.sgml
>
> (2-1)
>
> @@ -22,7 +22,7 @@ PostgreSQL documentation
>   <refsynopsisdiv>
>  <synopsis>
>  CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
> -    [ FOR ALL TABLES
> +    [ FOR ALL TABLES [EXCEPT TABLE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [, ... ]]
>        | FOR <replaceable class="parameter">publication_object</replaceable> [, ... ] ]
>      [ WITH ( <replaceable class="parameter">publication_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] ) ]
>
>
> Here I think we need to add two more whitespaces around square brackets.
> Please change
> FROM:
> "[ FOR ALL TABLES [EXCEPT TABLE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [, ... ]]"
> TO:
> "[ FOR ALL TABLES [ EXCEPT TABLE [ ONLY ] <replaceable class="parameter">table_name</replaceable> [ * ] [, ... ] ]"
>
> When I check other documentations, I see whitespaces before/after square brackets.
>
> (2-2)
> This whitespace alignment applies to alter_publication.sgml as well.

Modified

> (3)
>
>
> @@ -156,6 +156,24 @@ CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
>      </listitem>
>     </varlistentry>
>
> +
> +   <varlistentry>
> +    <term><literal>EXCEPT TABLE</literal></term>
> +    <listitem>
> +     <para>
> +      Marks the publication as one that excludes replicating changes for the
> +      specified tables.
> +     </para>
> +
> +     <para>
> +      <literal>EXCEPT TABLE</literal> can be specified only for
> +      <literal>FOR ALL TABLES</literal> publication. It is not supported for
> +      <literal>FOR ALL TABLES IN SCHEMA </literal> publication and
> +      <literal>FOR TABLE</literal> publication.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> +
>
> This EXCEPT TABLE clause is only for FOR ALL TABLES.
> So, how about extracting the main message from above part and
> moving it to an exising paragraph below, instead of having one independent paragraph ?
>
>    <varlistentry>
>     <term><literal>FOR ALL TABLES</literal></term>
>     <listitem>
>      <para>
>       Marks the publication as one that replicates changes for all tables in
>       the database, including tables created in the future.
>      </para>
>     </listitem>
>    </varlistentry>
>
> Something like
> "Marks the publication as one that replicates changes for all tables in
> the database, including tables created in the future. EXCEPT TABLE indicates
> excluded tables for the defined publication.
> "
>

Modified

> (4) One minor confirmation about the syntax
>
> Currently, we allow one way of writing to indicate excluded tables like below.
>
> (example) CREATE PUBLICATION mypub FOR ALL TABLES EXCEPT TABLE tab3, tab4, EXCEPT TABLE tab5;
>
> This is because we define ExceptPublicationObjSpec with EXCEPT TABLE.
> Is it OK to have a room to write duplicate "EXCEPT TABLE" clauses ?
> I think there is no harm in having this,
> but I'd like to confirm whether this syntax might be better to be adjusted or not.

Changed it to allow except table only once

>
> (5) CheckAlterPublication
>
> +
> +       if (excepttable && !stmt->for_all_tables)
> +               ereport(ERROR,
> +                               (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                                errmsg("publication \"%s\" is not defined as FOR ALL TABLES",
> +                                               NameStr(pubform->pubname)),
> +                                errdetail("except table cannot be added to, dropped from, or set on NON ALL TABLES publications.")));
>
> Could you please add a test for this ?

This code can be removed because of grammar optimization, it will not
allow tables without "ALL TABLES". Removed this code

The v6 patch attached at [1] has the changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm0iZZDB300Dez_97S8G6_RW5QpQ8ef6X3wq8tyK-8wnXQ%40mail.gmail.com

Regards,
Vignesh