Re: COPY FROM WHEN condition - Mailing list pgsql-hackers
From | Tomas Vondra |
---|---|
Subject | Re: COPY FROM WHEN condition |
Date | |
Msg-id | 0a3d8cff-0b9d-ea2e-634b-5dcb8d0d5766@2ndquadrant.com Whole thread Raw |
In response to | Re: COPY FROM WHEN condition (Andres Freund <andres@anarazel.de>) |
Responses |
Re: COPY FROM WHEN condition
(Tomas Vondra <tomas.vondra@2ndquadrant.com>)
|
List | pgsql-hackers |
On 1/21/19 7:51 PM, Andres Freund wrote: > Hi, > > On 2019-01-21 16:22:11 +0100, Tomas Vondra wrote: >> >> >> On 1/21/19 4:33 AM, Tomas Vondra wrote: >>> >>> >>> On 1/21/19 3:12 AM, Andres Freund wrote: >>>> On 2019-01-20 18:08:05 -0800, Andres Freund wrote: >>>>> On 2019-01-20 21:00:21 -0500, Tomas Vondra wrote: >>>>>> >>>>>> >>>>>> On 1/20/19 8:24 PM, Andres Freund wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 2019-01-20 00:24:05 +0100, Tomas Vondra wrote: >>>>>>>> On 1/14/19 10:25 PM, Tomas Vondra wrote: >>>>>>>>> On 12/13/18 8:09 AM, Surafel Temesgen wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Dec 12, 2018 at 9:28 PM Tomas Vondra >>>>>>>>>> <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Can you also update the docs to mention that the functions called from >>>>>>>>>> the WHERE clause does not see effects of the COPY itself? >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> /Of course, i also add same comment to insertion method selection >>>>>>>>>> / >>>>>>>>> >>>>>>>>> FWIW I've marked this as RFC and plan to get it committed this week. >>>>>>>>> >>>>>>>> >>>>>>>> Pushed, thanks for the patch. >>>>>>> >>>>>>> While rebasing the pluggable storage patch ontop of this I noticed that >>>>>>> the qual appears to be evaluated in query context. Isn't that a bad >>>>>>> idea? ISMT it should have been evaluated a few lines above, before the: >>>>>>> >>>>>>> /* Triggers and stuff need to be invoked in query context. */ >>>>>>> MemoryContextSwitchTo(oldcontext); >>>>>>> >>>>>>> Yes, that'd require moving the ExecStoreHeapTuple(), but that seems ok? >>>>>>> >>>>>> >>>>>> Yes, I agree. It's a bit too late for me to hack and push stuff, but I'll >>>>>> fix that tomorrow. >>>>> >>>>> NP. On second thought, the problem is probably smaller than I thought at >>>>> first, because ExecQual() switches to the econtext's per-tuple memory >>>>> context. But it's only reset once for each batch, so there's some >>>>> wastage. At least worth a comment. >>>> >>>> I'm tired, but perhaps its actually worse - what's being reset currently >>>> is the ESTate's per-tuple context: >>>> >>>> if (nBufferedTuples == 0) >>>> { >>>> /* >>>> * Reset the per-tuple exprcontext. We can only do this if the >>>> * tuple buffer is empty. (Calling the context the per-tuple >>>> * memory context is a bit of a misnomer now.) >>>> */ >>>> ResetPerTupleExprContext(estate); >>>> } >>>> >>>> but the quals are evaluated in the ExprContext's: >>>> >>>> ExecQual(ExprState *state, ExprContext *econtext) >>>> ... >>>> ret = ExecEvalExprSwitchContext(state, econtext, &isnull); >>>> >>>> >>>> which is created with: >>>> >>>> /* Get an EState's per-output-tuple exprcontext, making it if first use */ >>>> #define GetPerTupleExprContext(estate) \ >>>> ((estate)->es_per_tuple_exprcontext ? \ >>>> (estate)->es_per_tuple_exprcontext : \ >>>> MakePerTupleExprContext(estate)) >>>> >>>> and creates its own context: >>>> /* >>>> * Create working memory for expression evaluation in this context. >>>> */ >>>> econtext->ecxt_per_tuple_memory = >>>> AllocSetContextCreate(estate->es_query_cxt, >>>> "ExprContext", >>>> ALLOCSET_DEFAULT_SIZES); >>>> >>>> so this is currently just never reset. >>> >>> Actually, no. The ResetPerTupleExprContext boils down to >>> >>> MemoryContextReset((econtext)->ecxt_per_tuple_memory) >>> >>> and ExecEvalExprSwitchContext does this >>> >>> MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory); >>> >>> So it's resetting the right context, although only on batch boundary. > >>>> Seems just using ExecQualAndReset() ought to be sufficient? >>>> >>> >>> That may still be the right thing to do. >>> >> >> Actually, no, because that would reset the context far too early (and >> it's easy to trigger segfaults). So the reset would have to happen after >> processing the row, not this early. > > Yea, sorry, I was too tired yesterday evening. I'd spent 10h splitting > up the pluggable storage patch into individual pieces... > > >> But I think the current behavior is actually OK, as it matches what we >> do for defexprs. And the comment before ResetPerTupleExprContext says this: >> >> /* >> * Reset the per-tuple exprcontext. We can only do this if the >> * tuple buffer is empty. (Calling the context the per-tuple >> * memory context is a bit of a misnomer now.) >> */ >> >> So the per-tuple context is not quite per-tuple anyway. Sure, we might >> rework that but I don't think that's an issue in this patch. > > I'm *not* convinced by this. I think it's bad enough that we do this for > normal COPY, but for WHEN, we could end up *never* resetting before the > end. Consider a case where a single tuple is inserted, and then *all* > rows are filtered. I think this needs a separate econtext that's reset > every round. Or alternatively you could fix the code not to rely on > per-tuple not being reset when tuples are buffered - that actually ought > to be fairly simple. > I think separating the per-tuple and per-batch contexts is the right thing to do, here. It seems the batching was added somewhat later and using the per-tuple context is rather confusing. cheers -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
pgsql-hackers by date: