Re: [REVIEW] Re: Compression of full-page-writes

On Mon, Feb 16, 2015 at 9:08 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2015-02-16 20:55:20 +0900, Michael Paquier wrote:
>> On Mon, Feb 16, 2015 at 8:30 PM, Syed, Rahila <Rahila(dot)Syed(at)nttdata(dot)com>
>> wrote:
>>
>> >
>> > Regarding the sanity checks that have been added recently. I think that
>> > they are useful but I am suspecting as well that only a check on the record
>> > CRC is done because that's reliable enough and not doing those checks
>> > accelerates a bit replay. So I am thinking that we should simply replace
>> > >them by assertions.
>> >
>> > Removing the checks makes sense as CRC ensures correctness . Moreover ,as
>> > error message for invalid length of record is present in the code ,
>> > messages for invalid block length can be redundant.
>> >
>> > Checks have been replaced by assertions in the attached patch.
>> >
>>
>> After more thinking, we may as well simply remove them, an error with CRC
>> having high chances to complain before reaching this point...
>
> Surely not. The existing code explicitly does it like
> if (blk->has_data && blk->data_len == 0)
> report_invalid_record(state,
> "BKPBLOCK_HAS_DATA set, but no data included at %X/%X",
> (uint32) (state->ReadRecPtr >> 32), (uint32) state->ReadRecPtr);
> these cross checks are important. And I see no reason to deviate from
> that. The CRC sum isn't foolproof - we intentionally do checks at
> several layers. And, as you can see from some other locations, we
> actually try to *not* fatally error out when hitting them at times - so
> an Assert also is wrong.
>
> Heikki:
> /* cross-check that the HAS_DATA flag is set iff data_length > 0 */
> if (blk->has_data && blk->data_len == 0)
> report_invalid_record(state,
> "BKPBLOCK_HAS_DATA set, but no data included at %X/%X",
> (uint32) (state->ReadRecPtr >> 32), (uint32) state->ReadRecPtr);
> if (!blk->has_data && blk->data_len != 0)
> report_invalid_record(state,
> "BKPBLOCK_HAS_DATA not set, but data length is %u at %X/%X",
> (unsigned int) blk->data_len,
> (uint32) (state->ReadRecPtr >> 32), (uint32) state->ReadRecPtr);
> those look like they're missing a goto err; to me.

Yes. I pushed the fix. Thanks!

Regards,

--
Fujii Masao