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
dataincluded 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.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
