Horton

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

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
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:41:02Z
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.

On Thu, Sep 18, 2025 at 8:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> 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_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."
>
> If this isn't a statement that xmin has already been advanced, then
> I'm not sure quite what it means.
>

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.

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

Right, so will use your version.

> 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.  Otherwise, it seems doable to once again stop the
retention and resume due to a non-zero value of
max_retention_duration.

-- 
With Regards,
Amit Kapila.