Re: patch: function xmltable

Hi

2017-01-25 1:35 GMT+01:00 Andres Freund <andres(at)anarazel(dot)de>:

> On 2017-01-24 21:32:56 -0300, Alvaro Herrera wrote:
> > Andres Freund wrote:
> > > Hi,
> > >
> > > On 2017-01-24 17:38:49 -0300, Alvaro Herrera wrote:
> > > > +static Datum ExecEvalTableExpr(TableExprState *tstate, ExprContext
> *econtext,
> > > > + bool *isnull);
> > > > +static Datum ExecEvalTableExprFast(TableExprState *exprstate,
> ExprContext *econtext,
> > > > + bool *isNull);
> > > > +static Datum tabexprFetchRow(TableExprState *tstate, ExprContext
> *econtext,
> > > > + bool *isNull);
> > > > +static void tabexprInitialize(TableExprState *tstate, ExprContext
> *econtext,
> > > > + Datum doc);
> > > > +static void ShutdownTableExpr(Datum arg);
> > >
> > > To me this (and a lot of the other code) hints quite strongly that
> > > expression evalution is the wrong approach to implementing this. What
> > > you're essentially doing is building a vulcano style scan node. Even
> if
> > > we can this, we shouldn't double down on the bad decision to have these
> > > magic expressions that return multiple rows. There's historical reason
> > > for tSRFs, but we shouldn't add more weirdness like this.
> >
> > Thanks for giving it a look. I have long thought that this patch would
> > be at odds with your overall executor work.
>
> Not fundamentally, but it makes it harder.
>

If you plan to hold support SRFin target list, then nothing is different.
In last patch is executed under nodeProjectSet.

>
>
> > XMLTABLE is specified by the standard to return multiple rows ... but
> > then as far as my reading goes, it is only supposed to be supported in
> > the range table (FROM clause) not in the target list. I wonder if
> > this would end up better if we only tried to support it in RT. I asked
> > Pavel to implement it like that a few weeks ago, but ...
>
> Right - it makes sense in the FROM list - but then it should be an
> executor node, instead of some expression thingy.
>

The XMLTABLE function is from user perspective, from implementation
perspective a form of SRF function. I use own executor node, because fcinfo
is complex already and not too enough to hold all information about result
columns.

The implementation as RT doesn't reduce code - it is just moving to
different file.

I'll try to explain my motivation. Please, check it and correct me if I am
wrong. I don't keep on my implementation - just try to implement XMLTABLE
be consistent with another behave and be used all time without any
surprise.

1. Any function that produces a content can be used in target list. We
support SRF in target list and in FROM part. Why XMLTABLE should be a
exception?

2. In standard the XMLTABLE is placed only on FROM part - but standard
doesn't need to solve my question - there are not SRF functions allowed in
targetlist.

If there be a common decision so this inconsistency (in behave of this kind
of functions) is expected, required - then I have not a problem to remove
this support from XMLTABLE.

In this moment I don't see a technical reason for this step - with last
Andres changes the support of XMLTABLE in target list needs less than 40
lines and there is not any special path for XMLTABLE only. Andres write
support for SRF functions and SRF operator. TableExpr is third category.

Regards

Pavel

> Greetings,
>
> Andres Freund
>