Date: Tue, 16 Jan 2024 11:57:03 +0900 (JST)
Message-Id: <20240116.115703.2298339297458587094.horikyota.ntt@gmail.com>
To: aleksander@timescale.com
Cc: pgsql-hackers@lists.postgresql.org, vignesh21@gmail.com,
 stark.cfm@gmail.com, alvherre@alvh.no-ip.org, pryzby@telsasoft.com,
 andres@anarazel.de, ashu.coek88@gmail.com, pashkin.elfe@gmail.com,
 michael@paquier.xyz, bossartn@amazon.com, david@pgmasters.net,
 peter.eisentraut@2ndquadrant.com, jtc331@gmail.com, robertmhaas@gmail.com
Subject: Re: Make mesage at end-of-recovery less scary.
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <CAJ7c6TMpmD5hvbi=H7qbmwHTFa=joG-XkUcDzQ54S8LH1U5ctA@mail.gmail.com>
References: <CALDaNm1XcZhSbXxNNOoCqaz5nA55E5na0OL2hXVn6AxMEySWVQ@mail.gmail.com>
	<20240111.161816.1259268425616323411.horikyota.ntt@gmail.com>
	<CAJ7c6TMpmD5hvbi=H7qbmwHTFa=joG-XkUcDzQ54S8LH1U5ctA@mail.gmail.com>
User-Agent: Mew version 6.8 on Emacs 27.2
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20240116.115703.2298339297458587094.horikyota.ntt%40gmail.com>
Precedence: bulk

Thank you for the comments.

At Fri, 12 Jan 2024 15:03:26 +0300, Aleksander Alekseev <aleksander@timescale.com> wrote in 
> ```
> +        p = (char *) record;
> +        pe = p + XLOG_BLCKSZ - (RecPtr & (XLOG_BLCKSZ - 1));
> +
> +        while (p < pe && *p == 0)
> +            p++;
> +
> +        if (p == pe)
> ```
> 
> Just as a random thought: perhaps we should make this a separate
> function, as a part of src/port/. It seems to me that this code could
> benefit from using vector instructions some day, similarly to
> memcmp(), memset() etc. Surprisingly there doesn't seem to be a
> standard C function for this. Alternatively one could argue that one
> cycle doesn't make much code to reuse and that the C compiler will
> place SIMD instructions for us. However a counter-counter argument
> would be that we could use a macro or even better an inline function
> and have the same effect except getting a slightly more readable code.

Creating a function with a name like memcmp_byte() should be
straightforward, but implementing it with SIMD right away seems a bit
challenging. Similar operations are already being performed elsewhere
in the code, probably within the stats collector, where memcmp is used
with a statically allocated area that's filled with zeros. If we can
achieve a performance equivalent to memcmp with this new function,
then it definitely seems worth pursuing.

> ```
> - * This is just a convenience subroutine to avoid duplicated code in
> + * This is just a convenience subroutine to avoid duplicate code in
> ```
> 
> This change doesn't seem to be related to the patch. Personally I
> don't mind it though.

Ah, I'm sorry. That was something I mistakenly thought I had written
at the last moment and made modifications to.

> All in all I find v28 somewhat scary. It does much more than "making
> one message less scary" as it was initially intended and what bugged
> me personally, and accordingly touches many more places including
> xlogreader.c, xlogrecovery.c, etc.
> 
> Particularly I have mixed feeling about this:
> 
> ```
> +            /*
> +             * Consider it as end-of-WAL if all subsequent bytes of this page
> +             * are zero. We don't bother checking the subsequent pages since
> +             * they are not zeroed in the case of recycled segments.
> +             */
> ```
> 
> If I understand correctly, if somehow several FS blocks end up being
> zeroed (due to OS bug, bit rot, restoring from a corrupted for
> whatever reason backup, hardware failures, ...) there is non-zero
> chance that PG will interpret this as a normal situation. To my
> knowledge this is not what we typically do - typically PG would report
> an error and ask a human to figure out what happened. Of course the
> possibility of such a scenario is small, but I don't think that as
> DBMS developers we can ignore it.

For now, let me explain the basis for this patch. The fundamental
issue is that these warnings that always appear are, in practice, not
a problem in almost all cases. Some of those who encounter them for
the first time may feel uneasy and reach out with inquiries. On the
other hand, those familiar with these warnings tend to ignore them and
only pay attention to details when actual issues arise. Therefore, the
intention of this patch is to label them as "no issue" unless a
problem is blatantly evident, in order to prevent unnecessary concern.

> Does anyone agree or maybe I'm making things up?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center