From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Surafel Temesgen <surafel3000@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Adam Berlin <berlin.ab@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: COPY FROM WHEN condition
Date: Wed, 23 Jan 2019 01:34:58 +0100
Message-ID: <68edcb85-37cd-132c-b97c-2c7924affb53@2ndquadrant.com> (raw)
In-Reply-To: <CALAY4q8PZFGyK_4215-hFUk9kpgvO7kC=qvbcOghRHfKJRdMJQ@mail.gmail.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>
<f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com>
<CALAY4q8PZFGyK_4215-hFUk9kpgvO7kC=qvbcOghRHfKJRdMJQ@mail.gmail.com>
On 1/22/19 10:00 AM, Surafel Temesgen wrote:
>
>
> On Mon, Jan 21, 2019 at 6:22 PM Tomas Vondra
> <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote:
>
>
> I think the condition can be just
>
> if (contain_volatile_functions(cstate->whereClause)) { ... }
>
>
I've pushed a fix for the volatility check.
Attached is a patch for the other issue, creating a separate batch
context long the lines outlined in the previous email. It's a bit too
late for me to push it now, especially right before a couple of days
off. So I'll push that in a couple of days.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
[text/x-patch] copy-context-fix.patch (6.1K, ../68edcb85-37cd-132c-b97c-2c7924affb53@2ndquadrant.com/2-copy-context-fix.patch)
download | inline diff:diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.cindex 03745cca75..41dbcd5b42 100644--- a/src/backend/commands/copy.c+++ b/src/backend/commands/copy.c@@ -2323,9 +2323,9 @@ CopyFrom(CopyState cstate)
ExprContext *econtext;
TupleTableSlot *myslot;
MemoryContext oldcontext = CurrentMemoryContext;
+ MemoryContext batchcontext;
PartitionTupleRouting *proute = NULL;
- ExprContext *secondaryExprContext = NULL;
ErrorContextCallback errcallback;
CommandId mycid = GetCurrentCommandId(true);
int hi_options = 0; /* start with default heap_insert options */
@@ -2639,20 +2639,10 @@ CopyFrom(CopyState cstate)
* Normally, when performing bulk inserts we just flush the insert
* buffer whenever it becomes full, but for the partitioned table
* case, we flush it whenever the current tuple does not belong to the
- * same partition as the previous tuple, and since we flush the- * previous partition's buffer once the new tuple has already been- * built, we're unable to reset the estate since we'd free the memory- * in which the new tuple is stored. To work around this we maintain- * a secondary expression context and alternate between these when the- * partition changes. This does mean we do store the first new tuple- * in a different context than subsequent tuples, but that does not- * matter, providing we don't free anything while it's still needed.+ * same partition as the previous tuple.
*/
if (proute)
- {
insertMethod = CIM_MULTI_CONDITIONAL;
- secondaryExprContext = CreateExprContext(estate);- }
else
insertMethod = CIM_MULTI;
@@ -2685,6 +2675,14 @@ CopyFrom(CopyState cstate)
errcallback.previous = error_context_stack;
error_context_stack = &errcallback;
+ /*+ * Set up memory context for batches. For cases without batching we could+ * use the per-tuple context, but it does not seem worth the complexity.+ */+ batchcontext = AllocSetContextCreate(CurrentMemoryContext,+ "batch context",+ ALLOCSET_DEFAULT_SIZES);+
for (;;)
{
TupleTableSlot *slot;
@@ -2692,18 +2690,14 @@ CopyFrom(CopyState cstate)
CHECK_FOR_INTERRUPTS();
- 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);- }+ /*+ * Reset the per-tuple exprcontext. We do this after every tuple, to+ * clean-up after expression evaluations etc.+ */+ ResetPerTupleExprContext(estate);- /* Switch into its memory context */- MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));+ /* Switch into per-batch memory context. */+ MemoryContextSwitchTo(batchcontext);
if (!NextCopyFrom(cstate, econtext, values, nulls))
break;
@@ -2756,7 +2750,7 @@ CopyFrom(CopyState cstate)
*/
if (nBufferedTuples > 0)
{
- ExprContext *swapcontext;+ MemoryContext oldcontext;
CopyFromInsertBatch(cstate, estate, mycid, hi_options,
prevResultRelInfo, myslot, bistate,
@@ -2765,29 +2759,26 @@ CopyFrom(CopyState cstate)
nBufferedTuples = 0;
bufferedTuplesSize = 0;
- Assert(secondaryExprContext);-
/*
- * Normally we reset the per-tuple context whenever- * the bufferedTuples array is empty at the beginning- * of the loop, however, it is possible since we flush- * the buffer here that the buffer is never empty at- * the start of the loop. To prevent the per-tuple- * context from never being reset we maintain a second- * context and alternate between them when the- * partition changes. We can now reset- * secondaryExprContext as this is no longer needed,- * since we just flushed any tuples stored in it. We- * also now switch over to the other context. This- * does mean that the first tuple in the buffer won't- * be in the same context as the others, but that does- * not matter since we only reset it after the flush.+ * The tuple is allocated in the batch context, which we+ * want to reset. So to keep the tuple we copy the tuple+ * into the short-lived (per-tuple) context, reset the+ * batch context and then copy it back into it.
*/
- ReScanExprContext(secondaryExprContext);+ oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));+ tuple = heap_copytuple(tuple);+ MemoryContextSwitchTo(oldcontext);- swapcontext = secondaryExprContext;- secondaryExprContext = estate->es_per_tuple_exprcontext;- estate->es_per_tuple_exprcontext = swapcontext;+ /* cleanup the old batch */+ MemoryContextReset(batchcontext);++ /* copy the tuple back to the per-tuple context */+ oldcontext = MemoryContextSwitchTo(batchcontext);+ tuple = heap_copytuple(tuple);+ MemoryContextSwitchTo(oldcontext);++ /* push the tuple copy to the slot */+ ExecStoreHeapTuple(tuple, slot, false);
}
nPartitionChanges++;
@@ -2893,10 +2884,10 @@ CopyFrom(CopyState cstate)
slot = execute_attr_map_slot(map->attrMap, slot, new_slot);
/*
- * Get the tuple in the per-tuple context, so that it will be+ * Get the tuple in the per-batch context, so that it will be
* freed after each batch insert.
*/
- oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));+ oldcontext = MemoryContextSwitchTo(batchcontext);
tuple = ExecCopySlotHeapTuple(slot);
MemoryContextSwitchTo(oldcontext);
}
@@ -2972,6 +2963,9 @@ CopyFrom(CopyState cstate)
firstBufferedLineNo);
nBufferedTuples = 0;
bufferedTuplesSize = 0;
++ /* free memory occupied by tuples from the batch */+ MemoryContextReset(batchcontext);
}
}
else
@@ -3053,6 +3047,8 @@ CopyFrom(CopyState cstate)
MemoryContextSwitchTo(oldcontext);
+ MemoryContextDelete(batchcontext);+
/*
* In the old protocol, tell pqcomm that we can process normal protocol
* messages again.
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, surafel3000@gmail.com, andres@anarazel.de, alvherre@2ndquadrant.com, berlin.ab@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: COPY FROM WHEN condition
In-Reply-To: <68edcb85-37cd-132c-b97c-2c7924affb53@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 DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox