Hi Vignesh.
Some review comments for patch v52-0003
==
1. GENERAL - change to use enum.
On Thu, Jan 16, 2025 at 7:47 PM vignesh C wrote:
>
> On Wed, 15 Jan 2025 at 11:17, Peter Smith wrote:
> > 2.
> > As suggested in more detail below, I think it would be better if you
> > can define a C
On Thu, Jan 16, 2025 at 7:47 PM vignesh C wrote:
>
...
>
> v52-0002 - One typo related to a publication name which Peter reported.
Hi Vignesh,
Patch v52-0002 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Vignesh.
Some review comments for v52-0001.
==
Commit message
1.
I guess the root cause is that in PG18 we decided to put the
"Generated columns" column to the left of the "Via Root" column
instead of to the right, and in doing so introduced a mistake ordering
the code.
The commit messag
On Wed, 15 Jan 2025 at 14:41, Shlok Kyal wrote:
>
> I have reviewed the patch and have following comments:
>
> In file: create_publication.sgml
>
> 1.
> +
> + If set to stored, the generated columns present
> in
> + the tables associated with publication will be replica
On Wed, 15 Jan 2025 at 12:00, Peter Smith wrote:
>
> During my review of Vignesh's patch for the enum-version of
> publish_generated_columns, I was thinking of yet another way to
> specify which columns to replicate.
>
> My idea below is analogous to the existing 'publish' option; Instead
> of add
On Mon, 13 Jan 2025 at 23:52, vignesh C wrote:
>
> On Mon, 13 Jan 2025 at 14:57, Amit Kapila wrote:
> >
> > On Mon, Jan 13, 2025 at 5:25 AM Peter Smith wrote:
> > >
> > > Future -- there probably need to be further clarifications/emphasis to
> > > describe how the generated column replication fe
On Mon, Jan 13, 2025 at 8:27 PM Amit Kapila wrote:
>
> On Mon, Jan 13, 2025 at 5:25 AM Peter Smith wrote:
> >
> > Future -- there probably need to be further clarifications/emphasis to
> > describe how the generated column replication feature only works for
> > STORED generated columns (not VIRTU
Hi Vignesh.
Some review comments for patch 0001
==
GENERAL
1.
AFAIK there are still many places in the docs where there is no
distinction made between the stored/virtual generated cols. Maybe we
need another patch in this patch set to address all of these?
For example, CREATE PUBLICATION sa
On Mon, 13 Jan 2025 at 14:57, Amit Kapila wrote:
>
> On Mon, Jan 13, 2025 at 5:25 AM Peter Smith wrote:
> >
> > Future -- there probably need to be further clarifications/emphasis to
> > describe how the generated column replication feature only works for
> > STORED generated columns (not VIRTUAL
On Mon, Jan 13, 2025 at 5:25 AM Peter Smith wrote:
>
> Future -- there probably need to be further clarifications/emphasis to
> describe how the generated column replication feature only works for
> STORED generated columns (not VIRTUAL ones), but IMO it is better to
> address that separately *aft
On Mon, Jan 13, 2025 at 10:55 AM Peter Smith wrote:
>
> Hi,
>
> Some patches of this thread have fallen though the cracks for more
> than 2 months now, so I am re-posting them so that do not get
> overlooked any longer.
>
> For v49 [1] there were 2 patches:
> v49-0001-Enable-support-for-publish_ge
Hi,
Some patches of this thread have fallen though the cracks for more
than 2 months now, so I am re-posting them so that do not get
overlooked any longer.
For v49 [1] there were 2 patches:
v49-0001-Enable-support-for-publish_generated_columns-par
v49-0002-DOCS-Generated-Column-Replication
Then,
On Thu, Nov 07, 2024 at 03:46:54PM +0530, Amit Kapila wrote:
> Can we slightly modify it as: "If true, this publication replicates
> the generated columns in the tables associated with the publication."?
> BTW, we might want to say: "If true, this publication replicates the
> stored generated colum
On Thu, Nov 7, 2024 at 2:45 PM Shinoda, Noriyoshi (SXD Japan FSIP)
wrote:
>
> Hi, Hackers.
>
> Thanks for developing this great feature.
> There seems to be a missing description of the "pubgencols" column added to
> the "pg_publication" catalog. The attached patch adds the description to the
>
On Thu, Nov 7, 2024 at 12:03 PM Peter Eisentraut wrote:
>
> On 07.11.24 05:13, Amit Kapila wrote:
> > On Wed, Nov 6, 2024 at 4:18 PM vignesh C wrote:
> >>
> >> The attached v50 version patch has the changes for the same.
>
> Could you (everybody on this thread) please provide guidance how this
>
xplanation.
Regards,
Noriyoshi Shinoda
-Original Message-
From: Amit Kapila
Sent: Thursday, November 7, 2024 1:14 PM
To: vignesh C
Cc: Peter Smith ; Shubham Khanna
; Masahiko Sawada ;
Rajendra Kumar Dangwal ;
pgsql-hackers@lists.postgresql.org; eu...@eulerto.com
Subject: Re: Pgoutp
On 07.11.24 05:13, Amit Kapila wrote:
On Wed, Nov 6, 2024 at 4:18 PM vignesh C wrote:
The attached v50 version patch has the changes for the same.
Could you (everybody on this thread) please provide guidance how this
feature is supposed to interact with virtual generated columns [0]. I
do
On Wed, Nov 6, 2024 at 4:18 PM vignesh C wrote:
>
> The attached v50 version patch has the changes for the same.
>
Pushed.
--
With Regards,
Amit Kapila.
On Wed, Nov 6, 2024 at 11:35 AM Peter Smith wrote:
>
> I am observing some unexpected errors with the following scenario.
>
You are getting an expected ERROR. It is because of the design of
logical decoding which relies on historic snapshots.
> ==
> Tables:
>
> Publisher table:
> test_pub=#
Hi Vignesh,
I am observing some unexpected errors with the following scenario.
==
Tables:
Publisher table:
test_pub=# create table t1 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
CREATE TABLE
test_pub=# insert into t1 values (1);
INSERT 0 1
~
And Subscriber table:
test_sub=# create t
On Wed, Nov 6, 2024 at 10:26 AM Peter Smith wrote:
>
> On Wed, Nov 6, 2024 at 3:26 PM Amit Kapila wrote:
> >
> > On Wed, Nov 6, 2024 at 7:34 AM Peter Smith wrote:
> > >
> > > Hi Vignesh,
> > >
> > > Here are my review comments for patch v49-0001.
> > >
> >
> > I have a question on the display of
On Wed, Nov 6, 2024 at 3:26 PM Amit Kapila wrote:
>
> On Wed, Nov 6, 2024 at 7:34 AM Peter Smith wrote:
> >
> > Hi Vignesh,
> >
> > Here are my review comments for patch v49-0001.
> >
>
> I have a question on the display of this new parameter.
>
> postgres=# \dRp+
>
On Wed, Nov 6, 2024 at 7:34 AM Peter Smith wrote:
>
> Hi Vignesh,
>
> Here are my review comments for patch v49-0001.
>
I have a question on the display of this new parameter.
postgres=# \dRp+
Publication pub_gen
Owner | All tables | Inserts | Updates |
Hi Vignesh,
Here are my review comments for patch v49-0001.
==
src/backend/catalog/pg_publication.c
1. check_fetch_column_list
+bool
+check_fetch_column_list(Publication *pub, Oid relid, MemoryContext mcxt,
+ Bitmapset **cols)
+{
+ HeapTuple cftuple = NULL;
+ Datum cfdatum = 0;
+ bool found
On Tue, 5 Nov 2024 at 12:32, Peter Smith wrote:
>
> Hi Vignesh,
>
> Here are my review comments for patch v48-0001.
>
> ==
> src/backend/catalog/pg_publication.c
>
> has_column_list_defined:
>
> 1.
> + if (HeapTupleIsValid(cftuple))
> + {
> + bool isnull = true;
> +
> + /* Lookup the column li
On Tue, Nov 5, 2024 at 12:32 PM Peter Smith wrote:
>
> has_column_list_defined:
>
> 1.
> + if (HeapTupleIsValid(cftuple))
> + {
> + bool isnull = true;
> +
> + /* Lookup the column list attribute. */
> + (void) SysCacheGetAttr(PUBLICATIONRELMAP, cftuple,
> +Anum_pg_publication_rel_prattrs,
> +
Hi Vignesh,
Here are my review comments for patch v48-0001.
==
src/backend/catalog/pg_publication.c
has_column_list_defined:
1.
+ if (HeapTupleIsValid(cftuple))
+ {
+ bool isnull = true;
+
+ /* Lookup the column list attribute. */
+ (void) SysCacheGetAttr(PUBLICATIONRELMAP, cftuple,
+An
On Tue, 5 Nov 2024 at 07:55, Peter Smith wrote:
>
> Hi Vignesh,
>
> Here are my review comments for the v47-0002 (DOCS) patch.
>
> ==
> diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
> index 577bcb4b71..a13f19bdbe 100644
> --- a/doc/src/sgml/ddl.sgml
> +++ b/doc/src/sgml/ddl.sgml
>
On Tue, Nov 5, 2024 at 7:00 AM Peter Smith wrote:
>
>
> ~
>
> 1b.
> Everywhere in this patch (except here), this is called the
> 'publish_generated_columns' parameter (not "option") so it should be
> called a parameter here also. Anyway, apparently that is the docs rule
> -- see [1].
>
In the thr
Hi Vignesh,
Here are my review comments for the v47-0002 (DOCS) patch.
==
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 577bcb4b71..a13f19bdbe 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -517,7 +517,8 @@ CREATE TABLE people (
Generated columns a
Hi Vignesh,
Here are my review comments for your latest patch v47-0001.
==
doc/src/sgml/ddl.sgml
1.
- Generated columns can be replicated during logical replication by
- including them in the column list of the
- CREATE PUBLICATION command.
+ Generated columns are
On Fri, 1 Nov 2024 at 09:23, Peter Smith wrote:
>
> On Thu, Oct 31, 2024 at 3:16 AM vignesh C wrote:
>
> > Thanks for committing this patch, here is a rebased version of the
> > remaining patches.
> >
>
> Hi Vignesh.
>
> Here are my review comments for the docs patch v1-0002.
>
> ==
> Commit
On Mon, 4 Nov 2024 at 16:25, Amit Kapila wrote:
>
> On Wed, Oct 30, 2024 at 9:46 PM vignesh C wrote:
> >
> ...
> + /*
> + * For non-column list publications—such as TABLE (without a column
> + * list), ALL TABLES, or ALL TABLES IN SCHEMA publications consider
> + * all columns of the table, inclu
On Wed, Oct 30, 2024 at 9:46 PM vignesh C wrote:
>
...
+ /*
+ * For non-column list publications—such as TABLE (without a column
+ * list), ALL TABLES, or ALL TABLES IN SCHEMA publications consider
+ * all columns of the table, including generated columns, based on the
+ * pubgencols option.
+ */
On Fri, Nov 1, 2024 at 7:10 AM Peter Smith wrote:
>
>
> ==
> doc/src/sgml/protocol.sgml
>
> 3.
>
> - Next, one of the following submessages appears for each column:
> + Next, one of the following submessages appears for each column
> (except generated columns):
>
> Hmm. Now th
On Mon, Nov 4, 2024 at 10:30 AM Peter Smith wrote:
>
> On Mon, Nov 4, 2024 at 12:28 AM vignesh C wrote:
>
> Thanks for the latest doc v2 "fix" patch. Here are my review comments about
> it.
>
> ==
> src/sgml/logical-replication.sgml
>
> 1.
> During initial data synchronization, only the
On Mon, Nov 4, 2024 at 12:28 AM vignesh C wrote:
>
> On Thu, 31 Oct 2024 at 16:44, Ajin Cherian wrote:
> >
> >
> >
> > On Thu, Oct 31, 2024 at 9:55 PM Ajin Cherian wrote:
> >>
> >> I ran some tests and verified that the patch works with previous versions
> >> of PG12 and PG17
> >> 1. Verified w
On Mon, Nov 4, 2024 at 2:20 PM Amit Kapila wrote:
>
> On Thu, Oct 31, 2024 at 4:26 PM Ajin Cherian wrote:
> >
> > 5. Verified that publications with different column list are disallowed to
> > be subscribed by one subscription
> >a. PUB_A(column list = (a, b)) PUB_B(no column list, with
> >
On Thu, Oct 31, 2024 at 4:26 PM Ajin Cherian wrote:
>
> 5. Verified that publications with different column list are disallowed to be
> subscribed by one subscription
>a. PUB_A(column list = (a, b)) PUB_B(no column list, with
> publish_generated_column) - OK
>b. PUB_A(column list = (a, b
On Thu, Oct 31, 2024 at 3:16 AM vignesh C wrote:
>
> On Wed, 30 Oct 2024 at 15:06, Amit Kapila wrote:
> >
> > On Tue, Oct 29, 2024 at 8:50 PM vignesh C wrote:
> > >
> > > Thank you for reporting this issue. The attached v46 patch addresses
> > > the problem and includes some adjustments to the c
On Fri, 1 Nov 2024 at 13:27, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Vignesh,
>
> Thanks for rebasing the patch! Before reviewing deeply, I want to confirm the
> specification.
> I understood like below based on the documentation.
>
> - If publish_generated_columns is false, the publication won't
On Thu, 31 Oct 2024 at 16:44, Ajin Cherian wrote:
>
>
>
> On Thu, Oct 31, 2024 at 9:55 PM Ajin Cherian wrote:
>>
>> I ran some tests and verified that the patch works with previous versions of
>> PG12 and PG17
>> 1. Verified with publications with generated columns and without generated
>> colu
Dear Vignesh,
Thanks for rebasing the patch! Before reviewing deeply, I want to confirm the
specification.
I understood like below based on the documentation.
- If publish_generated_columns is false, the publication won't replicate
generated columns
- If publish_generated_columns is true, the b
On Thu, Oct 31, 2024 at 3:16 AM vignesh C wrote:
> Thanks for committing this patch, here is a rebased version of the
> remaining patches.
>
Hi Vignesh.
Here are my review comments for the docs patch v1-0002.
==
Commit message
1.
This patch updates docs to describe the new feature allowin
On Thu, Oct 31, 2024 at 3:16 AM vignesh C wrote:
> Thanks for committing this patch, here is a rebased version of the
> remaining patches.
>
Hi Vignesh, thanks for the rebased patches.
Here are my review comments for patch v1-0001.
==
Commit message.
1.
The commit message text is stale, so
On Thu, Oct 31, 2024 at 9:55 PM Ajin Cherian wrote:
> I ran some tests and verified that the patch works with previous versions
> of PG12 and PG17
> 1. Verified with publications with generated columns and without generated
> columns on patched code and subscriptions on PG12 and PG17
> Observatio
I ran some tests and verified that the patch works with previous versions
of PG12 and PG17
1. Verified with publications with generated columns and without generated
columns on patched code and subscriptions on PG12 and PG17
Observations:
a. If publication is created with publish_generated_colu
On Thu, Oct 31, 2024 at 1:14 PM vignesh C wrote:
>
> On Thu, 31 Oct 2024 at 04:42, Peter Smith wrote:
> >
> > On Thu, Oct 31, 2024 at 3:16 AM vignesh C wrote:
> > >
> > > On Wed, 30 Oct 2024 at 15:06, Amit Kapila wrote:
> > > >
> > > > On Tue, Oct 29, 2024 at 8:50 PM vignesh C wrote:
> > > > >
On Thu, 31 Oct 2024 at 04:42, Peter Smith wrote:
>
> On Thu, Oct 31, 2024 at 3:16 AM vignesh C wrote:
> >
> > On Wed, 30 Oct 2024 at 15:06, Amit Kapila wrote:
> > >
> > > On Tue, Oct 29, 2024 at 8:50 PM vignesh C wrote:
> > > >
> > > > Thank you for reporting this issue. The attached v46 patch
On Thu, Oct 31, 2024 at 3:16 AM vignesh C wrote:
>
> On Wed, 30 Oct 2024 at 15:06, Amit Kapila wrote:
> >
> > On Tue, Oct 29, 2024 at 8:50 PM vignesh C wrote:
> > >
> > > Thank you for reporting this issue. The attached v46 patch addresses
> > > the problem and includes some adjustments to the c
On Tue, Oct 29, 2024 at 8:50 PM vignesh C 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.
On Tue, 29 Oct 2024 at 17:09, Shubham Khanna
wrote:
>
> While performing the Backward Compatibility Test, I found that
> 'tablesync' is not working for the older versions i.e., from
> version-12 till version-15.
> I created 2 nodes ; PUBLISHER on old versions and SUBSCRIBER on HEAD +
> v45 Patch f
On Tue, Oct 29, 2024 at 3:18 PM vignesh C wrote:
>
> On Tue, 29 Oct 2024 at 11:30, Peter Smith wrote:
> >
> > Here are my review comments for patch v44-0002.
> >
> > ==
> > Commit message.
> >
> > 1.
> > The commit message is missing.
>
> This patch is now merged, so no change required.
>
> >
On Tue, 29 Oct 2024 at 11:30, Peter Smith wrote:
>
> Here are my review comments for patch v44-0002.
>
> ==
> Commit message.
>
> 1.
> The commit message is missing.
This patch is now merged, so no change required.
> ==
> src/backend/replication/logical/tablesync.c
>
> fetch_remote_table
On Tue, 29 Oct 2024 at 11:19, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch! Here are my comments for v44.
>
> 01. fetch_remote_table_info()
>
> `bool *remotegencolpresent` is accessed unconditionally, but it can cause
> crash
> if NULL is passed to the functi
On Tue, 29 Oct 2024 at 07:44, Peter Smith wrote:
>
> Here are my review comments for v44-0001.
>
> ==
> doc/src/sgml/ref/create_publication.sgml
>
> 1.
> - When a column list is specified, only the named columns are replicated.
> + When a column list is specified, all columns (except
On Tue, Oct 29, 2024 at 11:30 AM Peter Smith wrote:
> ==
> src/backend/replication/logical/tablesync.c
>
> fetch_remote_table_info:
>
> 2.
> +fetch_remote_table_info(char *nspname, char *relname, LogicalRepRelation
> *lrel,
> + List **qual, bool *remotegencolpresent)
>
> The name 'remotegenco
On Tue, Oct 29, 2024 at 11:19 AM Hayato Kuroda (Fujitsu)
wrote:
>
> 01. fetch_remote_table_info()
>
> `bool *remotegencolpresent` is accessed unconditionally, but it can cause
> crash
> if NULL is passed to the function. Should we add an Assert to verify it?
>
This is a static function being cal
Here are my review comments for patch v44-0002.
==
Commit message.
1.
The commit message is missing.
==
src/backend/replication/logical/tablesync.c
fetch_remote_table_info:
2.
+fetch_remote_table_info(char *nspname, char *relname, LogicalRepRelation *lrel,
+ List **qual, bool *remotege
Dear Shubham,
Thanks for updating the patch! Here are my comments for v44.
01. fetch_remote_table_info()
`bool *remotegencolpresent` is accessed unconditionally, but it can cause crash
if NULL is passed to the function. Should we add an Assert to verify it?
02. fetch_remote_table_info()
```
+
On Tue, Oct 29, 2024 at 7:44 AM Peter Smith wrote:
>
> ==
> src/backend/replication/logical/proto.c
>
> 2.
> +bool
> +logicalrep_should_publish_column(Form_pg_attribute att, Bitmapset *columns)
> +{
> + if (att->attisdropped)
> + return false;
> +
> + /*
> + * Skip publishing generated columns
Here are my review comments for v44-0001.
==
doc/src/sgml/ref/create_publication.sgml
1.
- When a column list is specified, only the named columns are replicated.
+ When a column list is specified, all columns (except generated columns)
+ of the table are replicated.
If
On Mon, Oct 28, 2024 at 8:47 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch! I resumed reviewing the patch set.
> Here are only cosmetic comments as my rehabilitation.
>
> 01. getPublications()
>
> I feel we could follow the notation like getSubscriptions(),
On Mon, Oct 28, 2024 at 7:43 AM Peter Smith wrote:
>
> Hi, here are my review comments for patch v43-0001.
>
> ==
>
> 1. Missing docs update?
>
> The CREATE PUBLICATION docs currently says:
> When a column list is specified, only the named columns are
> replicated. If no column list is specifi
On Monday, October 28, 2024 2:54 PM Hayato Kuroda (Fujitsu)
wrote:
>
>
> 02. 031_column_list.pl
>
> ```
> -# TEST: Generated and dropped columns are not considered for the column
> list.
> +# TEST: Dropped columns are not considered for the column list.
> # So, the publication having a column
On Monday, October 28, 2024 1:34 PM Amit Kapila wrote:
>
> On Mon, Oct 28, 2024 at 7:43 AM Peter Smith
> wrote:
>
> >
> > 4. pgoutput_column_list_init
> >
> > - if (att->attisdropped || att->attgenerated)
> > + if (att->attisdropped)
> > continue;
> >
> > + if (att->attgenerated)
> > + {
> >
On Mon, Oct 28, 2024 at 12:27 PM Peter Smith wrote:
>
> On Mon, Oct 28, 2024 at 4:34 PM Amit Kapila wrote:
> >
> > On Mon, Oct 28, 2024 at 7:43 AM Peter Smith wrote:
> > >
> > > Hi, here are my review comments for patch v43-0001.
> > >
> > > ==
> > > src/backend/replication/logical/proto.c
>
On Mon, Oct 28, 2024 at 5:45 PM Amit Kapila wrote:
>
> On Mon, Oct 28, 2024 at 7:43 AM Peter Smith wrote:
> >
> > 7.
> > +$node_publisher->wait_for_catchup('sub_gen');
> > +
> > +is( $node_subscriber->safe_psql(
> > + 'postgres', "SELECT * FROM test_gen ORDER BY a"),
> > + qq(1|2),
> > + 'replica
On Mon, Oct 28, 2024 at 4:34 PM Amit Kapila wrote:
>
> On Mon, Oct 28, 2024 at 7:43 AM Peter Smith wrote:
> >
> > Hi, here are my review comments for patch v43-0001.
> >
> > ==
> > src/backend/replication/logical/proto.c
> >
> > 2.
> > +static bool
> > +should_publish_column(Form_pg_attribute
Dear Shubham,
More comments for v43-0001.
01. publication.out and publication.sql
I think your fix is not sufficient, even if it pass tests.
```
-- error: system attributes "ctid" not allowed in column list
ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, ctid);
-ERROR: cannot u
On Mon, Oct 28, 2024 at 7:43 AM Peter Smith wrote:
>
> 7.
> +$node_publisher->wait_for_catchup('sub_gen');
> +
> +is( $node_subscriber->safe_psql(
> + 'postgres', "SELECT * FROM test_gen ORDER BY a"),
> + qq(1|2),
> + 'replication with generated columns in column list');
> +
>
> But, this is only
On Mon, Oct 28, 2024 at 7:43 AM Peter Smith wrote:
>
> Hi, here are my review comments for patch v43-0001.
>
> ==
> src/backend/replication/logical/proto.c
>
> 2.
> +static bool
> +should_publish_column(Form_pg_attribute att, Bitmapset *columns)
> +{
> + if (att->attisdropped)
> + return false
Dear Shubham,
Thanks for updating the patch! I resumed reviewing the patch set.
Here are only cosmetic comments as my rehabilitation.
01. getPublications()
I feel we could follow the notation like getSubscriptions(), because number of
parameters became larger. How do you feel like attached?
02
Hi, here are my review comments for patch v43-0001.
==
1. Missing docs update?
The CREATE PUBLICATION docs currently says:
When a column list is specified, only the named columns are
replicated. If no column list is specified, all columns of the table
are replicated through this publication,
On Fri, Oct 25, 2024 at 3:54 PM Amit Kapila wrote:
>
> On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila wrote:
> >
> > On Thu, Oct 24, 2024 at 8:50 PM vignesh C wrote:
> > >
> > > The v42 version patch attached at [1] has the changes for the same.
> > >
> >
> > Some more comments:
> >
>
> 1.
> +pgou
On Fri, Oct 25, 2024 at 12:07 PM Amit Kapila wrote:
>
> On Thu, Oct 24, 2024 at 8:50 PM vignesh C wrote:
> >
> > The v42 version patch attached at [1] has the changes for the same.
> >
>
> Some more comments:
>
1.
+pgoutput_pubgencol_init(PGOutputData *data, List *publications,
+ RelationSyncEnt
On Thu, 24 Oct 2024 at 16:44, Amit Kapila wrote:
>
> On Thu, Oct 24, 2024 at 12:15 PM vignesh C wrote:
> >
> > The attached v41 version patch has the changes for the same.
> >
>
> Please find comments for the new version as follows:
> 1.
> + Generated columns may be skipped during logical re
On Thu, Oct 24, 2024 at 8:50 PM vignesh C wrote:
>
> The v42 version patch attached at [1] has the changes for the same.
>
Some more comments:
1.
@@ -1017,7 +1089,31 @@ pgoutput_column_list_init(PGOutputData *data,
List *publications,
{
ListCell *lc;
bool first = true;
+ Bitmapset *relcol
On Thu, Oct 24, 2024 at 12:15 PM vignesh C wrote:
>
> The attached v41 version patch has the changes for the same.
>
Please find comments for the new version as follows:
1.
+ Generated columns may be skipped during logical replication
according to the
+ CREATE PUBLICATION option
+
On Wed, Oct 23, 2024 at 11:51 AM Amit Kapila wrote:
>
> Thanks. One more thing that I didn't like about the patch is that it
> used column_list to address the "publish_generated_columns = false"
> case such that we build column_list without generated columns for the
> same. The first problem is th
On Wed, Oct 23, 2024 at 12:26 PM Peter Smith wrote:
>
> On Wed, Oct 23, 2024 at 5:21 PM Amit Kapila wrote:
>
> > Additional comment on the 0003 patch
> > +#
> > =
> > +# Misc test.
> > +#
> > +# A "normal -> generated" r
On Wed, Oct 23, 2024 at 5:21 PM Amit Kapila wrote:
> Additional comment on the 0003 patch
> +#
> =
> +# Misc test.
> +#
> +# A "normal -> generated" replication.
> +#
> +# In this test case we use DROP EXPRESSION to chan
On Tue, Oct 22, 2024 at 9:42 PM Masahiko Sawada wrote:
>
> On Tue, Oct 22, 2024 at 3:50 AM Amit Kapila wrote:
> >
>
> > Considering this, I feel if find this behavior buggy then we should
> > fix this separately rather than part of this patch. What do you think?
>
> Agreed. It's better to fix it
Recently (~ version v39/v40) some changes to 'get_publications_str'
calls got removed from this patchset because it was decided it was
really a separate problem, unrelated to this generated columns
feature.
FYI - I've started a new thread "Refactor to use common function
'get_publications_str'" [1
On Tue, Oct 22, 2024 at 3:50 AM Amit Kapila wrote:
>
> On Wed, Oct 9, 2024 at 10:19 PM Masahiko Sawada wrote:
> >
> > Regarding the 0001 patch, it seems to me that UPDATE and DELETE are
> > allowed on the table even if its replica identity is set to generated
> > columns that are not published. F
On Wed, Oct 9, 2024 at 10:19 PM Masahiko Sawada wrote:
>
> Regarding the 0001 patch, it seems to me that UPDATE and DELETE are
> allowed on the table even if its replica identity is set to generated
> columns that are not published. For example, consider the following
> scenario:
>
> create table
Hi Shubham, here are some comments for v40-0003 (TAP) patch.
==
Combo tests
1.
+# =
+# The following test cases exercise logical replication for the combinations
+# where there is a generated column on one or both sid
Hi SHubham, Here are my review comments for v40-0001 (code)
Please don't post a blanket response of "I have fixed all the
comments" response to this. Sometimes things get missed. Instead,
please reply done/not-done/whatever individually, so I can track the
changes properly.
==
src/backend/cat
On Fri, Oct 18, 2024 at 5:42 PM Shubham Khanna
wrote:
> > >
> > > I have fixed all the given comments. The attached patches contain the
> > > required changes.
Review comments:
===
1.
>
B. when generated columns are not published
* Publisher not-generated column => subscriber not-gen
On Fri, 18 Oct 2024 at 17:42, Shubham Khanna
wrote:
>
> On Thu, Oct 17, 2024 at 12:58 PM vignesh C wrote:
> >
> > On Wed, 16 Oct 2024 at 23:25, Shubham Khanna
> > wrote:
> > >
> > > On Wed, Oct 9, 2024 at 9:08 AM vignesh C wrote:
> > > >
> > > > On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> >
On Fri, 18 Oct 2024 at 17:42, Shubham Khanna
wrote:
>
>
> I have fixed all the given comments. The attached v40-0001 patch
> contains the required changes.
1) The recent patch removed the function header comment where
generated column is specified, that change is required:
@@ -511,7 +511,6 @@ pub
On Thu, Oct 17, 2024 at 3:59 PM vignesh C wrote:
>
> On Wed, 16 Oct 2024 at 23:25, Shubham Khanna
> wrote:
> >
> > On Wed, Oct 9, 2024 at 9:08 AM vignesh C wrote:
> > >
> > > On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> > > wrote:
> > > >
> > > > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith
>
On Wed, 16 Oct 2024 at 23:25, Shubham Khanna
wrote:
>
> On Wed, Oct 9, 2024 at 9:08 AM vignesh C wrote:
> >
> > On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> > wrote:
> > >
> > > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith wrote:
> > > >
> > > > Hi Shubham, here are my review comments for v36-0
On Wed, 16 Oct 2024 at 23:25, Shubham Khanna
wrote:
>
> On Wed, Oct 9, 2024 at 9:08 AM vignesh C wrote:
> >
> > On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> > wrote:
> > >
> > > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith wrote:
> > > >
> > > > Hi Shubham, here are my review comments for v36-0
On Thu, Oct 10, 2024 at 10:53 AM Peter Smith wrote:
>
> Here are some comments for TAP test patch v37-0003.
>
> I’m not in favour of the removal of such a large number of
> 'combination' and other 'misc' tests. In the commit message, please
> delete me as a "co-author" of this patch.
>
> ==
>
On Wed, Oct 9, 2024 at 11:52 AM vignesh C wrote:
>
> On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> wrote:
> >
> > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith wrote:
> > >
> > > Hi Shubham, here are my review comments for v36-0001.
> > >
> > > ==
> > > 1. General - merge patches
> > >
> > >
On Wed, Oct 9, 2024 at 11:13 AM Peter Smith wrote:
>
> Hi, here are my review comments for patch v37-0001.
>
> ==
> Commit message
>
> 1.
> Example usage of subscription option:
> CREATE PUBLICATION FOR TABLE tab_gencol WITH (publish_generated_columns
> = true);
>
> ~
>
> This is wrong -- it's
On Wed, Oct 9, 2024 at 11:00 AM vignesh C wrote:
>
> On Tue, 8 Oct 2024 at 11:37, Shubham Khanna
> wrote:
> >
> > On Fri, Oct 4, 2024 at 9:36 AM Peter Smith wrote:
> > >
> > > Hi Shubham, here are my review comments for v36-0001.
> > >
> > > ==
> > > 1. General - merge patches
> > >
> > >
Here are some comments for TAP test patch v37-0003.
I’m not in favour of the removal of such a large number of
'combination' and other 'misc' tests. In the commit message, please
delete me as a "co-author" of this patch.
==
1.
Any description or comment that still mentions "all combinations"
On Mon, Oct 7, 2024 at 11:07 PM Shubham Khanna
wrote:
>
> On Fri, Oct 4, 2024 at 9:36 AM Peter Smith wrote:
> >
> > Hi Shubham, here are my review comments for v36-0001.
> >
> > ==
> > 1. General - merge patches
> >
> > It is long past due when patches 0001 and 0002 should've been merged.
>
1 - 100 of 257 matches
Mail list logo
