Re: TupleTableSlot abstraction

On 2018-09-04 18:35:34 +0530, Amit Khandekar wrote:
> The attached v11 tar has the above set of changes.


> Subject: [PATCH 07/14] Restructure TupleTableSlot to allow tuples other than
>  HeapTuple

> +
> +/*
> + * This is a function used by all getattr() callbacks which deal with a heap
> + * tuple or some tuple format which can be represented as a heap tuple e.g. a
> + * minimal tuple.
> + *
> + * heap_getattr considers any attnum beyond the attributes available in the
> + * tuple as NULL. This function however returns the values of missing
> + * attributes from the tuple descriptor in that case. Also this function does
> + * not support extracting system attributes.
> + *
> + * If the attribute needs to be fetched from the tuple, the function fills in
> + * tts_values and tts_isnull arrays upto the required attnum.
> + */
> +Datum
> +tts_heap_getattr_common(TupleTableSlot *slot, HeapTuple tuple, uint32 *offp,
> +						int attnum, bool
> *isnull)

I'm still *vehemently* opposed to the introduction of this.



> @@ -2024,7 +2024,18 @@ FormIndexDatum(IndexInfo *indexInfo,
>  		Datum		iDatum;
>  		bool		isNull;
>  
> -		if (keycol != 0)
> +		if (keycol < 0)
> +		{
> +			HeapTupleTableSlot *hslot = (HeapTupleTableSlot *)slot;
> +
> +			/* Only heap tuples have system attributes. */
> +			Assert(TTS_IS_HEAPTUPLE(slot) || TTS_IS_BUFFERTUPLE(slot));
> +
> +			iDatum = heap_getsysattr(hslot->tuple, keycol,
> +									 slot->tts_tupleDescriptor,
> +									 &isNull);
> +		}
> +		else if (keycol != 0)
>  		{
>  			/*
>  			 * Plain index column; get the value we need directly from the

This now should access the system column via the slot, right?  There's
other places like this IIRC.



> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
> index 9d6e25a..1b4e726 100644
> --- a/src/backend/executor/execExprInterp.c
> +++ b/src/backend/executor/execExprInterp.c
> @@ -490,54 +490,21 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>  
>  		EEO_CASE(EEOP_INNER_SYSVAR)
>  		{
> -			int			attnum = op->d.var.attnum;
> -			Datum		d;
> -
> -			/* these asserts must match defenses in slot_getattr */
> -			Assert(innerslot->tts_tuple != NULL);
> -			Assert(innerslot->tts_tuple != &(innerslot->tts_minhdr));
> -
> -			/* heap_getsysattr has sufficient defenses against bad attnums */
> -			d = heap_getsysattr(innerslot->tts_tuple, attnum,
> -								innerslot->tts_tupleDescriptor,
> -								op->resnull);
> -			*op->resvalue = d;
> +			ExecEvalSysVar(state, op, econtext, innerslot);

These changes should be in a separate commit.


> +const TupleTableSlotOps TTSOpsHeapTuple = {
> +	sizeof(HeapTupleTableSlot),
> +	.init = tts_heap_init,

The first field should also use a named field (same in following cases).


> @@ -185,6 +1020,7 @@ ExecResetTupleTable(List *tupleTable,	/* tuple table */
>  	{
>  		TupleTableSlot *slot = lfirst_node(TupleTableSlot, lc);
>  
> +		slot->tts_cb->release(slot);
>  		/* Always release resources and reset the slot to empty */
>  		ExecClearTuple(slot);
>  		if (slot->tts_tupleDescriptor)
> @@ -240,6 +1076,7 @@ void
>  ExecDropSingleTupleTableSlot(TupleTableSlot *slot)
>  {
>  	/* This should match ExecResetTupleTable's processing of one slot */
> +	slot->tts_cb->release(slot);
>  	Assert(IsA(slot, TupleTableSlot));
>  	ExecClearTuple(slot);
>  	if (slot->tts_tupleDescriptor)

ISTM that release should be called *after* clearing the slot.


> @@ -56,11 +56,28 @@ tqueueReceiveSlot(TupleTableSlot *slot, DestReceiver *self)
>  	TQueueDestReceiver *tqueue = (TQueueDestReceiver *) self;
>  	HeapTuple	tuple;
>  	shm_mq_result result;
> +	bool		tuple_copied = false;
> +
> +	/* Get the tuple out of slot, if necessary converting the slot's contents
> +	 * into a heap tuple by copying. In the later case we need to free the copy.
> +	 */
> +	if (TTS_IS_HEAPTUPLE(slot) || TTS_IS_BUFFERTUPLE(slot))
> +	{
> +		tuple = ExecFetchSlotTuple(slot, true);
> +		tuple_copied = false;
> +	}
> +	else
> +	{
> +		tuple = ExecCopySlotTuple(slot);
> +		tuple_copied = true;
> +	}

This seems like a bad idea to me.  We shouldn't hardcode slots like
this.  I've previously argued that we should instead allow
ExecFetchSlotTuple() for all types of tuples, but add a new bool
*shouldFree argument, which will then allow the caller to free the
tuple.  We gain zilch by having this kind of logic in multiple callers.


> From 280eac5c6758061d0a46b7ef9dd324e9a23226b5 Mon Sep 17 00:00:00 2001
> From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
> Date: Fri, 31 Aug 2018 10:53:42 +0530
> Subject: [PATCH 08/14] Change tuple table slot creation routines to suite
>  tuple table slot abstraction
> 
> This change requires ExecInitResultTupleSlotTL, ExecInitScanTupleSlot,
> ExecCreateScanSlotFromOuterPlan, ExecInitNullTupleSlot,
> ExecInitExtraTupleSlot, MakeTupleTableSlot, ExecAllocTableSlot to
> accept TupleTableSlotType as a new parameter. Change all their calls.
> 
> Ashutosh Bapat and Andres Freund
> 
> This by itself won't compile. Neither the tuple table slot abstraction
> patch would compile and work without this change. Both of those need
> to be committed together.

I don't like this kind of split - all commits should individually
compile. I think we should instead introduce dummy / empty structs for
&TTSOpsHeapTuple etc, and add the parameters necessary to pass them
through. And then move this patch to *before* the "core" abstract slot
patch.  That way every commit, but the super verbose stuff is still
split out.


> From 06d31f91831a1da78d56e4217f08d3866c7be6f8 Mon Sep 17 00:00:00 2001
> From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
> Date: Fri, 31 Aug 2018 11:00:30 +0530
> Subject: [PATCH 09/14] Rethink ExecFetchSlotMinimalTuple()
> 
> Before this work, a TupleTableSlot could "own" a minimal tuple as
> well. Thus ExecFetchSlotMinimalTuple() returned a minimal tuple after
> constructing and "owning" it if it was not readily available. When the
> slot "owned" the minimal tuple, the memory consumed by the tuple was
> freed when a new tuple was stored in it or the slot was cleared.
> 
> With this work, not all TupleTableSlot types can "own" a minimal
> tuple.  When fetching a minimal tuple from a slot that can not "own" a
> tuple, memory is allocated to construct the minimal tuple, which needs
> to be freed explicitly. Hence ExecFetchSlotMinimalTuple() is modified
> to flag the caller whether it should free the memory consumed by the
> returned minimal tuple.
> 
> Right now only a MinimalTupleTableSlot can own a minimal tuple. But we
> may change that in future or a user defined TupleTableSlot type (added
> through an extension) may be able to "own" a minimal tuple in it.
> Hence instead of relying upon TTS_IS_MINIMAL() macro to tell us
> whether the TupleTableSlot can "own" a minimal tuple or not, we rely
> on the set of callbacks.  A TupleTableSlot which can hold a minimal
> tuple implements get_minimal_tuple callback. Other TupleTableSlot
> types leave the callback NULL.
> 
> The minimal tuple returned by this function is usually copied into a
> hash table or a file. But, unlike ExecFetchSlotTuple() it's never
> written to. Hence the difference between signatures of the two
> functions.

I'm somewhat inclined to think this should be done in an earlier patch,
before the main "abstract slot" work.

Ok, gotta switch to a smaller patch for a bit ;)

Greetings,

Andres Freund