Re: COPY FROM WHEN condition

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