Re: Skipping schema changes in publication

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