Re: Batch insert in CTAS/MatView code

Hi,

On 2019-09-09 18:31:54 +0800, Paul Guo wrote:
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index e9544822bf..8a844b3b5f 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -2106,7 +2106,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
> CommandId cid, int options, BulkInsertState bistate)
> {
> TransactionId xid = GetCurrentTransactionId();
> - HeapTuple *heaptuples;
> int i;
> int ndone;
> PGAlignedBlock scratch;
> @@ -2115,6 +2114,10 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
> Size saveFreeSpace;
> bool need_tuple_data = RelationIsLogicallyLogged(relation);
> bool need_cids = RelationIsAccessibleInLogicalDecoding(relation);
> + /* Declare it as static to let this memory be not on stack. */
> + static HeapTuple heaptuples[MAX_MULTI_INSERT_TUPLES];
> +
> + Assert(ntuples <= MAX_MULTI_INSERT_TUPLES);
>
> /* currently not needed (thus unsupported) for heap_multi_insert() */
> AssertArg(!(options & HEAP_INSERT_NO_LOGICAL));
> @@ -2124,7 +2127,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
> HEAP_DEFAULT_FILLFACTOR);
>
> /* Toast and set header data in all the slots */
> - heaptuples = palloc(ntuples * sizeof(HeapTuple));
> for (i = 0; i < ntuples; i++)
> {
> HeapTuple tuple;

I don't think this is a good idea. We shouldn't unnecessarily allocate
8KB on the stack. Is there any actual evidence this is a performance
benefit? To me this just seems like it'll reduce the flexibility of the
API, without any benefit. I'll also note that you've apparently not
updated tableam.h to document this new restriction.

Greetings,

Andres Freund