Re: patch: function xmltable

2016-09-12 3:58 GMT+02:00 Craig Ringer <craig(at)2ndquadrant(dot)com>:

> On 9 September 2016 at 21:44, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> >
> > 2016-09-09 10:35 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> >>
> >> Hi,
> >>
> >> I am sending new version of this patch
> >>
> >> 1. now generic TableExpr is better separated from a real content
> >> generation
> >> 2. I removed cached typmod - using row type cache everywhere - it is
> >> consistent with other few places in Pg where dynamic types are used -
> the
> >> result tupdesc is generated few times more - but it is not on critical
> path.
> >> 3. More comments, few more lines in doc.
> >> 4. Reformated by pgindent
>
> Thanks.
>
> I applied this on top of the same base as your prior patch so I could
> compare changes.
>
> The new docs look good. Thanks for that, I know it's a pain. It'll
> need to cover ORDINAL too, but that's not hard. I'll try to find some
> time to help with the docs per the references you sent offlist.
>
> Out of interest, should the syntax allow room for future expansion to
> permit reading from file rather than just string literal / column
> reference? It'd be ideal to avoid reading big documents wholly into
> memory when using INSERT INTO ... SELECT XMLTABLE (...) . I don't
> suggest adding that to this patch, just making sure adding it later
> would not cause problems.
>

this is little bit different question - it is server side function, so
first question is - how to push usually client side content to server? Next
question is how to get this content to a executor. Now only COPY statement
is able to do.

I am thinking so this should not be problem, but it requires maybe some
special keywords - fileref, local fileref, and some changes in protocol.
Because this function has own implementation in parser/transform stage,
then nothing will be lost in process, and we can implement lazy parameter
evaluation. Another question if libxml2 has enough possibility to work with
stream.

One idea - we can introduce "external (server side|client side) blobs" with
special types and special streaming IO. With these types, there no changes
are necessary on syntax level. With this, the syntax sugar flag "BY REF"
can be useful.

>
> I see you added a builder context abstraction as discussed, so there's
> no longer any direct reference to XMLTABLE specifics from TableExpr
> code. Good, thanks for that. It'll make things much less messy when
> adding other table expression types as you expressed the desire to do,
> and means the TableExpr code now makes more sense as generic
> infrastructure.
>
> ExecEvalTableExprProtected and ExecEvalTableExpr are OK with me, or
> better than execEvalTableExpr and ExecEvalTableExpr were anyway.
> Eventual committer will probably have opinions here.
>
> Mild nitpick: since you can have multiple namespaces, shouldn't
> builder->SetNS be builder->AddNS ?
>

good idea.

>
> Added comments are helpful, thanks.
>
> On first read-through this is a big improvement and addresses all the
> concerns I raised. Documentation is much much better, thanks, I know
> that's a pain.
>
> I'll take a closer read-through shortly.
>

updated patch attached - with your documentation.

Regards

Pavel

>
> --
> Craig Ringer http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>