Amit Kapila <amit.kapila16@gmail.com> writes:
> LGTM. Horiguchi-San, do let me know if you have suggestions here. I am
> planning to push this tomorrow.
+1 for the first change, but for this:
- ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the configured
max_retention_durationof %u ms.",
+ ? errdetail("Retention is re-enabled because the apply process can advance its xmin within the configured
max_retention_durationof %u ms.",
would it be better to say
"Retention is re-enabled because the apply process was able to advance its xmin within the configured
max_retention_durationof %u ms."
If this isn't a statement that xmin has already been advanced, then
I'm not sure quite what it means.
Also here:
- : errdetail("Retention is re-enabled as max_retention_duration is set to unlimited."));
+ : errdetail("Retention is re-enabled because max_retention_duration is set to unlimited."));
I think maybe what is meant is
"Retention is re-enabled because max_retention_duration has been set to unlimited."
or you could say "has been changed to". We'd never have got to this
if max_retention_duration had been unlimited all along, correct?
Passing by mere grammatical issues ... the patch shows that only one
of these three cases is reached in the regression tests. Is that
a coverage gap that we should worry about?
regards, tom lane