Thread: pgsql: Logical Tape Set: lazily allocate read buffer.
Logical Tape Set: lazily allocate read buffer. The write buffer was already lazily-allocated, so this is more symmetric. It also means that a freshly-rewound tape (whether for reading or writing) is not consuming memory for the buffer. Discussion: https://postgr.es/m/97c46a59c27f3c38e486ca170fcbc618d97ab049.camel%40j-davis.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/7fdd919ae7550f478e7ae4031f7f439278cf2282 Modified Files -------------- src/backend/utils/sort/logtape.c | 43 +++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)
Jeff Davis <jdavis@postgresql.org> writes: > Logical Tape Set: lazily allocate read buffer. Coverity is not very pleased with this patch: it's spewing warnings like 1112 if (lt->buffer == NULL) 1113 ltsInitReadBuffer(lts, lt); 1114 1115 if (blocknum != lt->curBlockNumber) 1116 { >>> CID 1458440: Null pointer dereferences (FORWARD_NULL) >>> Passing null pointer "lt->buffer" to "ltsReadBlock", which dereferences it. 1117 ltsReadBlock(lts, blocknum, (void *) lt->buffer); 1118 lt->curBlockNumber = blocknum; Evidently, it doesn't think that ltsInitReadBuffer() is guaranteed to make lt->buffer not null, which is not so surprising considering that that's coded this way: if (lt->firstBlockNumber != -1L) { Assert(lt->buffer_size > 0); lt->buffer = palloc(lt->buffer_size); } Is there a reason for that to be coded in such an obscure and fragile fashion, rather than having the test be say "if (lt->buffer == NULL)"? If we really need a connection to firstBlockNumber, I'd suggest something like - if (lt->firstBlockNumber != -1L) + if (lt->buffer == NULL) { + Assert(lt->firstBlockNumber != -1L); Assert(lt->buffer_size > 0); lt->buffer = palloc(lt->buffer_size); } but I'm not sure the extra Assert has any value. regards, tom lane
On Sun, 2020-02-16 at 10:51 -0500, Tom Lane wrote: > Is there a reason for that to be coded in such an obscure and fragile > fashion, rather than having the test be say "if (lt->buffer == > NULL)"? I did that to match the original behavior, which is to only allocate the read buffer if the tape is non-empty. A tape with a NULL buffer is in a valid state after a rewind, though it's precarious. It raises quite a few questions about what the valid states of a tape are, and what usages of the API are allowed. That was all true even before 7fdd919a (or perhaps I made a mistake and moving the code around was not safe after all). I think the best fix now is to just allocate the buffer even if the tape is empty. That would stop Coverity from complaining, and I couldn't detect any obvious performance regression. Later, we can document the valid states a little better, and validate them with Asserts and/or the type system. Regards, Jeff Davis