Amit Kapila <amit.kapila16@gmail.com> writes:
> On Thu, Sep 18, 2025 at 8:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1 for the first change, but for this:
>>
>> - ? errdetail("Retention is re-enabled as the apply process is advancing its xmin within the
configuredmax_retention_duration of %u ms.",
>> + ? errdetail("Retention is re-enabled because the apply process can advance its xmin within
theconfigured 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_durationof %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
