From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Reword messages using "as" instead of "because" |
Date: | 2025-09-18 04:41:02 |
Message-ID: | CAA4eK1+VwKRM80BKvT1gR1bbk-Sv4SH7AG6NQqe23_8txJs9bw@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Sep 18, 2025 at 8:56 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Smith | 2025-09-18 04:46:17 | Re: [WIP]Vertical Clustered Index (columnar store extension) - take2 |
Previous Message | Michael Paquier | 2025-09-18 04:28:43 | Re: Orphan page in _bt_split |