On 14/12/2024 01:44, Andres Freund wrote:
> The zero_damaged_pages path in bufmgr.c makes sense to me, but this one seems
> less sane to me. If you want to recover from a data corruption event and
> can't dump the data because a seqscan stumbles over an invalid page -
> zero_damaged_pages makes sense.
>
> Seqscans or tidscans won't reach the mdreadv() path, because they check the
> relation size first. Which leaves access from indexes - e.g. an index pointer
> beyond the end of the heap. But in that case it's not sane to use
> zero_damaged_pages, because that's almost a guarantee for worsening corruption
> in the future, because the now empty heap page will eventually be filled with
> new tuples, which now will be pointed to by index entries pointing that were
> created before the zeroing.
Well, if you need to do zero_damage_pages=off, you're screwed already,
so I don't know think the worsening corruption argument matters much.
And you have the same problem by pages zeroed by a seqscan too. To avoid
that, you'd want to mark the page explicitly as "damaged, do not reuse"
rather than just zero it, but that'd be a lot of new code.
Hmm, looking at index_fetch_heap(), I'm surprised it doesn't throw an
error or even a warning if the heap tuple isn't found. That would seem
like a useful sanity check. An index tuple should never point to a
non-existent heap TID I believe.
> I'm wondering if we should just put an error into the relevant paths in HEAD
> and see whether it triggers for anybody in the next months. Having all these
> untested paths in md.c forever doesn't seem great.
+1
--
Heikki Linnakangas
Neon (https://neon.tech)
