Re: Pgoutput not capturing the generated columns

Hi Shubham,

Here are my general comments about the v30-0002 TAP test patch.

======

1.
As mentioned in a previous post [1] there are still several references
to the old 'include_generated_columns' option remaining in this patch.
They need replacing.

~~~

2.
+# Furthermore, all combinations are tested for publish_generated_columns=false
+# (see subscription sub1 of database 'postgres'), and
+# publish_generated_columns=true (see subscription sub2 of database
+# 'test_igc_true').

Those 'see subscription' notes and 'test_igc_true' are from the old
implementation. Those need fixing. BTW, 'test_pgc_true' is a better
name for the database now that the option name is changed.

In the previous implementation, the TAP test environment was:
- a common publication pub, on the 'postgres' database
- a subscription sub1 with option include_generated_columns=false, on
the 'postgres' database
- a subscription sub2 with option include_generated_columns=true, on
the 'test_igc_true' database

Now it is like:
- a publication pub1, on the 'postgres' database, with option
publish_generated_columns=false
- a publication pub2, on the 'postgres' database, with option
publish_generated_columns=true
- a subscription sub1, on the 'postgres' database for publication pub1
- a subscription sub2, on the 'test_pgc_true' database for publication pub2

It would be good to document that above convention because knowing how
the naming/numbering works makes it a lot easier to read the
subsequent test cases. Of course, it is really important to
name/number everything consistently otherwise these tests become hard
to follow. AFAICT it is mostly OK, but the generated -> generated
publication should be called 'regress_pub2_gen_to_gen'

~~~

3.
+# Create table.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_gen_to_nogen (a int, b int GENERATED ALWAYS AS (a *
2) STORED);
+ INSERT INTO tab_gen_to_nogen (a) VALUES (1), (2), (3);
+));
+
+# Create publication with publish_generated_columns=false.
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION regress_pub1_gen_to_nogen FOR TABLE
tab_gen_to_nogen WITH (publish_generated_columns = false)"
+);
+
+# Create table and subscription with copy_data=true.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_gen_to_nogen (a int, b int);
+ CREATE SUBSCRIPTION regress_sub1_gen_to_nogen CONNECTION
'$publisher_connstr' PUBLICATION regress_pub1_gen_to_nogen WITH
(copy_data = true);
+));
+
+# Create publication with publish_generated_columns=true.
+$node_publisher->safe_psql('postgres',
+ "CREATE PUBLICATION regress_pub2_gen_to_nogen FOR TABLE
tab_gen_to_nogen WITH (publish_generated_columns = true)"
+);
+

The code can be restructured to be simpler. Both publications are
always created on the 'postgres' database at the publisher node, so
let's just create them at the same time as the creating the publisher
table. It also makes readability much better e.g.

# Create table, and publications
$node_publisher->safe_psql(
'postgres', qq(
CREATE TABLE tab_gen_to_nogen (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
INSERT INTO tab_gen_to_nogen (a) VALUES (1), (2), (3);
CREATE PUBLICATION regress_pub1_gen_to_nogen FOR TABLE
tab_gen_to_nogen WITH (publish_generated_columns = false);
CREATE PUBLICATION regress_pub2_gen_to_nogen FOR TABLE
tab_gen_to_nogen WITH (publish_generated_columns = true);
));

IFAICT this same simplification can be repeated multiple times in this TAP file.

~~

Similarly, it would be neater to combine DROP PUBLICATION's together too.

~~~

4.
Hopefully, the generated column 'copy_data' can be implemented again
soon for subscriptions, and then the initial sync tests here can be
properly implemented instead of the placeholders currently in patch
0002.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPuDJToG%3DV-ogTi9_6fnhhn2S0%2BsVRGPynhcf9mEh0Q%3DLA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia