Обсуждение: [PATCH]Comment improvement in publication.sql
Hi Hackers When review and test another patch at [1], I found some comments in existing test code of " src/test/regress/sql/publication.sql" is a little bit confused. Attached a patch to fix them, please take a check. Here is the detail: Existing code: CREATE TABLE testpub_tbl2 (id serial primary key, data text); -- fail - can't add to for all tables publication ALTER PUBLICATION testpub_foralltables ADD TABLE testpub_tbl2; -- fail - can't drop from all tables publication ALTER PUBLICATION testpub_foralltables DROP TABLE testpub_tbl2; -- fail - can't add to for all tables publication ALTER PUBLICATION testpub_foralltables SET TABLE pub_test.testpub_nopk; After patch: CREATE TABLE testpub_tbl2 (id serial primary key, data text); -- fail - tables can't be added to or dropped form FOR ALL TABLES publications ALTER PUBLICATION testpub_foralltables ADD TABLE testpub_tbl2; ALTER PUBLICATION testpub_foralltables DROP TABLE testpub_tbl2; ALTER PUBLICATION testpub_foralltables SET TABLE pub_test.testpub_nopk; You see the comment for SET TABLE is not appropriate. And above three operations(ADD DROP SET) output the same message as below: "DETAIL: Tables cannot be added to or dropped from FOR ALL TABLES publications." So maybe we can combine the existing three comments to one, thoughts? Besides, another comment in the same file is not clear enough to me: -- fail - already added CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1; Maybe it will be better if we use 'already exists'. Thoughts? [1] https://www.postgresql.org/message-id/OS0PR01MB6113CC160D0F134448567FDDFBE99%40OS0PR01MB6113.jpnprd01.prod.outlook.com Regards Tang
Вложения
On Mon, Aug 2, 2021 at 3:31 PM tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > > Hi Hackers > > When review and test another patch at [1], I found some comments in existing test code of " src/test/regress/sql/publication.sql" is a little bit confused. > Attached a patch to fix them, please take a check. > > Here is the detail: > > Existing code: > CREATE TABLE testpub_tbl2 (id serial primary key, data text); > -- fail - can't add to for all tables publication > ALTER PUBLICATION testpub_foralltables ADD TABLE testpub_tbl2; > -- fail - can't drop from all tables publication > ALTER PUBLICATION testpub_foralltables DROP TABLE testpub_tbl2; > -- fail - can't add to for all tables publication > ALTER PUBLICATION testpub_foralltables SET TABLE pub_test.testpub_nopk; > > After patch: > CREATE TABLE testpub_tbl2 (id serial primary key, data text); > -- fail - tables can't be added to or dropped form FOR ALL TABLES publications > ALTER PUBLICATION testpub_foralltables ADD TABLE testpub_tbl2; > ALTER PUBLICATION testpub_foralltables DROP TABLE testpub_tbl2; > ALTER PUBLICATION testpub_foralltables SET TABLE pub_test.testpub_nopk; > > You see the comment for SET TABLE is not appropriate. > And above three operations(ADD DROP SET) output the same message as below: > "DETAIL: Tables cannot be added to or dropped from FOR ALL TABLES publications." > > So maybe we can combine the existing three comments to one, thoughts? > > Besides, another comment in the same file is not clear enough to me: > -- fail - already added > CREATE PUBLICATION testpub_fortbl FOR TABLE testpub_tbl1; > > Maybe it will be better if we use 'already exists'. Thoughts? > > [1] https://www.postgresql.org/message-id/OS0PR01MB6113CC160D0F134448567FDDFBE99%40OS0PR01MB6113.jpnprd01.prod.outlook.com Few minor suggestions: 1) Should we change below to "fail - tables can't be added, dropped or set to "FOR ALL TABLES" publications" --- fail - can't add to for all tables publication +-- fail - tables can't be added to or dropped from FOR ALL TABLES publications 2) Should we change this to "--fail - already existing publication" --- fail - already added +-- fail - already exists Regards, Vignesh
On Monday, August 2, 2021 11:56 PM vignesh C <vignesh21@gmail.com> wrote: > > Few minor suggestions: > 1) Should we change below to "fail - tables can't be added, dropped or > set to "FOR ALL TABLES" publications" > --- fail - can't add to for all tables publication > +-- fail - tables can't be added to or dropped from FOR ALL TABLES publications Thanks for your kindly comments. I'm not sure should we ignore that 'drop' should be followed by 'from', but I think there's no misunderstandings. So, modified as you described. Besides, in other file comments (src/test/subscription/t/100_bugs.pl) I saw FOR ALL TABLES without using quotes. So I don't think we need the quotes, too. > 2) Should we change this to "--fail - already existing publication" > --- fail - already added > +-- fail - already exists Modified. A modified patch is attached. Regards Tang
Вложения
On Tue, Aug 3, 2021 at 8:36 AM tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > > On Monday, August 2, 2021 11:56 PM vignesh C <vignesh21@gmail.com> wrote: > > > > Few minor suggestions: > > 1) Should we change below to "fail - tables can't be added, dropped or > > set to "FOR ALL TABLES" publications" > > --- fail - can't add to for all tables publication > > +-- fail - tables can't be added to or dropped from FOR ALL TABLES publications > > Thanks for your kindly comments. > > I'm not sure should we ignore that 'drop' should be followed by 'from', but I > think there's no misunderstandings. So, modified as you described. > > Besides, in other file comments (src/test/subscription/t/100_bugs.pl) I saw FOR ALL TABLES without using quotes. > So I don't think we need the quotes, too. > > > 2) Should we change this to "--fail - already existing publication" > > --- fail - already added > > +-- fail - already exists > > Modified. > > A modified patch is attached. Thanks for the updated patch, the changes look good to me. Regards, Vignesh
Hi
I saw some inaccurate comments for AlterPublicationStmt structure when
reviewing patches related to publication[1].
The variable tables are used for 'ALTER PUBLICATION ... ADD/DROP/SET TABLE',
but the comments only say 'ADD/DROP'. How about add 'SET' to the comments?
typedef struct AlterPublicationStmt
{
NodeTag type;
char *pubname; /* Name of the publication */
/* parameters used for ALTER PUBLICATION ... WITH */
List *options; /* List of DefElem nodes */
/* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */
List *tables; /* List of tables to add/drop */
bool for_all_tables; /* Special publication for all tables in db */
DefElemAction tableAction; /* What action to perform with the tables */
} AlterPublicationStmt;
It's also a comment improvement, so I add this change to this patch.
A new version patch is attached, pleases have a look.
[1]
https://www.postgresql.org/message-id/OS0PR01MB61132C2C4E2232258EB192FDFBF19%40OS0PR01MB6113.jpnprd01.prod.outlook.com
Regards
Tang
Вложения
On Fri, Aug 6, 2021 at 3:33 PM tanghy.fnst@fujitsu.com
<tanghy.fnst@fujitsu.com> wrote:
>
> Hi
>
> I saw some inaccurate comments for AlterPublicationStmt structure when
> reviewing patches related to publication[1].
>
> The variable tables are used for 'ALTER PUBLICATION ... ADD/DROP/SET TABLE',
> but the comments only say 'ADD/DROP'. How about add 'SET' to the comments?
>
> typedef struct AlterPublicationStmt
> {
> NodeTag type;
> char *pubname; /* Name of the publication */
>
> /* parameters used for ALTER PUBLICATION ... WITH */
> List *options; /* List of DefElem nodes */
>
> /* parameters used for ALTER PUBLICATION ... ADD/DROP TABLE */
> List *tables; /* List of tables to add/drop */
> bool for_all_tables; /* Special publication for all tables in db */
> DefElemAction tableAction; /* What action to perform with the tables */
> } AlterPublicationStmt;
>
> It's also a comment improvement, so I add this change to this patch.
Thanks for the updated patch, your changes look good to me. You might
want to include the commit message in the patch, that will be useful.
Regards,
Vignesh
On Sunday, August 8, 2021 6:34 PM, vignesh C <vignesh21@gmail.com> wrote >Thanks for the updated patch, your changes look good to me. You might >want to include the commit message in the patch, that will be useful. Thanks for your kindly suggestion. Updated patch attached. Added the patch commit message with no new fix. Regards, Tang
Вложения
On Sun, Aug 8, 2021 at 4:26 PM tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > > On Sunday, August 8, 2021 6:34 PM, vignesh C <vignesh21@gmail.com> wrote > >Thanks for the updated patch, your changes look good to me. You might > >want to include the commit message in the patch, that will be useful. > > Thanks for your kindly suggestion. > Updated patch attached. Added the patch commit message with no new fix. The patch no longer applies, could you post a rebased patch. Regards, Vignesh
On Wednesday, December 8, 2021 1:49 PM, vignesh C <vignesh21@gmail.com> wrote: > The patch no longer applies, could you post a rebased patch. Thanks for your kindly reminder. Attached a rebased patch. Some changes in v4 patch has been fixed by 5a28324, so I deleted those changes. Regards, Tang
Вложения
On Wed, Dec 8, 2021 at 11:07 AM tanghy.fnst@fujitsu.com <tanghy.fnst@fujitsu.com> wrote: > > On Wednesday, December 8, 2021 1:49 PM, vignesh C <vignesh21@gmail.com> wrote: > > > The patch no longer applies, could you post a rebased patch. > > Thanks for your kindly reminder. Attached a rebased patch. > Some changes in v4 patch has been fixed by 5a28324, so I deleted those changes. Thanks for the updated patch, should we make a similar change in AlterPublicationTables Function header to mention Set too: /* * Add or remove table to/from publication. */ static void AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup, List *tables, List *schemaidlist) Regards, Vignesh
On Monday, December 13, 2021 11:53 PM vignesh C <vignesh21@gmail.com> wrote: > > On Wed, Dec 8, 2021 at 11:07 AM tanghy.fnst@fujitsu.com > <tanghy.fnst@fujitsu.com> wrote: > > > > On Wednesday, December 8, 2021 1:49 PM, vignesh C <vignesh21@gmail.com> > wrote: > > > > > The patch no longer applies, could you post a rebased patch. > > > > Thanks for your kindly reminder. Attached a rebased patch. > > Some changes in v4 patch has been fixed by 5a28324, so I deleted those > changes. > > Thanks for the updated patch, should we make a similar change in > AlterPublicationTables Function header to mention Set too: > /* > * Add or remove table to/from publication. > */ > static void > AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup, > List *tables, List *schemaidlist) > Sorry for the late reply. I am not sure if we need this change because the way to SET the tables in publication is that drop tables and then add tables, instead of directly replacing the list of tables in the publication. (We can see this point in AlterPublicationTables function.). Thoughts? Regards, Tang