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

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Peter Geoghegan <pg(at)bowt(dot)ie>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Subtle bug in "Simplify tape block format" commit
Date: 2017-02-01 10:18:43
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

> 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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Albe Laurenz 2017-02-01 10:22:28 Re: CONNECTION LIMIT and Parallel Query don't play well together
Previous Message valeriof 2017-02-01 09:52:26 Re: Deployment of an output plugin in Unix-like environment