Re: Introduce XID age and inactive timeout based replication slot invalidation - Mailing list pgsql-hackers

From Peter Smith
Subject Re: Introduce XID age and inactive timeout based replication slot invalidation
Date
Msg-id CAHut+PsrSLh3+fSyYWBgHvoPmES1w3RS9xzAQ8_iFLfOqDbF4A@mail.gmail.com
Whole thread Raw
In response to Re: Introduce XID age and inactive timeout based replication slot invalidation  (vignesh C <vignesh21@gmail.com>)
List pgsql-hackers
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 remained idle since %s, which is longer than the
configured \"%s\" duration."),
+ timestamptz_to_str(inactive_since),
+ "idle_replication_slot_timeout");
+ break;
+

errdetail:

I guess it is no fault of this patch because I see you've only copied
nearby code, but AFAICT this function is still having an each-way bet
by using a mixture of _() macro which is for strings intended be
translated, but then only using them in errdetail_internal() which is
for strings that are NOT intended to be translated. Isn't it
contradictory? Why don't we use errdetail() here?

errhint:

Also, the way the 'hint' is implemented can only be meaningful for
RS_INVAL_WAL_REMOVED. This is also existing code that IMO it was
always strange, but now that this patch has added another kind of
switch (cause) this hint implementation now looks increasingly hacky
to me; it is also inflexible -- e.g. if you ever wanted to add
different hints. A neater implementation would be to make the code
more like how the err_detail is handled, so then the errhint string
would only be assigned within the "case RS_INVAL_WAL_REMOVED:"

======
Kind Regards,
Peter Smith.
Fujitsu Australia



pgsql-hackers by date:

Previous
From: Melanie Plageman
Date:
Subject: Re: BitmapHeapScan streaming read user and prelim refactoring
Next
From: Ashutosh Bapat
Date:
Subject: EvictUnpinnedBuffer and buffer free list