Horton

Re: Reword messages using "as" instead of "because"

Tom Lane <tgl@sss.pgh.pa.us>

View thread
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2025-09-18T04:52:35Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Improve few errdetail messages introduced in commit 0d48d393d46.

  2. Resume conflict-relevant data retention automatically.

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 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