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(dot)bapat(at)enterprisedb(dot)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(dot)bapat(at)enterprisedb(dot)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