On Wed, Jan 29, 2025 at 2:48 PM Amit Kapila wrote:
>
> On Wed, Jan 29, 2025 at 6:03 AM Peter Smith wrote:
> >
> > On Tue, Jan 28, 2025 at 7:59 PM Amit Kapila wrote:
> > >
> > > On Thu, Jan 23, 2025 at 9:58 AM vignesh C wrote:
> > > >
> > >
> > > I have pushed the remaining part of this patch. N
On Wed, Jan 29, 2025 at 2:48 PM Amit Kapila wrote:
>
> On Wed, Jan 29, 2025 at 6:03 AM Peter Smith wrote:
> >
> > On Tue, Jan 28, 2025 at 7:59 PM Amit Kapila wrote:
> > >
> > > On Thu, Jan 23, 2025 at 9:58 AM vignesh C wrote:
> > > >
> > >
> > > I have pushed the remaining part of this patch. N
On Wed, Jan 29, 2025 at 6:03 AM Peter Smith wrote:
>
> On Tue, Jan 28, 2025 at 7:59 PM Amit Kapila wrote:
> >
> > On Thu, Jan 23, 2025 at 9:58 AM vignesh C wrote:
> > >
> >
> > I have pushed the remaining part of this patch. Now, we can review the
> > proposed documentation part.
> >
> > I feel
On Tue, Jan 28, 2025 at 7:59 PM Amit Kapila wrote:
>
> On Thu, Jan 23, 2025 at 9:58 AM vignesh C wrote:
> >
>
> I have pushed the remaining part of this patch. Now, we can review the
> proposed documentation part.
>
> I feel we don't need the Examples sub-section for this part of the
> docs. The
On Thu, Jan 23, 2025 at 9:58 AM vignesh C wrote:
>
I have pushed the remaining part of this patch. Now, we can review the
proposed documentation part.
I feel we don't need the Examples sub-section for this part of the
docs. The behavior is quite clear from the "Behavior Summary"
sub-section tabl
On Fri, Jan 24, 2025 at 6:38 PM vignesh C wrote:
>
> On Fri, 24 Jan 2025 at 10:36, vignesh C wrote:
> >
> > On Fri, 24 Jan 2025 at 09:51, Tom Lane wrote:
> > >
> > > Amit Kapila writes:
> > > > On Fri, Jan 24, 2025 at 8:39 AM Tom Lane wrote:
> > > >> I think the problem is not so much the unde
On Fri, Jan 24, 2025 at 1:08 PM vignesh C wrote:
>
> The attached patch has the changes for the same.
>
LGTM. Unless there are more comments, I'll push this in a day or so.
--
With Regards,
Amit Kapila.
On Fri, 24 Jan 2025 at 10:36, vignesh C wrote:
>
> On Fri, 24 Jan 2025 at 09:51, Tom Lane wrote:
> >
> > Amit Kapila writes:
> > > On Fri, Jan 24, 2025 at 8:39 AM Tom Lane wrote:
> > >> I think the problem is not so much the underscore as the
> > >> inconsistency. You've got "pub", "gen", and
On Fri, 24 Jan 2025 at 09:51, Tom Lane wrote:
>
> Amit Kapila writes:
> > On Fri, Jan 24, 2025 at 8:39 AM Tom Lane wrote:
> >> I think the problem is not so much the underscore as the
> >> inconsistency. You've got "pub", "gen", and "cols" run together,
> >> but then you feel a need to separate
Amit Kapila writes:
> On Fri, Jan 24, 2025 at 8:39 AM Tom Lane wrote:
>> I think the problem is not so much the underscore as the
>> inconsistency. You've got "pub", "gen", and "cols" run together,
>> but then you feel a need to separate "type"?
> It was easy to read and to avoid getting a sing
On Fri, Jan 24, 2025 at 8:39 AM Tom Lane wrote:
>
> Amit Kapila writes:
> > On Fri, Jan 24, 2025 at 4:41 AM Peter Smith wrote:
> >> However, in hindsight, I am not sure that the column should have been
> >> renamed 'pubgencols_type' in the first place because I cannot find any
> >> other catalog
Amit Kapila writes:
> On Fri, Jan 24, 2025 at 4:41 AM Peter Smith wrote:
>> However, in hindsight, I am not sure that the column should have been
>> renamed 'pubgencols_type' in the first place because I cannot find any
>> other catalogs with an underscore in their column names.
> See pg_rewrite
On Fri, Jan 24, 2025 at 4:41 AM Peter Smith wrote:
>
> However, in hindsight, I am not sure that the column should have been
> renamed 'pubgencols_type' in the first place because I cannot find any
> other catalogs with an underscore in their column names.
>
See pg_rewrite.ev_type for a similar c
On Fri, Jan 24, 2025 at 12:03 AM Shinoda, Noriyoshi (SXD Japan FSI)
wrote:
>
> Hi, hackers.
>
> Thanks for developing this great feature.
> The documentation for the pg_publication catalog shows a 'pubgencols' column,
> but the actual column name is the 'pubgencols_type' column.
> Also, the order
el Paquier ; Shinoda, Noriyoshi (SXD Japan FSI)
; Shubham Khanna ;
Masahiko Sawada ; Rajendra Kumar Dangwal
; pgsql-hackers@lists.postgresql.org;
eu...@eulerto.com
Subject: Re: Pgoutput not capturing the generated columns
> On 23 Jan 2025, at 13:19, vignesh C wrote:
> When dumping from Po
> On 23 Jan 2025, at 13:19, vignesh C wrote:
> When dumping from Postgres <=PG17 servers, the query generated for
> pubgencols_type incorrectly included the macro name instead of the
> macro value. This resulted in dump failures. This commit fixes the
> issue by correctly specifying the macro val
On Thu, 23 Jan 2025 at 17:03, Amit Kapila wrote:
>
> On Thu, Jan 23, 2025 at 11:18 AM Peter Smith wrote:
> >
> > Patch v56-0001 LGTM.
> >
>
> I have pushed this patch with minor modifications (especially I didn't
> took Peter Smith's last suggestion to convert some functions to return
> enum inst
On Thu, Jan 23, 2025 at 11:18 AM Peter Smith wrote:
>
> Patch v56-0001 LGTM.
>
I have pushed this patch with minor modifications (especially I didn't
took Peter Smith's last suggestion to convert some functions to return
enum instead of char as the proposed one was consistent with
subscription si
Patch v56-0001 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jan 22, 2025 at 7:22 PM Peter Eisentraut wrote:
>
> On 21.01.25 09:27, vignesh C wrote:
> >> Maybe I have some fundamental misunderstanding here, but I don't see
> >> why "this approach cannot be used with psql because older version
> >> servers may not have these columns". Not having the
Hi Vignesh,
Patch v55-0002 (the docs chapter one) is a re-badged v54-0005.
I've addressed the PeterE review comments [1] as detailed below.:
FYI, because v55-0001 was broken I have posted this DOCS patch
separately. Please add it back into the patch set as vXX-0002 when you
next post it.
On Th
On Thu, Jan 23, 2025 at 1:00 AM vignesh C wrote:
>
> On Wed, 22 Jan 2025 at 16:22, Amit Kapila wrote:
> >
> > On Tue, Jan 21, 2025 at 1:58 PM vignesh C wrote:
> > >
> >
> > I have pushed the 0001 patch. I don't think 0002 and 0003 need to be
> > committed separately. So, combine them in 0004 and
Some review comments for patch v54-0004.
(since preparing this post, I saw you have already posted v55-0001 but
AFAIK, since 55-0001 is just a merge, the same review comments are
still applicable)
==
doc/src/sgml/catalogs.sgml
1.
- If true, this publication replicates the store
On 21.01.25 09:27, vignesh C wrote:
Maybe I have some fundamental misunderstanding here, but I don't see
why "this approach cannot be used with psql because older version
servers may not have these columns". Not having the columns is the
whole point of using an alias approach in the first place e
On Tue, Jan 21, 2025 at 1:58 PM vignesh C wrote:
>
I have pushed the 0001 patch. I don't think 0002 and 0003 need to be
committed separately. So, combine them in 0004 and post a new version.
--
With Regards,
Amit Kapila.
Patch v54-0002 LGTM, although it is missing an explanatory commit message.
e.g. Should be something like:
--
Adds the documentation for the 'pubgencols' column of catalog
pg_publication. This was accidentally missing from commit 7054186.
--
==
Kind Regards,
Peter Smith.
Fujitsu Austr
On Tue, Jan 21, 2025 at 7:28 PM vignesh C wrote:
>
> On Mon, 20 Jan 2025 at 06:14, Peter Smith wrote:
> >
> > Hi Vignesh,
> >
> > Review comments for patch v53-0001:
> >
> >
> > Maybe I have some fundamental misunderstanding here, but I don't see
> > why "this approach cannot be used with psql be
On Mon, 20 Jan 2025 at 08:59, Peter Smith wrote:
>
> IIUC, patch v53-0004 is primarily a bug fix for a docs omission of the
> master implementation.
>
> So,
>
> 1. IMO think this patch in its current form must come *before* the
> 0003 patch where you changed the PUBLICATION option from bool to enu
Hi Vignesh,
Thanks for including my patch in your patch set, ensuring that it does
not get left behind.
1.
These docs are mostly still OK, but have become slightly stale because:
a) they should mention "stored" in some places
b) the examples now need to be using the new enum form of the
publica
Hi Vignesh.
Here are my review comments for patch v53-0003.
On Sun, Jan 19, 2025 at 11:17 PM vignesh C wrote:
>
> On Fri, 17 Jan 2025 at 11:23, Peter Smith wrote:
> >
> > Hi Vignesh.
> >
> > Some review comments for patch v52-0003
> >
> > ==
> >
> > 1. GENERAL - change to use enum.
> >
> >
IIUC, patch v53-0004 is primarily a bug fix for a docs omission of the
master implementation.
So,
1. IMO think this patch in its current form must come *before* the
0003 patch where you changed the PUBLICATION option from bool to enum.
2. Then the patch (currently called) 0003 needs to update th
Hi Vignesh,
Review comments for patch v53-0001:
I don't doubt that the latest 0001 patch is working OK, but IMO it
didn't need to be this complicated. I still think just using an alias
for all the columns not present as I suggested yesterday [1] would be
far simpler than the current patch.
On S
On Sun, 19 Jan 2025 at 06:39, Peter Smith wrote:
>
> Hi Vignesh,
>
> I was having some second thoughts about this patch and my previous suggestion.
>
> Currently the code is current written something like:
>
> printfPQExpBuffer(&buf,
> "SELECT oid, pubname,\n"
> " pg_catalog.pg_get_userbyid(pub
Hi Vignesh,
I was having some second thoughts about this patch and my previous suggestion.
Currently the code is current written something like:
printfPQExpBuffer(&buf,
"SELECT oid, pubname,\n"
" pg_catalog.pg_get_userbyid(pubowner) AS owner,\n"
" puballtables, pubinsert, pubupdate, pubdele
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)
> > + {
> >
1 - 100 of 291 matches
Mail list logo
