Batch insert in CTAS/MatView code

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: Batch insert in CTAS/MatView code

Michael Paquier-2
On Mon, Sep 30, 2019 at 12:38:02AM -0700, Andres Freund wrote:
> That does not strike me as a good idea. The upper layer is going to need
> to manage some resources (e.g. it's the only bit that knows about how to
> manage lifetime of the incoming data), and by exposing it to each AM
> we're going to duplicate the necessary code too.

Latest status of the thread maps with a patch which still applies,
still the discussion could go on as more review is needed.  So I have
moved it to next CF.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Batch insert in CTAS/MatView code

Paul Guo-2
I took some time on digging the issue yesterday so the main concern of the
patch is to get the tuple length from ExecFetchSlotHeapTuple().

+   ExecCopySlot(batchslot, slot);
+   tuple = ExecFetchSlotHeapTuple(batchslot, true, NULL);
+
+   myState->mi_slots_num++;
+   myState->mi_slots_size += tuple->t_len;

We definitely should remove ExecFetchSlotHeapTuple() here but we need to
know the tuple length (at least rough length). One solution might be using
memory context stat info as mentioned, the code looks like this.
 
tup_len = MemoryContextUsedSize(batchslot->tts_mcxt);
ExecCopySlot(batchslot, slot);
ExecMaterializeSlot(batchslot);
tup_len = MemoryContextUsedSize(batchslot->tts_mcxt) - tup_len;

MemoryContextUsedSize() is added to calculate the total used size (simply hack to use the stats interface).

+int
+MemoryContextUsedSize(MemoryContext context)
+{
+   MemoryContextCounters total;
+
+   memset(&total, 0, sizeof(total));
+   context->methods->stats(context, NULL, NULL, &total);
+
+   return total.totalspace - total.freespace;
+}

This basically works but there are concerns:

   The length is not accurate (though we do not need to be that accurate) since there are
   some additional memory allocations, but we are not sure if the size is not much
   larger than the real length for some slot types in the future and I'm not sure whether we
   definitely allocate at least the tuple length in the memory context after materialization
   for all slot types in the future. Last is that the code seems to be a bit ugly also.

   As a reference, For "create table t1 as select * from t2", the above code returns
   "tuple length" is 88 (real tuple length is 4).

Another solution is that maybe return the real size in ExecMaterializeSlot()? e.g.
ExecMaterializeSlot(slot, &tup_len); For this we probably want to store the length
in the slot struct for performance.

For the COPY case the tuple length is known in advance but I can image more cases
which do not know the size but need that for the multi_insert interface, at least I'm
wondering if we should use that in 'refresh matview' and fast path for Insert node
(I heard some complaints about the performance of "insert into tbl from select..."
from some of our users)? So the concern is not just for the case in this patch.

Besides, My colleagues Ashwin Agrawal and Adam Lee found maybe could
try raw_heap_insert() similar code for ctas and compare since it is do insert in
a new created table. That would involve more discussions, much more code
change and need to test more (stability and performance). So multi insert seems
to be a stable solution in a short time given that has been used in COPY for a
long time?

Whatever the solution for CTAS we need to address the concern of tuple size
for multi insert cases.





Reply | Threaded
Open this post in threaded view
|

Re: Batch insert in CTAS/MatView code

Daniel Gustafsson
In an off-list discussion with Paul, we decided to withdraw this patch for now
and instead create a new entry when there is a re-worked patch.  This has now
been done in the CF app.

cheers ./daniel


12