Re: Skipping schema changes in publication - Mailing list pgsql-hackers
From | vignesh C |
---|---|
Subject | Re: Skipping schema changes in publication |
Date | |
Msg-id | CALDaNm1Z1Rmqj9s6P9ZzmrVA9F_vZ_DwwhYAJmsjqmY6dS3-hA@mail.gmail.com Whole thread Raw |
In response to | Re: Skipping schema changes in publication (vignesh C <vignesh21@gmail.com>) |
Responses |
Re: Skipping schema changes in publication
|
List | pgsql-hackers |
On Mon, Aug 8, 2022 at 2:53 PM vignesh C <vignesh21@gmail.com> wrote: > > On Mon, Aug 8, 2022 at 12:46 PM vignesh C <vignesh21@gmail.com> wrote: > > > > 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 andALL 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. The patch needed to be rebased on top of HEAD because of a recent commit. The updated v8 patch has the changes for the same. Regards, Vignesh
Attachment
pgsql-hackers by date: