[PATCH]Comment improvement in publication.sql

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] /messages/by-id/OS0PR01MB6113CC160D0F134448567FDDFBE99@OS0PR01MB6113.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]/messages/by-id/OS0PR01MB61132C2C4E2232258EB192FDFBF19@OS0PR01MB6113.jpnprd01.prod.outlook.com.

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]: /messages/by-id/OS0PR01MB61132C2C4E2232258EB192FDFBF19@OS0PR01MB6113.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