On Tue, Jan 28, 2025 at 10:58 PM Nisha Moond <nisha.moond...@gmail.com> 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:
> 1) 001 patch: Implements the main feature - inactive timeout-based
> slot invalidation.
> 2) 002 patch: Separates the TAP test "044_invalidate_inactive_slots"
> as suggested above.
>
Hi Nisha.
Some review comments for patch v64-0001.
======
1. General
Too much of this patch v64-0001 is identical/duplicated code with the
recent "spin-off" patch v1-0002 [1]. e.g. Most of v1-0001 is now also
embedded in the v64-0001.
This is making for an unnecessarily tricky 2 x review of all the same
code, and it will also cause rebase hassles later.
Even if you wanted the 'error_in_invalid' stuff to be discussed and
pushed separately, I think it will be much easier to keep a "COPY" of
that v1-0002 patch here as a pre-requisite for v64-0001 so then all of
the current code duplications can be removed.
======
src/backend/replication/slot.c
ReplicationSlotAcquire:
2.
+ *
+ * An error is raised if error_if_invalid is true and the slot is found to
+ * be invalid.
*/
and
+ /*
+ * An error is raised if error_if_invalid is true and the slot has been
+ * previously invalidated due to inactive timeout.
+ */
+ if (error_if_invalid && s->data.invalidated == RS_INVAL_IDLE_TIMEOUT)
+ {
Although those comments are correct for v1-0001 [1] it is a misleading
comment in the hacked into v64-0001 because here you are only checking
invalidation cause RS_INVAL_IDLE_TIMEOUT but none of the other
possible causes.
~~~
ReportSlotInvalidation:
3.
+ case RS_INVAL_IDLE_TIMEOUT:
+ Assert(inactive_since > 0);
+ /* translator: second %s is a GUC variable name */
+ appendStringInfo(&err_detail,
+ _("The slot has remained idle since %s, which is longer than the
configured \"%s\" duration."),
+ timestamptz_to_str(inactive_since),
+ "idle_replication_slot_timeout");
I have the same question already asked for my review of patch v1-0002
[1]. e.g. Isn't there some mismatch between using the _() macro which
is for translations, and using the errdetail_internal which is for
strings *not* requiring translation?
~~~
InvalidatePossiblyObsoleteSlot:
4.
/*
* The logical replication slots shouldn't be invalidated as GUC
* max_slot_wal_keep_size is set to -1 during the binary upgrade. See
* check_old_cluster_for_valid_slots() where we ensure that no
* invalidated before the upgrade.
*/
Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));
Unless I am mistaken, all of the v63 cleanups of the above binary
upgrade code assert stuff have vanished somewhere between v63 and v64.
I cannot find them in the spin-off thread. All accidentally lost? (in
2 places)
Not only that but the accompanying comment modification (to mention
"and idle_replication_slot_timeout is set to 0") is also MIA last seen
in v63 (??)
======
[1]
https://www.postgresql.org/message-id/CABdArM6pBL5hPnSQ%2B5nEVMANcF4FCH7LQmgskXyiLY75TMnKpw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
