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

On Mon, Feb 16, 2015 at 9:08 PM, Andres Freund <andres@2ndquadrant.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.Syed@nttdata.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