On Mon, Feb 16, 2015 at 9:08 PM, Andres Freund <and...@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.s...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
