Subject: Re: Pgoutput not capturing the generated columns

From: Shlok Kyal <[email-masked]>
Date: Mon, 17 Jun 2024 11:50:33 +0530

On Fri, 14 Jun 2024 at 15:52, Shubham Khanna
<[email-masked]> wrote:
>
> The attached Patch contains the suggested changes.
>

Hi Shubham, thanks for providing a patch.
I have some comments for v6-0001.

1. create_subscription.sgml
There is repetition of the same line.

+         <para>
+          Specifies whether the generated columns present in the tables
+          associated with the subscription should be replicated. 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.
+          <literal>false</literal>.
+         </para>
+
+         <para>
+          This parameter can only be set true if
<literal>copy_data</literal> is
+          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.
+         </para>

==============================
2. subscriptioncmds.c

2a. The macro name should be in uppercase. We can use a short name
like 'SUBOPT_INCLUDE_GEN_COL'. Thought?
+#define SUBOPT_include_generated_columns 0x00010000

2b.Update macro name accordingly
+ if (IsSet(supported_opts, SUBOPT_include_generated_columns))
+ opts->include_generated_columns = false;

2c. Update macro name accordingly
+ else if (IsSet(supported_opts, SUBOPT_include_generated_columns) &&
+ strcmp(defel->defname, "include_generated_columns") == 0)
+ {
+ if (IsSet(opts->specified_opts, SUBOPT_include_generated_columns))
+ errorConflictingDefElem(defel, pstate);
+
+ opts->specified_opts |= SUBOPT_include_generated_columns;
+ opts->include_generated_columns = defGetBoolean(defel);
+ }

2d. Update macro name accordingly
+   SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER | SUBOPT_ORIGIN |
+   SUBOPT_include_generated_columns);

==============================

3. decoding_into_rel.out

3a. In comment, I think it should be "When 'include-generated-columns'
= '1' the generated column 'b' values will be replicated"
+-- 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

3b. In comment, I think it should be "When 'include-generated-columns'
= '1' the generated column 'b' values will not be replicated"
+-- 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)

=========================

4. Here names for both the tests are the same. I think we should use
different names.

+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
+is( $result, qq(4|8
+5|10), 'generated columns replicated to non-generated column on subscriber');
+
+$node_publisher->safe_psql('postgres', "INSERT INTO tab3 VALUES (4), (5)");
+
+$node_publisher->wait_for_catchup('sub3');
+
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab3");
+is( $result, qq(4|24
+5|25), 'generated columns replicated to non-generated column on subscriber');

Thanks and Regards,
Shlok Kyal

---

From: Shubham Khanna <[email-masked]>
Date: Wed, 19 Jun 2024 16:52:20 +0530

> +          subscriber-side computed or default data.

All the comments are handled.

The attached Patch contains all the suggested changes.

Thanks and Regards,
Shubham Khanna.

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 19 Jun 2024 18:13:14 +0200

> The attached Patch contains all the suggested changes.

Please also take a look at the proposed patch for virtual generated 
columns [0] and consider how that would affect your patch.  I think your 
feature can only replicate *stored* generated columns.  So perhaps the 
documentation and terminology in your patch should reflect that.

[0]: 
https://www.postgresql.org/message-id/flat/[email-masked]

---

From: vignesh C <[email-masked]>
Date: Thu, 20 Jun 2024 12:51:57 +0530

> documentation and terminology in your patch should reflect that.

This patch is unable to manage virtual generated columns because it
stores NULL values for them. Along with documentation the initial sync
command being generated also should be changed to sync data
exclusively for stored generated columns, omitting virtual ones. I
suggest treating these changes as a separate patch(0003) for future
merging or a separate commit, depending on the order of patch
acceptance.

Regards,
Vignesh

---

From: Shlok Kyal <[email-masked]>
Date: Thu, 20 Jun 2024 18:33:15 +0530

> merging or a separate commit, depending on the order of patch

I have addressed the issue and updated the documentation accordingly.
And created a new 0003 patch.
Please refer to v9-0003 patch for the same in [1].

[1]: https://www.postgresql.org/message-id/CANhcyEXmjLEPNgOSAtjS4YGb9JvS8w-SO9S%2BjRzzzXo2RavNWw%40mail.gmail.com

Thanks and Regards,
Shlok Kyal