On Fri, 14 Jun 2024 at 15:52, Shubham Khanna
<khannashubham1...@gmail.com> wrote:
>
> > Thanks for the updated patch, few comments:
> > 1) The option name seems wrong here:
> > In one place include_generated_column is specified and other place
> > include_generated_columns is specified:
> >
> > + else if (IsSet(supported_opts,
> > SUBOPT_INCLUDE_GENERATED_COLUMN) &&
> > + strcmp(defel->defname,
> > "include_generated_column") == 0)
> > + {
> > + if (IsSet(opts->specified_opts,
> > SUBOPT_INCLUDE_GENERATED_COLUMN))
> > + errorConflictingDefElem(defel, pstate);
> > +
> > + opts->specified_opts |=
> > SUBOPT_INCLUDE_GENERATED_COLUMN;
> > + opts->include_generated_column =
> > defGetBoolean(defel);
> > + }
>
> Fixed.
>
> > diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> > index d453e224d9..e8ff752fd9 100644
> > --- a/src/bin/psql/tab-complete.c
> > +++ b/src/bin/psql/tab-complete.c
> > @@ -3365,7 +3365,7 @@ psql_completion(const char *text, int start, int end)
> > COMPLETE_WITH("binary", "connect", "copy_data",
> > "create_slot",
> > "disable_on_error",
> > "enabled", "failover", "origin",
> > "password_required",
> > "run_as_owner", "slot_name",
> > - "streaming",
> > "synchronous_commit", "two_phase");
> > + "streaming",
> > "synchronous_commit", "two_phase","include_generated_columns");
> >
> > 2) This small data table need not have a primary key column as it will
> > create an index and insertion will happen in the index too.
> > +-- check include-generated-columns option with generated column
> > +CREATE TABLE gencoltable (a int PRIMARY KEY, b int GENERATED ALWAYS
> > AS (a * 2) STORED);
> > +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',
> > 'include-generated-columns', '1');
>
> Fixed.
>
> > 3) Please add a test case for this:
> > + set to <literal>false</literal>. If the subscriber-side
> > column is also a
> > + generated column then this option has no effect; the
> > replicated data will
> > + be ignored and the subscriber column will be filled as
> > normal with the
> > + subscriber-side computed or default data.
>
> Added the required test case.
>
> > 4) You can use a new style of ereport to remove the brackets around errcode
> > 4.a)
> > + else if (!parse_bool(strVal(elem->arg),
> > &data->include_generated_columns))
> > + ereport(ERROR,
> > +
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("could not
> > parse value \"%s\" for parameter \"%s\"",
> > +
> > strVal(elem->arg), elem->defname)));
> >
> > 4.b) similarly here too:
> > + ereport(ERROR,
> > + (errcode(ERRCODE_SYNTAX_ERROR),
> > + /*- translator: both %s are strings of the form
> > "option = value" */
> > + errmsg("%s and %s are mutually
> > exclusive options",
> > + "copy_data = true",
> > "include_generated_column = true")));
> >
> > 4.c) similarly here too:
> > + if (include_generated_columns_option_given)
> > + ereport(ERROR,
> > +
> > (errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("conflicting
> > or redundant options")));
>
> Fixed.
>
> > 5) These variable names can be changed to keep it smaller, something
> > like gencol or generatedcol or gencolumn, etc
> > +++ b/src/include/catalog/pg_subscription.h
> > @@ -98,6 +98,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
> > BKI_SHARED_RELATION BKI_ROW
> > * slots) in the upstream database are enabled
> > * to be synchronized to the standbys. */
> >
> > + bool subincludegeneratedcolumn; /* True if generated columns must be
> > published */
> > +
> > #ifdef CATALOG_VARLEN /* variable-length fields start here */
> > /* Connection string to the publisher */
> > text subconninfo BKI_FORCE_NOT_NULL;
> > @@ -157,6 +159,7 @@ typedef struct Subscription
> > List *publications; /* List of publication names to subscribe to */
> > char *origin; /* Only publish data originating from the
> > * specified origin */
> > + bool includegeneratedcolumn; /* publish generated column data */
> > } Subscription;
>
> Fixed.
>
> The attached Patch contains the suggested changes.
Few comments:
1) Here tab1 and tab2 are exactly the same tables, just check if the
table tab1 itself can be used for your tests.
@@ -24,20 +24,50 @@ $node_publisher->safe_psql('postgres',
"CREATE TABLE tab1 (a int PRIMARY KEY, b int GENERATED ALWAYS
AS (a * 2) STORED)"
);
+$node_publisher->safe_psql('postgres',
+ "CREATE TABLE tab2 (a int PRIMARY KEY, b int GENERATED ALWAYS
AS (a * 2) STORED)"
+);
2) We can document that the include_generate_columns option cannot be altered.
3) You can mention that include-generated-columns is true by default
and generated column data will be selected
+-- When 'include-generated-columns' is not set
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+ 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
+ COMMIT
+(5 rows)
4) The comment seems to be wrong here, the comment says b will not be
replicated but b is being selected:
-- When 'include-generated-columns' = '1' the generated column 'b'
values will not 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',
'include-generated-columns', '1');
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
COMMIT
(5 rows)
5) Similarly here too the comment seems to be wrong, the comment says
b will not replicated but b is not being selected:
INSERT INTO gencoltable (a) VALUES (4), (5), (6);
-- When 'include-generated-columns' = '0' the generated column 'b'
values will be replicated
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
COMMIT
(5 rows)
6) SUBOPT_include_generated_columns change it to SUBOPT_GENERATED to
keep the name consistent:
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -72,6 +72,7 @@
#define SUBOPT_FAILOVER 0x00002000
#define SUBOPT_LSN 0x00004000
#define SUBOPT_ORIGIN 0x00008000
+#define SUBOPT_include_generated_columns 0x00010000
7) The comment style seems to be inconsistent, both of them can start
in lower case
+-- check include-generated-columns option with generated column
+CREATE TABLE gencoltable (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
+INSERT INTO gencoltable (a) VALUES (1), (2), (3);
+-- When 'include-generated-columns' is not set
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+ 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
+ COMMIT
+(5 rows)
+
+-- When 'include-generated-columns' = '1' the generated column 'b'
values will not be replicated
8) This could be changed to remove the insert statements by using
pg_logical_slot_peek_changes:
-- When 'include-generated-columns' is not set
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 not 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',
'include-generated-columns', '1');
INSERT INTO gencoltable (a) VALUES (4), (5), (6);
-- When 'include-generated-columns' = '0' the generated column 'b'
values will be replicated
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
'include-generated-columns', '0');
to:
-- When 'include-generated-columns' is not set
SELECT data FROM pg_logical_slot_peek_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
-- When 'include-generated-columns' = '1' the generated column 'b'
values will not be replicated
SELECT data FROM pg_logical_slot_peek_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 be replicated
SELECT data FROM pg_logical_slot_peek_changes('regression_slot', NULL,
NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
'include-generated-columns', '0');
9) In commit message the option used is wrong
include_generated_columns should actually be
include-generated-columns:
Usage from test_decoding plugin:
SELECT data FROM pg_logical_slot_get_changes('slot2', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1',
'include_generated_columns','1');
Regards,
Vignesh
