On Thu, Mar 7, 2024 at 7:35 AM Peter Smith wrote:
>
> Here are some review comments for v107-0001
>
> ==
> src/backend/replication/slot.c
>
> 1.
> +/*
> + * Struct for the configuration of standby_slot_names.
> + *
> + * Note: this must be a flat representation that can be held in a single
>
On Wed, Mar 6, 2024 at 5:53 PM Amit Kapila wrote:
>
> On Wed, Mar 6, 2024 at 12:07 PM Masahiko Sawada wrote:
> >
> > On Fri, Mar 1, 2024 at 3:22 PM Peter Smith wrote:
> > >
> > > On Fri, Mar 1, 2024 at 5:11 PM Masahiko Sawada
> > > wrote:
> > > >
> > > ...
> > > > +/*
> > > > +
On Wed, Mar 6, 2024 at 6:54 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, March 6, 2024 9:13 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Wednesday, March 6, 2024 11:04 AM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Wednesday, March 6, 2024 9:30 AM Masahiko Sawada
> > > wrote:
> > >
> >
Here are some review comments for v107-0001
==
src/backend/replication/slot.c
1.
+/*
+ * Struct for the configuration of standby_slot_names.
+ *
+ * Note: this must be a flat representation that can be held in a single chunk
+ * of guc_malloc'd memory, so that it can be stored as the "extra"
On Wednesday, March 6, 2024 9:13 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, March 6, 2024 11:04 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Wednesday, March 6, 2024 9:30 AM Masahiko Sawada
> > wrote:
> >
> > Hi,
> >
> > > On Fri, Mar 1, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
> > >
> > >
On Wednesday, March 6, 2024 11:04 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, March 6, 2024 9:30 AM Masahiko Sawada
> wrote:
>
> Hi,
>
> > On Fri, Mar 1, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
> >
> > wrote:
> > >
> > > On Friday, March 1, 2024 2:11 PM Masahiko Sawada
> > wrote:
> > > >
On Wed, Mar 6, 2024 at 12:07 PM Masahiko Sawada wrote:
>
> On Fri, Mar 1, 2024 at 3:22 PM Peter Smith wrote:
> >
> > On Fri, Mar 1, 2024 at 5:11 PM Masahiko Sawada
> > wrote:
> > >
> > ...
> > > +/*
> > > + * "*" is not accepted as in that case primary will not be able
> > >
On Fri, Mar 1, 2024 at 3:22 PM Peter Smith wrote:
>
> On Fri, Mar 1, 2024 at 5:11 PM Masahiko Sawada wrote:
> >
> ...
> > +/*
> > + * "*" is not accepted as in that case primary will not be able to
> > know
> > + * for which all standbys to wait for. Even if we have
On Wed, Mar 6, 2024 at 12:47 PM Amit Kapila wrote:
>
> On Wed, Mar 6, 2024 at 7:36 AM Masahiko Sawada wrote:
> >
> > On Tue, Mar 5, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
> > wrote:
> >
> > I have one question about PhysicalWakeupLogicalWalSnd():
> >
> > +/*
> > + * Wake up the logical walsender
On Wed, Mar 6, 2024 at 7:36 AM Masahiko Sawada wrote:
>
> On Tue, Mar 5, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
> wrote:
>
> I have one question about PhysicalWakeupLogicalWalSnd():
>
> +/*
> + * Wake up the logical walsender processes with logical failover slots if the
> + * currently acquired
On Wednesday, March 6, 2024 9:30 AM Masahiko Sawada
wrote:
Hi,
> On Fri, Mar 1, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Friday, March 1, 2024 2:11 PM Masahiko Sawada
> wrote:
> > >
> > >
> > > ---
> > > +void
> > > +assign_standby_slot_names(const char *newval, void *extra)
On Tue, Mar 5, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, March 5, 2024 2:35 PM shveta malik wrote:
> >
> > On Tue, Mar 5, 2024 at 9:15 AM Amit Kapila wrote:
> > >
> > > On Tue, Mar 5, 2024 at 6:10 AM Peter Smith
> > wrote:
> > > >
> > > > ==
> > > >
On Fri, Mar 1, 2024 at 4:21 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, March 1, 2024 2:11 PM Masahiko Sawada
> wrote:
> >
> >
> > ---
> > +void
> > +assign_standby_slot_names(const char *newval, void *extra) {
> > +List *standby_slots;
> > +MemoryContext oldcxt;
> > +
On Tuesday, March 5, 2024 2:35 PM shveta malik wrote:
>
> On Tue, Mar 5, 2024 at 9:15 AM Amit Kapila wrote:
> >
> > On Tue, Mar 5, 2024 at 6:10 AM Peter Smith
> wrote:
> > >
> > > ==
> > > src/backend/replication/walsender.c
> > >
> > > 5. NeedToWaitForWal
> > >
> > > + /*
> > > + * Check
On Tuesday, March 5, 2024 8:40 AM Peter Smith wrote:
>
> Here are some review comments for v105-0001
>
> ==
> doc/src/sgml/config.sgml
>
> 1.
> +
> +The standbys corresponding to the physical replication slots in
> +standby_slot_names must configure
> +
On Monday, March 4, 2024 11:44 PM Bertrand Drouvot
wrote:
>
> Hi,
>
> On Mon, Mar 04, 2024 at 01:28:04PM +, Zhijie Hou (Fujitsu) wrote:
> > Attach the V105 patch set
>
> Thanks!
>
> Sorry I missed those during the previous review:
No problem, thanks for the comments!
>
> 1 ===
>
>
I did performance tests for the v99 patch w.r.t. wait time analysis.
As this patch is introducing a wait for standby before sending changes
to a subscriber, at the primary node, logged time at the start and end
of the XLogSendLogical() call (which eventually calls
WalSndWaitForWal()) and
On Tue, Mar 5, 2024 at 9:15 AM Amit Kapila wrote:
>
> On Tue, Mar 5, 2024 at 6:10 AM Peter Smith wrote:
> >
> > ==
> > src/backend/replication/walsender.c
> >
> > 5. NeedToWaitForWal
> >
> > + /*
> > + * Check if the standby slots have caught up to the flushed position. It
> > + * is good to
On Tue, Mar 5, 2024 at 6:10 AM Peter Smith wrote:
>
> ==
> src/backend/replication/walsender.c
>
> 5. NeedToWaitForWal
>
> + /*
> + * Check if the standby slots have caught up to the flushed position. It
> + * is good to wait up to the flushed position and then let the WalSender
> + * send
On Mon, Mar 4, 2024 at 2:27 PM Bertrand Drouvot
wrote:
>
> On Thu, Feb 29, 2024 at 03:38:59PM +0530, Amit Kapila wrote:
> > On Thu, Feb 29, 2024 at 9:13 AM Peter Smith wrote:
> > >
> > > On Tue, Feb 27, 2024 at 11:35 PM Amit Kapila
> > > wrote:
> > > >
> > >
> > > > Also, adding wait sounds
>
Here are some review comments for v105-0001
==
doc/src/sgml/config.sgml
1.
+
+The standbys corresponding to the physical replication slots in
+standby_slot_names must configure
+sync_replication_slots = true so they can receive
+logical failover
Hi,
On Mon, Mar 04, 2024 at 01:28:04PM +, Zhijie Hou (Fujitsu) wrote:
> Attach the V105 patch set
Thanks!
Sorry I missed those during the previous review:
1 ===
Commit message: "these functions will block until"
s/block/wait/ ?
2 ===
+when used with logical failover slots, will
On Monday, March 4, 2024 5:52 PM Amit Kapila wrote:
>
> On Mon, Mar 4, 2024 at 9:35 AM Peter Smith
> wrote:
> >
> > OK, if the code will remain as-is wouldn't it be better to anyway
> > change the function name to indicate what it really does?
> >
> > e.g. NeedToWaitForWal -->
On Monday, March 4, 2024 9:55 AM Peter Smith wrote:
>
> On Sun, Mar 3, 2024 at 6:51 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Sunday, March 3, 2024 7:47 AM Peter Smith
> wrote:
> > >
>
> > > 3.
> > > +
> > > +
> > > + Value * is not accepted as it is
> > >
On Monday, March 4, 2024 7:22 PM Bertrand Drouvot
wrote:
>
> Hi,
>
> On Sun, Mar 03, 2024 at 07:56:32AM +, Zhijie Hou (Fujitsu) wrote:
> > Here is the V104 patch which addressed above and Peter's comments.
>
> Thanks!
>
> A few more random comments:
Thanks for the comments!
>
> 1 ===
On Mon, Mar 4, 2024 at 4:52 PM Bertrand Drouvot
wrote:
>
> On Sun, Mar 03, 2024 at 07:56:32AM +, Zhijie Hou (Fujitsu) wrote:
> > Here is the V104 patch which addressed above and Peter's comments.
>
> Thanks!
>
>
> 4 ===
>
> + /*
> +* Don't need to wait for the standbys to catch
Hi,
On Sun, Mar 03, 2024 at 07:56:32AM +, Zhijie Hou (Fujitsu) wrote:
> Here is the V104 patch which addressed above and Peter's comments.
Thanks!
A few more random comments:
1 ===
+The function may be blocked if the specified slot is a failover enabled
s/blocked/waiting/ ?
2
On Mon, Mar 4, 2024 at 9:35 AM Peter Smith wrote:
>
> OK, if the code will remain as-is wouldn't it be better to anyway
> change the function name to indicate what it really does?
>
> e.g. NeedToWaitForWal --> NeedToWaitForWalFlushOrStandbys
>
This seems too long. I would prefer the current
Hi,
On Thu, Feb 29, 2024 at 03:38:59PM +0530, Amit Kapila wrote:
> On Thu, Feb 29, 2024 at 9:13 AM Peter Smith wrote:
> >
> > On Tue, Feb 27, 2024 at 11:35 PM Amit Kapila
> > wrote:
> > >
> >
> > > Also, adding wait sounds
> > > more like a boolean. So, I don't see the proposed names any
On Mon, Mar 4, 2024 at 2:38 PM Amit Kapila wrote:
>
> On Mon, Mar 4, 2024 at 6:57 AM Peter Smith wrote:
> >
> > On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila wrote:
> > >
> > > On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
> > > >
> > ...
> > > > 9. NeedToWaitForWal
> > > >
> > > > + /*
> > >
On Mon, Mar 4, 2024 at 2:49 PM Amit Kapila wrote:
>
> On Mon, Mar 4, 2024 at 7:25 AM Peter Smith wrote:
> >
> >
> > ==
> > doc/src/sgml/config.sgml
> >
> > 2.
> > +pg_logical_slot_peek_changes,
> > +when used with failover enabled logical slots, will block until all
> > +
On Mon, Mar 4, 2024 at 7:25 AM Peter Smith wrote:
>
>
> ==
> doc/src/sgml/config.sgml
>
> 2.
> +pg_logical_slot_peek_changes,
> +when used with failover enabled logical slots, will block until all
> +physical slots specified in standby_slot_names
> have
> +
On Mon, Mar 4, 2024 at 6:57 AM Peter Smith wrote:
>
> On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila wrote:
> >
> > On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
> > >
> ...
> > > 9. NeedToWaitForWal
> > >
> > > + /*
> > > + * Check if the standby slots have caught up to the flushed position.
On Sun, Mar 3, 2024 at 6:51 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Sunday, March 3, 2024 7:47 AM Peter Smith wrote:
> >
> > 3.
> > +
> > +
> > + Value * is not accepted as it is inappropriate
> > to
> > + block logical replication for physical slots that either
On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila wrote:
>
> On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
> >
...
> > 9. NeedToWaitForWal
> >
> > + /*
> > + * Check if the standby slots have caught up to the flushed position. It
> > + * is good to wait up to flushed position and then let it send
On Saturday, March 2, 2024 6:55 PM Amit Kapila wrote:
>
> On Sat, Mar 2, 2024 at 9:21 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > Apart from the comments, the code in WalSndWaitForWal was refactored a
> > bit to make it neater. Thanks Shveta for helping writing the code and doc.
> >
>
> A few
On Sunday, March 3, 2024 7:47 AM Peter Smith wrote:
>
> On Sat, Mar 2, 2024 at 2:51 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Friday, March 1, 2024 12:23 PM Peter Smith
> wrote:
> > >
> ...
> > > ==
> > > src/backend/replication/slot.c
> > >
> > > 2. validate_standby_slots
> > >
> > > +
On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
>
> ~~~
>
> 3.
> +
> +
> + Value * is not accepted as it is inappropriate to
> + block logical replication for physical slots that either lack
> + associated standbys or have standbys associated that are not
On Sat, Mar 2, 2024 at 2:51 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, March 1, 2024 12:23 PM Peter Smith wrote:
> >
...
> > ==
> > src/backend/replication/slot.c
> >
> > 2. validate_standby_slots
> >
> > + else if (!ReplicationSlotCtl)
> > + {
> > + /*
> > + * We cannot validate the
On Sat, Mar 2, 2024 at 9:21 AM Zhijie Hou (Fujitsu)
wrote:
>
> Apart from the comments, the code in WalSndWaitForWal was refactored
> a bit to make it neater. Thanks Shveta for helping writing the code and doc.
>
A few more comments:
==
1.
+# Wait until the primary server logs a
On Thursday, February 29, 2024 11:16 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, February 28, 2024 7:36 PM Bertrand Drouvot
> wrote:
> >
> > 4 ===
> >
> > Regarding the test, what about adding one to test the "new" behavior
> > discussed up-thread? (logical replication will wait if slot
On Friday, March 1, 2024 12:23 PM Peter Smith wrote:
>
> Here are some review comments for v102-0001.
>
> ==
> doc/src/sgml/config.sgml
>
> 1.
> +
> +Lists the streaming replication standby server slot names that
> logical
> +WAL sender processes will wait for.
On Fri, Mar 1, 2024 at 11:41 AM Masahiko Sawada wrote:
>
> On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu)
> wrote:
> ---
> I was a bit surprised by the fact that standby_slot_names value is
> handled in a different way than a similar parameter
> synchronous_standby_names. For example, the
On Thu, Feb 29, 2024 at 12:34 PM Zhijie Hou (Fujitsu) <
houzj.f...@fujitsu.com> wrote:
> On Monday, February 26, 2024 7:52 PM Amit Kapila
> wrote:
> >
> > On Mon, Feb 26, 2024 at 7:49 AM Zhijie Hou (Fujitsu) <
> houzj.f...@fujitsu.com>
> > wrote:
> > >
> > > Attach the V98 patch set which
On Friday, March 1, 2024 2:11 PM Masahiko Sawada wrote:
>
> On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu)
> wrote:
> > >
> > >
> > > Attach the V102 patch set which addressed Amit and Shveta's comments.
> > > Thanks
Hi,
On Fri, Mar 01, 2024 at 03:22:55PM +1100, Peter Smith wrote:
> Here are some review comments for v102-0001.
>
> ==
> doc/src/sgml/config.sgml
>
> 1.
> +
> +Lists the streaming replication standby server slot names that
> logical
> +WAL sender processes will wait
On Fri, Mar 1, 2024 at 9:53 AM Peter Smith wrote:
>
> Here are some review comments for v102-0001.
>
>
> 7.
> + /*
> + * Return false if not all the standbys have caught up to the specified WAL
> + * location.
> + */
> + if (caught_up_slot_num != list_length(standby_slot_names_list))
> + return
On Fri, Mar 1, 2024 at 5:11 PM Masahiko Sawada wrote:
>
...
> +/*
> + * "*" is not accepted as in that case primary will not be able to
> know
> + * for which all standbys to wait for. Even if we have physical slots
> + * info, there is no way to confirm whether
On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> >
> > Attach the V102 patch set which addressed Amit and Shveta's comments.
> > Thanks Shveta for helping addressing the comments off-list.
>
> The cfbot
On Fri, Mar 1, 2024 at 8:58 AM shveta malik wrote:
>
> On Wed, Feb 28, 2024 at 4:51 PM Amit Kapila wrote:
> >
> > On Wed, Feb 28, 2024 at 3:26 PM shveta malik wrote:
> > >
> > >
> > > Here is the patch which addresses the above comments. Also optimized
> > > the test a little bit. Now we use
Here are some review comments for v102-0001.
==
doc/src/sgml/config.sgml
1.
+
+Lists the streaming replication standby server slot names that logical
+WAL sender processes will wait for. Logical WAL sender processes will
+send decoded changes to plugins only
On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu)
wrote:
>
>
> Attach the V102 patch set which addressed Amit and Shveta's comments.
> Thanks Shveta for helping addressing the comments off-list.
The cfbot reported a compile warning, here is the new version patch which fixed
it,
Also
On Wed, Feb 28, 2024 at 4:51 PM Amit Kapila wrote:
>
> On Wed, Feb 28, 2024 at 3:26 PM shveta malik wrote:
> >
> >
> > Here is the patch which addresses the above comments. Also optimized
> > the test a little bit. Now we use pg_sync_replication_slots() function
> > instead of worker to test the
On Thursday, February 29, 2024 5:54 PM Amit Kapila
wrote:
>
> On Thu, Feb 29, 2024 at 8:29 AM Peter Smith
> wrote:
> >
> > On Wed, Feb 28, 2024 at 1:23 PM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Tuesday, February 27, 2024 3:18 PM Peter Smith
> wrote:
> > ...
> > > > 20.
> > > > +#
> >
On Thursday, February 29, 2024 7:36 PM shveta malik
wrote:
>
> On Thu, Feb 29, 2024 at 7:04 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > Here is the v101 patch set which addressed above comments.
> >
>
> Thanks for the patch. Few comments:
>
> 1) Shall we mention in doc that shutdown will wait
On Thursday, February 29, 2024 7:17 PM Amit Kapila
wrote:
>
> On Thu, Feb 29, 2024 at 3:23 PM Amit Kapila
> wrote:
> >
> > Few additional comments on the latest patch:
> > =
> > 1.
> > static XLogRecPtr
> > WalSndWaitForWal(XLogRecPtr loc)
> > {
> > ...
> > +
On Thu, Feb 29, 2024 at 7:04 AM Zhijie Hou (Fujitsu)
wrote:
>
> Here is the v101 patch set which addressed above comments.
>
Thanks for the patch. Few comments:
1) Shall we mention in doc that shutdown will wait for standbys in
standby_slot_names to confirm receiving WAL:
Suggestion for
On Thu, Feb 29, 2024 at 3:23 PM Amit Kapila wrote:
>
> Few additional comments on the latest patch:
> =
> 1.
> static XLogRecPtr
> WalSndWaitForWal(XLogRecPtr loc)
> {
> ...
> + if (!XLogRecPtrIsInvalid(RecentFlushPtr) && loc <= RecentFlushPtr &&
> +
On Thu, Feb 29, 2024 at 9:13 AM Peter Smith wrote:
>
> On Tue, Feb 27, 2024 at 11:35 PM Amit Kapila wrote:
> >
>
> > Also, adding wait sounds
> > more like a boolean. So, I don't see the proposed names any better
> > than the current one.
> >
>
> Anyway, the point is that the current GUC name
On Thu, Feb 29, 2024 at 8:29 AM Peter Smith wrote:
>
> On Wed, Feb 28, 2024 at 1:23 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Tuesday, February 27, 2024 3:18 PM Peter Smith
> > wrote:
> ...
> > > 20.
> > > +#
> > > +# | > standby1 (primary_slot_name = sb1_slot) # | > standby2
> > >
Hi,
On Thu, Feb 29, 2024 at 10:43:07AM +1100, Peter Smith wrote:
> - if (logical)
> + /*
> + * Set always-secure search path for the cases where the connection is
> + * used to run SQL queries, so malicious users can't get control.
> + */
> + if (logical || !replication)
> {
> PGresult
On Tue, Feb 27, 2024 at 11:35 PM Amit Kapila wrote:
>
> On Tue, Feb 27, 2024 at 12:48 PM Peter Smith wrote:
> >
> > Here are some review comments for v99-0001
> >
> > ==
> > 0. GENERAL.
> >
> > +#standby_slot_names = '' # streaming replication standby server slot names
> > that
> > + #
On Wednesday, February 28, 2024 7:36 PM Bertrand Drouvot
wrote:
>
> On Wed, Feb 28, 2024 at 02:23:27AM +, Zhijie Hou (Fujitsu) wrote:
> > Attach the V100 patch set which addressed above comments.
>
> A few random comments:
Thanks for the comments!
>
> 1 ===
>
> + if (!ok)
> +
On Wed, Feb 28, 2024 at 1:23 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, February 27, 2024 3:18 PM Peter Smith
> wrote:
...
> > 20.
> > +#
> > +# | > standby1 (primary_slot_name = sb1_slot) # | > standby2
> > +(primary_slot_name = sb2_slot) # primary - | # | > subscriber1
> >
On Monday, February 26, 2024 7:52 PM Amit Kapila
wrote:
>
> On Mon, Feb 26, 2024 at 7:49 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > Attach the V98 patch set which addressed above comments.
> >
>
> Few comments:
> =
> 1.
> WalSndWaitForWal(XLogRecPtr loc)
> {
> int wakeEvents;
>
On Wed, Feb 28, 2024 at 10:21 PM Amit Kapila wrote:
>
> On Wed, Feb 28, 2024 at 3:26 PM shveta malik wrote:
> >
> >
> > Here is the patch which addresses the above comments. Also optimized
> > the test a little bit. Now we use pg_sync_replication_slots() function
> > instead of worker to test
Hi,
On Wed, Feb 28, 2024 at 04:50:55PM +0530, Amit Kapila wrote:
> On Wed, Feb 28, 2024 at 3:26 PM shveta malik wrote:
> >
> >
> > Here is the patch which addresses the above comments. Also optimized
> > the test a little bit. Now we use pg_sync_replication_slots() function
> > instead of worker
Hi,
On Wed, Feb 28, 2024 at 02:23:27AM +, Zhijie Hou (Fujitsu) wrote:
> Attach the V100 patch set which addressed above comments.
Thanks!
A few random comments:
1 ===
+ if (!ok)
+ {
+ GUC_check_errdetail("List syntax is invalid.");
+ }
What about to get
On Wed, Feb 28, 2024 at 3:26 PM shveta malik wrote:
>
>
> Here is the patch which addresses the above comments. Also optimized
> the test a little bit. Now we use pg_sync_replication_slots() function
> instead of worker to test the operator-redirection using search-patch.
> This has been done to
On Wed, Feb 28, 2024 at 1:33 PM Bertrand Drouvot
wrote:
>
> Hi,
> A few comments:
Thanks for reviewing.
>
> 1 ===
>
> +* used to run normal SQL queries
>
> s/run normal SQL/run SQL/ ?
>
> As mentioned up-thread I don't like that much the idea of creating such a test
> but if we do then
On Wed, Feb 28, 2024 at 12:31 PM Bertrand Drouvot
wrote:
>
> On Wed, Feb 28, 2024 at 06:48:37AM +, Zhijie Hou (Fujitsu) wrote:
> > On Wednesday, February 28, 2024 2:38 PM Bertrand Drouvot
> > wrote:
> > > > 2.
> > > > Can we add a test case to demonstrate that the '=' operator can be
> > >
Hi,
On Wed, Feb 28, 2024 at 12:29:01PM +0530, shveta malik wrote:
> On Wed, Feb 28, 2024 at 8:49 AM Amit Kapila wrote:
> >
> >
> > Few comments:
>
> Thanks for the feedback.
>
> > ===
> > 1.
> > - if (logical)
> > + if (logical || !replication)
> > {
> >
> > Can we add a comment
Hi,
On Wed, Feb 28, 2024 at 06:48:37AM +, Zhijie Hou (Fujitsu) wrote:
> On Wednesday, February 28, 2024 2:38 PM Bertrand Drouvot
> wrote:
> > On Wed, Feb 28, 2024 at 08:49:19AM +0530, Amit Kapila wrote:
> > > On Mon, Feb 26, 2024 at 9:13 AM shveta malik
> > wrote:
> > > >
> > > > On Fri,
On Wed, Feb 28, 2024 at 8:49 AM Amit Kapila wrote:
>
>
> Few comments:
Thanks for the feedback.
> ===
> 1.
> - if (logical)
> + if (logical || !replication)
> {
>
> Can we add a comment about connection types that require
> ALWAYS_SECURE_SEARCH_PATH_SQL?
>
> 2.
> Can we add a test
On Wednesday, February 28, 2024 2:38 PM Bertrand Drouvot
wrote:
> On Wed, Feb 28, 2024 at 08:49:19AM +0530, Amit Kapila wrote:
> > On Mon, Feb 26, 2024 at 9:13 AM shveta malik
> wrote:
> > >
> > > On Fri, Feb 23, 2024 at 7:41 PM Bertrand Drouvot
> > > wrote:
> > > >
> > > > Hi,
> > > > > I
Hi,
On Wed, Feb 28, 2024 at 08:49:19AM +0530, Amit Kapila wrote:
> On Mon, Feb 26, 2024 at 9:13 AM shveta malik wrote:
> >
> > On Fri, Feb 23, 2024 at 7:41 PM Bertrand Drouvot
> > wrote:
> > >
> > > Hi,
> > > > I think to set secure search path for remote connection, the standard
> > > >
On Mon, Feb 26, 2024 at 9:13 AM shveta malik wrote:
>
> On Fri, Feb 23, 2024 at 7:41 PM Bertrand Drouvot
> wrote:
> >
> > Hi,
> > > I think to set secure search path for remote connection, the standard
> > > approach
> > > could be to extend the code in libpqrcv_connect[1], so that we don't
On Tuesday, February 27, 2024 3:18 PM Peter Smith wrote:
>
> Here are some review comments for v99-0001
Thanks for the comments!
> Commit Message
>
> 1.
> A new parameter named standby_slot_names is introduced.
>
> Maybe quote the GUC names here to make it more readable.
Added.
>
> ~~
>
On Tue, Feb 27, 2024 at 12:48 PM Peter Smith wrote:
>
> Here are some review comments for v99-0001
>
> ==
> 0. GENERAL.
>
> +#standby_slot_names = '' # streaming replication standby server slot names
> that
> + # logical walsender processes will wait for
>
> IMO the GUC name is too
On Tue, Feb 27, 2024 at 4:07 PM Bertrand Drouvot
wrote:
>
> On Tue, Feb 27, 2024 at 06:17:44PM +1100, Peter Smith wrote:
> > +static bool
> > +validate_standby_slots(char **newval)
> > +{
> > + char*rawname;
> > + List*elemlist;
> > + ListCell *lc;
> > + bool ok;
> > +
> > + /* Need a
Hi,
On Tue, Feb 27, 2024 at 06:17:44PM +1100, Peter Smith wrote:
> +static bool
> +validate_standby_slots(char **newval)
> +{
> + char*rawname;
> + List*elemlist;
> + ListCell *lc;
> + bool ok;
> +
> + /* Need a modifiable copy of string */
> + rawname = pstrdup(*newval);
> +
> + /*
Here are some review comments for v99-0001
==
0. GENERAL.
+#standby_slot_names = '' # streaming replication standby server slot names that
+ # logical walsender processes will wait for
IMO the GUC name is too generic. There is nothing in this name to
suggest it has anything to do with
On Monday, February 26, 2024 1:19 PM Amit Kapila
wrote:
>
> On Fri, Feb 23, 2024 at 4:45 PM Bertrand Drouvot
> wrote:
> >
> > On Fri, Feb 23, 2024 at 09:46:00AM +, Zhijie Hou (Fujitsu) wrote:
> > >
> > > Besides, I'd like to clarify and discuss the behavior of
> > > standby_slot_names
>
Hi,
On Mon, Feb 26, 2024 at 05:52:40PM +0530, shveta malik wrote:
> On Mon, Feb 26, 2024 at 5:18 PM Amit Kapila wrote:
> >
> > > > > + if (!ok)
> > > > > + GUC_check_errdetail("List syntax is invalid.");
> > > > > +
> > > > > + /*
> > > > > +* If there is a
Hi,
On Mon, Feb 26, 2024 at 05:18:25PM +0530, Amit Kapila wrote:
> On Mon, Feb 26, 2024 at 12:59 PM Bertrand Drouvot
> wrote:
> > > > 10 ===
> > > >
> > > > + if (slot->data.invalidated != RS_INVAL_NONE)
> > > > + {
> > > > +
On Mon, Feb 26, 2024 at 5:18 PM Amit Kapila wrote:
>
> > > > + if (!ok)
> > > > + GUC_check_errdetail("List syntax is invalid.");
> > > > +
> > > > + /*
> > > > +* If there is a syntax error in the name or if the replication
> > > > slots'
> > > > +*
On Mon, Feb 26, 2024 at 7:49 AM Zhijie Hou (Fujitsu)
wrote:
>
> Attach the V98 patch set which addressed above comments.
>
Few comments:
=
1.
WalSndWaitForWal(XLogRecPtr loc)
{
int wakeEvents;
+ bool wait_for_standby = false;
+ uint32 wait_event;
+ List*standby_slots = NIL;
On Mon, Feb 26, 2024 at 12:59 PM Bertrand Drouvot
wrote:
>
> On Mon, Feb 26, 2024 at 02:18:58AM +, Zhijie Hou (Fujitsu) wrote:
> > On Friday, February 23, 2024 6:12 PM Bertrand Drouvot
> > wrote:
> > > + if (!ok)
> > > + GUC_check_errdetail("List syntax is invalid.");
>
Hi,
On Mon, Feb 26, 2024 at 10:48:38AM +0530, Amit Kapila wrote:
> On Fri, Feb 23, 2024 at 4:45 PM Bertrand Drouvot
> wrote:
> >
> > On Fri, Feb 23, 2024 at 09:46:00AM +, Zhijie Hou (Fujitsu) wrote:
> > >
> > > Besides, I'd like to clarify and discuss the behavior of
> > >
Hi,
On Mon, Feb 26, 2024 at 09:13:05AM +0530, shveta malik wrote:
> On Fri, Feb 23, 2024 at 7:41 PM Bertrand Drouvot
> wrote:
> >
> > Hi,
> > > I think to set secure search path for remote connection, the standard
> > > approach
> > > could be to extend the code in libpqrcv_connect[1], so that
Hi,
On Mon, Feb 26, 2024 at 02:18:58AM +, Zhijie Hou (Fujitsu) wrote:
> On Friday, February 23, 2024 6:12 PM Bertrand Drouvot
> wrote:
> > + if (!ok)
> > + GUC_check_errdetail("List syntax is invalid.");
> > +
> > + /*
> > +* If there is a syntax error in
On Fri, Feb 23, 2024 at 4:45 PM Bertrand Drouvot
wrote:
>
> On Fri, Feb 23, 2024 at 09:46:00AM +, Zhijie Hou (Fujitsu) wrote:
> >
> > Besides, I'd like to clarify and discuss the behavior of standby_slot_names
> > once.
> >
> > As it stands in the patch, If the slots specified in
On Fri, Feb 23, 2024 at 7:41 PM Bertrand Drouvot
wrote:
>
> Hi,
> > I think to set secure search path for remote connection, the standard
> > approach
> > could be to extend the code in libpqrcv_connect[1], so that we don't need
> > to schema
> > qualify all the operators in the queries.
> >
>
On Friday, February 23, 2024 6:12 PM Bertrand Drouvot
wrote:
>
> Here are some random comments:
Thanks for the comments!
>
> 1 ===
>
> Commit message "Allow logical walsenders to wait for the physical"
>
> s/physical/physical standby/?
>
> 2 ==
>
> +++
Hi,
On Fri, Feb 23, 2024 at 09:30:58AM +, Zhijie Hou (Fujitsu) wrote:
> On Friday, February 23, 2024 5:07 PM Bertrand Drouvot
> wrote:
> > On Fri, Feb 23, 2024 at 02:15:11PM +0530, shveta malik wrote:
> > >
> > > Thanks for the details. I understand it now. We do not use '=' in our
> > >
Hi,
On Fri, Feb 23, 2024 at 09:46:00AM +, Zhijie Hou (Fujitsu) wrote:
> On Friday, February 23, 2024 1:22 PM shveta malik
> wrote:
>
> >
> > Thanks for the patches. Had a quick look at v95_2, here are some
> > trivial comments:
>
> Thanks for the comments.
>
> > 6) streaming replication
Hi,
On Fri, Feb 23, 2024 at 04:36:44AM +, Zhijie Hou (Fujitsu) wrote:
> On Friday, February 23, 2024 10:18 AM Zhijie Hou (Fujitsu)
> wrote:
> > >
> > > Hi,
> > >
> > > Since the slotsync worker patch has been committed, I rebased the
> > > remaining patches.
> > > And here is the V95 patch
On Friday, February 23, 2024 1:22 PM shveta malik
wrote:
>
> Thanks for the patches. Had a quick look at v95_2, here are some
> trivial comments:
Thanks for the comments.
> 6) streaming replication standby server slot names that logical walsender
> processes will wait for
>
> Is it better
On Friday, February 23, 2024 5:07 PM Bertrand Drouvot
wrote:
>
> On Fri, Feb 23, 2024 at 02:15:11PM +0530, shveta malik wrote:
> > On Fri, Feb 23, 2024 at 1:28 PM Bertrand Drouvot
> > wrote:
> > >
> > > Hi,
> > >
> > > Because one could create say the "=" OPERATOR in their own schema,
> > >
Hi,
On Fri, Feb 23, 2024 at 02:15:11PM +0530, shveta malik wrote:
> On Fri, Feb 23, 2024 at 1:28 PM Bertrand Drouvot
> wrote:
> >
> > Hi,
> >
> > Because one could create say the "=" OPERATOR in their own schema, attach a
> > function to it doing undesired stuff and change the search_path for
101 - 200 of 850 matches
Mail list logo
