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

On Mon, Jan 30, 2017 at 4:04 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Hmm. The LogicalTapeEndWriting() function is very similar to the
> LogicalTapePause() function I had in the pause/resume patch. They both flush
> the last block to disk. The difference is that LogicalTapePause() also
> free'd the buffer, and read back the last block from the disk if you
> continued writing, while LogicalTapeEndWriting() keeps a copy of it in
> memory.

Right.

> With the proposed fix (or with the pause/resume), you can only write to a
> single tape at a time. Not a problem at the moment, but something to
> consider. At least, would need more comments to make that more clear, and an
> Assert would be nice.

Agreed on that.

> Alternatively, we could fix this with a small change in ltsWriteBlock(), see
> attached patch. When we're about to create a hole in the file, write
> all-zero blocks to avoid the creating hole, before the block itself. That's
> not quite as efficient as writing the actual block contents into the hole,
> which avoids having to write it later, but probably won't make any
> measurable difference in practice. I'm inclined to do that, because it
> relaxes the rules on what you're allowed to do, in what order, which makes
> this more robust in general.

Why write an all-zero block, rather than arbitrary garbage, which is
all that BufFile really requires? I think it's because sizeof(int) nul
bytes represent the end of a run for tuplesort's purposes. That makes
me doubtful that this is any more robust or general than what I
proposed. So, I don't have a problem with the performance implications
of doing this, which should be minor, but I'm concerned that it
appears to be more general than it actually is.

--
Peter Geoghegan