Re: Multi Inserts in CREATE TABLE AS - revived patch

On 23-11-2020 11:23, Bharath Rupireddy wrote:
> On Mon, Nov 23, 2020 at 3:26 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>
>> On 23/11/2020 11:15, Bharath Rupireddy wrote:
>>> Attaching v2 patch, rebased on the latest master 17958972.
>>
>> I just broke this again with commit c532d15ddd to split up copy.c.
>> Here's another rebased version.
>>
> 
> Thanks! I noticed that and am about to post a new patch. Anyways,
> thanks for the rebased v3 patch. Attaching here v3 again for
> visibility.
> 
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com
> 

Hi,

Thanks for reviving the patch! I did unfortunately have to shift my 
priorities somewhat and did not find much time to work on open source 
things the last week(s).

I'm wondering about the use of the GetTupleSize function. As far as I 
understand the idea is to limit the amount of buffered data, presumably 
to not write too much data at once for intorel_flush_multi_insert.
If I understood correctly how it all works, the table slot can however 
be of different type than the source slot, which makes that the call to 
CopySlot() potentially stores a different amount of data than computed 
by GetTupleSize(). Not sure if this is a big problem as an estimation 
might be good enough?

Some other solutions/implementations would be:
- compute the size after doing CopySlot. Maybe the relation never wants 
a virtual tuple and then you can also simplify GetTupleSize?
- after CopySlot ask for the memory consumed in the slot using 
MemoryContextMemAllocated.

Some small things to maybe change are:
===========
+        if (myState->mi_slots[myState->mi_slots_num] == NULL)
+        {
+            batchslot = table_slot_create(myState->rel, NULL);
+            myState->mi_slots[myState->mi_slots_num] = batchslot;
+        }
+        else
+            batchslot = myState->mi_slots[myState->mi_slots_num];

Alternative:
+        if (myState->mi_slots[myState->mi_slots_num] == NULL)
+            myState->mi_slots[myState->mi_slots_num] = 
table_slot_create(myState->rel, NULL);
+        batchslot = myState->mi_slots[myState->mi_slots_num];

==============

+            sz = att_align_nominal(sz, att->attalign);
This could be moved out of the if statement?

==============

Regards,
Luc
Swarm64