On Tue, Jan 16, 2024 at 02:46:02PM +0300, Aleksander Alekseev wrote:
>> 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.
>
> I agree and don't mind affecting the error message per se.
>
> However I see that the actual logic of how WAL is processed is being
> changed. If we do this, at very least it requires thorough thinking. I
> strongly suspect that the proposed code is wrong and/or not safe
> and/or less safe than it is now for the reasons named above.
FWIW, that pretty much sums up my feeling regarding this patch,
because an error, basically any error, would hurt back very badly.
Sure, the error messages we generate now when reaching the end of WAL
can sound scary, and they are (I suspect that's not really the case
for anybody who has history doing support with PostgreSQL because a
bunch of these messages are old enough to vote, but I can understand
that anybody would freak out the first time they see that).
However, per the recent issues we've had in this area, like
cd7f19da3468 but I'm more thinking about 6b18b3fe2c2f and
bae868caf222, I am of the opinion that the header validation, the
empty page case in XLogReaderValidatePageHeader() and the record read
changes are risky enough that I am not convinced that the gains are
worth the risks taken.
The error stack in the WAL reader is complicated enough that making it
more complicated as the patch proposes does not sound like not a good
tradeoff to me to make the reports related to the end of WAL cleaner
for the end-user. I agree that we should do something, but the patch
does not seem like a good step towards this goal. Perhaps somebody
would be more excited about this proposal than I am, of course.
--
Michael