Re: Pgoutput not capturing the generated columns

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