agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Andres Freund <andres@anarazel.de>
Cc: Surafel Temesgen <surafel3000@gmail.com>
Cc: alvherre@2ndquadrant.com, Adam Berlin <berlin.ab@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: COPY FROM WHEN condition
Date: Mon, 21 Jan 2019 16:22:11 +0100
Message-ID: <f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com> (raw)
In-Reply-To: <52b10375-aca3-ea9b-10a1-2e4e9a011665@2ndquadrant.com>
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>
<52b10375-aca3-ea9b-10a1-2e4e9a011665@2ndquadrant.com>
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.
> 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.
>
I think the condition can be just
if (contain_volatile_functions(cstate->whereClause)) { ... }
Per the attached patch. Surafel, do you agree?
>> 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.
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.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
[text/x-patch] copy-when-fix.patch (500B, ../f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com/2-copy-when-fix.patch)
download | inline diff:
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 05d53f96f6..e55b992857 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2612,8 +2612,7 @@ CopyFrom(CopyState cstate)
*/
insertMethod = CIM_SINGLE;
}
- else if (cstate->whereClause != NULL ||
- contain_volatile_functions(cstate->whereClause))
+ else if (contain_volatile_functions(cstate->whereClause))
{
/*
* Can't support multi-inserts if there are any volatile funcation
view thread (90+ messages) latest in thread
Message-ID: <f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com>
Permalink: ../f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com/
Also on: postgresql.org/message-id/f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, andres@anarazel.de, surafel3000@gmail.com, berlin.ab@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: COPY FROM WHEN condition
In-Reply-To: <f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox