On Wed, Feb 19, 2025 at 3:43 PM Amit Kapila wrote:
>
> Pushed after minor modifications.
>
I have closed the corresponding CF entry. Please feel free to start a
new thread for xid age based parameter.
--
With Regards,
Amit Kapila.
On Tue, Feb 18, 2025 at 8:42 AM Amit Kapila wrote:
>
> On Mon, Feb 17, 2025 at 10:18 PM Nathan Bossart
> wrote:
> >
> > On Mon, Feb 17, 2025 at 07:57:22AM +0530, Amit Kapila wrote:
> >
> > > Alvaro, Nathan, do let us know if you would like to discuss more on
> > > the use case for this new GUC id
On Mon, Feb 17, 2025 at 10:18 PM Nathan Bossart
wrote:
>
> On Mon, Feb 17, 2025 at 07:57:22AM +0530, Amit Kapila wrote:
>
> > Alvaro, Nathan, do let us know if you would like to discuss more on
> > the use case for this new GUC idle_replication_slot_timeout?
> > Otherwise, we can proceed with this
On Mon, Feb 17, 2025 at 07:57:22AM +0530, Amit Kapila wrote:
> On Wed, Feb 12, 2025 at 1:16 PM Zhijie Hou (Fujitsu)
> wrote:
>> On Wednesday, February 12, 2025 11:56 AM Amit Kapila
>> wrote:
>> > Also, we previously didn't have a good experience with XID-based threshold
>> > parameters like vacu
On Fri, Feb 14, 2025 at 5:30 PM Nisha Moond wrote:
>
> Here is a summary of changes in v78:
>
A few minor comments:
1.
Slots that appear idle due to a disrupted connection between
+the publisher and subscriber are also excluded, as they are managed by
+wal_sender_timeout.
...
How
On Mon, Feb 17, 2025 at 11:29 AM Amit Kapila wrote:
>
> On Fri, Feb 14, 2025 at 5:30 PM Nisha Moond wrote:
> >
> > Here is a summary of changes in v78:
> >
>
> A few minor comments:
> 1.
> Slots that appear idle due to a disrupted connection between
> +the publisher and subscriber are als
On Wed, Feb 12, 2025 at 1:16 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, February 12, 2025 11:56 AM Amit Kapila
> wrote:
> >
> > On Tue, Feb 11, 2025 at 9:39 PM Nathan Bossart
> > wrote:
> > >
> > > On Tue, Feb 11, 2025 at 03:22:49PM +0100, Álvaro Herrera wrote:
> > > > I find this propose
Some review comments for v78-0001.
==
src/backend/replication/slot.c
ReportSlotInvalidation:
1.
+ int minutes;
+ int secs;
The variables 'minutes' and 'seconds' are only used by case
RS_INVAL_IDLE_TIMEOUT, so I think it would be better to make a new
code block for that case where you can de
Please find the updated v78 patches after a few off-list review rounds.
Here is a summary of changes in v78:
patch-001:
- Fixed bugs reported by Hou-san and Peter in [1] and [2].
- Fixed a race condition reported by Hou-san off-list, which could
lead to an assert failure.
This failure happens when
On Wednesday, February 12, 2025 11:56 AM Amit Kapila
wrote:
>
> On Tue, Feb 11, 2025 at 9:39 PM Nathan Bossart
> wrote:
> >
> > On Tue, Feb 11, 2025 at 03:22:49PM +0100, Álvaro Herrera wrote:
> > > I find this proposed patch a bit strange and I feel it needs more
> > > explanation.
> > >
> > >
On Tue, Feb 11, 2025 at 9:39 PM Nathan Bossart wrote:
>
> On Tue, Feb 11, 2025 at 03:22:49PM +0100, Álvaro Herrera wrote:
> > I find this proposed patch a bit strange and I feel it needs more
> > explanation.
> >
> > When this thread started, Bharath justified his patches saying that a
> > slot th
On Wed, Feb 12, 2025 at 12:36 AM Nisha Moond wrote:
>
> On Tue, Feb 11, 2025 at 8:49 AM Peter Smith wrote:
> >
> > Hi Nisha.
> >
> > Some review comments about v74-0001
> >
> > ==
> > src/backend/replication/slot.c
> >
> > 1.
> > /* Maximum number of invalidation causes */
> > -#define RS_IN
On Tue, Feb 11, 2025 at 03:22:49PM +0100, Álvaro Herrera wrote:
> I find this proposed patch a bit strange and I feel it needs more
> explanation.
>
> When this thread started, Bharath justified his patches saying that a
> slot that's inactive for a very long time could be problematic because
> of
Hello,
I find this proposed patch a bit strange and I feel it needs more
explanation.
When this thread started, Bharath justified his patches saying that a
slot that's inactive for a very long time could be problematic because
of XID wraparound. Fine, that sounds a reasonable feature. If you
wa
On Tue, Feb 11, 2025 at 11:42 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Monday, February 10, 2025 8:03 PM Nisha Moond
> wrote:
> >
> > On Sat, Feb 8, 2025 at 12:28 PM Zhijie Hou (Fujitsu)
> >
> > wrote:
> > >
> >
> > > 3.
> > >
> > > + if (cause & RS_INVAL_HORIZON)
> > > +
On Tue, Feb 11, 2025 at 8:49 AM Peter Smith wrote:
>
> Hi Nisha.
>
> Some review comments about v74-0001
>
> ==
> src/backend/replication/slot.c
>
> 1.
> /* Maximum number of invalidation causes */
> -#define RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL
> -
> -StaticAssertDecl(lengthof(SlotInvalida
On Tue, Feb 11, 2025 at 8:49 AM Peter Smith wrote:
>
>
> InvalidatePossiblyObsoleteSlot:
>
> 2.
> + if (possible_causes & RS_INVAL_IDLE_TIMEOUT)
> + {
> + /*
> + * Assign the current time here to avoid system call overhead
> + * while holding the spinlock in subsequent code.
> + */
> + now = GetCu
On Monday, February 10, 2025 8:03 PM Nisha Moond
wrote:
>
> On Sat, Feb 8, 2025 at 12:28 PM Zhijie Hou (Fujitsu)
> wrote:
> >
>
> > 3.
> >
> > + if (cause & RS_INVAL_HORIZON)
> > + {
> > + if (!SlotIsLogical(s))
> > +
Hi Nisha.
Some review comments about v74-0001
==
src/backend/replication/slot.c
1.
/* Maximum number of invalidation causes */
-#define RS_INVAL_MAX_CAUSES RS_INVAL_WAL_LEVEL
-
-StaticAssertDecl(lengthof(SlotInvalidationCauses) == (RS_INVAL_MAX_CAUSES + 1),
- "array length mismatch");
+#def
On Mon, Feb 10, 2025 at 6:12 PM vignesh C wrote:
>
> On Mon, 10 Feb 2025 at 17:33, Nisha Moond wrote:
> >
> > Here are the v73 patches incorporating the comments above and the
> > subsequent comments from [1].
> > - patch 002 is rebased on 001 with no new changes.
>
> Few comments:
> 1) For some
On Mon, 10 Feb 2025 at 17:33, Nisha Moond wrote:
>
> Here are the v73 patches incorporating the comments above and the
> subsequent comments from [1].
> - patch 002 is rebased on 001 with no new changes.
Few comments:
1) For some reason SlotInvalidationCauses was with PGDLLIMPORT, this
is remove
On Sat, Feb 8, 2025 at 12:28 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, February 7, 2025 9:06 PM Nisha Moond
> wrote:
> >
> > Attached v72 patches, addressed the above comments as well as Vignesh's
> > comments in [2].
> > - There are no new changes in patch-002.
>
> Thanks for updating the
On Monday, February 10, 2025 2:10 PM Amit Kapila
wrote:
>
> On Sat, Feb 8, 2025 at 12:28 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > 3.
> >
> > + if (cause & RS_INVAL_HORIZON)
> > + {
> > + if (!SlotIsLogical(s))
> > +
On Mon, Feb 10, 2025 at 11:33 AM Peter Smith wrote:
>
> Some review comments for v72-0001.
>
> ==
> GENERAL
>
> My preference was to just keep the enum as per v70 for the *actual*
> cause, and introduce a separate set of bit flags for *possible* causes
> to be checked. This creates a clear cod
On Fri, Feb 7, 2025 at 4:53 PM Amit Kapila wrote:
>
> On Fri, Feb 7, 2025 at 8:00 AM Peter Smith wrote:
> >
> > ==
> > src/backend/access/transam/xlog.c
> >
> > 1.
> > XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
> > KeepLogSeg(recptr, &_logSegNo);
> > - if (InvalidateObsoleteRep
On Sat, Feb 8, 2025 at 12:28 PM Zhijie Hou (Fujitsu)
wrote:
>
> 3.
>
> + if (cause & RS_INVAL_HORIZON)
> + {
> + if (!SlotIsLogical(s))
> + goto invalidation_marked;
>
> I am not sure if
Hi Nisha,
Some review comments for v72-0001.
==
GENERAL
My preference was to just keep the enum as per v70 for the *actual*
cause, and introduce a separate set of bit flags for *possible* causes
to be checked. This creates a clear code separation between the actual
and possible. It also elim
On Friday, February 7, 2025 9:06 PM Nisha Moond
wrote:
>
> Attached v72 patches, addressed the above comments as well as Vignesh's
> comments in [2].
> - There are no new changes in patch-002.
Thanks for updating the patch, I have few review comments:
1.
> InvalidateObsoleteReplicationSlots(R
On Fri, Feb 7, 2025 at 8:00 AM Peter Smith wrote:
>
> Hi Nisha,
>
> Some review comments for v71-0001.
>
> ==
> src/backend/replication/slot.c
>
> SlotInvalidationCauses[]
>
> 2.
> [RS_INVAL_WAL_REMOVED] = "wal_removed",
> [RS_INVAL_HORIZON] = "rows_removed",
> [RS_INVAL_WAL_LEVEL] = "wa
On Fri, Feb 7, 2025 at 8:00 AM Peter Smith wrote:
>
> ==
> src/backend/access/transam/xlog.c
>
> 1.
> XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
> KeepLogSeg(recptr, &_logSegNo);
> - if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED,
> + if (InvalidateObsoleteReplicati
Hi Nisha,
Some review comments for v71-0001.
==
src/backend/access/transam/xlog.c
1.
XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
KeepLogSeg(recptr, &_logSegNo);
- if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED,
+ if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_
On Thu, 6 Feb 2025 at 16:08, Nisha Moond wrote:
> Here are the v71 patches with the above comments incorporated.
Few comments:
1) While changing the switch to an if condition, the behavior of the
break statement has changed. Previously, it would exit the switch, but
now it exits the main for loop
On Thu, Feb 6, 2025 at 10:17 AM Amit Kapila wrote:
>
> On Thu, Feb 6, 2025 at 8:02 AM Nisha Moond wrote:
> >
> > On Wed, Feb 5, 2025 at 2:42 PM Amit Kapila wrote:
> > >
> > > Would it address your concern if we write the actual idle duration
> > > (now - inactive_since) instead of directly using
On Thu, Feb 6, 2025 at 10:17 AM Amit Kapila wrote:
>
> On Thu, Feb 6, 2025 at 8:02 AM Nisha Moond wrote:
> >
>
> >
> > > 2.
> > > + * Flush all replication slots to disk. Also, invalidate obsolete slots
> > > during
> > > + * non-shutdown checkpoint.
> > > *
> > > * It is convenient to flush
On Thu, Feb 6, 2025 at 8:02 AM Nisha Moond wrote:
>
> On Wed, Feb 5, 2025 at 2:42 PM Amit Kapila wrote:
> >
> > Would it address your concern if we write the actual idle duration
> > (now - inactive_since) instead of directly using inactive_since in the
> > above message?
> >
>
> Simply using the
On Wed, Feb 5, 2025 at 2:42 PM Amit Kapila wrote:
>
> On Wed, Feb 5, 2025 at 10:30 AM vignesh C wrote:
> >
> > On Tue, 4 Feb 2025 at 19:56, Nisha Moond wrote:
> > >
> > > Here is v69 patch set addressing above and Kuroda-san's comments in [1].
> >
> > Few minor suggestions:
> > 1) In the slot in
On Wed, Feb 5, 2025 at 12:58 PM Peter Smith wrote:
>
> Hi Nisha,
>
> Some review comments for the patch v69-0002.
>
> ==
> .../t/044_invalidate_inactive_slots.pl
>
> 2.
> +if ($ENV{enable_injection_points} ne 'yes')
> +{
> + plan skip_all => 'Injection points not supported by this build';
> +}
On Wed, Feb 5, 2025 at 10:30 AM vignesh C wrote:
>
> On Tue, 4 Feb 2025 at 19:56, Nisha Moond wrote:
> >
> > Here is v69 patch set addressing above and Kuroda-san's comments in [1].
>
> 2) Here we have mentioned about invalidation happens only for a)
> released slots b) inactive slots replication
On Wed, Feb 5, 2025 at 10:30 AM vignesh C wrote:
>
> On Tue, 4 Feb 2025 at 19:56, Nisha Moond wrote:
> >
> > Here is v69 patch set addressing above and Kuroda-san's comments in [1].
>
> Few minor suggestions:
> 1) In the slot invalidation reporting below:
> + case RS_INVAL_IDLE_TIME
Hi Nisha,
Some review comments for the patch v69-0002.
==
src/backend/replication/slot.c
1.
+#ifdef USE_INJECTION_POINTS
+
+ /*
+ * To test idle timeout slot invalidation, if the
+ * slot-time-out-inval injection point is attached,
+ * immediately invalidate the slot.
+ */
+ if (IS_INJECTION
On Tue, 4 Feb 2025 at 19:56, Nisha Moond wrote:
>
> Here is v69 patch set addressing above and Kuroda-san's comments in [1].
Few minor suggestions:
1) In the slot invalidation reporting below:
+ case RS_INVAL_IDLE_TIMEOUT:
+ Assert(inactive_since > 0);
+
+
Review comments for v69-0001.
==
doc/src/sgml/config.sgml
1.
+
+Invalidate replication slots that have remained idle longer than this
+duration. If this value is specified without units, it is taken as
+minutes. A value of zero (which is default) disables the id
On Tue, Feb 4, 2025 at 4:42 PM vignesh C wrote:
>
> On Tue, 4 Feb 2025 at 15:58, Nisha Moond wrote:
> >
> > Here are the v68 patches, incorporating above as well as comments from [1].
> >
> Few comments:
> 1) Let's call TimestampDifferenceExceedsSeconds only if
> idle_replication_slot_timeout_min
Dear Nisha,
Thanks for updating the patch! Here are my comments.
01.
```
+# Test for replication slots invalidation
```
Since the file tests only timeout invalidations, the comment seems too general.
02.
```
+ # Check that an invalidated slot cannot be acquired
+ my ($result, $stdou
On Tue, 4 Feb 2025 at 15:58, Nisha Moond wrote:
>
> Here are the v68 patches, incorporating above as well as comments from [1].
>
Few comments:
1) Let's call TimestampDifferenceExceedsSeconds only if
idle_replication_slot_timeout_mins is set to avoid the
TimestampDifferenceExceedsSeconds function
On Tue, Feb 4, 2025 at 10:45 AM Amit Kapila wrote:
>
> On Mon, Feb 3, 2025 at 6:35 PM Shlok Kyal wrote:
> >
> > I reviewed the v66 patch. I have few comments:
> >
> > 1. I also feel the default value should be set to '0' as suggested by
> > Vignesh in 1st point of [1].
> >
>
> +1. This will ensur
On Tue, 4 Feb 2025 at 10:45, Amit Kapila wrote:
>
> On Mon, Feb 3, 2025 at 6:35 PM Shlok Kyal wrote:
> >
> > I reviewed the v66 patch. I have few comments:
> >
> > 1. I also feel the default value should be set to '0' as suggested by
> > Vignesh in 1st point of [1].
> >
>
> +1. This will ensure t
On Mon, Feb 3, 2025 at 6:35 PM Shlok Kyal wrote:
>
> I reviewed the v66 patch. I have few comments:
>
> 1. I also feel the default value should be set to '0' as suggested by
> Vignesh in 1st point of [1].
>
+1. This will ensure that the idle slots won't be invalidated by
default, the same as HEAD
On Tue, Feb 4, 2025 at 4:02 AM Peter Smith wrote:
>
> On Mon, Feb 3, 2025 at 5:34 PM Amit Kapila wrote:
> >
> > On Mon, Feb 3, 2025 at 6:16 AM Peter Smith wrote:
> > >
> > >
> > > 2.
> > > + appendStringInfo(&err_hint, "You might need to increase \"%s\".",
> > > + "max_slot_wal_keep_size");
> >
Hi Nisha.
Some review comments for patch v67-0001.
==
src/backend/replication/slot.c
ReportSlotInvalidation:
Please see my previous post [1] where I gave some reasons why I think
the _() macro should be used only for the *common* part of the hint
messages. If you agree, then the following c
On Mon, Feb 3, 2025 at 5:34 PM Amit Kapila wrote:
>
> On Mon, Feb 3, 2025 at 6:16 AM Peter Smith wrote:
> >
> >
> > 2.
> > + appendStringInfo(&err_hint, "You might need to increase \"%s\".",
> > + "max_slot_wal_keep_size");
> > break;
> > 2a.
> > In this case, shouldn't you really be using macr
On Mon, Feb 3, 2025 at 12:04 PM Amit Kapila wrote:
>
> On Mon, Feb 3, 2025 at 6:16 AM Peter Smith wrote:
> >
> >
> > 2.
> > + appendStringInfo(&err_hint, "You might need to increase \"%s\".",
> > + "max_slot_wal_keep_size");
> > break;
> > 2a.
> > In this case, shouldn't you really be using mac
On Fri, 31 Jan 2025 at 17:50, Nisha Moond wrote:
>
> On Fri, Jan 31, 2025 at 2:32 PM Amit Kapila wrote:
> >
> > On Fri, Jan 31, 2025 at 10:40 AM Peter Smith wrote:
> > >
> > > ==
> > > src/backend/replication/slot.c
> > >
> > > ReportSlotInvalidation:
> > >
> > > 1.
> > > +
> > > + case RS_I
On Mon, Feb 3, 2025 at 2:55 PM Amit Kapila wrote:
>
> On Fri, Jan 31, 2025 at 5:50 PM Nisha Moond wrote:
> >
> > Please find the attached v66 patch set. The base patch(v65-001) is
> > committed now, so I have rebased the patches.
> >
>
> *
>
> The time when the slot became inacti
On Fri, Jan 31, 2025 at 5:50 PM Nisha Moond wrote:
>
> Please find the attached v66 patch set. The base patch(v65-001) is
> committed now, so I have rebased the patches.
>
*
The time when the slot became inactive. NULL if the
-slot is currently being streamed.
+sl
On Mon, Feb 3, 2025 at 6:16 AM Peter Smith wrote:
>
>
> 2.
> + appendStringInfo(&err_hint, "You might need to increase \"%s\".",
> + "max_slot_wal_keep_size");
> break;
> 2a.
> In this case, shouldn't you really be using macro _("You might need to
> increase \"%s\".") so that the common format s
On Mon, Feb 3, 2025 at 4:04 PM Amit Kapila wrote:
>
> On Mon, Feb 3, 2025 at 9:04 AM Peter Smith wrote:
> >
> > On Fri, Jan 31, 2025 at 8:02 PM Amit Kapila wrote:
> > >
> > > On Fri, Jan 31, 2025 at 10:40 AM Peter Smith
> > > wrote:
> > > >
> > > > ==
> > > > src/backend/replication/slot.c
On Mon, Feb 3, 2025 at 9:04 AM Peter Smith wrote:
>
> On Fri, Jan 31, 2025 at 8:02 PM Amit Kapila wrote:
> >
> > On Fri, Jan 31, 2025 at 10:40 AM Peter Smith wrote:
> > >
> > > ==
> > > src/backend/replication/slot.c
> > >
> > > ReportSlotInvalidation:
> > >
> > > 1.
> > > +
> > > + case RS_
On Fri, Jan 31, 2025 at 8:02 PM Amit Kapila wrote:
>
> On Fri, Jan 31, 2025 at 10:40 AM Peter Smith wrote:
> >
> > ==
> > src/backend/replication/slot.c
> >
> > ReportSlotInvalidation:
> >
> > 1.
> > +
> > + case RS_INVAL_IDLE_TIMEOUT:
> > + Assert(inactive_since > 0);
> > + /* translator: se
Hi Nisha,
Some review comments for v66-0001.
==
src/backend/replication/slot.c
ReportSlotInvalidation:
1.
StringInfoData err_detail;
+ StringInfoData err_hint;
bool hint = false;
initStringInfo(&err_detail);
+ initStringInfo(&err_hint);
I don't think you still need the 'hint' boole
On Fri, 31 Jan 2025 at 17:50, Nisha Moond wrote:
>
> Thanks for the review! I have incorporated the above comments. The
> test in patch-002 has been optimized as suggested and now completes in
> less than a second.
> Please find the attached v66 patch set. The base patch(v65-001) is
> committed no
On Fri, Jan 31, 2025 at 2:32 PM Amit Kapila wrote:
>
> On Fri, Jan 31, 2025 at 10:40 AM Peter Smith wrote:
> >
> > ==
> > src/backend/replication/slot.c
> >
> > ReportSlotInvalidation:
> >
> > 1.
> > +
> > + case RS_INVAL_IDLE_TIMEOUT:
> > + Assert(inactive_since > 0);
> > + /* translator: se
On Fri, Jan 31, 2025 at 10:40 AM Peter Smith wrote:
>
> ==
> src/backend/replication/slot.c
>
> ReportSlotInvalidation:
>
> 1.
> +
> + case RS_INVAL_IDLE_TIMEOUT:
> + Assert(inactive_since > 0);
> + /* translator: second %s is a GUC variable name */
> + appendStringInfo(&err_detail,
> + _("The
Hi Nisha.
Here are some review comments for patch v65-0002
==
src/backend/replication/slot.c
ReportSlotInvalidation:
1.
+
+ case RS_INVAL_IDLE_TIMEOUT:
+ Assert(inactive_since > 0);
+ /* translator: second %s is a GUC variable name */
+ appendStringInfo(&err_detail,
+ _("The slot has remain
On Tue, Jan 28, 2025 at 5:28 PM Nisha Moond wrote:
>
> On Tue, Jan 28, 2025 at 3:26 PM Amit Kapila wrote:
> >
> > On Mon, Dec 30, 2024 at 11:05 AM Peter Smith wrote:
> > >
> > > I think we are often too quick to throw out perfectly good tests.
> > > Citing that some similar GUCs don't do testing
On Wed, Jan 29, 2025 at 12:44 PM vignesh C wrote:
>
> On Tue, 28 Jan 2025 at 17:28, Nisha Moond wrote:
> >
> > Please find the attached v64 patches. The changes in this version
> > w.r.t. older patch v63 are as -
> > - The changes from the v63-0001 patch have been moved to a separate thread
> >
On Tue, 28 Jan 2025 at 17:28, Nisha Moond wrote:
>
> Please find the attached v64 patches. The changes in this version
> w.r.t. older patch v63 are as -
> - The changes from the v63-0001 patch have been moved to a separate thread
> [1].
> - The v63-0002 patch has been split into two parts in v64:
On Tue, Jan 28, 2025 at 10:58 PM Nisha Moond wrote:
> Please find the attached v64 patches. The changes in this version
> w.r.t. older patch v63 are as -
> - The changes from the v63-0001 patch have been moved to a separate thread
> [1].
> - The v63-0002 patch has been split into two parts in v64
On Tue, 28 Jan 2025 at 17:28, Nisha Moond wrote:
>
> On Tue, Jan 28, 2025 at 3:26 PM Amit Kapila wrote:
> >
> > On Mon, Dec 30, 2024 at 11:05 AM Peter Smith wrote:
> > >
> > > I think we are often too quick to throw out perfectly good tests.
> > > Citing that some similar GUCs don't do testing a
On Tue, Jan 28, 2025 at 3:26 PM Amit Kapila wrote:
>
> On Mon, Dec 30, 2024 at 11:05 AM Peter Smith wrote:
> >
> > I think we are often too quick to throw out perfectly good tests.
> > Citing that some similar GUCs don't do testing as a reason to skip
> > them just seems to me like an example of
On Mon, Jan 27, 2025 at 4:20 PM Amit Kapila wrote:
>
> On Mon, Jan 27, 2025 at 11:00 AM Nisha Moond wrote:
> >
> > I discussed the above comments further with Peter off-list, and here
> > are the v63 patches with the following changes:
> > patch-001: The Assert and related comments have been upd
On Mon, Dec 30, 2024 at 11:05 AM Peter Smith wrote:
>
> I think we are often too quick to throw out perfectly good tests.
> Citing that some similar GUCs don't do testing as a reason to skip
> them just seems to me like an example of "two wrongs don't make a
> right".
>
> There is a third option.
On Mon, Jan 27, 2025 at 11:00 AM Nisha Moond wrote:
>
> I discussed the above comments further with Peter off-list, and here
> are the v63 patches with the following changes:
> patch-001: The Assert and related comments have been updated for clarity.
>
The 0001 patch should be discussed in a sep
On Wed, Jan 22, 2025 at 10:49 AM Nisha Moond wrote:
>
> On Tue, Jan 21, 2025 at 8:22 AM Peter Smith wrote:
> >
> > Some review comments for patch v61-0001.
> >
> > ==
> > src/backend/replication/slot.c
> >
> > 2.
> > + /*
> > + * The logical replication slots shouldn't be invalidated as GUC
>
On Tue, Jan 21, 2025 at 8:22 AM Peter Smith wrote:
>
> Some review comments for patch v61-0001.
>
> ==
> src/backend/replication/slot.c
>
> InvalidatePossiblyObsoleteSlot:
>
> 1.
> /*
> * Check if the slot needs to be invalidated. If it needs to be
> - * invalidated, and is not currently a
On Tue, Jan 21, 2025 at 8:26 AM Peter Smith wrote:
>
> Some review comments for patch v61-0002
>
> ==
> src/backend/replication/slot.c
>
> 1.
> * Whether a slot needs to be invalidated depends on the cause. A slot is
> - * removed if it:
> + * invalidated if it:
> * - RS_INVAL_WAL_REMOVED:
Some review comments for patch v61-0002
==
src/backend/replication/slot.c
1.
* Whether a slot needs to be invalidated depends on the cause. A slot is
- * removed if it:
+ * invalidated if it:
* - RS_INVAL_WAL_REMOVED: requires a LSN older than the given segment
* - RS_INVAL_HORIZON: req
Some review comments for patch v61-0001.
==
src/backend/replication/slot.c
InvalidatePossiblyObsoleteSlot:
1.
/*
* Check if the slot needs to be invalidated. If it needs to be
- * invalidated, and is not currently acquired, acquire it and mark it
- * as having been invalidated. We do th
On Fri, Jan 17, 2025 at 6:50 PM Shlok Kyal wrote:
>
> Hi Nisha,
>
> Thanks for providing an updated patch. I have tested the patch and ran
> some tests. The patch works fine. I have few comments:
>
Thanks for your review. Attached are the v61 patches.
I've addressed the comments and rebased patc
On Thu, 16 Jan 2025 at 12:35, Nisha Moond wrote:
>
> On Wed, Jan 15, 2025 at 11:37 AM Shlok Kyal wrote:
> >
> > On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
> > >
> > > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> > > >
> > > > Hi Nisha,
> > > >
> > > > Here are some minor review comm
On Mon, Jan 13, 2025 at 12:22 PM vignesh C wrote:
>
> On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
> >
> > Thank you for your feedback! Please find the v59 patch set addressing
> > all the comments.
> > Note: There are no new changes in patch-0001.
>
> Few comments:
> 1) I felt we should not i
On Wed, Jan 15, 2025 at 11:37 AM Shlok Kyal wrote:
>
> On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
> >
> > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> > >
> > > Hi Nisha,
> > >
> > > Here are some minor review comments for patch v58-0002.
> > >
> >
> > Thank you for your feedback! Pl
On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
>
> On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> >
> > Hi Nisha,
> >
> > Here are some minor review comments for patch v58-0002.
> >
>
> Thank you for your feedback! Please find the v59 patch set addressing
> all the comments.
> Note: There a
On Mon, 13 Jan 2025 at 12:48, Peter Smith wrote:
>
> On Mon, Jan 13, 2025 at 5:52 PM vignesh C wrote:
> >
> > On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
> > >
> > > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> > > >
> > > > Hi Nisha,
> > > >
> > > > Here are some minor review commen
On Mon, Jan 13, 2025 at 5:52 PM vignesh C wrote:
>
> On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
> >
> > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> > >
> > > Hi Nisha,
> > >
> > > Here are some minor review comments for patch v58-0002.
> > >
...
>
> 2) We can mention this as 1d ins
On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
>
> On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> >
> > Hi Nisha,
> >
> > Here are some minor review comments for patch v58-0002.
> >
>
> Thank you for your feedback! Please find the v59 patch set addressing
> all the comments.
> Note: There a
On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
>
> Hi Nisha,
>
> Here are some minor review comments for patch v58-0002.
>
Thank you for your feedback! Please find the v59 patch set addressing
all the comments.
Note: There are no new changes in patch-0001.
--
Thanks,
Nisha
From 8154e2baee6fcf
On Thu, Jan 2, 2025 at 5:44 AM Peter Smith wrote:
>
> Hi Nisha.
>
> My review comments for patch v58-0001.
>
> ==
> src/backend/replication/slot.c
>
> InvalidatePossiblyObsoleteSlot:
>
> 1.
> /*
> - * If the slot can be acquired, do so and mark it invalidated
> - * immediately. Otherwise we
Hi Nisha,
Here are some minor review comments for patch v58-0002.
==
src/backend/replication/slot.c
check_replication_slot_inactive_timeout:
1.
+
+/*
+ * GUC check_hook for idle_replication_slot_timeout
+ *
+ * We don't allow the value of idle_replication_slot_timeout other
+ * than 0 durin
Hi Nisha.
My review comments for patch v58-0001.
==
src/backend/replication/slot.c
InvalidatePossiblyObsoleteSlot:
1.
/*
- * If the slot can be acquired, do so and mark it invalidated
- * immediately. Otherwise we'll signal the owning process, below, and
- * retry.
+ * If the slot can be
On Fri, Dec 27, 2024 at 9:22 AM vignesh C wrote:
>
>
> Few comments:
> 1) We have disabled the similar configuration max_slot_wal_keep_size
> by setting to -1, as this GUC also is in similar lines, should we
> disable this and let the user configure it?
> +/*
> + * Invalidate replication slots tha
On Mon, Dec 30, 2024 at 11:05 AM Peter Smith wrote:
>
> On Tue, Dec 24, 2024 at 10:37 PM Nisha Moond wrote:
> >
> > On Fri, Dec 20, 2024 at 3:12 PM Amit Kapila wrote:
> >>
> >> On Mon, Dec 16, 2024 at 4:10 PM Nisha Moond
> >> wrote:
> >> >
> >> > Here is the v56 patch set with the above commen
On Tue, Dec 24, 2024 at 10:37 PM Nisha Moond wrote:
>
> On Fri, Dec 20, 2024 at 3:12 PM Amit Kapila wrote:
>>
>> On Mon, Dec 16, 2024 at 4:10 PM Nisha Moond wrote:
>> >
>> > Here is the v56 patch set with the above comments incorporated.
>> >
>>
>> Review comments:
>> ===
>> 1.
>> +
Hi Nisha,
Here are some review comments for the patch v57-0001.
==
src/backend/replication/slot.c
1.
+/*
+ * Invalidate replication slots that have remained idle longer than this
+ * duration; '0' disables it.
+ */
+int idle_replication_slot_timeout_min = HOURS_PER_DAY * MINS_PER_HOUR;
IMO
Hi Nisha.
Here are some review comments for patch v57-0001.
==
src/backend/replication/slot.c
1.
+
+/*
+ * Raise an error based on the invalidation cause of the slot.
+ */
+static void
+RaiseSlotInvalidationError(ReplicationSlot *slot)
+{
+ StringInfoData err_detail;
+
+ initStringInfo(&err_
On Tue, 24 Dec 2024 at 17:07, Nisha Moond wrote:
>
> On Fri, Dec 20, 2024 at 3:12 PM Amit Kapila wrote:
>>
>> On Mon, Dec 16, 2024 at 4:10 PM Nisha Moond wrote:
>> >
>> > Here is the v56 patch set with the above comments incorporated.
>> >
>>
>> Review comments:
>> ===
>> 1.
>> + {
>
Hello, Hou!
> So, I think it's not a bug in the committed patch but an issue in the
testing
> venvironment. Besides, since we have not seen such failures on BF, I
think it
> may not be necessary to improve the testcases.
Thanks for your analysis!
Yes, probably WSL2/Windows interactions cause st
On Tuesday, December 24, 2024 8:57 PM Michail Nikolaev
wrote:
Hi,
> Yesterday I got a strange set of test errors, probably somehow related to
> that patch. It happened on changed master branch (based on
> d96d1d5152f30d15678e08e75b42756101b7cab6) but I don't think my changes were
> affecting i
Hello everyone!
Yesterday I got a strange set of test errors, probably somehow related to
that patch.
It happened on changed master branch (based
on d96d1d5152f30d15678e08e75b42756101b7cab6) but I don't think my changes
were affecting it.
My setup is a little bit tricky: Windows 11 run WSL2 with
On Fri, Dec 20, 2024 at 3:12 PM Amit Kapila wrote:
> On Mon, Dec 16, 2024 at 4:10 PM Nisha Moond
> wrote:
> >
> > Here is the v56 patch set with the above comments incorporated.
> >
>
> Review comments:
> ===
> 1.
> + {
> + {"idle_replication_slot_timeout", PGC_SIGHUP, REPLICATION_SE
1 - 100 of 411 matches
Mail list logo
