Hi Shubham, here are some comments for v40-0003 (TAP) patch.
======
Combo tests
1.
+# =============================================================================
+# The following test cases exercise logical replication for the combinations
+# where there is a generated column on one or both sides of pub/sub:
+# - generated -> normal
Saying "where there is a generated column on one or both sides of the
pub/sub" was a valid comment back when all possible combinations were
tested. Now, if you are only going to test one case ("generated ->
normal") then that comment is plain wrong.
2.
Why have you removed "copy_data=true" from some comments, but not consistently?
======
DROP EXPRESSION test
3.
(from v39-0003)
+# A "normal -> generated" replication fails, reporting an error that the
+# subscriber side column is missing.
In v40-003 this comment was broken. In v39-0003 the above comment
mentioned about failing, but in v40-0003 that comment was removed. The
v39 comment was better, because that is the whole point of this test
-- that normal->generated would fail *unless* the DROP EXPRESSION
worked correctly and changed it to a normal->normal test.
Previously we had already "proved" normal->generated failed as
expected, but since you've removed all those combo tests, now all we
have is this comment to explain it.
~~~
4.
+# Insert data to verify replication.
+
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO tab_alter VALUES (1,1), (2,2), (3,3)");
Remove unnecessary whitespace.
======
First "Testcase"
5.
+# --------------------------------------------------
+# Testcase: Publisher replicates the column list data including generated
+# columns even though publish_generated_columns option is false.
+# --------------------------------------------------
+
That comment needs updating to reflect what this test case is *really*
doing. E.g., now you are testing tables without column lists as well
as tables with column lists.
~~~
6.
+# Create table and publications.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_gen_to_gen (a int, gen1 int GENERATED ALWAYS AS (a
* 2) STORED);
+ CREATE TABLE tab_gen_to_gen2 (a int, gen1 int GENERATED ALWAYS AS (a
* 2) STORED);
+ CREATE PUBLICATION pub1 FOR table tab_gen_to_gen,
tab_gen_to_gen2(gen1) WITH (publish_generated_columns=false);
+));
+
Calling these tables 'gen_to_gen' makes no sense. Replication to a
subscriber-side generated column does not work, and anyway, your
subscriber-side tables do not have generated columns in them. Please
be very careful with the table names -- misleading names cause a lot
of unnecessary confusion.
~~~
7.
+# Insert values into tables.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ INSERT INTO tab_gen_to_gen (a) VALUES (1), (1);
+ INSERT INTO tab_gen_to_gen2 (a) VALUES (1), (1);
+));
+
It seems unusual to insert initial values 1,1, and then later insert
values 2,3. Wouldn't values 1,2, and then 3,4 be better?
~~~
8.
+# Create table and subscription with copy_data=true.
Sometimes you say "copy_data-true" in the comments and other times you
do not. I could not make sense of why the differences -- I guess
perhaps this was supposed to be a global replacement but some got
missed by mistake (???)
~~~
9.
Remove multiple unnecessary whitespace, in various places in this test.
~~~
10.
+# Initial sync test when publish_generated_columns=false.
It would be better if this test comments (and others like this one)
would also say more about what is the result expected and why.
~~~
11.
+# Incremental replication test when publish_generated_columns=false.
+# Verify that column 'b' is not replicated.
+$node_publisher->wait_for_catchup('sub1');
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT * FROM tab_gen_to_gen ORDER BY a");
+is( $result, qq(1|
+1|
+2|
+3|),
+ 'tab_gen_to_gen incremental replication, when publish_generated_columns=false'
+);
There is not a column 'b' (I think now you called it "gen1") so the
comment is bogus. Please take care that comments are kept up-to-date
whenever you change the code.
~~~
12.
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT * FROM tab_gen_to_gen2 ORDER BY a");
+is( $result, qq(|2
+|2
+|4
+|6),
+ 'tab_gen_to_gen2 incremental replication, when
publish_generated_columns=false'
+);
+
Here you should have a comment to explain that the expected result was
generated column 'gen1' should be replicated because it was specified
in a column list, so that overrides the
publish_generated_columns=false.
======
Second "Testcase"
13.
All the above comments also apply to this second test case:
e.g. the "Testcase" comment needed updating.
e.g. table names like 'gen_to_gen' make no sense.
e.g. the initial data values 1,1 seem strange to me.
e.g. some comments have spurious "copy_data = true" and some do not.
e.g. unnecessary blank lines.
e.g. not enough comments describing what are the expected results and why.
e.g. multiple bogus mentions of a column 'b', when there is no such column name
======
FYI - The attached nitpicks just show some of the blank lines I
removed; nothing else.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index ff44c87..24134fa 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -239,7 +239,6 @@ $node_subscriber->safe_psql('postgres',
"ALTER TABLE tab_alter ALTER COLUMN b DROP EXPRESSION");
# Insert data to verify replication.
-
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_alter VALUES (1,1), (2,2), (3,3)");
@@ -297,7 +296,6 @@ $node_subscriber->safe_psql(
# Wait for initial sync.
$node_subscriber->wait_for_subscription_sync;
-
$node_publisher->wait_for_catchup('sub1');
# Initial sync test when publish_generated_columns=false.
@@ -306,7 +304,6 @@ $result = $node_subscriber->safe_psql('postgres',
is( $result, qq(1|
1|),
'tab_gen_to_gen initial sync, when publish_generated_columns=false');
-
$result = $node_subscriber->safe_psql('postgres',
"SELECT * FROM tab_gen_to_gen2 ORDER BY a");
is( $result, qq(|2
@@ -331,7 +328,6 @@ is( $result, qq(1|
3|),
'tab_gen_to_gen incremental replication, when
publish_generated_columns=false'
);
-
$result = $node_subscriber->safe_psql('postgres',
"SELECT * FROM tab_gen_to_gen2 ORDER BY a");
is( $result, qq(|2
@@ -376,7 +372,6 @@ $node_subscriber->safe_psql(
# Wait for initial sync.
$node_subscriber->wait_for_subscription_sync;
-
$node_publisher->wait_for_catchup('sub1');
# Initial sync test when publish_generated_columns=true.
@@ -385,7 +380,6 @@ $result = $node_subscriber->safe_psql('postgres',
is( $result, qq(1|2
1|2),
'tab_gen_to_gen3 initial sync, when publish_generated_columns=true');
-
$result = $node_subscriber->safe_psql('postgres',
"SELECT * FROM tab_gen_to_gen4 ORDER BY a");
is( $result, qq(|2
@@ -411,7 +405,6 @@ is( $result, qq(1|2
3|6),
'tab_gen_to_gen3 incremental replication, when
publish_generated_columns=true'
);
-
$result = $node_subscriber->safe_psql('postgres',
"SELECT * FROM tab_gen_to_gen4 ORDER BY a");
is( $result, qq(|2
