Re: Skipping schema changes in publication

On Thu, May 26, 2022 at 7:04 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Monday, May 23, 2022 2:13 PM vignesh C <vignesh21(at)gmail(dot)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 publication options, 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 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.

Regards,
Vignesh