Re: [HACKERS] Missing CHECK_FOR_INTERRUPTS in hash joins
Thomas Munrowrites: > On Wed, Feb 15, 2017 at 2:22 PM, Tom Lane 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
Re: [HACKERS] Missing CHECK_FOR_INTERRUPTS in hash joins
On Wed, Feb 15, 2017 at 2:22 PM, Tom Lanewrote: > 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
[HACKERS] Missing CHECK_FOR_INTERRUPTS in hash joins
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, , 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