On Mon, Sep 14, 2020 at 7:09 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Oh, wait. Of course the point was that we wouldn't even have to use
> nBlocksAllocated in LogicalTapeSetBlocks() anymore -- we would make
> the assumption that nBlocksWritten could be used for all callers in
> all cases. Which is a reasonable assumption once you enforce that
> there are no active write buffers. Which is evidently a good idea
> anyway, since it saves on temp file disk space in
> HashAggs-that-spill/prealloc cases with very little work_mem.
Let's assume that we'll teach LogicalTapeSetBlocks() to use
nBlocksWritten in place of nBlocksAllocated in all cases, as now seems
likely. Rather than asserting "nBlocksWritten == nBlocksAllocated"
inside LogicalTapeSetBlocks() (as I suggested earlier at one point),
we could instead teach LogicalTapeSetBlocks() to iterate through each
tape from the tapeset and make sure each tape has no writes buffered
(so everything must be flushed). We could add a loop that would only
be used on assert-enabled builds.
This looping-through-tapes-to assert code would justify relying on
nBlocksWritten in LogicalTapeSetBlocks(), and would make sure that we
don't let any bugs like this slip in in the future. It would
necessitate that we commit Jeff's hashagg-release-write-buffers.patch
patch from earlier, I think, but that seems like a good idea anyway.
You suggested this yourself, Jeff (my suggestion about the assertion
is just an expansion on your suggestion from earlier). This all seems
like a good idea to me. Can you write a patch that adjusts
LogicalTapeSetBlocks() along these lines? Hopefully the assertion loop
thing won't reveal some other problem with this plan.
Thanks
--
Peter Geoghegan