Re: Subtle bug in "Simplify tape block format" commit

On 01/31/2017 01:51 AM, Peter Geoghegan wrote:
> * If you're writing out any old bit pattern, I suggest using 0x7F
> bytes rather than nul bytes (I do agree that it does seem worth making
> this some particular bit pattern, so as to not have to bother with
> Valgrind suppressions and so on). That pattern immediately gives you a
> hint on where to look for more information when there is a problem. To
> me, it suggests "this is something weird". We don't want this to
> happen very often.

Somehow writing zeros still feels more natural to me. That's what you'd
get if you just seeked past the end of file, too, for example. I
understand your point of using 0x7F to catch bugs, but an all-zeros page
is a tell-tale too. So I went with all-zeros, anyway.

> * I think that you should put the new code into a new function, called
> ltsAllocBlocksUntil(), or similar -- this can do the BufFileWrite()
> stuff itself, with a suitable custom defensive elog(ERROR) message.
> That way, the only new branch needed in ltsWriteBlock() is "if
> (blocknum > lts->nBlocksWritten)" (maybe use unlikely() too?), and you
> can make it clear that ltsAllocBlocksUntil() is a rarely needed thing,
> which seems appropriate.

I started to refactor this with something like ltsAllocBlocksUntil(),
but in the end, it just seemed like more almost identical code with
ltsWriteBlock.

> We really don't want ltsAllocBlocksUntil() logic to be called very
> often, and certainly not to write more than 1 or 2 blocks at a time
> (no more than 1 with current usage). After all, that would mean
> writing to the same position twice or more, for no reason at all.
> Maybe note in comments that it's only called at the end of a run in
> practice, or something to that effect. Keeping writes sequential is
> very important, to keep logtape block reclamation effective.

Added a comment explaining that.

Pushed with those little changes. Thanks!

- Heikki