I spent some time on understanding the proposal and the patch. Here
are a few comments wrt the test cases.
> +ALTER PUBLICATION testpub_reset ADD TABLE pub_sch1.tbl1;
> +
> +-- Verify that tables associated with the publication are dropped after RESET
> +\dRp+ testpub_reset
> +ALTER PUBLICATION testpub_reset RESET;
> +\dRp+ testpub_reset
>
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES IN SCHEMA public;
> +
> +-- Verify that schemas associated with the publication are dropped after
> RESET
> +\dRp+ testpub_reset
> +ALTER PUBLICATION testpub_reset RESET;
> +\dRp+ testpub_reset
The results for the above two cases are the same before and after the
reset. Is there any way to verify that?
---
> +-- Can't add EXCEPT TABLE to 'FOR ALL TABLES' publication
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1;
> +
>
> +-- Can't add EXCEPT TABLE to 'FOR TABLE' publication
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1;
> +
>
> +-- Can't add EXCEPT TABLE to 'FOR ALL TABLES IN SCHEMA' publication
> +ALTER PUBLICATION testpub_reset ADD ALL TABLES EXCEPT TABLE pub_sch1.tbl1;
> +
I did not understand the objective of these tests. I think we need to
improve the comments.
Thanks & Regards,
On Mon, Aug 8, 2022 at 2:53 PM vignesh C <vignes...@gmail.com> wrote:
>
> On Mon, Aug 8, 2022 at 12:46 PM vignesh C <vignes...@gmail.com> wrote:
> >
> > On Fri, Jun 3, 2022 at 3:36 PM vignesh C <vignes...@gmail.com> wrote:
> > >
> > > On Thu, May 26, 2022 at 7:04 PM osumi.takami...@fujitsu.com
> > > <osumi.takami...@fujitsu.com> wrote:
> > > >
> > > > On Monday, May 23, 2022 2:13 PM vignesh C <vignes...@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 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.
> >
> > 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
