On Wed, 3 Apr 2019 at 18:56, Andres Freund <and...@anarazel.de> wrote: > > +/* Class the buffer full if there are >= this many bytes of tuples stored > > */ > > +#define MAX_BUFFERED_BYTES 65535 > > Random aside: This seems pretty small (but should be changed separately.
Yeah, my fingers were hovering over this. I was close to making it 1MB, but thought I'd better not. > > +typedef struct CopyMultiInsertBuffer > > +{ > > + TupleTableSlot *slots[MAX_BUFFERED_TUPLES]; /* Array to store tuples > > */ > > + ResultRelInfo *resultRelInfo; /* ResultRelInfo for 'relid' */ > > + BulkInsertState bistate; /* BulkInsertState for this rel */ > > + int nused; /* number of 'slots' > > containing tuples */ > > + uint64 linenos[MAX_BUFFERED_TUPLES]; /* Line # of tuple in > > copy > > + > > * stream */ > > +} CopyMultiInsertBuffer; > > I don't think it's needed now, but you'd probably achieve a bit better > locality by storing slots + linenos in a single array of (slot,lineno). You're right, but right now I only zero the slots array and leave the linenos uninitialised. Merging them to one would double the area of memory to zero. I'm not sure if that's worse or not, but I do know if the number of partitions exceeds MAX_PARTITION_BUFFERS and we change partitions quite a lot during the copy then we could end up initialising buffers quite often. I wanted to keep that as cheap as possible. > > +/* > > + * CopyMultiInsertBuffer_Init > > + * Allocate memory and initialize a new CopyMultiInsertBuffer > > for this > > + * ResultRelInfo. > > + */ > > +static CopyMultiInsertBuffer * > > +CopyMultiInsertBuffer_Init(ResultRelInfo *rri) > > +{ > > + CopyMultiInsertBuffer *buffer; > > + > > + buffer = (CopyMultiInsertBuffer *) > > palloc(sizeof(CopyMultiInsertBuffer)); > > + memset(buffer->slots, 0, sizeof(TupleTableSlot *) * > > MAX_BUFFERED_TUPLES); > > Is there a reason to not just directly palloc0? Yeah, I'm too cheap to zero the entire thing, per what I mentioned above. linenos does not really need anything meaningful put there during init. It'll get set when we consume the slots. > > + > > + /* Remove back-link to ourself */ > > + buffer->resultRelInfo->ri_CopyMultiInsertBuffer = NULL; > > + > > + ReleaseBulkInsertStatePin(buffer->bistate); > > Hm, afaict this still leaks the bistate itself? Oops, I forgot about that one. v4 attached. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
david_tableam_copy_v4.patch
Description: Binary data