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@anarazel.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