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.
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 ?
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.
-- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services