Subject: Re: COPY FROM WHEN condition
To: Surafel Temesgen <surafel3000@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
 Alvaro Herrera <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>
 <52b10375-aca3-ea9b-10a1-2e4e9a011665@2ndquadrant.com>
 <f66a0132-c707-0a3e-fb53-5880ec06bf28@2ndquadrant.com>
 <CALAY4q8PZFGyK_4215-hFUk9kpgvO7kC=qvbcOghRHfKJRdMJQ@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Message-ID: <68edcb85-37cd-132c-b97c-2c7924affb53@2ndquadrant.com>
Date: Wed, 23 Jan 2019 01:34:58 +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: 
 <CALAY4q8PZFGyK_4215-hFUk9kpgvO7kC=qvbcOghRHfKJRdMJQ@mail.gmail.com>
Content-Type: multipart/mixed;
 boundary="------------FCBAE275A70A53C0DCF2F0DA"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------FCBAE275A70A53C0DCF2F0DA
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit


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

--------------FCBAE275A70A53C0DCF2F0DA
Content-Type: text/x-patch;
 name="copy-context-fix.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="copy-context-fix.patch"

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 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.

--------------FCBAE275A70A53C0DCF2F0DA--