Dear Vignesh,
Thanks for updating the patch. Few comments:
01.
```
/* Find the leader apply worker and signal it. */
logicalrep_worker_wakeup(MyLogicalRepWorker->subid, InvalidOid);
```
Sequencesync worker does not need to send a signal to the apply worker.
Should we skip in the c
Dear Vignesh,
Thanks for updating the patch. Below are my comments.
01.
```
/* Relation is either a sequence or a table */
relkind = get_rel_relkind(subrel->srrelid);
if (relkind != RELKIND_SEQUENCE)
continue;
```
Can you up
Dear Shveta,
> Say we have triggered REFRESH on sub, and when seq-sync worker is in
> copy_sequences() where it has retrieved the local sequence using
> seqname while it has not locked the sequence-relation yet, if
> meanwhile we alter sequence and change its name, seq-sync worker ends
> up syncin
On Thu, Aug 21, 2025 at 10:08 PM vignesh C wrote:
>
> I have also addressed all the comments from [1] in the attached
> v20250823 version patch.
> [1] -
> https://www.postgresql.org/message-id/CAA4eK1%2BoVQW8oP%3DLo1X8qac6dzg-fgGQ6R_F_psfokUEqe%2Ba6w%40mail.gmail.com
>
Thank You for the patches
On Thu, Aug 21, 2025 at 10:52 PM Masahiko Sawada wrote:
>
> On Wed, Aug 20, 2025 at 9:04 PM Amit Kapila wrote:
> >
> > On Wed, Aug 20, 2025 at 11:00 PM Masahiko Sawada
> > wrote:
> > >
> > > On Tue, Aug 19, 2025 at 9:14 PM Amit Kapila
> > > wrote:
> > > >
> > > > If so, I don't think we can d
On Wed, Aug 20, 2025 at 9:04 PM Amit Kapila wrote:
>
> On Wed, Aug 20, 2025 at 11:00 PM Masahiko Sawada
> wrote:
> >
> > On Tue, Aug 19, 2025 at 9:14 PM Amit Kapila wrote:
> > >
> > > If so, I don't think we can do much with the design
> > > choice we made. During DDL replication of sequences,
On Wed, Aug 20, 2025 at 2:25 PM vignesh C wrote:
>
> On Tue, 19 Aug 2025 at 23:33, Masahiko Sawada wrote:
> >
> > I imagined something like case 2. For logical replication of tables,
> > if we support DDL replication (i.e., CREATE/ALTER/DROP TABLE), all
> > changes the apply worker executes are s
On Wed, Aug 20, 2025 at 11:00 PM Masahiko Sawada wrote:
>
> On Tue, Aug 19, 2025 at 9:14 PM Amit Kapila wrote:
> >
> > If so, I don't think we can do much with the design
> > choice we made. During DDL replication of sequences, we need to
> > consider it as a conflict.
> >
> > BTW, note that the
On Tue, Aug 19, 2025 at 9:14 PM Amit Kapila wrote:
>
> On Tue, Aug 19, 2025 at 11:33 PM Masahiko Sawada
> wrote:
> >
> > On Tue, Aug 19, 2025 at 1:44 AM vignesh C wrote:
> > >
> > >
> > > Case 2: Sequence value Conflict While Applying DDL Changes(Future patch)
> > >
> > > Example:
> > > -- Publ
On Mon, Aug 18, 2025 at 3:36 PM vignesh C wrote:
>
> Thanks for the comments, the updated version has the changes for the same.
>
I wanted to first discuss a few design points. The patch implements
"ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES" such that it
copies the existing sequences v
On Tue, 19 Aug 2025 at 23:33, Masahiko Sawada wrote:
>
> I imagined something like case 2. For logical replication of tables,
> if we support DDL replication (i.e., CREATE/ALTER/DROP TABLE), all
> changes the apply worker executes are serialized in commit LSN order.
> Therefore, users would not ha
On Tue, Aug 19, 2025 at 11:33 PM Masahiko Sawada wrote:
>
> On Tue, Aug 19, 2025 at 1:44 AM vignesh C wrote:
> >
> >
> > Case 2: Sequence value Conflict While Applying DDL Changes(Future patch)
> >
> > Example:
> > -- Publisher
> > CREATE SEQUENCE s1 MINVALUE 10 MAXVALUE 20;
> > SELECT nextval('s
On Tue, Aug 19, 2025 at 1:44 AM vignesh C wrote:
>
> On Tue, 19 Aug 2025 at 06:47, Masahiko Sawada wrote:
> >
> > On Mon, Aug 18, 2025 at 4:21 PM Masahiko Sawada
> > wrote:
> > >
> > > For example, if a sequence is dropped
> > > on the publisher, the subscriber would encounter synchronization
>
On Tue, 19 Aug 2025 at 06:47, Masahiko Sawada wrote:
>
> On Mon, Aug 18, 2025 at 4:21 PM Masahiko Sawada wrote:
> >
> > For example, if a sequence is dropped
> > on the publisher, the subscriber would encounter synchronization
> > failures unless the DROP SEQUENCE is properly applied.
>
> This ex
On Mon, Aug 18, 2025 at 4:21 PM Masahiko Sawada wrote:
>
> For example, if a sequence is dropped
> on the publisher, the subscriber would encounter synchronization
> failures unless the DROP SEQUENCE is properly applied.
This example is wrong. It seems DROP SEQUENCE works but we might have
proble
On Mon, Aug 18, 2025 at 2:13 AM vignesh C wrote:
>
> On Sat, 16 Aug 2025 at 14:15, Masahiko Sawada wrote:
> >
> > As I understand it, the logical replication of sequences implemented
> > by these patches shares the same user interface as table replication
> > (ut
On Sat, 16 Aug 2025 at 14:15, Masahiko Sawada wrote:
>
> As I understand it, the logical replication of sequences implemented
> by these patches shares the same user interface as table replication
> (utilizing CREATE PUBLICATION and CREATE SUBSCRIPTION commands for
> configuration
llPublicationRelations,
> >GetPublicationRelationsForAll,
> >GetPublicationRelationsForAllTablesSequences
>
> Modified it to GetAllPublicationRelations
>
> Please find my response for the comments from [2]:
> >patch-0005: sequencesync.c
> >+ aclresult = pg_class_a
Dear Vignesh,
Thanks for updating the patch. Here are my small comments:
01.
Per pgindent report, publicationcmds.c should be fixed.
02.
```
+ ScanKeyInit(&skey[1],
+ Anum_pg_subscription_rel_srsubstate,
+ BTEqualStrategyNumber, F
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
1 - 100 of 243 matches
Mail list logo
