On Wed, Aug 6, 2025 at 4:29 PM shveta malik wrote:
>
> On Wed, Aug 6, 2025 at 2:28 PM vignesh C wrote:
> >
> > The attached v20250806 version patch has the changes for the same.
> >
>
> Thank You for the patches. Please find a few comments:
>
> 1)
> * If 'resync_all_sequences' is false:
> *
On Wed, Aug 6, 2025 at 2:28 PM vignesh C wrote:
>
>
> The attached v20250806 version patch has the changes for the same.
>
Thank You for the patches.
patch-0005: sequencesync.c
+ aclresult = pg_class_aclcheck(RelationGetRelid(sequence_rel), GetUserId(),
+ ACL_UPDATE);
+ if (aclresult != ACLCHEC
On Wed, Aug 6, 2025 at 2:28 PM vignesh C wrote:
>
> The attached v20250806 version patch has the changes for the same.
>
Thank You for the patches. Please find a few comments:
1)
* If 'resync_all_sequences' is false:
* Add or remove tables and sequences that have been added to or removed
Dear Vignesh,
I played with your patch and found something.
01.
In LogicalRepSyncSequences() and GetSubscriptionRelations(), there is a
possibility
that the sequence on the subscriber could be dropped before opens that.
This can cause `could not open relation with OID %u` error, which is not
us
On Wed, Jul 30, 2025 at 11:16 AM shveta malik wrote:
>
> On Mon, Jul 28, 2025 at 3:37 PM vignesh C wrote:
>
> > Thanks for the comments, the attached v20250728 version patch has the
> > changes for the same.
> >
> Thanks for the patches, please find a few comments:
>
> 1)
> WARNING: WITH clause
On Mon, Jul 28, 2025 at 3:37 PM vignesh C wrote:
> Thanks for the comments, the attached v20250728 version patch has the
> changes for the same.
>
Thanks for the patches, please find a few comments:
1)
WARNING: WITH clause parameters do not affect sequence synchronization
a)
How about:
WITH cl
Dear Vignesh,
Here are remained comments for v20250723 0003-0005. I've not checked the latest
version.
01.
```
*PostgreSQL logical replication: common synchronization code
```
How about: "Common code for synchronizations"? Since this file locates in
replication/logical, initial part is
Dear Vignesh,
Thanks for working the project. Here are my comments only for 0001 and 0002.
Sorry if my points have already been discussed, this thread is too huge to
catchup for me :-(.
01.
Do we have to document the function and open to users? Previously it was not.
Another example is pg_get_pu
On Mon, Jul 21, 2025 at 2:55 PM Amit Kapila wrote:
>
> On Mon, Jul 21, 2025 at 2:36 PM vignesh C wrote:
> >
> > On Mon, 21 Jul 2025 at 11:15, Dilip Kumar wrote:
> > >
> >
> > > 3. Some of the syntaxes works for sequence which doesn't make sense to
> > > me, as listed below, I think there are mor
On Mon, Jul 21, 2025 at 2:36 PM vignesh C wrote:
>
> On Mon, 21 Jul 2025 at 11:15, Dilip Kumar wrote:
> >
> > On Mon, Jul 21, 2025 at 10:36 AM Dilip Kumar wrote:
> > >
> > > I was just trying a different test, so I realized that ALTER
> > > PUBLICATION ADD SEQUENCE is not supported, any reason f
On Mon, Jul 21, 2025 at 2:23 PM vignesh C wrote:
>
> On Mon, 21 Jul 2025 at 10:36, Dilip Kumar wrote:
> >
> > I was just trying a different test, so I realized that ALTER
> > PUBLICATION ADD SEQUENCE is not supported, any reason for the same?
> >
> > postgres[154731]=# ALTER PUBLICATION pub ADD s
On Mon, Jul 21, 2025 at 2:36 PM vignesh C wrote:
>
> On Mon, 21 Jul 2025 at 11:15, Dilip Kumar wrote:
> >
>
> > 3. Some of the syntaxes works for sequence which doesn't make sense to
> > me, as listed below, I think there are more
> >
> > postgres[154731]=# CREATE PUBLICATION insert_only FOR ALL
On Mon, 21 Jul 2025 at 11:15, Dilip Kumar wrote:
>
> On Mon, Jul 21, 2025 at 10:36 AM Dilip Kumar wrote:
> >
> > I was just trying a different test, so I realized that ALTER
> > PUBLICATION ADD SEQUENCE is not supported, any reason for the same?
> >
> > postgres[154731]=# ALTER PUBLICATION pub AD
On Mon, 21 Jul 2025 at 10:36, Dilip Kumar wrote:
>
> I was just trying a different test, so I realized that ALTER
> PUBLICATION ADD SEQUENCE is not supported, any reason for the same?
>
> postgres[154731]=# ALTER PUBLICATION pub ADD sequence s1;
> ERROR: 42601: invalid publication object list
> L
On Mon, Jul 21, 2025 at 11:15 AM Dilip Kumar wrote:
>
> 3. Some of the syntaxes works for sequence which doesn't make sense to
> me, as listed below, I think there are more
>
> postgres[154731]=# CREATE PUBLICATION insert_only FOR ALL SEQUENCES
> WITH (publish = 'insert');
> CREATE PUBLICATION
>
>
On Mon, Jul 21, 2025 at 10:36 AM Dilip Kumar wrote:
>
> I was just trying a different test, so I realized that ALTER
> PUBLICATION ADD SEQUENCE is not supported, any reason for the same?
>
> postgres[154731]=# ALTER PUBLICATION pub ADD sequence s1;
> ERROR: 42601: invalid publication object list
On Sun, Jul 20, 2025 at 7:48 PM vignesh C wrote:
>
> On Fri, 18 Jul 2025 at 14:11, Dilip Kumar wrote:
> >
> > On Fri, Jul 18, 2025 at 10:44 AM Dilip Kumar wrote:
> > >
> > > On Thu, Jul 17, 2025 at 4:52 PM vignesh C wrote:
> > > >
> >
> > I was looking at the high level idea of sequence sync wo
On Fri, 18 Jul 2025 at 14:11, Dilip Kumar wrote:
>
> On Fri, Jul 18, 2025 at 10:44 AM Dilip Kumar wrote:
> >
> > On Thu, Jul 17, 2025 at 4:52 PM vignesh C wrote:
> > >
>
> I was looking at the high level idea of sequence sync worker patch
> i.e. 0005, so far I haven't found anything problematic
On Thu, Jul 17, 2025 at 4:52 PM vignesh C wrote:
>
> The attached v20250717 version patch has the changes for the same.
>
Few comments on 0001 and 0002:
0001
1. Instead of introducing a new function, can we think of extending
the existing function pg_get_sequence_data()?
0002
2.
postgres=# Creat
On Fri, Jul 18, 2025 at 10:44 AM Dilip Kumar wrote:
>
> On Thu, Jul 17, 2025 at 4:52 PM vignesh C wrote:
> >
I was looking at the high level idea of sequence sync worker patch
i.e. 0005, so far I haven't found anything problematic there, but I
haven't completed the review and testing yet. Here
On Thu, Jul 17, 2025 at 4:52 PM vignesh C wrote:
>
> On Wed, 16 Jul 2025 at 11:15, Dilip Kumar wrote:
> >
> > On Mon, Jul 14, 2025 at 10:03 AM vignesh C wrote:
> > >
> > > On Fri, 11 Jul 2025 at 14:26, shveta malik wrote:
> > > >
> > I have picked this up again for final review, started with 00
On Mon, Jul 14, 2025 at 10:03 AM vignesh C wrote:
>
> On Fri, 11 Jul 2025 at 14:26, shveta malik wrote:
> >
I have picked this up again for final review, started with 0001, I
think mostly 0001 looks good to me, except few comments
1.
+ lsn = PageGetLSN(page);
+ last_value = seq->last_value;
+ lo
On Mon, Jul 14, 2025 at 10:03 AM vignesh C wrote:
>
> On Fri, 11 Jul 2025 at 14:26, shveta malik wrote:
> >
> > On Wed, Jul 9, 2025 at 4:11 PM vignesh C wrote:
> >
> > >
> > > > 3)
> > > > SyncFetchRelationStates:
> > > > Earlier the name was FetchTableStates. If we really want to use the
> > >
On Wed, Jul 9, 2025 at 4:11 PM vignesh C wrote:
>
> > 3)
> > SyncFetchRelationStates:
> > Earlier the name was FetchTableStates. If we really want to use the
> > 'Sync' keyword, we can name it FetchRelationSyncStates, as we are
> > fetching sync-status only. Thoughts?
>
> Instead of FetchRelatio
On Wed, Jul 9, 2025 at 4:11 PM vignesh C wrote:
>
> The attached v20250709 version patch has the changes for the same.
>
Thanks for the patches.
In Patch-004: sequencesync.c : I think below function logic can be simplified.
+void
+ProcessSyncingSequencesForApply(void)
+{
+ bool started_tx = fal
Please find a few more comments on July4 patch
6)
+
+ To synchronize sequences from a publisher to a subscriber, first publish
+ them using
+ CREATE PUBLICATION ... FOR ALL SEQUENCES.
+
This sentence looks odd, as we have 'first' but no follow-up sentence
after that. Can we please comb
On Mon, Jul 7, 2025 at 2:37 PM shveta malik wrote:
>
> On Fri, Jul 4, 2025 at 3:53 PM vignesh C wrote:
> >
> > On Tue, 1 Jul 2025 at 15:20, shveta malik wrote:
> > > On Mon, Jun 30, 2025 at 3:21 PM Nisha Moond
> > > wrote:
> > > >
> > > > Please find the attached v20250630 patch set addressing
On Fri, Jul 4, 2025 at 3:53 PM vignesh C wrote:
>
> On Tue, 1 Jul 2025 at 15:20, shveta malik wrote:
> > On Mon, Jun 30, 2025 at 3:21 PM Nisha Moond
> > wrote:
> > >
> > > Please find the attached v20250630 patch set addressing above comments
> > > and other comments in [1],[2],[3] and [4].
> >
Few more concerns:
4)
In UpdateSubscriptionRelState():
if (!HeapTupleIsValid(tup))
elog(ERROR, "subscription table %u in subscription %u
does not exist",
relid, subid);
table-->relation as now it can be hit for both sequence and table.
5)
In Logic
On Mon, Jun 30, 2025 at 3:21 PM Nisha Moond wrote:
>
> Please find the attached v20250630 patch set addressing above comments
> and other comments in [1],[2],[3] and [4].
Thanks for the patches. I am still in process of reviewing it but
please find few comments:
1)
+ if (pset.sversion >= 18)
On Mon, Jun 30, 2025 at 3:21 PM Nisha Moond wrote:
>
> Tab-completion is not supported after a comma (,) in any other cases.
> For example, the following commands are valid, but tab-completion does
> not work after the comma:
>
> CREATE PUBLICATION pub7 FOR TABLE t1, TABLES IN SCHEMA public;
> CRE
On Wed, Jun 25, 2025 at 3:10 PM Shlok Kyal wrote:
>
>
> 4. Since we are not adding sequences in the list 'sub_remove_rels',
> should we only palloc for (the count of no. of tables)? Is it worth
> the effort?
> /*
> * Rels that we want to remove from subscription and drop any slots
> * and origins
On Wed, Jun 25, 2025 at 9:26 AM shveta malik wrote:
>
> On Tue, Jun 24, 2025 at 6:44 PM Shlok Kyal wrote:
> >
> >
> > 1. Initially, I have created a publication on sequence s1.
> > postgres=# CREATE PUBLICATION pub1 FOR ALL SEQUENCES;
> > CREATE PUBLICATION
> > postgres=# ALTER PUBLICATION pub1 S
On Fri, Jun 27, 2025 at 8:50 AM shveta malik wrote:
>
> On Wed, Jun 25, 2025 at 7:42 PM Shlok Kyal wrote:
> > Hi Vignesh,
> >
> > I tested with all patches applied. I have a comment:
> >
> > Let consider following case:
> > On publisher create a publication pub1 on all sequence. publication
> > h
On Wed, Jun 25, 2025 at 7:42 PM Shlok Kyal wrote:
>
> On Sun, 22 Jun 2025 at 08:05, vignesh C wrote:
> >
> > On Thu, 19 Jun 2025 at 11:26, Nisha Moond wrote:
> > >
> > > Hi,
> > >
> > > Here are my review comments for v20250610 patches:
> > >
> > > Patch-0005:sequencesync.c
> > >
> > > 1) report
On Sun, 22 Jun 2025 at 08:05, vignesh C wrote:
>
> On Thu, 19 Jun 2025 at 11:26, Nisha Moond wrote:
> >
> > Hi,
> >
> > Here are my review comments for v20250610 patches:
> >
> > Patch-0005:sequencesync.c
> >
> > 1) report_error_sequences()
> >
> > In case there are both missing and mismatched se
On Sun, 22 Jun 2025 at 08:05, vignesh C wrote:
>
> On Thu, 19 Jun 2025 at 11:26, Nisha Moond wrote:
> >
> > Hi,
> >
> > Here are my review comments for v20250610 patches:
> >
> > Patch-0005:sequencesync.c
> >
> > 1) report_error_sequences()
> >
> > In case there are both missing and mismatched se
On Sun, 22 Jun 2025 at 08:05, vignesh C wrote:
>
> On Thu, 19 Jun 2025 at 11:26, Nisha Moond wrote:
> >
> > Hi,
> >
> > Here are my review comments for v20250610 patches:
> >
> > Patch-0005:sequencesync.c
> >
> > 1) report_error_sequences()
> >
> > In case there are both missing and mismatched se
On Tue, Jun 24, 2025 at 6:44 PM Shlok Kyal wrote:
>
>
> 1. Initially, I have created a publication on sequence s1.
> postgres=# CREATE PUBLICATION pub1 FOR ALL SEQUENCES;
> CREATE PUBLICATION
> postgres=# ALTER PUBLICATION pub1 SET TABLE t1;
> ALTER PUBLICATION
> postgres=# \d s1
>
On Tue, Jun 24, 2025 at 3:07 PM Nisha Moond wrote:
>
> On Sun, Jun 22, 2025 at 8:05 AM vignesh C wrote:
> >
> > Thanks for the comment, the attached v20250622 version patch has the
> > changes for the same.
> >
>
> Thanks for the patches, please find my review comments for patches 001 and
> 002:
On Sun, Jun 22, 2025 at 8:05 AM vignesh C wrote:
>
> Thanks for the comment, the attached v20250622 version patch has the
> changes for the same.
>
Thanks for the patches, please find my review comments for patches 001 and 002:
1) patch-001 :pg_sequence_state()
+ /* open and lock sequence */
+
>
> Thanks for the comment, the attached v20250622 version patch has the
> changes for the same.
>
Thanks for the patches, I am not done with review yet, but please find
the feedback so far:
1)
+ if (!OidIsValid(seq_relid))
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+
Hi,
Here are my review comments for v20250610 patches:
Patch-0005:sequencesync.c
1) report_error_sequences()
In case there are both missing and mismatched sequences, the ERROR
message logged is -
```
2025-05-28 14:22:19.898 IST [392259] ERROR: logical replication
sequence synchronization fail
On Thu, May 29, 2025 at 8:09 PM vignesh C wrote:
>
> These comments are handled in the attached v2025029 version patch.
>
1. The current syntax to publish sequences is:
CREATE PUBLICATION pub1 FOR ALL TABLES, ALL SEQUENCES;
The other alternative could be:
CREATE PUBLICATION pub1 FOR ALL TABLES,
On Thu, May 29, 2025 at 8:09 PM vignesh C wrote:
> These comments are handled in the attached v2025029 version patch.
>
Thanks for the patches. I am still reviewing but please find few comments:
1)
Only persistent sequences are included in the publication. Temporary
sequences a
On Fri, May 23, 2025 at 3:12 AM vignesh C wrote:
>
>
> The attached v20250522 patch has the changes for the same.
>
> Regards,
> Vignesh
>
Some review comments for patch 0001:
1. In src/backend/commands/sequence.c
in pg_sequence_state()
+ /* open and lock sequence */
+ init_sequence(seq_relid, &
On Thu, May 22, 2025 at 10:42 PM vignesh C wrote:
>
>
> The attached v20250522 patch has the changes for the same.
>
Thank you for the patches, please find comments for patch-0004.
1)
+/*
+ * report_error_sequences
+ *
+ * Logs a warning listing all sequences that are missing on the publisher,
+
On Tue, 20 May 2025 at 09:54, shveta malik wrote:
>
> On Tue, May 20, 2025 at 8:35 AM Nisha Moond wrote:
> >
> > >
> > > Thanks for the comments, these are handled in the attached v20250516
> > > version patch.
> > >
> >
> > Thanks for the patches. Here are my review comments -
> >
> > Patch-0004
On Tue, May 20, 2025 at 11:13 AM Peter Smith wrote:
>
> > Test-scenario:
> > --Created 250 sequences on both pub and sub.
> > --There were 10 sequences mismatched.
> > --Sequence replication worked as expected. Logs look better now:
> >
> > LOG: Logical replication sequence synchronization for su
On Tue, May 20, 2025 at 8:35 AM Nisha Moond wrote:
>
> >
> > Thanks for the comments, these are handled in the attached v20250516
> > version patch.
> >
>
> Thanks for the patches. Here are my review comments -
>
> Patch-0004: src/backend/replication/logical/sequencesync.c
>
Hi,
Currently, the b
> Test-scenario:
> --Created 250 sequences on both pub and sub.
> --There were 10 sequences mismatched.
> --Sequence replication worked as expected. Logs look better now:
>
> LOG: Logical replication sequence synchronization for subscription
> "sub1" - total unsynchronized: 250; batch #1 = 100 att
On Tue, May 20, 2025 at 8:35 AM Nisha Moond wrote:
>
> >
> > Thanks for the comments, these are handled in the attached v20250516
> > version patch.
> >
>
> Thanks for the patches. Here are my review comments -
>
> Patch-0004: src/backend/replication/logical/sequencesync.c
>
> The sequence count l
>
> Thanks for the comments, these are handled in the attached v20250516
> version patch.
>
Thanks for the patches. Here are my review comments -
Patch-0004: src/backend/replication/logical/sequencesync.c
The sequence count logic using curr_seq in copy_sequences() seems buggy.
Currently, curr_se
Hi Vignesh.
Some minor review comments for the patches in set v20250514.
==
Patch 0001.
1.1
For function 'pg_sequence_state', the DOCS call the 2nd parameter
'sequence_name', but the pg_proc.dat file calls it 'seq_name'. Should
these be made the same?
Patch 0004.
pg_
On Wed, 14 May 2025 at 09:55, Nisha Moond wrote:
>
> On Sat, May 3, 2025 at 7:28 PM vignesh C wrote:
> >
> >
> > There was one pending open comment #6 from [1]. This has been
> > addressed in the attached patch.
>
> Thank you for the patches, here are my comments for patch-004: sequencesync.c
>
>
On Sat, May 3, 2025 at 7:28 PM vignesh C wrote:
>
>
> There was one pending open comment #6 from [1]. This has been
> addressed in the attached patch.
Thank you for the patches, here are my comments for patch-004: sequencesync.c
copy_sequences()
---
1)
+ if (first)
+ first = fals
On Sat, May 3, 2025 at 7:27 PM vignesh C wrote:
>
> >
> > Thanks for the comments, the updated patch has the changes for the same.
>
Thanks for the patches. Please find few comments:
1)
patch004 commit msg:
- Drop published sequences are removed from pg_subscription_rel.
Drop -->Dropped
2)
c
Hi Vignesh.
Some trivial review comments for DOCS patch v20250428-0005.
==
doc/src/sgml/logical-replication.sgml
1.
+ Publications may currently only contain tables or sequences. Objects must be
+ added explicitly, except when a publication is created using
+ FOR TABLES IN SCHEMA, or F
Hi Vignesh.
Some review comments for v20250426-0005.
==
doc/src/sgml/catalogs.sgml
1.
- State code:
+ State code for tables:
i = initialize,
d = data is being copied,
f = finished table copy,
s = synchronized,
r = ready (normal repl
Hi Vignesh.
FYI, patch v20250424-0004 reported whitespace errors when applied.
[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v20250424-0004-Enhance-sequence-synchronization-during-su.patch
../patches_misc/v20250424-0004-Enhance-sequence-synchronization-during-su.patch:366:
t
page_lsn of the remote sequence at the
+ * moment it was synchronized.
+ *
SUGGESTION (minor rewording)
The page LSN will be used in logical replication of sequences to
record the LSN of the sequence page in the pg_subscription_rel system
catalog. It reflects the LSN of the remote sequence at the
Hi Vignesh,
Some review comments for patch v20250422-0003.
==
src/backend/replication/logical/syncutils.c
1.
+/*
+ * Exit routine for synchronization worker.
+ */
+pg_noreturn void
+SyncFinishWorker(void)
Why does this have the pg_noreturn annotation? None of the other void
functions do.
~
Hi Vignesh.
Review comments for patch v20250422-0001.
==
Commit message
1.
This patch introduces a new function: pg_sequence_state function
allows retrieval of sequence values including LSN.
SUGGESTION
This patch introduces a new function, 'pg_sequence_state', which
allows retrieval of sequ
Hi Vignesh,
No comments for patch v20250416-0001
No comments for patch v20250416-0002
No comments for patch v20250416-0003
Here are some comments for patch v20250416-0004
==
src/backend/catalog/system_views.sql
1.
+CREATE VIEW pg_publication_sequences AS
+SELECT
+P.pubname AS pu
Hi Vignesh,
Some review comments for patch v20250416-0005 (docs)
==
doc/src/sgml/catalogs.sgml
(52.55. pg_subscription_rel)
1.
State code:
i = initialize,
- d = data is being copied,
- f = finished table copy,
- s = synchronized,
+ d = data is
Hi Vignesh,
Some review comments for patch v20250325-0005 (docs).
==
doc/src/sgml/catalogs.sgml
(52.55. pg_subscription_rel)
1.
State code: i = initialize, d = data is being copied, f = finished
table copy, s = synchronized, r = ready (normal replication)
~
This is not part of the patch,
Hi Vignesh,
Some review comments for v20250525-0004.
==
Commit message
1.
A new sequencesync worker is launched as needed to synchronize sequences.
It does the following:
a) Retrieves remote values of sequences with pg_sequence_state() INIT.
b) Log a warning if the sequence parameter
Hi Vignesh,
FYI, the patch v20250325-0004 failed to apply (atop 0001,0002,0002)
due to recent master changes.
Checking patch src/backend/commands/sequence.c...
error: while searching for:
(long long) minv, (long long) maxv)));
/* Set the currval() state only if iscalled = true */
if (iscalled)
{
Hi Vignesh,
Some review comments for patch v20250325-0002
==
Commit message
1.
Furthermore, enhancements to psql commands (\d and \dRp) now allow for better
display of publications containing specific sequences or sequences included
in a publication.
~
That doesn't seem as clear as it migh
Hi Vignesh,
Here are some review comments for patch v20250325-0001.
==
src/test/regress/expected/sequence.out
1.
+SELECT last_value, log_cnt, is_called FROM
pg_sequence_state('sequence_test');
+ last_value | log_cnt | is_called
++-+---
+ 99 | 32 | t
On Wed, 12 Mar 2025 at 09:14, vignesh C wrote:
>
> The patch was not applying on top of HEAD because of recent commits,
> here is a rebased version.
I have moved this to the next CommitFest since it will not be
committed in the current release. This also allows reviewers to focus
on the remaining
Hi Vignesh,
FYI, looks like your attached patchset was misnamed 20250204 instead
of 20250104. Anyway, it does not affect the reviews, but I am going to
refer to it as 0104 from now.
~~~
I have no comments for the patch v20250104-0001.
Some comments for the patch v20250104-0002.
==
doc/src/
On Fri, 3 Jan 2025 at 09:07, Peter Smith wrote:
>
> Hi Vignesh,
>
> Some minor review comments for the patch v20241230-0003.
>
> ==
> src/backend/replication/logical/syncutils.c
>
> 1.
> + * syncutils.c
> + * PostgreSQL logical replication: common synchronization code
> + *
> + * Copyright (
Hi Vignesh,
Some minor review comments for the patch v20241230-0003.
==
src/backend/replication/logical/syncutils.c
1.
+ * syncutils.c
+ * PostgreSQL logical replication: common synchronization code
+ *
+ * Copyright (c) 2024, PostgreSQL Global Development Group
Happy New Year.
s/2024/20
Hi Vignesh.
Here are some review comments for patch v20241230-0002
==
1. SYNTAX
The proposed syntax is currently:
CREATE PUBLICATION name
[ FOR ALL object_type [, ...]
| FOR publication_object [, ... ] ]
[ WITH ( publication_parameter [= value] [, ... ] ) ]
where object_type
Dear Vignesh,
Thanks for updating the patch! Here are my comments.
01. SyncingRelationsState
```
* SYNC_RELATIONS_STATE_NEEDS_REBUILD The subscription relations state is no
* longer valid and the subscription relations should be rebuilt.
```
Can we follow the style like other lines? Like:
SY
On Fri, 20 Dec 2024 at 08:05, Peter Smith wrote:
>
> Hi Vignesh.
>
> Here are some review comments for the patch v20241211-0005.
>
> ==
>
> Section "29.6.3. Examples"
>
> 2.
> Should the Examples section also have an example of ALTER SUBSCRIPTION
> ... REFRESH PUBLICATION to demonstrate (like
On Thu, 19 Dec 2024 at 04:58, Peter Smith wrote:
>
> Hi Vignesh,
>
> Here are some review comments for the patch v20241211-0003.
>
> ~~~
>
> 4.
> +/*
> + * Common code to fetch the up-to-date sync state info into the static lists.
> + *
> + * Returns true if subscription has 1 or more tables, else
Hi Vignesh.
Here are some review comments for the patch v20241211-0005.
==
doc/src/sgml/logical-replication.sgml
Section "29.6.1. Sequence Definition Mismatches"
1.
+
+
+ If there are differences in sequence definitions between the publisher and
+ subscriber, a WARNING is log
Hi Vignesh,
Here are some review comments for the patch v20241211-0004.
==
GENERAL
1.
There are more than a dozen places where the relation (relkind) is
checked to see if it is a SEQUENCE:
e.g. + get_rel_relkind(subrel->srrelid) != RELKIND_SEQUENCE &&
e.g. + if (get_rel_relkind(subrel->srre
Hi Vignesh,
Here are some review comments for the patch v20241211-0003.
==
src/backend/replication/logical/syncutils.c
1.
+typedef enum
+{
+ SYNC_RELATIONS_STATE_NEEDS_REBUILD,
+ SYNC_RELATIONS_STATE_REBUILD_STARTED,
+ SYNC_RELATIONS_STATE_VALID,
+} SyncingRelationsState;
+
Even though the
Hi Vignesh,
Here are some review comments for the patch v20241211-0002.
==
doc/src/sgml/ref/create_publication.sgml
1.
+where object
type is one of:
+
+TABLES
+SEQUENCES
The replaceable "object_type" is missing an underscore.
~~~
publish option effect fro SEQUENCE replication?
2.
Hi Vignesh.
Here are some review comments for patch v20241211-0001.
==
src/backend/commands/sequence.c
pg_sequence_state:
1.
+ TupleDesc tupdesc;
+ HeapTuple tuple;
+ Datum values[4];
+ bool nulls[4] = {false, false, false, false};
SUGGESTION
bool nulls[4] = {0};
The above achieves the s
On Tue, Oct 8, 2024 at 2:46 AM vignesh C wrote:
>
> On Fri, 4 Oct 2024 at 15:39, shveta malik wrote:
> >
> > On Sun, Sep 29, 2024 at 12:34 PM vignesh C wrote:
> > >
> > > On Thu, 26 Sept 2024 at 11:07, shveta malik
> > > wrote:
> > > >
> > > > On Fri, Sep 20, 2024 at 9:36 AM vignesh C wrote:
On Sun, Sep 29, 2024 at 12:34 PM vignesh C wrote:
>
> On Thu, 26 Sept 2024 at 11:07, shveta malik wrote:
> >
> > On Fri, Sep 20, 2024 at 9:36 AM vignesh C wrote:
> > >
> > > On Wed, 21 Aug 2024 at 11:54, vignesh C wrote:
> > > >
> > > > On Wed, 21 Aug 2024 at 08:33, Peter Smith wrote:
> > > >
On Thu, 26 Sept 2024 at 11:07, shveta malik wrote:
>
> On Fri, Sep 20, 2024 at 9:36 AM vignesh C wrote:
> >
> > On Wed, 21 Aug 2024 at 11:54, vignesh C wrote:
> > >
> > > On Wed, 21 Aug 2024 at 08:33, Peter Smith wrote:
> > > >
> > > > Hi Vignesh, Here are my only review comments for the latest
On Fri, Sep 20, 2024 at 9:36 AM vignesh C wrote:
>
> On Wed, 21 Aug 2024 at 11:54, vignesh C wrote:
> >
> > On Wed, 21 Aug 2024 at 08:33, Peter Smith wrote:
> > >
> > > Hi Vignesh, Here are my only review comments for the latest patch set.
> >
> > Thanks, these issues have been addressed in the
Hi Vignesh, Here are my only review comments for the latest patch set.
v20240820-0003.
nit - missing period for comment in FetchRelationStates
nit - typo in function name 'ProcessSyncingTablesFoSync'
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical
Hi Vignesh, Here are my review comments for the latest patchset
v20240819-0001. No changes. No comments.
v20240819-0002. No changes. No comments.
v20240819-0003. See below.
v20240819-0004. See below.
v20240819-0005. No changes. No comments.
///
PATCH v20240819-0003
==
sr
Here are my review comments for the latest patchset
v20240817-0001. No changes. No comments.
v20240817-0002. No changes. No comments.
v20240817-0003. See below.
v20240817-0004. See below.
v20240817-0005. No changes. No comments.
//
v20240817-0003 and 0004.
(This is a repeat of the same comm
Hi Vignesh. I looked at the latest v20240815* patch set.
I have only the following few comments for patch v20240815-0004, below.
==
Commit message.
Please see the attachment for some suggested updates.
==
src/backend/commands/subscriptioncmds.c
CreateSubscription:
nit - fix wording in
Hi Vignesh, I have reviewed your latest patchset:
v20240814-0001. No comments
v20240814-0002. No comments
v20240814-0003. No comments
v20240814-0004. See below
v20240814-0005. No comments
//
v20240814-0004.
==
src/backend/commands/subscriptioncmds.c
CreateSubscription:
nit - XXX commen
Hi Vignesh, Here are my review comments for the latest patchset:
Patch v20240813-0001. No comments
Patch v20240813-0002. No comments
Patch v20240813-0003. No comments
Patch v20240813-0004. See below
Patch v20240813-0005. No comments
//
Patch v20240813-0004
==
src/backend/catalog/pg_subs
On Tue, Aug 13, 2024 at 10:00 PM vignesh C wrote:
>
> On Tue, 13 Aug 2024 at 09:19, Peter Smith wrote:
> >
> > 3.1. GENERAL
> >
> > Hmm. I am guessing this was provided as a separate patch to aid review
> > by showing that existing functions are moved? OTOH you can't really
> > judge this patch p
On Tue, 13 Aug 2024 at 12:31, Peter Smith wrote:
>
> OBSERVATION #2
>
> When 1000s of sequences are refreshed (set to INIT) then there are
> 1000s of logs like below:
>
> ...
> 2024-08-13 16:13:57.873 AEST [10301] LOG: sequence "public.seq_0698"
> of subscription "sub3" set to INIT state
> 2024-0
Hi Vignesh,
I have been using the latest patchset, trying a few things using many
(1000) sequences.
Here are some observations, plus some suggestions for consideration.
~
OBSERVATION #1
When 1000s of sequences are refreshed using REFRESH PUBLICATION
SEQUENCES the logging is excessive. For
On Mon, Aug 12, 2024 at 11:07 PM vignesh C wrote:
>
> On Mon, 12 Aug 2024 at 10:40, Peter Smith wrote:
> >
> > Hi Vignesh,
> >
> > I found that when 2 subscriptions are both subscribing to a
> > publication publishing sequences, an ERROR occurs on refresh.
> >
> > ==
> >
> > Publisher:
> > --
Hi Vignesh, Here are my review comments for latest v20240812* patchset:
patch v20240812-0001. No comments.
patch v20240812-0002. Fixed docs.LGTM
patch v20240812-0003. This is new refactoring. See below.
patch v20240812-0004. (was 0003). See below.
patch v20240812-0005. (was 0004). No comments.
//
On Mon, 12 Aug 2024 at 09:59, Peter Smith wrote:
>
> Hi Vignesh,
>
> I noticed it is not currently possible (there is no syntax way to do
> it) to ALTER an existing publication so that it will publish
> SEQUENCES.
>
> Isn't that a limitation? Why?
>
> For example,. Why should users be prevented fr
On Mon, 12 Aug 2024 at 10:40, Peter Smith wrote:
>
> Hi Vignesh,
>
> I found that when 2 subscriptions are both subscribing to a
> publication publishing sequences, an ERROR occurs on refresh.
>
> ==
>
> Publisher:
> --
>
> test_pub=# create publication pub1 for all sequences;
>
> Subs
1 - 100 of 224 matches
Mail list logo
