On Mon, Jul 1, 2024 at 8:38 PM Shubham Khanna
<[email protected]> wrote:
>
>...
> > 8.
> > + else if (strcmp(elem->defname, "include-generated-columns") == 0)
> > + {
> > + if (elem->arg == NULL)
> > + data->include_generated_columns = true;
> >
> > Is there any way to test that "elem->arg == NULL" in the
> > generated.sql? OTOH, if it is not possible to get here then is the
> > code even needed?
> >
>
> Currently I could not find a case where the
> 'include_generated_columns' option is not specifying any value, but I
> was hesitant to remove this from here as the other options mentioned
> follow the same rules. Thoughts?
>
If you do manage to find a scenario for this then I think a test for
it would be good. But, I agree that the code seems OK because now I
see it is the same pattern as similar nearby code.
~~~
Thanks for the updated patch. Here are some review comments for patch v13-0001.
======
.../expected/generated_columns.out
nitpicks (see generated_columns.sql)
======
.../test_decoding/sql/generated_columns.sql
nitpick - use plural /column/columns/
nitpick - use consistent wording in the comments
nitpick - IMO it is better to INSERT different values for each of the tests
======
doc/src/sgml/protocol.sgml
nitpick - I noticed that none of the other boolean parameters on this
page mention about a default, so maybe here we should do the same and
omit that information.
~~~
1.
- <para>
- Next, the following message part appears for each column included in
- the publication (except generated columns):
- </para>
-
In a previous review [1 comment #11] I wrote that you can't just
remove this paragraph because AFAIK it is still meaningful. A minimal
change might be to just remove the "(except generated columns)" part.
Alternatively, you could give a more detailed explanation mentioning
the include_generated_columns protocol parameter.
I provided some updated text for this paragraph in my NITPICKS top-up
patch, Please have a look at that for ideas.
======
src/backend/commands/subscriptioncmds.c
It looks like pg_indent needs to be run on this file.
======
src/include/catalog/pg_subscription.h
nitpick - comment /publish/Publish/ for consistency
======
src/include/replication/walreceiver.h
nitpick - comment /publish/Publish/ for consistency
======
src/test/regress/expected/subscription.out
nitpicks - (see subscription.sql)
======
src/test/regress/sql/subscription.sql
nitpick - combine the invalid option combinations test with all the
others (no special comment needed)
nitpick - rename subscription as 'regress_testsub2' same as all its peers.
======
src/test/subscription/t/011_generated.pl
nitpick - add/remove blank lines
======
src/test/subscription/t/031_column_list.pl
nitpick - rewording for a comment. This issue was not strictly caused
by this patch, but since you are modifying the same comment we can fix
this in passing.
======
99.
Please also see the attached top-up patch for all those nitpicks
identified above.
======
[1] v11-0001 review
https://www.postgresql.org/message-id/CAHut%2BPv45gB4cV%2BSSs6730Kb8urQyqjdZ9PBVgmpwqCycr1Ybg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/contrib/test_decoding/expected/generated_columns.out
b/contrib/test_decoding/expected/generated_columns.out
index 4c3d6dd..f3b26aa 100644
--- a/contrib/test_decoding/expected/generated_columns.out
+++ b/contrib/test_decoding/expected/generated_columns.out
@@ -1,4 +1,4 @@
--- test decoding of generated column
+-- test decoding of generated columns
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot',
'test_decoding');
?column?
----------
@@ -7,7 +7,7 @@ SELECT 'init' FROM
pg_create_logical_replication_slot('regression_slot', 'test_d
-- column b' is a generated column
CREATE TABLE gencoltable (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
--- when 'include-generated-columns' is not set the generated column 'b' will
be replicated
+-- when 'include-generated-columns' is not set the generated column 'b' values
will be replicated
INSERT INTO gencoltable (a) VALUES (1), (2), (3);
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');
data
@@ -20,26 +20,26 @@ SELECT data FROM
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
(5 rows)
-- when 'include-generated-columns' = '1' the generated column 'b' values will
be replicated
-INSERT INTO gencoltable (a) VALUES (1), (2), (3);
+INSERT INTO gencoltable (a) VALUES (4), (5), (6);
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '1');
- data
--------------------------------------------------------------
+ data
+--------------------------------------------------------------
BEGIN
- table public.gencoltable: INSERT: a[integer]:1 b[integer]:2
- table public.gencoltable: INSERT: a[integer]:2 b[integer]:4
- table public.gencoltable: INSERT: a[integer]:3 b[integer]:6
+ table public.gencoltable: INSERT: a[integer]:4 b[integer]:8
+ table public.gencoltable: INSERT: a[integer]:5 b[integer]:10
+ table public.gencoltable: INSERT: a[integer]:6 b[integer]:12
COMMIT
(5 rows)
-- when 'include-generated-columns' = '0' the generated column 'b' values will
not be replicated
-INSERT INTO gencoltable (a) VALUES (4), (5), (6);
+INSERT INTO gencoltable (a) VALUES (7), (8), (9);
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '0');
data
------------------------------------------------
BEGIN
- table public.gencoltable: INSERT: a[integer]:4
- table public.gencoltable: INSERT: a[integer]:5
- table public.gencoltable: INSERT: a[integer]:6
+ table public.gencoltable: INSERT: a[integer]:7
+ table public.gencoltable: INSERT: a[integer]:8
+ table public.gencoltable: INSERT: a[integer]:9
COMMIT
(5 rows)
diff --git a/contrib/test_decoding/sql/generated_columns.sql
b/contrib/test_decoding/sql/generated_columns.sql
index 9f02f6f..a5bb598 100644
--- a/contrib/test_decoding/sql/generated_columns.sql
+++ b/contrib/test_decoding/sql/generated_columns.sql
@@ -1,22 +1,22 @@
--- test decoding of generated column
+-- test decoding of generated columns
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot',
'test_decoding');
-- column b' is a generated column
CREATE TABLE gencoltable (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
--- when 'include-generated-columns' is not set the generated column 'b' will
be replicated
+-- when 'include-generated-columns' is not set the generated column 'b' values
will be replicated
INSERT INTO gencoltable (a) VALUES (1), (2), (3);
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');
-- when 'include-generated-columns' = '1' the generated column 'b' values will
be replicated
-INSERT INTO gencoltable (a) VALUES (1), (2), (3);
+INSERT INTO gencoltable (a) VALUES (4), (5), (6);
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '1');
-- when 'include-generated-columns' = '0' the generated column 'b' values will
not be replicated
-INSERT INTO gencoltable (a) VALUES (4), (5), (6);
+INSERT INTO gencoltable (a) VALUES (7), (8), (9);
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '0');
DROP TABLE gencoltable;
-SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
\ No newline at end of file
+SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 9cf5050..226c364 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -3313,7 +3313,6 @@ psql "dbname=postgres replication=database" -c
"IDENTIFY_SYSTEM;"
Boolean option to enable generated columns. This option controls
whether generated columns should be included in the string
representation of tuples during logical decoding in PostgreSQL.
- The default is false.
</para>
</listitem>
</varlistentry>
@@ -6535,6 +6534,13 @@ psql "dbname=postgres replication=database" -c
"IDENTIFY_SYSTEM;"
</varlistentry>
</variablelist>
+ <para>
+ Next, the following message parts appear for each column included in
+ the publication (generated columns are excluded unless the parameter
+ <link linkend="protocol-logical-replication-params">
+ <literal>include_generated_columns</literal></link> specifies otherwise):
+ </para>
+
<variablelist>
<varlistentry>
<term>Int8</term>
diff --git a/src/include/catalog/pg_subscription.h
b/src/include/catalog/pg_subscription.h
index ccff291..4663f7c 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -160,7 +160,7 @@ typedef struct Subscription
List *publications; /* List of publication names to
subscribe to */
char *origin; /* Only publish data
originating from the
* specified
origin */
- bool includegencols; /* publish generated column data */
+ bool includegencols; /* Publish generated column data */
} Subscription;
/* Disallow streaming in-progress transactions. */
diff --git a/src/include/replication/walreceiver.h
b/src/include/replication/walreceiver.h
index c761c4b..9275b3a 100644
--- a/src/include/replication/walreceiver.h
+++ b/src/include/replication/walreceiver.h
@@ -186,7 +186,7 @@ typedef struct
*
prepare time */
char *origin; /* Only publish data originating
from the
* specified
origin */
- bool include_generated_columns; /*
publish generated
+ bool include_generated_columns; /*
Publish generated
* columns */
} logical;
} proto;
diff --git a/src/test/regress/expected/subscription.out
b/src/test/regress/expected/subscription.out
index b78e3c6..e8824fa 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -99,11 +99,10 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION
'dbname=regress_doesnotexist' PU
ERROR: subscription with slot_name = NONE must also set create_slot = false
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
ERROR: subscription with slot_name = NONE must also set enabled = false
--- fail - copy_data and include_generated_columns are mutually exclusive
options
-CREATE SUBSCRIPTION sub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION
testpub WITH (include_generated_columns = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (include_generated_columns = true);
ERROR: copy_data = true and include_generated_columns = true are mutually
exclusive options
-- fail - include_generated_columns must be boolean
-CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
ERROR: include_generated_columns requires a Boolean value
-- ok - with slot_name = NONE
CREATE SUBSCRIPTION regress_testsub3 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, connect = false);
diff --git a/src/test/regress/sql/subscription.sql
b/src/test/regress/sql/subscription.sql
index dbf0644..8c63c13 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -59,12 +59,10 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION
'dbname=regress_doesnotexist' PU
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE);
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
-
--- fail - copy_data and include_generated_columns are mutually exclusive
options
-CREATE SUBSCRIPTION sub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION
testpub WITH (include_generated_columns = true);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (include_generated_columns = true);
-- fail - include_generated_columns must be boolean
-CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
+CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
-- ok - with slot_name = NONE
CREATE SUBSCRIPTION regress_testsub3 CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (slot_name = NONE, connect = false);
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index 48efb20..25edc6f 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -93,20 +93,20 @@ is( $result, qq(1|22|
3|66|
4|88|
6|132|), 'generated columns replicated');
+
#
# TEST tab2: the publisher-side col 'b' is generated, and the subscriber-side
# col 'b' is not generated, so confirm that col 'b' IS replicated.
#
-
$node_publisher->safe_psql('postgres', "INSERT INTO tab2 VALUES (4), (5)");
$node_publisher->wait_for_catchup('sub2');
-
$result =
$node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab2 ORDER BY a");
is( $result, qq(4|8
5|10),
'confirm generated columns ARE replicated when the subscriber-side
column is not generated'
);
+
#
# TEST tab3: the publisher-side col 'b' is generated, and the subscriber-side
# col 'b' is also generated, so confirmed that col 'b' IS NOT replicated. We
diff --git a/src/test/subscription/t/031_column_list.pl
b/src/test/subscription/t/031_column_list.pl
index 9804158..5bfed27 100644
--- a/src/test/subscription/t/031_column_list.pl
+++ b/src/test/subscription/t/031_column_list.pl
@@ -1204,7 +1204,7 @@ t), 'check the number of columns in the old tuple');
# TEST: Dropped columns are not considered for the column list.
# So, the publication having a column list except for those columns and a
-# publication without any column list (aka all columns as part of the columns
+# publication without any column list (aka all columns are part of the column
# list) are considered to have the same column list.
$node_publisher->safe_psql(
'postgres', qq(
