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. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers