Re: Make mesage at end-of-recovery less scary. - Mailing list pgsql-hackers

Hi,

> The errors occurred in a part of the tests for end-of-WAL detection
> added in the master branch. These failures were primarily due to
> changes in the message contents introduced by this patch. During the
> revision, I discovered an issue with the handling of empty pages that
> appear in the middle of reading continuation records. In the previous
> version, such empty pages were mistakenly identified as indicating a
> clean end-of-WAL (that is a LOG). However, they should actually be
> handled as a WARNING, since the record curently being read is broken
> at the empty pages. The following changes have been made in this
> version:
>
> 1. Adjusting the test to align with the error message changes
>   introduced by this patch.
>
> 2. Adding tests for the newly added messages.
>
> 3. Correcting the handling of empty pages encountered during the
>   reading of continuation records. (XLogReaderValidatePageHeader)
>
> 4. Revising code comments.
>
> 5. Changing the term "log segment" to "WAL
>   segment". (XLogReaderValidatePageHeader)
>
> regards.

Thanks for the updated patch.

```
+        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.

```
- * 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.

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.

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

-- 
Best regards,
Aleksander Alekseev