On Mon, Sep 14, 2020 at 7:09 PM Peter Geoghegan <p...@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
