On Tue, Jul 23, 2013 at 9:27 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
> Fine- I'd have been as happy leaving this be and letting Greg commit it,
> but if you'd really like to hear my concerns, I'd start with pointing
> out that it's pretty horrid to have to copy every record around like
> this and that the hack of CreateTupleDescCopyExtend is pretty terrible
> and likely to catch people by surprise. Having FunctionNext() operate
> very differently depending on WITH ORDINALITY is ugly and the cause of
> the bug that was found. All-in-all, I'm not convinced that this is
> really a good approach and feel it'd be better off implemented at a
> different level, eg a node type instead of a hack on top of the existing
> SRF code.
Fwiw I've been mulling over the same questions and came to the
conclusion that the existing approach makes the most sense.
In an ideal world an extra executor node would be the prettiest,
cleanest implementation. But the amount of extra code and busywork
that would necessitate just isn't justified for the amount of work it
would be doing.
It might be different if WITH ORDINALITY made sense for any other
types of target tables. But it really only makes sense for SRFs. The
whole motivation for having them in the spec is that UNNEST is taking
an ordered list and turning it into a relation which is unordered. You
can't just do row_number() because there's nothing to make it ordered
by.
By the same token for any other data source you would just use
row_number *precisely because* any other data source is unordered and
you should have to specify an order in order to make row_number
produce something meaningful.
So given that WITH ORDINALITY is really only useful for UNNEST and we
can generalize it to all SRFs on the basis that Postgres SRFs do
produce ordered sets not unordered relations it isn't crazy for the
work to be in the Function node.
Now that I've written that though it occurs to me to wonder whether
FDW tables might be usefully said to be ordered too though?
--
greg
