Re: COPY FROM WHEN condition

Hi,

On 2019-04-02 04:23:20 +1300, David Rowley wrote:
> On Mon, 1 Apr 2019 at 08:05, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I'll work on pushing all the other pending tableam patches today -
> > leaving COPY the last non pluggable part. You'd written in a private
> > email that you might try to work on this on Monday, so I think I'll give
> > this a shot on Tuesday if you've not gotten around till then? I'd like
> > to push this sooner than the exact end of the freeze...
>
> I worked on this, but I've not got the patch finished yet.

Thanks! I'm not quite clear whether you planning to continue working on
this, or whether this is a handoff? Either is fine with me, just trying
to avoid unnecessary work / delay.

> Of course, it is possible that some of the bugs account for some of
> the improved time... but the rows did seem to be in the table
> afterwards.

The speedup likely seems to be from having much larger batches, right?
Unless we insert into enough partitions that batching doesn't ever
encounter two tuples, we'll now efficiently use multi_insert even if
there's no consecutive tuples.

> if (cstate->rel)
> {
> - Datum *values;
> - bool *nulls;
> + TupleTableSlot *slot;
> TableScanDesc scandesc;
> - HeapTuple tuple;
> -
> - values = (Datum *) palloc(num_phys_attrs * sizeof(Datum));
> - nulls = (bool *) palloc(num_phys_attrs * sizeof(bool));
>
> scandesc = table_beginscan(cstate->rel, GetActiveSnapshot(), 0, NULL);
> + slot = table_slot_create(cstate->rel, NULL);
>
> processed = 0;
> - while ((tuple = heap_getnext(scandesc, ForwardScanDirection)) != NULL)
> + while (table_scan_getnextslot(scandesc, ForwardScanDirection, slot))
> {
> CHECK_FOR_INTERRUPTS();
>
> - /* Deconstruct the tuple ... faster than repeated heap_getattr */
> - heap_deform_tuple(tuple, tupDesc, values, nulls);
> + /* Deconstruct the tuple ... */
> + slot_getallattrs(slot);
>
> /* Format and send the data */
> - CopyOneRowTo(cstate, values, nulls);
> + CopyOneRowTo(cstate, slot);
> processed++;
> }
>
> + ExecDropSingleTupleTableSlot(slot);
> table_endscan(scandesc);
> -
> - pfree(values);
> - pfree(nulls);
> }

Hm, I should just have committed this part separately. Not sure why I
didn't...

> +#define MAX_BUFFERED_TUPLES 1000
> +#define MAX_BUFFERED_BYTES 65535
> +#define MAX_PARTITION_BUFFERS 16
> +
> +typedef struct {
> + Oid relid;
> + TupleTableSlot **slots;
> + ResultRelInfo *resultRelInfo;
> + int nused;
> +} CopyMultiInsertBuffer;
> +
> +typedef struct {
> + HTAB *multiInsertBufferTab;
> + CopyMultiInsertBuffer *buffer;
> + int bufferedTuples;
> + int bufferedBytes;
> + int nbuffers;
> +} CopyMultiInsertInfo;

To me it seems a bit better to have this as generic facility - we really
should use multi_insert in more places (like inserting the pg_attribute
rows). But admittedly that can just be done later.

> +static inline void
> +CopyMultiInsertBuffer_RemoveBuffer(CopyMultiInsertInfo *miinfo, CopyMultiInsertBuffer *buffer)
> +{
> + Oid relid = buffer->relid;
> + int i = 0;
> +
> + while (buffer->slots[i] != NULL)
> + ExecClearTuple(buffer->slots[i++]);
> + pfree(buffer->slots);
> +
> + hash_search(miinfo->multiInsertBufferTab, (void *) &relid, HASH_REMOVE,
> + NULL);
> + miinfo->nbuffers--;
> +}

Hm, this leaves dangling slots, right?

> +static inline void
> +CopyMultiInsertInfo_SetCurrentBuffer(CopyMultiInsertInfo *miinfo,
> + ResultRelInfo *rri)
> +{
> + CopyMultiInsertBuffer *buffer;
> + Oid relid = RelationGetRelid(rri->ri_RelationDesc);
> + bool found;
> +
> + Assert(miinfo->multiInsertBufferTab != NULL);
> +
> + buffer = hash_search(miinfo->multiInsertBufferTab, &relid, HASH_ENTER, &found);
> +
> + if (!found)
> + {
> + buffer->relid = relid;
> + buffer->slots = palloc0(MAX_BUFFERED_TUPLES * sizeof(TupleTableSlot *));
> + buffer->resultRelInfo = rri;
> + buffer->nused = 0;
> + miinfo->nbuffers++;
> + }
> +
> + miinfo->buffer = buffer;
> +}

It still seems wrong to me to just perform a second hashtable search
here, givent that we've already done the partition dispatch.

> if (proute)
> insertMethod = CIM_MULTI_CONDITIONAL;
> else
> insertMethod = CIM_MULTI;

Hm, why do we still have CIM_MULTI and CIM_MULTI_CONDITIONAL?

> /*
> - * Tests have shown that using multi-inserts when the
> - * partition changes on every tuple slightly decreases the
> - * performance, however, there are benefits even when only
> - * some batches have just 2 tuples, so let's enable
> - * multi-inserts even when the average is quite low.
> + * Enable multi-inserts when the partition has BEFORE/INSTEAD
> + * OF triggers, or if the partition is a foreign partition.
> */
> leafpart_use_multi_insert = insertMethod == CIM_MULTI_CONDITIONAL &&
> - avgTuplesPerPartChange >= 1.3 &&
> - !has_before_insert_row_trig &&
> - !has_instead_insert_row_trig &&
> - resultRelInfo->ri_FdwRoutine == NULL;
> + !has_before_insert_row_trig &&
> + !has_instead_insert_row_trig &&
> + resultRelInfo->ri_FdwRoutine == NULL;

Not for now / this commit, but I kinda feel like we should determine
whether it's safe to multi-insert at a more centralized location.

Greetings,

Andres Freund