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.von...@2ndquadrant.com <mailto:tomas.von...@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.
So context, much simple. Wow.
> Seems just using ExecQualAndReset() ought to be sufficient?
>
Seems like it.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
