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
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;
Subscriber:
---
test_sub=# create subscription sub1 connection 'dbna
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 from changing a FOR ALL
TABLES publication into a FOR ALL TABLES, SEQ
Hi Vignesh,
v20240809-0001. No comments.
v20240809-0002. See below.
v20240809-0003. See below.
v20240809-0004. No comments.
//
Here are my review comments for patch v20240809-0002.
nit - Tweak wording in new docs example, because a publication only
publishes the sequences; it doesn't "s
On Fri, 9 Aug 2024 at 12:40, Peter Smith wrote:
>
> Hi Vignesh, here are my review comments for the sequences docs patch
> v20240808-0004.
>
> ==
> doc/src/sgml/logical-replication.sgml
>
> The new section content looked good.
>
> Just some nitpicks including:
> - renamed the section "Replicat
On Fri, 9 Aug 2024 at 05:51, Peter Smith wrote:
>
> Hi Vignesh, I reviewed the latest v20240808-0003 patch.
>
> Attached are my minor change suggestions.
Thanks, these changes are merged in the v20240809 version posted at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm0LJCtGoBCO6DFY-RDj
On Fri, 9 Aug 2024 at 12:13, shveta malik wrote:
>
> On Wed, Aug 7, 2024 at 2:00 PM vignesh C wrote:
> >
> > On Wed, 7 Aug 2024 at 08:09, Amit Kapila wrote:
> > >
> > > On Tue, Aug 6, 2024 at 5:13 PM vignesh C wrote:
> > > >
> > > > On Mon, 5 Aug 2024 at 18:05, shveta malik
> > > > wrote:
> >
Hi Vignesh, here are my review comments for the sequences docs patch
v20240808-0004.
==
doc/src/sgml/logical-replication.sgml
The new section content looked good.
Just some nitpicks including:
- renamed the section "Replicating Sequences"
- added missing mention about how to publish sequence
On Wed, Aug 7, 2024 at 2:00 PM vignesh C wrote:
>
> On Wed, 7 Aug 2024 at 08:09, Amit Kapila wrote:
> >
> > On Tue, Aug 6, 2024 at 5:13 PM vignesh C wrote:
> > >
> > > On Mon, 5 Aug 2024 at 18:05, shveta malik wrote:
> > > >
> > > > On Mon, Aug 5, 2024 at 11:04 AM vignesh C wrote:
> > > > >
>
Hi Vignesh, I reviewed the latest v20240808-0003 patch.
Attached are my minor change suggestions.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/catalog/pg_subscription.c
b/src/backend/catalog/pg_subscription.c
index 4e2f960..a77e810 100644
--- a/src/backend/catalog
On Wed, 7 Aug 2024 at 10:27, shveta malik wrote:
>
> On Mon, Aug 5, 2024 at 10:26 AM vignesh C wrote:
> >
> > On Thu, 1 Aug 2024 at 04:25, Peter Smith wrote:
> > >
> > > Hi Vignesh,
> > >
> > > I noticed that when replicating sequences (using the latest patches
> > > 0730_2*) the subscriber-sid
On Thu, Aug 8, 2024 at 11:09 AM Peter Smith wrote:
>
> But, I haven't invented a new overloading for "copy_data" option
> (meaning "synchronize") for sequences. The current patchset already
> interprets copy_data exactly this way.
>
> For example, below are patch 0003 results:
>
> ALTER SUBSCRIPTI
On Wed, Aug 7, 2024 at 1:45 PM vignesh C wrote:
>
>
> The remaining comments have been addressed, and the changes are
> included in the attached v20240807 version patch.
Thanks for addressing the comment. Please find few comments for v20240807 :
patch002:
1)
create_publication.sgml:
--I think i
On Thu, Aug 8, 2024 at 1:55 PM Amit Kapila wrote:
>
> On Wed, Aug 7, 2024 at 10:12 AM Peter Smith wrote:
> >
> > This is mostly a repeat of my previous mail from a while ago [1] but
> > includes some corrections, answers, and more examples. I'm going to
> > try to persuade one last time because t
On Wed, Aug 7, 2024 at 10:12 AM Peter Smith wrote:
>
> This is mostly a repeat of my previous mail from a while ago [1] but
> includes some corrections, answers, and more examples. I'm going to
> try to persuade one last time because the current patch is becoming
> stable, so I wanted to revisit t
Hi Vignesh, Here are my v20240807-0003 review comments.
==
1. GENERAL DOCS.
IMO the replication of SEQUENCES is a big enough topic that it
deserves to have its own section in the docs chapter 31 [1].
Some of the create/alter subscription docs content would stay where it
is in, but a new chap
On Wed, 7 Aug 2024 at 08:09, Amit Kapila wrote:
>
> On Tue, Aug 6, 2024 at 5:13 PM vignesh C wrote:
> >
> > On Mon, 5 Aug 2024 at 18:05, shveta malik wrote:
> > >
> > > On Mon, Aug 5, 2024 at 11:04 AM vignesh C wrote:
> > > >
> > > > On Wed, 31 Jul 2024 at 14:39, shveta malik
> > > > wrote:
>
On Mon, 5 Aug 2024 at 17:28, Amit Kapila wrote:
>
> On Mon, Aug 5, 2024 at 2:36 PM vignesh C wrote:
> >
> > On Fri, 2 Aug 2024 at 14:24, shveta malik wrote:
> > >
> > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik
> > > wrote:
> > > >
> > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote:
>
On Tue, 6 Aug 2024 at 09:28, shveta malik wrote:
>
> On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote:
> >
>
> Do we need some kind of coordination between table sync and sequence
> sync for internally generated sequences? Lets say we have an identity
> column with a 'GENERATED ALWAYS' sequence.
On Mon, Aug 5, 2024 at 10:26 AM vignesh C wrote:
>
> On Thu, 1 Aug 2024 at 04:25, Peter Smith wrote:
> >
> > Hi Vignesh,
> >
> > I noticed that when replicating sequences (using the latest patches
> > 0730_2*) the subscriber-side checks the *existence* of the sequence,
> > but apparently it is n
Hi Vignesh,
This is mostly a repeat of my previous mail from a while ago [1] but
includes some corrections, answers, and more examples. I'm going to
try to persuade one last time because the current patch is becoming
stable, so I wanted to revisit this syntax proposal before it gets too
late to ch
On Tue, Aug 6, 2024 at 5:13 PM vignesh C wrote:
>
> On Mon, 5 Aug 2024 at 18:05, shveta malik wrote:
> >
> > On Mon, Aug 5, 2024 at 11:04 AM vignesh C wrote:
> > >
> > > On Wed, 31 Jul 2024 at 14:39, shveta malik wrote:
> > > >
> > > > On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
> > > > >
On Mon, 5 Aug 2024 at 18:05, shveta malik wrote:
>
> On Mon, Aug 5, 2024 at 11:04 AM vignesh C wrote:
> >
> > On Wed, 31 Jul 2024 at 14:39, shveta malik wrote:
> > >
> > > On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
> > > >
> > > > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote:
> > > > >
>
Here are some review comments for the patch v20240805_2-0003.
==
doc/src/sgml/catalogs.sgml
nitpick - removed the word "either"
==
doc/src/sgml/ref/alter_subscription.sgml
I felt the discussions about "how to handle warnings" are a bit scattered:
e.g.1 - ALTER SUBSCRIPTION REFRESH PUBLI
On Tue, 6 Aug 2024 at 10:24, shveta malik wrote:
>
> On Tue, Aug 6, 2024 at 9:54 AM Amit Kapila wrote:
> >
> > On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote:
> > >
> > > > > > I was reviewing the design of patch003, and I have a query. Do we
> > > > > > need
> > > > > > to even start an app
On Tue, Aug 6, 2024 at 9:54 AM Amit Kapila wrote:
>
> On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote:
> >
> > > > > I was reviewing the design of patch003, and I have a query. Do we need
> > > > > to even start an apply worker and create replication slot when
> > > > > subscription created is
On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote:
>
> On Mon, Aug 5, 2024 at 5:28 PM Amit Kapila wrote:
> >
> > On Mon, Aug 5, 2024 at 2:36 PM vignesh C wrote:
> > >
> > > On Fri, 2 Aug 2024 at 14:24, shveta malik wrote:
> > > >
> > > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik
> > > > wro
On Tue, Aug 6, 2024 at 8:49 AM shveta malik wrote:
>
Do we need some kind of coordination between table sync and sequence
sync for internally generated sequences? Lets say we have an identity
column with a 'GENERATED ALWAYS' sequence. When the sequence is synced
to subscriber, subscriber can also
On Mon, Aug 5, 2024 at 5:28 PM Amit Kapila wrote:
>
> On Mon, Aug 5, 2024 at 2:36 PM vignesh C wrote:
> >
> > On Fri, 2 Aug 2024 at 14:24, shveta malik wrote:
> > >
> > > On Thu, Aug 1, 2024 at 9:26 AM shveta malik
> > > wrote:
> > > >
> > > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote:
On Mon, Aug 5, 2024 at 11:04 AM vignesh C wrote:
>
> On Wed, 31 Jul 2024 at 14:39, shveta malik wrote:
> >
> > On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
> > >
> > > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote:
> > > >
> > > >
> > > >
> > > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wro
On Mon, Aug 5, 2024 at 2:36 PM vignesh C wrote:
>
> On Fri, 2 Aug 2024 at 14:24, shveta malik wrote:
> >
> > On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote:
> > >
> > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote:
> > > >
> > > > Thanks for reporting this, these issues are fixed in the a
On Fri, 2 Aug 2024 at 14:33, shveta malik wrote:
>
> On Fri, Aug 2, 2024 at 2:24 PM shveta malik wrote:
> >
> > On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote:
> > >
> > > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote:
> > > >
> > > > Thanks for reporting this, these issues are fixed in th
On Fri, 2 Aug 2024 at 14:24, shveta malik wrote:
>
> On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote:
> >
> > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote:
> > >
> > > Thanks for reporting this, these issues are fixed in the attached
> > > v20240730_2 version patch.
> > >
>
> I was reviewin
On Wed, 31 Jul 2024 at 14:39, shveta malik wrote:
>
> On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
> >
> > On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote:
> > >
> > >
> > >
> > > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote:
> > >>
> > >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote:
> >
On Thu, 1 Aug 2024 at 04:25, Peter Smith wrote:
>
> Hi Vignesh,
>
> I noticed that when replicating sequences (using the latest patches
> 0730_2*) the subscriber-side checks the *existence* of the sequence,
> but apparently it is not checking other sequence attributes.
>
> For example, consider:
On Thu, 1 Aug 2024 at 03:33, Peter Smith wrote:
>
> Hi Vignesh,
>
> I have a question about the subscriber-side behaviour of currval().
>
> ==
>
> AFAIK it is normal for currval() to give error is nextval() has not
> yet been called [1]
>
> For example.
> test_pub=# create sequence s1;
> CREAT
On Fri, Aug 2, 2024 at 2:24 PM shveta malik wrote:
>
> On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote:
> >
> > On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote:
> > >
> > > Thanks for reporting this, these issues are fixed in the attached
> > > v20240730_2 version patch.
> > >
>
> I was review
On Thu, Aug 1, 2024 at 9:26 AM shveta malik wrote:
>
> On Mon, Jul 29, 2024 at 4:17 PM vignesh C wrote:
> >
> > Thanks for reporting this, these issues are fixed in the attached
> > v20240730_2 version patch.
> >
I was reviewing the design of patch003, and I have a query. Do we need
to even star
Hi Vignesh,
I noticed that when replicating sequences (using the latest patches
0730_2*) the subscriber-side checks the *existence* of the sequence,
but apparently it is not checking other sequence attributes.
For example, consider:
Publisher: "CREATE SEQUENCE s1 START 1 INCREMENT 2;" should be
Hi Vignesh,
I have a question about the subscriber-side behaviour of currval().
==
AFAIK it is normal for currval() to give error is nextval() has not
yet been called [1]
For example.
test_pub=# create sequence s1;
CREATE SEQUENCE
test_pub=# select * from currval('s1');
2024-08-01 07:42:48.
On Sat, 20 Jul 2024 at 20:48, vignesh C wrote:
>
> On Fri, 12 Jul 2024 at 08:22, Peter Smith wrote:
> >
> > Hi Vignesh. Here are the rest of my comments for patch v20240705-0003.
> > ==
> >
> > 8. logicalrep_sequence_sync_worker_find
> >
> > +/*
> > + * Walks the workers array and searches fo
On Mon, Jun 10, 2024 at 5:00 PM vignesh C wrote:
>
> On Mon, 10 Jun 2024 at 12:24, Amul Sul wrote:
> >
> >
> >
> > On Sat, Jun 8, 2024 at 6:43 PM vignesh C wrote:
> >>
> >> On Wed, 5 Jun 2024 at 14:11, Amit Kapila wrote:
> >> [...]
> >> A new catalog table, pg_subscription_seq, has been introdu
Hi Vignesh,
Here are my review comments for your latest 0730_2* patches.
Patch v20240730_2-0001 looks good to me.
Patch v20240730_2-0002 looks good to me.
My comments for the v20240730_2-0003 patch are below:
//
GENERAL
1. Inconsistent terms
I've noticed there are many variations of
On Thu, 25 Jul 2024 at 15:41, shveta malik wrote:
>
> On Thu, Jul 25, 2024 at 12:08 PM shveta malik wrote:
> >
> > On Thu, Jul 25, 2024 at 9:06 AM vignesh C wrote:
> > >
> > > The attached v20240725 version patch has the changes for the same.
> >
> > Thank You for addressing the comments. Please
On Fri, 26 Jul 2024 at 11:46, Peter Smith wrote:
>
> Hi Vignesh,
>
> There are still pending changes from my previous review of the
> 0720-0003 patch [1], but here are some new review comments for your
> latest patch v20240525-0003.
> 2b.
> Is it better to name these returned by-ref ptrs like 'ret
On Fri, 26 Jul 2024 at 08:04, Peter Smith wrote:
>
> Here are some review comments for latest patch v20240725-0002
>
> ==
> doc/src/sgml/ref/create_publication.sgml
>
> nitpick - tweak to the description of the example.
>
> ==
> src/backend/parser/gram.y
>
> preprocess_pub_all_objtype_list
Hi Vignesh,
There are still pending changes from my previous review of the
0720-0003 patch [1], but here are some new review comments for your
latest patch v20240525-0003.
==
doc/src/sgml/catalogs.sgml
nitpick - fix plurals and tweak the description.
~~~
1.
- This catalog only contai
Here are some review comments for latest patch v20240725-0002
==
doc/src/sgml/ref/create_publication.sgml
nitpick - tweak to the description of the example.
==
src/backend/parser/gram.y
preprocess_pub_all_objtype_list:
nitpick - typo "allbjects_list"
nitpick - reword function header
nit
On Thu, Jul 25, 2024 at 12:08 PM shveta malik wrote:
>
> On Thu, Jul 25, 2024 at 9:06 AM vignesh C wrote:
> >
> > The attached v20240725 version patch has the changes for the same.
>
> Thank You for addressing the comments. Please review below issues:
>
> 1) Sub ahead of pub due to wrong initial
Hi, here are more review comments for patch v20240720-0003.
==
src/backend/catalog/pg_subscription.c
(Numbers are starting at #4 because this is a continuation of the docs review)
4. GetSubscriptionRelations
nitpick - rearranged the function header comment
~
5.
TBH, I'm thinking that just
On Thu, Jul 25, 2024 at 9:06 AM vignesh C wrote:
>
> The attached v20240725 version patch has the changes for the same.
Thank You for addressing the comments. Please review below issues:
1) Sub ahead of pub due to wrong initial sync of last_value for
non-incremented sequences. Steps at [1]
2) Se
On Wed, Jul 24, 2024 at 11:52 AM shveta malik wrote:
>
> On Wed, Jul 24, 2024 at 9:17 AM Peter Smith wrote:
> >
>
> I had a look at patches v20240720* (considering these as the latest
> one) and tried to do some basic testing (WIP). Few comments:
>
> 1)
> I see 'last_value' is updated wrongly aft
On Tue, 23 Jul 2024 at 11:03, Peter Smith wrote:
>
> Here are some review comments for patch v20240720-0002.
>
> ==
> 1. Commit message:
>
> 1a.
> The commit message is stale. It is still referring to functions and
> views that have been moved to patch 0003.
Modified
> 1b.
> "ALL SEQUENCES"
On Wed, Jul 24, 2024 at 9:17 AM Peter Smith wrote:
>
I had a look at patches v20240720* (considering these as the latest
one) and tried to do some basic testing (WIP). Few comments:
1)
I see 'last_value' is updated wrongly after create-sub. Steps:
---
pub:
CREATE SEQUENCE myseq0 INCREM
Hi, here are some review comments for patch v20240720-0003.
This review is a WIP. This post is only about the docs (*.sgml) of patch 0003.
==
doc/src/sgml/ref/alter_subscription.sgml
1. REFRESH PUBLICATION and copy_data
nitpicks:
- IMO the "synchronize the sequence data" info was misleading
On Tue, 16 Jul 2024 at 06:00, Peter Smith wrote:
>
> Hi,
>
> I was reading back through this thread to find out how the proposed new
> command for refreshing sequences, came about. The patch 0705 introduces a
> new command syntax for ALTER SUBSCRIPTION ... REFRESH SEQUENCES
>
> So now there are
Here are some review comments for patch v20240720-0002.
==
1. Commit message:
1a.
The commit message is stale. It is still referring to functions and
views that have been moved to patch 0003.
1b.
"ALL SEQUENCES" is not a command. It is a clause of the CREATE
PUBLICATION command.
==
doc/
can be *ahead* from lastval('s') on the
> publisher? That doesn't seem good.
Added comments for "last_value + log_cnt"
Yes it can be ahead in subscribers. This will happen because every
change of the sequence is not wal logged. It is WAL logged once in
SEQ_LOG_VALS. Th
1 - 100 of 167 matches
Mail list logo
