pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedMissing CHECK_FOR_INTERRUPTS in hash joins
3+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2017-02-15 f2ec57dee9 354dfa235b 96ba176400 d0e9c0e319 6a4941f8c6 030705e4fe Make sure that hash join's bulk-tuple-transfer loops are interruptible.
* Missing CHECK_FOR_INTERRUPTS in hash joins
@ 2017-02-15 01:22 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 3+ messages in thread
From: Tom Lane @ 2017-02-15 01:22 UTC (permalink / raw)
To: pgsql-hackers
I ran into a case where a hash join took a really long time to respond
to a cancel request --- long enough that I gave up and kill -9'd it,
because its memory usage was also growing to the point where the kernel
would likely soon choose to do that for me. The culprit seems to be
that there's no CHECK_FOR_INTERRUPTS anywhere in this loop in
ExecHashJoinNewBatch():
while ((slot = ExecHashJoinGetSavedTuple(hjstate,
innerFile,
&hashvalue,
hjstate->hj_HashTupleSlot)))
{
/*
* NOTE: some tuples may be sent to future batches. Also, it is
* possible for hashtable->nbatch to be increased here!
*/
ExecHashTableInsert(hashtable, slot, hashvalue);
}
so that if you try to cancel while it's sucking a really large batch file
into memory, you lose. (In the pathological case I was checking, the
batch file was many gigabytes in size, and had certainly never all been
resident earlier.)
Adding a C.F.I. inside this loop is the most straightforward fix, but
I am leaning towards adding one in ExecHashJoinGetSavedTuple instead,
because that would also ensure that all successful paths through
ExecHashJoinOuterGetTuple will do a C.F.I. somewhere, and it seems good
for that to be consistent. The other possibility is to put one inside
ExecHashTableInsert, but the only other caller of that doesn't really need
it, since it's relying on ExecProcNode to do one.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Missing CHECK_FOR_INTERRUPTS in hash joins
@ 2017-02-15 02:16 Thomas Munro <thomas.munro@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 3+ messages in thread
From: Thomas Munro @ 2017-02-15 02:16 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers
On Wed, Feb 15, 2017 at 2:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Adding a C.F.I. inside this loop is the most straightforward fix, but
> I am leaning towards adding one in ExecHashJoinGetSavedTuple instead,
> because that would also ensure that all successful paths through
> ExecHashJoinOuterGetTuple will do a C.F.I. somewhere, and it seems good
> for that to be consistent. The other possibility is to put one inside
> ExecHashTableInsert, but the only other caller of that doesn't really need
> it, since it's relying on ExecProcNode to do one.
Would it also make sense to put one in the loop in
ExecHashIncreaseNumBatches (or perhaps
ExecHashJoinSaveTuple for symmetry with the above)? Otherwise you
might have to wait for a few hundred MB of tuples to be written out
which could be slow if IO is somehow overloaded.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Missing CHECK_FOR_INTERRUPTS in hash joins
@ 2017-02-15 21:03 Tom Lane <tgl@sss.pgh.pa.us>
parent: Thomas Munro <thomas.munro@enterprisedb.com>
0 siblings, 0 replies; 3+ messages in thread
From: Tom Lane @ 2017-02-15 21:03 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: pgsql-hackers
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Wed, Feb 15, 2017 at 2:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Adding a C.F.I. inside this loop is the most straightforward fix, but
>> I am leaning towards adding one in ExecHashJoinGetSavedTuple instead,
> Would it also make sense to put one in the loop in
> ExecHashIncreaseNumBatches (or perhaps
> ExecHashJoinSaveTuple for symmetry with the above)? Otherwise you
> might have to wait for a few hundred MB of tuples to be written out
> which could be slow if IO is somehow overloaded.
Mmm, good point. I think in that case the C.F.I. had better be in
the loop in ExecHashIncreaseNumBatches, because if you were unlucky
the loop might not take the ExecHashJoinSaveTuple path for a long time.
Looking around at other callers of ExecHashJoinSaveTuple, the only one
that seems to be in need of a C.F.I. is the loop in
ExecHashRemoveNextSkewBucket, and there again there's a code path
whereby the loop doesn't call ExecHashJoinSaveTuple.
Will CFI-ify all three places.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2017-02-15 21:03 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-02-15 01:22 Missing CHECK_FOR_INTERRUPTS in hash joins Tom Lane <tgl@sss.pgh.pa.us>
2017-02-15 02:16 ` Thomas Munro <thomas.munro@enterprisedb.com>
2017-02-15 21:03 ` Tom Lane <tgl@sss.pgh.pa.us>
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