Fix missing call to table_finish_bulk_insert during COPY

david-rowley · david-rowley · commit 4de60244e2fc · 2019-07-02T01:23:26.000+12:00
86b8504 abstracted calls to heap functions in COPY FROM to support a generic table AM. However, when performing a copy into a partitioned table, this commit neglected to call table_finish_bulk_insert for each partition. Before 86b8504, when we always called the heap functions, there was no need to call heapam_finish_bulk_insert for partitions since it only did any work when performing a copy without WAL. For partitioned tables, this was unsupported anyway, so there was no issue. With pluggable storage, we can't make any assumptions about what the table AM might want to do in its equivalent function, so we'd better ensure we always call table_finish_bulk_insert each partition that's received a row. For now, we make the table_finish_bulk_insert call whenever we evict a CopyMultiInsertBuffer out of the CopyMultiInsertInfo. This does mean that it's possible that we call table_finish_bulk_insert multiple times per partition, which is not a problem other than being an inefficiency. Improving this requires a more invasive patch, so let's leave that for another day. In passing, move the table_finish_bulk_insert for the target of the COPY command so that it's only called when we're actually performing bulk inserts. We don't need to call this when inserting 1 row at a time. Reported-by: Robert Haas Discussion: https://postgr.es/m/CA+TgmoYK=6BpxiJ0tN-p9wtH0BTAfbdxzHhwou0mdud4+BkYuQ@mail.gmail.com
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
@@ -2518,7 +2518,8 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
  * The buffer must be flushed before cleanup.
  */
 static inline void
-CopyMultiInsertBufferCleanup(CopyMultiInsertBuffer *buffer)
+CopyMultiInsertBufferCleanup(CopyMultiInsertInfo *miinfo,
+							 CopyMultiInsertBuffer *buffer)
 {
 	int			i;
 
@@ -2534,6 +2535,9 @@ CopyMultiInsertBufferCleanup(CopyMultiInsertBuffer *buffer)
 	for (i = 0; i < MAX_BUFFERED_TUPLES && buffer->slots[i] != NULL; i++)
 		ExecDropSingleTupleTableSlot(buffer->slots[i]);
 
+	table_finish_bulk_insert(buffer->resultRelInfo->ri_RelationDesc,
+							 miinfo->ti_options);
+
 	pfree(buffer);
 }
 
@@ -2585,7 +2589,7 @@ CopyMultiInsertInfoFlush(CopyMultiInsertInfo *miinfo, ResultRelInfo *curr_rri)
 			buffer = (CopyMultiInsertBuffer *) linitial(miinfo->multiInsertBuffers);
 		}
 
-		CopyMultiInsertBufferCleanup(buffer);
+		CopyMultiInsertBufferCleanup(miinfo, buffer);
 		miinfo->multiInsertBuffers = list_delete_first(miinfo->multiInsertBuffers);
 	}
 }
@@ -2599,7 +2603,7 @@ CopyMultiInsertInfoCleanup(CopyMultiInsertInfo *miinfo)
 	ListCell   *lc;
 
 	foreach(lc, miinfo->multiInsertBuffers)
-		CopyMultiInsertBufferCleanup(lfirst(lc));
+		CopyMultiInsertBufferCleanup(miinfo, lfirst(lc));
 
 	list_free(miinfo->multiInsertBuffers);
 }
@@ -3321,9 +3325,6 @@ CopyFrom(CopyState cstate)
 	{
 		if (!CopyMultiInsertInfoIsEmpty(&multiInsertInfo))
 			CopyMultiInsertInfoFlush(&multiInsertInfo, NULL);
-
-		/* Tear down the multi-insert buffer data */
-		CopyMultiInsertInfoCleanup(&multiInsertInfo);
 	}
 
 	/* Done, clean up */
@@ -3366,7 +3367,13 @@ CopyFrom(CopyState cstate)
 
 	FreeExecutorState(estate);
 
-	table_finish_bulk_insert(cstate->rel, ti_options);
+	if (insertMethod != CIM_SINGLE)
+	{
+		table_finish_bulk_insert(cstate->rel, ti_options);
+
+		/* Tear down the multi-insert buffer data */
+		CopyMultiInsertInfoCleanup(&multiInsertInfo);
+	}
 
 	return processed;
 }

Original file line number	Diff line number	Diff line change
`@@ -2518,7 +2518,8 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,`
`2518`	`2518`	`* The buffer must be flushed before cleanup.`
`2519`	`2519`	`*/`
`2520`	`2520`	`static inline void`
`2521`		`-CopyMultiInsertBufferCleanup(CopyMultiInsertBuffer *buffer)`
	`2521`	`+CopyMultiInsertBufferCleanup(CopyMultiInsertInfo *miinfo,`
	`2522`	`+ CopyMultiInsertBuffer *buffer)`
`2522`	`2523`	`{`
`2523`	`2524`	`int i;`
`2524`	`2525`
`@@ -2534,6 +2535,9 @@ CopyMultiInsertBufferCleanup(CopyMultiInsertBuffer *buffer)`
`2534`	`2535`	`for (i = 0; i < MAX_BUFFERED_TUPLES && buffer->slots[i] != NULL; i++)`
`2535`	`2536`	`ExecDropSingleTupleTableSlot(buffer->slots[i]);`
`2536`	`2537`
	`2538`	`+ table_finish_bulk_insert(buffer->resultRelInfo->ri_RelationDesc,`
	`2539`	`+ miinfo->ti_options);`
	`2540`	`+`
`2537`	`2541`	`pfree(buffer);`
`2538`	`2542`	`}`
`2539`	`2543`
`@@ -2585,7 +2589,7 @@ CopyMultiInsertInfoFlush(CopyMultiInsertInfo miinfo, ResultRelInfo curr_rri)`
`2585`	`2589`	`buffer = (CopyMultiInsertBuffer *) linitial(miinfo->multiInsertBuffers);`
`2586`	`2590`	`}`
`2587`	`2591`
`2588`		`- CopyMultiInsertBufferCleanup(buffer);`
	`2592`	`+ CopyMultiInsertBufferCleanup(miinfo, buffer);`
`2589`	`2593`	`miinfo->multiInsertBuffers = list_delete_first(miinfo->multiInsertBuffers);`
`2590`	`2594`	`}`
`2591`	`2595`	`}`
`@@ -2599,7 +2603,7 @@ CopyMultiInsertInfoCleanup(CopyMultiInsertInfo *miinfo)`
`2599`	`2603`	`ListCell *lc;`
`2600`	`2604`
`2601`	`2605`	`foreach(lc, miinfo->multiInsertBuffers)`
`2602`		`- CopyMultiInsertBufferCleanup(lfirst(lc));`
	`2606`	`+ CopyMultiInsertBufferCleanup(miinfo, lfirst(lc));`
`2603`	`2607`
`2604`	`2608`	`list_free(miinfo->multiInsertBuffers);`
`2605`	`2609`	`}`
`@@ -3321,9 +3325,6 @@ CopyFrom(CopyState cstate)`
`3321`	`3325`	`{`
`3322`	`3326`	`if (!CopyMultiInsertInfoIsEmpty(&multiInsertInfo))`
`3323`	`3327`	`CopyMultiInsertInfoFlush(&multiInsertInfo, NULL);`
`3324`		`-`
`3325`		`- /* Tear down the multi-insert buffer data */`
`3326`		`- CopyMultiInsertInfoCleanup(&multiInsertInfo);`
`3327`	`3328`	`}`
`3328`	`3329`
`3329`	`3330`	`/* Done, clean up */`
`@@ -3366,7 +3367,13 @@ CopyFrom(CopyState cstate)`
`3366`	`3367`
`3367`	`3368`	`FreeExecutorState(estate);`
`3368`	`3369`
`3369`		`- table_finish_bulk_insert(cstate->rel, ti_options);`
	`3370`	`+ if (insertMethod != CIM_SINGLE)`
	`3371`	`+ {`
	`3372`	`+ table_finish_bulk_insert(cstate->rel, ti_options);`
	`3373`	`+`
	`3374`	`+ /* Tear down the multi-insert buffer data */`
	`3375`	`+ CopyMultiInsertInfoCleanup(&multiInsertInfo);`
	`3376`	`+ }`
`3370`	`3377`
`3371`	`3378`	`return processed;`
`3372`	`3379`	`}`