Re: Review: UNNEST (and other functions) WITH ORDINALITY

Tom Lane said:
> I haven't read this patch, but that comment scares the heck out of me.
> Even if the patch isn't broken today, it will be tomorrow, if it's
> making random changes like that in data structure semantics.
> Also, if you're confused, so will be everybody else who has to deal with
> the code later --- so I don't think fixing the comments and variable
> names is optional.

I must admit to finding all of this criticism of unread code a bit
bizarre.

There are no "random changes in data structure semantics". All that
happens is that FunctionScan, in the ordinality case, has two tupdescs
to deal with: the one for the function return, and the one for
FunctionScan's own scan type. Likewise two slots, one of each type.
Absolutely no liberties are taken with any of the semantics. However,
since the scan structure already has a place for the scan result slot,
the "extra" slot that we allocate for this case is the function
result, func_slot, while in the non-ordinality case, we use the scan
result slot for the function result too.

[Greg: we just found a bug (actually two, one code + one docs); I
think David just posted the fixed version. And ironically, the bug in
the code has nothing to do with all of this discussion.]

Here, to hopefully end the issue, is the new version of FunctionNext,
which is the core of the whole patch (everything else is just setup
for this). If anyone wants to point out exactly what is unclear, or
which changes any semantics improperly, then please do indicate
exactly what you are referring to.

/* ----------------------------------------------------------------
* FunctionNext
*
* This is a workhorse for ExecFunctionScan
* ----------------------------------------------------------------
*/
static TupleTableSlot *
FunctionNext(FunctionScanState *node)
{
EState *estate;
ScanDirection direction;
Tuplestorestate *tuplestorestate;
TupleTableSlot *scanslot;
TupleTableSlot *funcslot;

if (node->func_slot)
{
/*
* ORDINALITY case: FUNCSLOT is the function return,
* SCANSLOT the scan result
*/

funcslot = node->func_slot;
scanslot = node->ss.ss_ScanTupleSlot;
}
else
{
funcslot = node->ss.ss_ScanTupleSlot;
scanslot = NULL;
}

/*
* get information from the estate and scan state
*/
estate = node->ss.ps.state;
direction = estate->es_direction;

tuplestorestate = node->tuplestorestate;

/*
* If first time through, read all tuples from function and put them in a
* tuplestore. Subsequent calls just fetch tuples from tuplestore.
*/
if (tuplestorestate == NULL)
{
node->tuplestorestate = tuplestorestate =
ExecMakeTableFunctionResult(node->funcexpr,
node->ss.ps.ps_ExprContext,
node->func_tupdesc,
node->eflags & EXEC_FLAG_BACKWARD);
}

/*
* Get the next tuple from tuplestore. Return NULL if no more tuples.
*/
(void) tuplestore_gettupleslot(tuplestorestate,
ScanDirectionIsForward(direction),
false,
funcslot);

if (!scanslot)
return funcslot;

/*
* we're doing ordinality, so we copy the values from the function return
* slot to the (distinct) scan slot. We can do this because the lifetimes
* of the values in each slot are the same; until we reset the scan or
* fetch the next tuple, both will be valid.
*/

ExecClearTuple(scanslot);

if (ScanDirectionIsForward(direction))
node->ordinal++;
else
node->ordinal--;

if (!TupIsNull(funcslot))
{
int natts = funcslot->tts_tupleDescriptor->natts;
int i;

slot_getallattrs(funcslot);

for (i = 0; i < natts; ++i)
{
scanslot->tts_values[i] = funcslot->tts_values[i];
scanslot->tts_isnull[i] = funcslot->tts_isnull[i];
}

scanslot->tts_values[natts] = Int64GetDatumFast(node->ordinal);
scanslot->tts_isnull[natts] = false;

ExecStoreVirtualTuple(scanslot);
}

return scanslot;
}

--
Andrew (irc:RhodiumToad)