Amit Kapila <[email protected]> writes:
> On Thu, Sep 18, 2025 at 8:56 AM Tom Lane <[email protected]> wrote:
>> +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_duration
>> of %u ms.",
>> + ? errdetail("Retention is re-enabled because the
>> apply process can advance its xmin within the configured
>> max_retention_duration of %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_duration of %u ms."
> xmin is not yet advanced. In this state, we ensured that the
> subscriber has caught up with the publisher and now the apply worker
> can start maintaining/advancing its xmin.
Hm, so what has max_retention_duration got to do with it? That
is, should the message just read
"Retention is re-enabled because the apply process can advance its
xmin."
or better
"Retention is re-enabled because the apply process has caught up with
the publisher."
This now reminds me of a point that I meant to make in my previous
reply and forgot: this whole business of "advancing xmin" is
implementation jargon. Can we rephrase it to make sense to a
user who has no idea what xmin is? I think the concepts of being
caught up to the publisher or falling behind the publisher should
be pretty clear to most users, but none of these proposed messages
are that.
>> 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?
> We thought adding for one of these cases is sufficient to avoid
> increasing the test timing further. These are time sensitive tests as
> apply-worker on subscriber is dependent on actions of publisher, so we
> need wait logic.
Hmm, if it's time-sensitive then yeah, building a stable test
case would be very hard.
regards, tom lane
