(the previous mail was crossing with yours..)
At Mon, 05 Sep 2022 14:15:27 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
me> +1 for showing any message for the failure, but I think we shouldn't
me> hide an existing message if any.
At Mon, 5 Sep 2022 16:54:07 +1200, Thomas Munro <thomas.munro@gmail.com> wrote in
> On reflection, it'd be better not to clobber any pre-existing error
> there, but report one only if there isn't one already queued. I've
> done that in this version, which I'm planning to do a bit more testing
> on and commit soonish if there are no comments/objections, especially
> for that part.
It looks fine in this regard. I still think that the message looks
somewhat internal.
me> And the error messages around are
me> just telling that "<some error happened> at RecPtr". So I think
me> "missing contrecord at RecPtr" is sufficient here.
> I'll have to check whether a doc change is necessary somewhere to
> advertise that maintenance_io_concurrency=0 turns off prefetching, but
> IIRC that's kinda already implied.
>
> I've tested quite a lot of scenarios including make check-world with
> maintenance_io_concurrency = 0, 1, 10, 1000, and ALTER SYSTEM for all
> relevant GUCs on a standby running large pgbench to check expected
> effect on pg_stat_recovery_prefetch view and generate system calls.
+ if (likely(record = prefetcher->reader->record))
Isn't this confusing a bit?
+ if (likely(record = prefetcher->reader->record))
+ {
+ XLogRecPtr replayed_up_to = record->next_lsn;
+
+ XLogReleasePreviousRecord(prefetcher->reader);
+
The likely condition is the prerequisite for
XLogReleasePreviousRecord. But is is a little hard to read the
condition as "in case no previous record exists". Since there is one
in most cases, can't call XLogReleasePreviousRecord() unconditionally
then the function returns true when the previous record exists and
false if not?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center