Subject: Re: COPY FROM WHEN condition
To: Andres Freund <andres@anarazel.de>
Cc: Surafel Temesgen <surafel3000@gmail.com>, alvherre@2ndquadrant.com,
 Adam Berlin <berlin.ab@gmail.com>, pgsql-hackers@lists.postgresql.org
References: <194e2225-b186-9325-0cd1-9a5b09d28251@2ndquadrant.com>
 <20181204094418.wpr6mxrtsuxb5mlq@alvherre.pgsql>
 <CALAY4q_Ej0QyQJOXMdMF6i9ufBnDfJP0DRhm910F4Qm5AV5xUw@mail.gmail.com>
 <3b2b1aac-5861-acd8-fda3-054c4e4da888@2ndquadrant.com>
 <CALAY4q-6ffQPi+CZMVjddEMXe_QBrqfz6aauc+QPxSLEO9vUQQ@mail.gmail.com>
 <b5234b76-9ee0-6a44-6a21-d90c385a27b8@2ndquadrant.com>
 <ea03bbf3-c201-5ce1-eee9-9fc4b527fd03@2ndquadrant.com>
 <20190121012405.633row4iw7nfxz7h@alap3.anarazel.de>
 <6e80b7c1-58cd-4212-41d3-7c519aed0500@2ndquadrant.com>
 <20190121020805.3kdscvwzwvijlees@alap3.anarazel.de>
 <20190121021220.7tbu3oizvydov5by@alap3.anarazel.de>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <52b10375-aca3-ea9b-10a1-2e4e9a011665@2ndquadrant.com>
Date: Mon, 21 Jan 2019 04:33:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190121021220.7tbu3oizvydov5by@alap3.anarazel.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.
But now I see 31f38174 does this:

    else if (cstate->whereClause != NULL ||
             contain_volatile_functions(cstate->whereClause))
    {
        ...
        insertMethod = CIM_SINGLE;
    }

so it does not do batching with WHERE. But the condition seems wrong, I
guess it should be && instead of ||. Will investigate in the morning.

> Seems just using ExecQualAndReset() ought to be sufficient?
> 

That may still be the right thing to do.


cheers

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services