Re: patch: function xmltable - Mailing list pgsql-hackers
| From | Pavel Stehule |
|---|---|
| Subject | Re: patch: function xmltable |
| Date | |
| Msg-id | CAFj8pRAHpZ6+W-BE4_3PW-_XTEHEb3bjAzayCHS80re13Mv=cQ@mail.gmail.com Whole thread Raw |
| In response to | Re: patch: function xmltable (Craig Ringer <craig@2ndquadrant.com>) |
| Responses |
Re: patch: function xmltable
|
| List | pgsql-hackers |
2016-09-12 3:58 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:
On 9 September 2016 at 21:44, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2016-09-09 10:35 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.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
Attachment
pgsql-hackers by date: