Hi Shubhab.

Here are some more review comments for the v23-0001.

======
011_generated.pl b/src/test/subscription/t/011_generated.pl

nitpick - renamed /regress_pub/regress_pub_tab1/ and
/regress_sub1/regress_sub1_tab1/
nitpick - typo /inital data /initial data/
nitpick - typo /snode_subscriber2/node_subscriber2/
nitpick - tweak the combo initial sync comments and messages
nitpick - /#Cleanup/# cleanup/
nitpick - tweak all the combo normal replication comments
nitpick - removed blank line at the end

~~~

1. Refactor tab_gen_to_missing initial sync tests.

I moved the tab_gen_to_missing initial sync for node_subscriber2 to be
back where all the other initial sync tests are done.
See the nitpicks patch file.

~~~

2. Refactor tab_nogen_to_gen initial sync tests

I moved all the tab_nogen_to_gen initial sync tests back to where the
other initial sync tests are done.
See the nitpicks patch file.

~~~

3. Added another test case:

Because the (current PG17) nogen-to-gen initial sync test case (with
copy_data=true) gives an ERROR, I have added another combination to
cover normal replication (e.g. using copy_data=false).
See the nitpicks patch file.

(This has exposed an inconsistency which IMO might be a PG17 bug. I
have included TAP test comments about this, and plan to post a
separate thread for it later).

~

4. GUC

Moving and adding more CREATE SUBSCRIPTION exceeded some default GUCs,
so extra configuration was needed.
See the nitpick patch file.

======
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 0b596b7..2be06c6 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -12,16 +12,25 @@ use Test::More;
 
 my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
 $node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+       "max_wal_senders = 20
+        max_replication_slots = 20");
 $node_publisher->start;
 
 # All subscribers on this node will use parameter include_generated_columns = 
false
 my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
 $node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+       "max_logical_replication_workers = 20
+        max_worker_processes = 20");
 $node_subscriber->start;
 
 # All subscribers on this node will use parameter include_generated_columns = 
true
 my $node_subscriber2 = PostgreSQL::Test::Cluster->new('subscriber2');
 $node_subscriber2->init;
+$node_subscriber2->append_conf('postgresql.conf',
+       "max_logical_replication_workers = 20
+        max_worker_processes = 20");
 $node_subscriber2->start;
 
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -139,7 +148,7 @@ $node_publisher->safe_psql('postgres',
 # pub_combo_gen_to_missing is not included in pub_combo, because some tests 
give errors.
 
 $node_publisher->safe_psql('postgres',
-       "CREATE PUBLICATION regress_pub FOR TABLE tab1");
+       "CREATE PUBLICATION regress_pub_tab1 FOR TABLE tab1");
 $node_publisher->safe_psql('postgres',
        "CREATE PUBLICATION regress_pub_combo FOR TABLE tab_gen_to_gen, 
tab_gen_to_nogen, tab_missing_to_gen"
 );
@@ -157,10 +166,10 @@ $node_publisher->safe_psql('postgres',
 #
 # Note that all subscriptions created on node_subscriber2 use copy_data = 
false,
 # because copy_data = true with include_generated_columns is not yet supported.
-# For this reason, the expected inital data on snode_subscriber2 is always 
empty.
+# For this reason, the expected inital data on node_subscriber2 is always 
empty.
 
 $node_subscriber->safe_psql('postgres',
-       "CREATE SUBSCRIPTION regress_sub1 CONNECTION '$publisher_connstr' 
PUBLICATION regress_pub"
+       "CREATE SUBSCRIPTION regress_sub1_tab1 CONNECTION '$publisher_connstr' 
PUBLICATION regress_pub_tab1"
 );
 $node_subscriber->safe_psql('postgres',
        "CREATE SUBSCRIPTION regress_sub1_combo CONNECTION '$publisher_connstr' 
PUBLICATION regress_pub_combo"
@@ -168,11 +177,18 @@ $node_subscriber->safe_psql('postgres',
 $node_subscriber->safe_psql('postgres',
        "CREATE SUBSCRIPTION regress_sub1_combo_gen_to_missing CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_gen_to_missing"
 );
+# Note, regress_sub1_combo_nogen_to_gen is not created here due to expected 
errors. See later.
 
 $node_subscriber2->safe_psql('postgres',
        "CREATE SUBSCRIPTION regress_sub2_combo CONNECTION '$publisher_connstr' 
PUBLICATION regress_pub_combo WITH (include_generated_columns = true, copy_data 
= false)"
 );
 $node_subscriber2->safe_psql('postgres',
+       "CREATE SUBSCRIPTION regress_sub2_combo_gen_to_missing CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_gen_to_missing with 
(include_generated_columns = true, copy_data = false)"
+);
+$node_subscriber2->safe_psql('postgres',
+       "CREATE SUBSCRIPTION regress_sub2_combo_nogen_to_gen CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_nogen_to_gen WITH 
(include_generated_columns = true, copy_data = false)"
+);
+$node_subscriber2->safe_psql('postgres',
        "CREATE SUBSCRIPTION regress_sub2_misc CONNECTION '$publisher_connstr' 
PUBLICATION regress_pub_misc WITH (include_generated_columns = true, copy_data 
= false)"
 );
 
@@ -188,57 +204,82 @@ is( $result, qq(1|22
 2|44
 3|66), 'generated columns initial sync');
 
-# gen-to-gen
+#####################
+# TEST tab_gen_to_gen initial sync
+#####################
 $result =
   $node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab_gen_to_gen");
 is( $result, qq(1|22
 2|44
-3|66), 'generated columns initial sync, when include_generated_columns=false'
+3|66), 'tab_gen_to_gen initial sync, when include_generated_columns=false'
 );
 $result =
   $node_subscriber2->safe_psql('postgres', "SELECT a, b FROM tab_gen_to_gen");
 is($result, qq(),
-       'generated columns initial sync, when include_generated_columns=true');
+       'tab_gen_to_gen initial sync, when include_generated_columns=true');
 
-# gen-to-nogen
+#####################
+# TEST tab_gen_to_nogen initial sync
+#####################
 $result = $node_subscriber->safe_psql('postgres',
        "SELECT a, b FROM tab_gen_to_nogen");
 is( $result, qq(1|
 2|
-3|), 'generated columns initial sync, when include_generated_columns=false');
+3|), 'tab_gen_to_nogen, when include_generated_columns=false');
 $result = $node_subscriber2->safe_psql('postgres',
        "SELECT a, b FROM tab_gen_to_nogen");
 is($result, qq(),
-       'generated columns initial sync, when include_generated_columns=true');
+       'tab_gen_to_nogen initial sync, when include_generated_columns=true');
 
-# gen-to-missing
-# Note, node_subscriber2 is not subscribing to this yet. See later.
+#####################
+# TEST tab_gen_to_missing initial sync
+#####################
 $result =
   $node_subscriber->safe_psql('postgres', "SELECT a FROM tab_gen_to_missing");
 is( $result, qq(1
 2
-3), 'generated columns initial sync, when include_generated_columns=false');
+3), 'tab_gen_to_missing initial sync, when include_generated_columns=false');
+# Note, the following is expected to work only because copy_data = false
+$result =
+  $node_subscriber2->safe_psql('postgres', "SELECT a FROM tab_gen_to_missing");
+is( $result, qq(), 'tab_gen_to_missing initial sync, when 
include_generated_columns=true');
 
-# missing-to-gen
+#####################
+# TEST tab_missing_to_gen initial sync
+#####################
 $result = $node_subscriber->safe_psql('postgres',
        "SELECT a, b FROM tab_missing_to_gen");
 is( $result, qq(1|22
 2|44
-3|66), 'generated columns initial sync, when include_generated_columns=false'
+3|66), 'tab_missing_to_gen initial sync, when include_generated_columns=false'
 );
 $result = $node_subscriber2->safe_psql('postgres',
        "SELECT a, b FROM tab_missing_to_gen");
 is($result, qq(),
-       'generated columns initial sync, when include_generated_columns=true');
+       'tab_missing_to_gen initial sync, when include_generated_columns=true');
 
-# nogen-to-gen
-# Note, node_subscriber is not subscribing to this yet. See later
-# Note, node_subscriber2 is not subscribing to this yet. See later
+#####################
+# TEST tab_nogen_to_gen initial sync
+#####################
+# The subscription is created here, because it causes the tablesync worker to 
restart repetitively.
+my $offset = -s $node_subscriber->logfile;
+$node_subscriber->safe_psql('postgres',
+       "CREATE SUBSCRIPTION regress_sub1_combo_nogen_to_gen CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_nogen_to_gen WITH 
(include_generated_columns = false)"
+);
+$node_subscriber->wait_for_log(
+       qr/ERROR: ( [A-Z0-9]:)? column "b" is a generated column/, $offset);
+# Note, the following is expected to work only because copy_data = false
+$result = $node_subscriber2->safe_psql('postgres',
+       "SELECT a, b FROM tab_nogen_to_gen");
+is($result, qq(),
+       'tab_nogen_to_gen initial sync, when include_generated_columns=true');
 
+# tab_order:
 $result = $node_subscriber2->safe_psql('postgres',
        "SELECT a, b, c FROM tab_order ORDER BY a");
 is($result, qq(), 'generated column initial sync');
 
+# tab_alter:
 $result = $node_subscriber2->safe_psql('postgres',
        "SELECT a, b, c FROM tab_alter ORDER BY a");
 is($result, qq(), 'unsubscribed table initial data');
@@ -249,7 +290,7 @@ $node_publisher->safe_psql('postgres', "INSERT INTO tab1 
VALUES (4), (5)");
 
 $node_publisher->safe_psql('postgres', "UPDATE tab1 SET a = 6 WHERE a = 5");
 
-$node_publisher->wait_for_catchup('regress_sub1');
+$node_publisher->wait_for_catchup('regress_sub1_tab1');
 
 $result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab1");
 is( $result, qq(1|22|
@@ -259,7 +300,7 @@ is( $result, qq(1|22|
 6|132|), 'generated columns replicated');
 
 #####################
-# TEST tab_gen_to_gen
+# TEST tab_gen_to_gen replication
 #
 # publisher-side has generated col 'b'.
 # subscriber-side has generated col 'b', using a different computation.
@@ -298,7 +339,7 @@ is( $result, qq(4|88
 );
 
 #####################
-# TEST tab_gen_to_nogen
+# TEST tab_gen_to_nogen replication
 #
 # publisher-side has generated col 'b'.
 # subscriber-side has non-generated col 'b'.
@@ -334,7 +375,7 @@ is( $result, qq(4|8
 );
 
 #####################
-# TEST tab_gen_to_missing
+# TEST tab_gen_to_missing replication
 #
 # publisher-side has generated col 'b'.
 # subscriber-side col 'b' is missing.
@@ -360,21 +401,11 @@ is( $result, qq(1
 # regress_sub2_combo_gen_to_missing: (include_generated_columns = true)
 # Confirm that col 'b' is not replicated and it will throw an error.
 my $offset2 = -s $node_subscriber2->logfile;
-
-# The subscription is created here, because it causes the tablesync worker to 
restart repetitively.
-$node_subscriber2->safe_psql('postgres',
-       "CREATE SUBSCRIPTION regress_sub2_combo_gen_to_missing CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_gen_to_missing with 
(include_generated_columns = true, copy_data = false)"
-);
-
-# insert data
-$node_publisher->safe_psql('postgres',
-       "INSERT INTO tab_gen_to_missing VALUES (6)");
-
 $node_subscriber2->wait_for_log(
        qr/ERROR: ( [A-Z0-9]+:)? logical replication target relation 
"public.tab_gen_to_missing" is missing replicated column: "b"/,
        $offset2);
 
-#Cleanup
+# cleanup
 $node_subscriber->safe_psql('postgres',
        "DROP SUBSCRIPTION regress_sub1_combo_gen_to_missing");
 $node_subscriber2->safe_psql('postgres',
@@ -383,7 +414,7 @@ $node_publisher->safe_psql('postgres',
        "DROP PUBLICATION regress_pub_combo_gen_to_missing");
 
 #####################
-# TEST tab_missing_to_gen
+# TEST tab_missing_to_gen replication
 #
 # publisher-side col 'b' is missing.
 # subscriber-side col 'b' is generated.
@@ -426,57 +457,62 @@ $node_subscriber2->safe_psql('postgres',
 $node_publisher->safe_psql('postgres', "DROP PUBLICATION regress_pub_combo");
 
 #####################
-# TEST tab_nogen_to_gen
+# TEST tab_nogen_to_gen replication
 #
 # publisher-side has non-generated col 'b'.
 # subscriber-side has generated col 'b'.
 #####################
 
+# When copy_data=true a COPY error occurred. Try again but with 
copy_data=false.
+$node_subscriber->safe_psql('postgres',
+       "CREATE SUBSCRIPTION regress_sub1_combo_nogen_to_gen_nocopy CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_nogen_to_gen WITH 
(include_generated_columns = false, copy_data = false)"
+);
+
 # insert data
 $node_publisher->safe_psql('postgres',
        "INSERT INTO tab_nogen_to_gen VALUES (4), (5)");
 
 # regress_sub1_combo_nogen_to_gen: (include_generated_columns = false)
-# Confirm that col 'b' is not replicated and it will throw a COPY error.
 #
-# The subscription is created here, because it causes the tablesync worker to 
restart repetitively.
-my $offset = -s $node_subscriber->logfile;
-$node_subscriber->safe_psql('postgres',
-       "CREATE SUBSCRIPTION regress_sub1_combo_nogen_to_gen CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_nogen_to_gen WITH 
(include_generated_columns = false)"
+# XXX
+# The test below shows that current PG17 behavior does not give an error,
+# But this conflicts with the copy_data=true behavior so it might be a PG17 
bug.
+# Needs more study.
+$node_publisher->wait_for_catchup('regress_sub1_combo_nogen_to_gen_nocopy');
+$result =
+  $node_subscriber->safe_psql('postgres',
+       "SELECT a, b FROM tab_nogen_to_gen ORDER BY a");
+is($result, qq(4|88
+5|110),
+       'confirm when publisher col is not generated, subscriber generated 
columns are generated as normal'
 );
-$node_subscriber->wait_for_log(
-       qr/ERROR: ( [A-Z0-9]:)? column "b" is a generated column/, $offset);
 
 # regress_sub2_combo_nogen_to_gen: (include_generated_columns = true)
+# When copy_data=false, no COPY error occurs.
+# The col 'b' is not replicated; the subscriber-side generated value is 
inserted.
 #
 # XXX
-# when copy_data=false, no COPY error occurs.
-# the col 'b' is not replicated; the subscriber-side generated value is 
inserted.
-$node_subscriber2->safe_psql('postgres',
-       "CREATE SUBSCRIPTION regress_sub2_combo_nogen_to_gen CONNECTION 
'$publisher_connstr' PUBLICATION regress_pub_combo_nogen_to_gen WITH 
(include_generated_columns = true, copy_data = false)"
-);
-
-# insert data
-$node_publisher->safe_psql('postgres',
-       "INSERT INTO tab_nogen_to_gen VALUES (6)");
-
+# It is correct for this to give the same result as above, but it needs more
+# study to determine if the above result was actually correct, or a PG17 bug.
 $node_publisher->wait_for_catchup('regress_sub2_combo_nogen_to_gen');
 $result =
   $node_subscriber2->safe_psql('postgres',
        "SELECT a, b FROM tab_nogen_to_gen ORDER BY a");
-is($result, qq(6|132),
+is($result, qq(4|88
+5|110),
        'confirm when publisher col is not generated, subscriber generated 
columns are generated as normal'
 );
 
 # cleanup
-
+$node_subscriber->safe_psql('postgres',
+       "DROP SUBSCRIPTION regress_sub1_combo_nogen_to_gen_nocopy");
 $node_subscriber2->safe_psql('postgres',
        "DROP SUBSCRIPTION regress_sub2_combo_nogen_to_gen");
 $node_publisher->safe_psql('postgres',
        "DROP PUBLICATION regress_pub_combo_nogen_to_gen");
 
 #####################
-# TEST tab_order:
+# TEST tab_order replication
 #
 # publisher-side cols 'b' and 'c' are generated
 # subscriber-side col 'b' is not generated and col 'c' is generated.
@@ -498,7 +534,7 @@ is( $result, qq(4|8|88
        'replicate generated columns with different order on the subscriber');
 
 #####################
-# TEST tab_alter
+# TEST tab_alter replication
 #
 # Add a new table to existing publication, then
 # do ALTER SUBSCRIPTION ... REFRESH PUBLICATION
@@ -516,7 +552,7 @@ is( $result, qq(1||22
 3||66), 'add new table to existing publication');
 
 #####################
-# TEST tabl_alter
+# TEST tab_alter
 #
 # Drop the generated column's expression on subscriber side.
 # This changes the generated column into a non-generated column.
@@ -547,7 +583,6 @@ $node_publisher->safe_psql('postgres', "DROP PUBLICATION 
regress_pub_misc");
 #####################
 # try it with a subscriber-side trigger
 
-
 $node_subscriber->safe_psql(
        'postgres', q{
 CREATE FUNCTION tab1_trigger_func() RETURNS trigger
@@ -568,7 +603,7 @@ $node_publisher->safe_psql('postgres', "INSERT INTO tab1 
VALUES (7), (8)");
 
 $node_publisher->safe_psql('postgres', "UPDATE tab1 SET a = 9 WHERE a = 7");
 
-$node_publisher->wait_for_catchup('regress_sub1');
+$node_publisher->wait_for_catchup('regress_sub1_tab1');
 
 $result =
   $node_subscriber->safe_psql('postgres', "SELECT * FROM tab1 ORDER BY 1");

Reply via email to