On Mon, May 16, 2022 at 2:00 PM osumi.takami...@fujitsu.com
<osumi.takami...@fujitsu.com> wrote:
>
> On Saturday, May 14, 2022 10:33 PM vignesh C <vignes...@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
