Re: Incorrect error handling for two-phase state files resulting in data loss

On Wed, Jul 18, 2018 at 05:18:18PM +0900, Michael Paquier wrote:
> On Mon, Jul 09, 2018 at 02:03:09PM +0900, Michael Paquier wrote:
>> I think that we really need to harden things, by making
>> ReadTwoPhaseFile() fail hard is it finds something unexpected, which is
>> in this case anything except trying to open a file which fails on
>> ENOENT, and that this stuff should be back-patched.
>
> Rebased as attached because of the conflicts from 811b6e3.

So... I have been doing a self-review of this patch after letting it
aside for a couple of weeks, and I did not spot any fundamental issue
wit hit. I have spotted two minor issues:

- {
- CloseTransientFile(fd);
- return NULL;
- }
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("incorrect size of two-phase state file \"%s\": %zu bytes",
+ path, stat.st_size)));
This needs to use errmsg_plural.

+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("corrupted two-phase state file for \"%u\"",
This should use "for transaction %u".

As this is a data corruption issue, are there any objections if I patch
and back-patch? I also would like to get this stuff in first as I have
other refactoring work which would shave some more code.
--
Michael