Re: data corruption hazard in reorderbuffer.c - Mailing list pgsql-hackers

On 7/16/21 1:07 AM, Mark Dilger wrote:
> 
> 
>> On Jul 15, 2021, at 3:32 PM, Tomas Vondra
>> <tomas.vondra@enterprisedb.com> wrote:
>> 
>> I think it's mostly futile to list all the possible issues this
>> might have caused - if you skip arbitrary decoded changes, that can
>> trigger pretty much any bug in reorder buffer. But those bugs can
>> be triggered by various other issues, of course.
> 
> I thought that at first, too, but when I searched for bug reports
> with the given error messages, they all had to do with logical
> replication or logical decoding.  That seems a bit fishy to me.  If
> these bugs were coming from all over the system, why would that be
> so?
> 

No, I'm not suggesting those bugs are coming from all over the system. 
The theory is that there's a bug in logical decoding / reorder buffer, 
with the same symptoms. So it'd affect systems with logical replication.

>> It's hard to say what was the cause, but the "logic" bugs are
>> probably permanent, while the issues triggered by I/O probably
>> disappear after a restart?
> 
> If you mean "logic" bugs like passing an invalid file descriptor to
> close(), then yes, those would be permanent, but I don't believe we
> have any bugs of that kind.
> 

No, by logic bugs I mean cases like failing to update the snapshot, 
losing track of relfilenode changes, etc. due to failing to consider 
some cases etc. As opposed to "I/O errors" where this is caused by 
external events.

> The consequences of I/O errors are not going to go away after
> restart.  On the subscriber side, if logical replication has replayed
> less than a full transaction worth of changes without raising an
> error, the corruption will be permanent.
> 

True, good point. I was thinking about the "could not map relfilenode" 
error, which forces the decoding to restart, and on the retry it's 
unlikely to hit the same I/O error, so it'll succeed. But you're right 
that if it reaches the subscriber and gets committed (e.g. missing a 
row), it's permanent.

>> That being said, I agree this seems like an issue and we should not
>> ignore I/O errors.
> 
> I agree.
> 
>> I'd bet other places using transient files (like sorting or hashagg
>> spilling to disk) has the same issue, although in that case the
>> impact is likely limited to a single query.
> 
> I'm not so convinced.  In most places, the result is ignored for
> close()-type operations only when the prior operation failed and
> we're closing the handle in preparation for raising an error.  There
> are a small number of other places, such as
> pg_import_system_collations and perform_base_backup, which ignore the
> result from closing a handle, but those are reading from the handle,
> not writing to it, so the situation is not comparable.
> 
> I believe the oversight in reorderbuffer.c really is a special case.
> 

Hmm, you might be right. I have only briefly looked at buffile.c and 
tuplestore.c yesterday, and I wasn't sure about the error checking. But 
maybe that works fine, now that I look at it, because we don't really 
use the files after close().

>> I wonder if sync before the close is an appropriate solution,
>> though. It seems rather expensive, and those files are meant to be
>> "temporary" (i.e. we don't keep them over restart). So maybe we
>> could ensure the consistency is a cheaper way - perhaps tracking
>> some sort of checksum for each file, or something like that?
> 
> I'm open to suggestions of what to do, but thinking of these files as
> temporary may be what misleads developers to believe they don't have
> to be treated carefully.  The code was first committed in 2014 and as
> far as I am aware nobody else has complained about this before.  In
> some part that might be because CloseTemporaryFile() is less familiar
> than close() to most developers, and they may be assuming that it
> contains its own error handling and just don't realize that it
> returns an error code just like regular close() does.
> 

I don't think anyone thinks corruption of temporary files would not be 
an issue, but making them fully persistent (which is what fsync would 
do) just to eliminate a piece is not lost seems like an overkill to me. 
And the file may get corrupted even with fsync(), so I'm wondering if 
there's a way to ensure integrity without the fsync (so cheaper).

The scheme I was thinking about is a simple checksum for each BufFile. 
When writing a buffer to disk, update the crc32c checksum, and then 
check it after reading the file (and error-out if it disagrees).

> The point of the reorder buffer is to sort the changes from a single
> transaction so that they can be replayed in order.  The files used
> for the sorting are temporary, but there is nothing temporary about
> the failure to include all the changes in the files, as that will
> have permanent consequences for whoever replays them.  If lucky, the
> attempt to replay the changes will abort because they don't make
> sense, but I've demonstrated to my own satisfaction that nothing
> prevents silent data loss if the failure to write changes happens to
> destroy a complete rather than partial change.
> 

Sure, I agree with all of that.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company