pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Logical Tape Set: lazily allocate read buffer.
3+ messages / 3 participants
[nested] [flat]
* pgsql: Logical Tape Set: lazily allocate read buffer.
@ 2020-02-13 18:45 Jeff Davis <jdavis@postgresql.org>
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Davis @ 2020-02-13 18:45 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
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(-)
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pgsql: Logical Tape Set: lazily allocate read buffer.
@ 2020-02-16 15:51 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Davis <jdavis@postgresql.org>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2020-02-16 15:51 UTC (permalink / raw)
To: Jeff Davis <jdavis@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pgsql: Logical Tape Set: lazily allocate read buffer.
@ 2020-02-18 21:25 Jeff Davis <pgsql@j-davis.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Davis @ 2020-02-18 21:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Jeff Davis <jdavis@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2020-02-18 21:25 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13 18:45 pgsql: Logical Tape Set: lazily allocate read buffer. Jeff Davis <jdavis@postgresql.org>
2020-02-16 15:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-18 21:25 ` Jeff Davis <pgsql@j-davis.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox