On Thu, Oct 31, 2024 at 3:16 AM vignesh C <vignes...@gmail.com> wrote:
>
> On Wed, 30 Oct 2024 at 15:06, Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Tue, Oct 29, 2024 at 8:50 PM vignesh C <vignes...@gmail.com> wrote:
> > >
> > > Thank you for reporting this issue. The attached v46 patch addresses
> > > the problem and includes some adjustments to the comments. Thanks to
> > > Amit for sharing the comment changes offline.
> > >
> >
> > Pushed. Kindly rebase and send the remaining patches.
>
> Thanks for committing this patch, here is a rebased version of the
> remaining patches.
>
Here are some review comments for the patch v1-0003 (tap tests)
======
src/test/subscription/t/011_generated.pl
1.
+# The following combinations are tested:
+# - Publication pub1 on the 'postgres' database with the option
+# publish_generated_columns set to false.
+# - Publication pub2 on the 'postgres' database with the option
+# publish_generated_columns set to true.
+# - Subscription sub1 on the 'postgres' database for publication pub1.
+# - Subscription sub2 on the 'test_pgc_true' database for publication pub2.
Those aren't really "combinations" anymore. That's just describing how
these pub/sub tests are configured.
/The following combinations are tested:/The test environment is set up
as follows:/
~~~
2.
+# Wait for the initial synchronization of the 'regress_sub1_gen_to_nogen'
+# subscription in the 'postgres' database.
+$node_subscriber->wait_for_subscription_sync($node_publisher,
+ 'regress_sub1_gen_to_nogen', 'postgres');
+
+# Wait for the initial synchronization of the 'regress_sub2_gen_to_nogen'
+# subscription in the 'test_pgc_true' database.
+$node_subscriber->wait_for_subscription_sync($node_publisher,
+ 'regress_sub2_gen_to_nogen', 'test_pgc_true');
+
These detailed descriptions are not adding much value here. Just
combining these and saying "Wait for the initial synchronization of
both subscriptions" would have been enough, I think.
~~~
3.
+# =============================================================================
+# The following test cases demonstrate the behavior of generated column
+# replication with publish_generated_columns set to false and true:
+# Test: Publication column list includes generated columns when
+# publish_generated_columns is set to false.
+# Test: Publication column list excludes generated columns when
+# publish_generated_columns is set to false.
+# Test: Publication column list includes generated columns when
+# publish_generated_columns is set to true.
+# Test: Publication column list excludes generated columns when
+# publish_generated_columns is set to true.
+# =============================================================================
Some extra spacing and minor rewording would make this unreadable
comment readable. e.g.
# =============================================================================
# The following test cases demonstrate the behavior of generated column
# replication with publish_generated_columns set to false and true:
#
# When publish_generated_columns is set to false...
# Test: Publication column list includes generated columns
# Test: Publication column list excludes generated columns
#
# When publish_generated_columns is set to true...
# Test: Publication column list includes generated columns
# Test: Publication column list excludes generated columns
# =============================================================================
====
1st test:
4.
+# Create table and publications.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab2 (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
+ CREATE TABLE tab3 (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
+ CREATE PUBLICATION pub1 FOR table tab2, tab3(gen1) WITH
(publish_generated_columns=false);
+));
+
4a.
/Create table/Create tables/
~
4b.
TBH, I am not sure why you are including the table 'tab2' like this,
because the test case for replication without any column list at all
was already tested in your earlier tests. AFAICT 'tab2' should also
have a column list, but one that *exlcudes* the gencols. After all,
that's what the main comment said you were going to test.
~~~
5.
+# Create table and subscription.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab2 (a int, gen1 int);
+ CREATE TABLE tab3 (a int, gen1 int);
+ CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION
pub1 WITH (copy_data = true);
+));
/Create table/Create tables/
~~~
6.
+# Verify that the initial synchronization of generated columns is not
replicated
+# when they are not included in the column list, regardless of the
+# publish_generated_columns option.
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2 ORDER BY a");
+is( $result, qq(1|
+2|),
+ 'tab2 initial sync, when publish_generated_columns=false');
+
This comment doesn't make much sense to me. E.g.
a) IIUC tab2 should have used a column list that "excludes generated
columns". After all, that's what the main comment said you were going
to test.
b) the option is already false, so saying "... regardless of the
publish_generated_columns option" doesn't really mean anything
~~~
7.
+# Verify that incremental replication of generated columns does not occur
+# when they are not included in the column list, regardless of the
+# publish_generated_columns option.
+$node_publisher->wait_for_catchup('sub1');
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2 ORDER BY a");
+is( $result, qq(1|
+2|
+3|
+4|),
+ 'tab2 incremental replication, when publish_generated_columns=false');
+
This comment has the same issues described in the above review comment #6.
====
2nd test:
8.
+# --------------------------------------------------
+# Test Case: Even when publish_generated_columns is set to true, the publisher
+# only publishes the data of columns specified in the column list,
+# skipping other generated and non-generated columns.
+# --------------------------------------------------
+
This 2nd test has lots of the same problems as the first test.
For example:
- /# Create table and publications./# Create tables and publications./
- IMO 'tab4' should also have a publications column list, but one that
does not include gencols.
- /# Create table and subscription./# Create tables and subscription./
~~~
9.
+# Initial sync test when publish_generated_columns=true.
Why did you use a simple comment here, but a much more complicated
comment for the same scenario in the 1st test?
~~~
10.
+# Incremental replication test when publish_generated_columns=true.
+# Verify that column 'gen1' is replicated.
+$node_publisher->wait_for_catchup('sub1');
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT * FROM tab4 ORDER BY a");
+is( $result, qq(1|2
+2|4
+3|6
+4|8),
+ 'tab4 incremental replication, when publish_generated_columns=true');
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT * FROM tab5 ORDER BY a");
+is( $result, qq(|2
+|4
+|6
+|8),
+ 'tab5 incremental replication, when publish_generated_columns=true');
+
AFAICT the table 'tab4' also should have a column list, but one that
excludes the gencol. After all, that's what the main comment said you
were going to test. So this test comment and the tab4 test part is
currently broken.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
