patch: function xmltable

Hi

2016-08-19 10:58 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi

I am sending implementation of xmltable function. The code should to have
near to final quality and it is available for testing.

I invite any help with documentation and testing.

new update - the work with nodes is much more correct now.

Regards

Pavel

Show quoted text

Regards

Pavel

2016-08-23 21:00 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi

2016-08-19 10:58 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

Hi

I am sending implementation of xmltable function. The code should to have
near to final quality and it is available for testing.

I invite any help with documentation and testing.

new update - the work with nodes is much more correct now.

next update

fix memory leak

Pavel

Show quoted text

Regards

Pavel

Regards

Pavel

Hi

minor update - using DefElem instead own private parser type

Regards

Pavel

On 4 September 2016 at 16:06, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hi

minor update - using DefElem instead own private parser type

I'm really glad that you're doing this and I'll take a look at it for this CF.

It's quite a big patch so I expect this will take a few rounds of
review and updating.

Patch applies cleanly and builds cleanly on master both with and
without --with-xml .

Overall, I think this needs to be revised with appropriate comments.
Whitespace/formatting needs fixing since it's all over the place.
Documentation is insufficient (per notes below).

Re identifier naming, some of this code uses XmlTable naming patterns,
some uses TableExpr prefixes. Is that intended to indicate a bounary
between things re-usable for other structured data ingesting
functions? Do you expect a "JSONEXPR" or similar in future? That's
alluded to by

+/*----------
+ * TableExpr - used for XMLTABLE function
+ *
+ * This can be used for json_table, jsonb_table functions in future
+ *----------
+ */
+typedef struct TableExpr
+{
...

If so, should this really be two patches, one to add the table
expression infrastructure and another to add XMLTABLE that uses it?
Also, why in that case does so much of the TableExpr code call
directly into XmlTable code? It doesn't look very generic.

Overall I find identifier naming to be a bit inconsisent and think
it's necessary to make it clear that all the "TableExpr" stuff is for
XMLTABLE specifically, if that's the case, or make the delineation
clearer if not.

I'd also like to see tests that exercise the ruleutils get_rule_expr
parts of the code for the various XMLTABLE variants.

Similarly, since this seems to add a new xpath parser, that needs
comprehensive tests. Maybe re-purpose an existing xpath test data set?

More detailed comments:
====

Docs comments:

The <function>xmltable</function> produces [a] table based on
[the] passed XML value.

The docs are pretty minimal and don't explain the various clauses of
XMLTABLE. What is "BY REF" ? Is PATH an xpath expression? If so, is
there a good cross reference link available? The PASSING clause? etc.

How does XMLTABLE decide what to iterate over, and how to iterate over it?

Presumably the FOR ORDINALITY clause makes a column emit a numeric counter.

What standard, if any, does this conform to? Does it resemble
implementations elsewhere? What limitations or unsupported features
does it have relative to those standards?

execEvalTableExpr seems to be defined twice, with a difference in
case. This is probably not going to fly:

+static Datum
+execEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

+static Datum
+ExecEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

It looks like you've split the function into a "guts" and "wrapper"
part, with the error handling PG_TRY / PG_CATCH block in the wrapper.
That seems reasonable for readability, but the naming isn't.

A comment is needed to explain what ExecEvalTableExpr is / does. If
it's XMLTABLE specific (which it looks like based on the code), its
name should reflect that. This pattern is repeated elsewhere; e.g.
TableExprState is really the state for an XMLTABLE expression. But
PostgreSQL actually has TABLE statements, and in future we might want
to support table-expressions, so I don't think this naming is
appropriate. This is made worse by the lack of comments on things like
the definition of TableExprState. Please use something that makes it
clear it's for XMLTABLE and add appropriate comments.

Formatting of variables, arguments, function signatures etc is
random/haphazard and doesn't follow project convention. It's neither
aligned or unaligned in the normal way, I don't understand the random
spacing at all. Maybe you should try to run pgindent and then extract
just the changes related to your patch? Or run your IDE/editor's
indent function on your changes? Right now it's actually kind of hard
to read. Do you edit with tabstop set to 1 normally or something like
that?

There's a general lack of comments throughout the added code.

In execEvalTableExpr, why are we looping over namespaces? What's that
for? Comment would be nice.

Typo: Path caclulation => Path calculation

What does XmlTableSetRowPath() do? It seems to copy its argument.
Nothing further is done with the row_path argument after it's called
by execEvalTableExpr, so what context is that memory in and do we have
to worry about it if it's large?

execEvalTableExpr says it's doing "path calculation". What it actually
appears to do is evaluate the path expressions, if provided, and
otherwise use the column name as the implied path expression. (The
docs should mention that).

It's wasn't immediately obvious to me what the branch around
tstate->for_ordinality_col is for and what the alternate path's
purpose is in terms of XMLTABLE's behaviour, until I read the parser
definition. That's largely because the behaviour of XMLTABLE is
underspecified in the docs, since once you know ORDINALITY columns
exist it's pretty obvious what it's doing.

Similarly, for the alternate branch tstate->ncols , the
XmlTableGetRowValue call there is meant to do what exactly, and
why/under what conditions? Is it for situations where the field type
is a whole-row value? a composite type? (I'm deliberately not studying
this too deeply, these are points I'd like to see commented so it can
be understood to some reasonable degree at a skim-read).

/* result is one more columns every time */
"one or more"

/* when typmod is not valid, refresh it */
if (te->typmod == -1)

Is this a cache? How is it valid or not valid and when? The comment
(thanks!) on TableExprGetTupleDesc says:

/*
* When we skip transform stage (in view), then TableExpr's
* TupleDesc should not be valid. Refresh is necessary.
*/

but I'm not really grasping what you're trying to explain here. What
transform stage? What view? This could well be my ignorance of this
part of the code; if it should be understandable by a reader who is
appropriately familiar with the executor that's fine, but if it's
specific to how XMLTABLE works some more explanation would be good.

Good that you've got all the required node copy/in/out funcs in place.

Please don't use the name "used_dns". Anyone reading that will read it
as "domain name service" and that's actually confusing with XML
because of XML schema lookups. Maybe used_defnamespace ? used
def_ns?

I haven't looked closely at keyword/parser changes yet, but it doesn't
look like you added any reserved keywords, which is good. It does add
unreserved keywords PATH and COLUMNS ; I'm not sure what policy for
unreserved keywords is or the significance of that.

New ereport() calls specify ERRCODEs, which is good.

PostgreSQL already has XPATH support in the form of xmlexists(...)
etc. Why is getXPathToken() etc needed? What re-use is possible here?
There's no explanation in the patch header or comments. Should the new
xpath parser be re-used by the existing xpath stuff? Why can't we use
libxml's facilities? etc. This at least needs explaining in the
submission, and some kind of hint as to why we have two different ways
to do it is needed in the code. If we do need a new XML parser, should
it be bundled in adt/xml.c along with a lot of user-facing
functionality, or a separate file?

How does XmlTableGetValue(...) and XmlTableGetRowValue(...) relate to
this? It doesn't look like they're intended to be called directly by
the user, and they're not documented (or commented).

I don't understand this at all:

+/*
+ * There are different requests from XMLTABLE, JSON_TABLE functions
+ * on passed data than has CREATE TABLE command. It is reason for
+ * introduction special structure instead using ColumnDef.
+ */
+typedef struct TableExprRawCol
+{
+    NodeTag     type;
+    char       *colname;
+    TypeName   *typeName;
+    bool        for_ordinality;
+    bool        is_not_null;
+    Node       *path_expr;
+    Node       *default_expr;
+    int         location;
+} TableExprRawCol;

That's my first-pass commentary. I'll return to this once you've had a
chance to take a look at these and tell me all the places I got it
wrong ;)

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi

2016-09-06 6:54 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

On 4 September 2016 at 16:06, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Hi

minor update - using DefElem instead own private parser type

I'm really glad that you're doing this and I'll take a look at it for this
CF.

It's quite a big patch so I expect this will take a few rounds of
review and updating.

Thank you for review

Patch applies cleanly and builds cleanly on master both with and
without --with-xml .

Overall, I think this needs to be revised with appropriate comments.
Whitespace/formatting needs fixing since it's all over the place.
Documentation is insufficient (per notes below).

I am not able to write documentation in English language :( - This function
is pretty complex - so I hope so anybody with better language skills can
help with this. It respects standard and it respects little bit different
Oracle's behave too (different order of DEFAULT and PATH parts).

Re identifier naming, some of this code uses XmlTable naming patterns,
some uses TableExpr prefixes. Is that intended to indicate a bounary
between things re-usable for other structured data ingesting
functions? Do you expect a "JSONEXPR" or similar in future? That's
alluded to by

This structure should be reused by JSON_TABLE function. Now, it is little
bit strange, because there is only XMLTABLE implementation - and I have to
choose between a) using two different names now, b) renaming some part in
future.

And although XMLTABLE and JSON_TABLE functions are pretty similar - share
90% of data (input value, path, columns definitions), these functions has
different syntax - so only middle level code should be shared.

+/*----------
+ * TableExpr - used for XMLTABLE function
+ *
+ * This can be used for json_table, jsonb_table functions in future
+ *----------
+ */
+typedef struct TableExpr
+{
...

If so, should this really be two patches, one to add the table
expression infrastructure and another to add XMLTABLE that uses it?
Also, why in that case does so much of the TableExpr code call
directly into XmlTable code? It doesn't look very generic.

Currently the common part is not too big - just the Node related part - I
am not sure about necessity of two patches. I am agree, there is missing
some TableExpBuilder, where can be better isolated the XML part.

Overall I find identifier naming to be a bit inconsisent and think
it's necessary to make it clear that all the "TableExpr" stuff is for
XMLTABLE specifically, if that's the case, or make the delineation
clearer if not.

I'd also like to see tests that exercise the ruleutils get_rule_expr
parts of the code for the various XMLTABLE variants.

Similarly, since this seems to add a new xpath parser, that needs
comprehensive tests. Maybe re-purpose an existing xpath test data set?

sure

More detailed comments:
====

Docs comments:

The <function>xmltable</function> produces [a] table based on
[the] passed XML value.

The docs are pretty minimal and don't explain the various clauses of
XMLTABLE. What is "BY REF" ? Is PATH an xpath expression? If so, is
there a good cross reference link available? The PASSING clause? etc.

How does XMLTABLE decide what to iterate over, and how to iterate over it?

Presumably the FOR ORDINALITY clause makes a column emit a numeric counter.

What standard, if any, does this conform to? Does it resemble
implementations elsewhere? What limitations or unsupported features
does it have relative to those standards?

execEvalTableExpr seems to be defined twice, with a difference in
case. This is probably not going to fly:

+static Datum
+execEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

+static Datum
+ExecEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

It looks like you've split the function into a "guts" and "wrapper"
part, with the error handling PG_TRY / PG_CATCH block in the wrapper.
That seems reasonable for readability, but the naming isn't.

I invite any idea how these functions should be named.

A comment is needed to explain what ExecEvalTableExpr is / does. If
it's XMLTABLE specific (which it looks like based on the code), its
name should reflect that. This pattern is repeated elsewhere; e.g.
TableExprState is really the state for an XMLTABLE expression. But
PostgreSQL actually has TABLE statements, and in future we might want
to support table-expressions, so I don't think this naming is
appropriate. This is made worse by the lack of comments on things like
the definition of TableExprState. Please use something that makes it
clear it's for XMLTABLE and add appropriate comments.

I understand, so using TableExpr can be strange (for XMLTABLE function).
But when we will have JSON_TABLE function, then it will have a sense.

"TableExprState" is consistent with "TableExpr".

Any idea how it should be changed?

Formatting of variables, arguments, function signatures etc is
random/haphazard and doesn't follow project convention. It's neither
aligned or unaligned in the normal way, I don't understand the random
spacing at all. Maybe you should try to run pgindent and then extract
just the changes related to your patch? Or run your IDE/editor's
indent function on your changes? Right now it's actually kind of hard
to read. Do you edit with tabstop set to 1 normally or something like
that?

There's a general lack of comments throughout the added code.

In execEvalTableExpr, why are we looping over namespaces? What's that
for? Comment would be nice.

Typo: Path caclulation => Path calculation

What does XmlTableSetRowPath() do? It seems to copy its argument.
Nothing further is done with the row_path argument after it's called
by execEvalTableExpr, so what context is that memory in and do we have
to worry about it if it's large?

execEvalTableExpr says it's doing "path calculation". What it actually
appears to do is evaluate the path expressions, if provided, and
otherwise use the column name as the implied path expression. (The
docs should mention that).

It's wasn't immediately obvious to me what the branch around
tstate->for_ordinality_col is for and what the alternate path's
purpose is in terms of XMLTABLE's behaviour, until I read the parser
definition. That's largely because the behaviour of XMLTABLE is
underspecified in the docs, since once you know ORDINALITY columns
exist it's pretty obvious what it's doing.

Similarly, for the alternate branch tstate->ncols , the
XmlTableGetRowValue call there is meant to do what exactly, and
why/under what conditions? Is it for situations where the field type
is a whole-row value? a composite type? (I'm deliberately not studying
this too deeply, these are points I'd like to see commented so it can
be understood to some reasonable degree at a skim-read).

/* result is one more columns every time */
"one or more"

/* when typmod is not valid, refresh it */
if (te->typmod == -1)

Is this a cache? How is it valid or not valid and when? The comment
(thanks!) on TableExprGetTupleDesc says:

/*
* When we skip transform stage (in view), then TableExpr's
* TupleDesc should not be valid. Refresh is necessary.
*/

but I'm not really grasping what you're trying to explain here. What
transform stage? What view? This could well be my ignorance of this
part of the code; if it should be understandable by a reader who is
appropriately familiar with the executor that's fine, but if it's
specific to how XMLTABLE works some more explanation would be good.

This is most difficult part of this patch, and I am not sure it it is fully
correctly implemented. I use TupleDesc cache. The TupleDesc is created in
parser/transform stage. When the XMLTABLE is used in some view, then the
transformed parser tree is materialized - and when the view is used in
query, then this tree is loaded and the parser/transform stage is
"skipped". I'll check this code against implementation of ROW constructor
and I'll try to do more comments there.

Good that you've got all the required node copy/in/out funcs in place.

Please don't use the name "used_dns". Anyone reading that will read it
as "domain name service" and that's actually confusing with XML
because of XML schema lookups. Maybe used_defnamespace ? used
def_ns?

good idea

I haven't looked closely at keyword/parser changes yet, but it doesn't
look like you added any reserved keywords, which is good. It does add
unreserved keywords PATH and COLUMNS ; I'm not sure what policy for
unreserved keywords is or the significance of that.

New ereport() calls specify ERRCODEs, which is good.

PostgreSQL already has XPATH support in the form of xmlexists(...)
etc. Why is getXPathToken() etc needed? What re-use is possible here?
There's no explanation in the patch header or comments. Should the new
xpath parser be re-used by the existing xpath stuff? Why can't we use
libxml's facilities? etc. This at least needs explaining in the
submission, and some kind of hint as to why we have two different ways
to do it is needed in the code. If we do need a new XML parser, should
it be bundled in adt/xml.c along with a lot of user-facing
functionality, or a separate file?

libxml2 and our XPATH function doesn't support default namespace (
http://plasmasturm.org/log/259/ ). This is pretty useful feature - so I
implemented. This is the mayor issue of libxml2 library. Another difference
between XPATH function and XMLTABLE function is using two phase searching
and implicit prefix "./" and suffix ("/text()") in XMLTABLE. XMLTABLE using
two XPATH expressions - for row data cutting and next for column data
cutting (from row data). The our XPATH functions is pretty simple mapped to
libxml2 XPATH API. But it is not possible with XMLTABLE function - due
design of this function in standard (it is more user friendly and doesn't
require exactly correct xpath expressions).

I didn't find any API in libxml2 for a work with parsed xpath expressions -
I need some info about the first and last token of xpath expression - it is
base for decision about using prefix or suffix.

This functionality (xpath expression parser) cannot be used for our XPATH
function now - maybe default namespace in future.

How does XmlTableGetValue(...) and XmlTableGetRowValue(...) relate to
this? It doesn't look like they're intended to be called directly by
the user, and they're not documented (or commented).

Probably I used wrong names. XMLTABLE function is running in two different
modes - with explicitly defined columns (XmlTableGetValue is used), and
without explicitly defined columns - so result is one XML column and only
one one step searching is used (there are not column related xpath
expressions) ( XmlTableGetRowValue is used). The function XmlTableGetValue
is used for getting one column value, the function XmlTableGetRowValue is
used for getting one value too, but in special case, when there are not any
other value.

I don't understand this at all:

+/*
+ * There are different requests from XMLTABLE, JSON_TABLE functions
+ * on passed data than has CREATE TABLE command. It is reason for
+ * introduction special structure instead using ColumnDef.
+ */
+typedef struct TableExprRawCol
+{
+    NodeTag     type;
+    char       *colname;
+    TypeName   *typeName;
+    bool        for_ordinality;
+    bool        is_not_null;
+    Node       *path_expr;
+    Node       *default_expr;
+    int         location;
+} TableExprRawCol;

I am sorry. It is my fault. Now we have very similar node ColumnDef. This
node is designed for usage in utility commands - and it is not designed for
usage inside a query. I had to decide between enhancing ColumnDef node or
introduction new special node. Because there are more special attributes
and it is hard to serialize current ColumnDef, I decided to use new node.

That's my first-pass commentary. I'll return to this once you've had a
chance to take a look at these and tell me all the places I got it
wrong ;)

Thank for this

Regard

Pavel

Show quoted text

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

libxml2 and our XPATH function doesn't support default namespace (
http://plasmasturm.org/log/259/ ). This is pretty useful feature - so I
implemented. This is the mayor issue of libxml2 library. Another difference
between XPATH function and XMLTABLE function is using two phase searching
and implicit prefix "./" and suffix ("/text()") in XMLTABLE. XMLTABLE using
two XPATH expressions - for row data cutting and next for column data
cutting (from row data). The our XPATH functions is pretty simple mapped to
libxml2 XPATH API. But it is not possible with XMLTABLE function - due
design of this function in standard (it is more user friendly and doesn't
require exactly correct xpath expressions).

libxm2 doesn't support xpath 2.0 where default namespace was introduced.

On 7 September 2016 at 04:13, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Overall, I think this needs to be revised with appropriate comments.
Whitespace/formatting needs fixing since it's all over the place.
Documentation is insufficient (per notes below).

I am not able to write documentation in English language :( - This function
is pretty complex - so I hope so anybody with better language skills can
help with this. It respects standard and it respects little bit different
Oracle's behave too (different order of DEFAULT and PATH parts).

OK, no problem. It can't be committed without more comprehensive docs
though, especially for new and nontrivial functionality.

Is there some reference material you can point to so someone else can
help with docs? And can you describe what differences there are
between your implementation and the reference?

Alternately, if you document it in Czech, do you know of anyone who
could assist in translating to English for the main documentation?

Re identifier naming, some of this code uses XmlTable naming patterns,
some uses TableExpr prefixes. Is that intended to indicate a bounary
between things re-usable for other structured data ingesting
functions? Do you expect a "JSONEXPR" or similar in future? That's
alluded to by

This structure should be reused by JSON_TABLE function. Now, it is little
bit strange, because there is only XMLTABLE implementation - and I have to
choose between a) using two different names now, b) renaming some part in
future.

OK. Are you planning on writing this JSON_TABLE or are you leaving
room for future growth? Either way is fine, just curious.

And although XMLTABLE and JSON_TABLE functions are pretty similar - share
90% of data (input value, path, columns definitions), these functions has
different syntax - so only middle level code should be shared.

That makes sense.

I think it would be best if you separated out the TableExpr
infrastructure from the XMLTABLE implementation though, so we can
review the first level infrastrcture separately and make this a
2-patch series. Most importantly, doing it that way will help you find
places where TableExpr code calls directly into XMLTABLE code. If
TableExpr is supposed to be reusable for json etc, it probably
shouldn't be calling XmlTable stuff directly.

That also means somewhat smaller simpler patches, which probably isn't bad.

I don't necessarily think this needs to be fully pluggable with
callbacks etc. It doesn't sound like you expect this to be used by
extensions or to have a lot of users, right? So it probably just needs
clearer separation of the infrastructure layer from the xmltable
layer. I think splitting the patch will make that easier to see and
make it easier to find problems.

My biggest complaint at the moment is that execEvalTableExpr calls
initXmlTableContext(...) directly, is aware of XML namespaces
directly, calls XmlTableSetRowPath() directly, calls
XmlTableFetchRow() directly, etc. It is in no way generic/reusable for
some later JSONTABLE feature. That needs to be fixed by:

* Renaming it so it's clearly only for XMLTABLE; or
* Abstracting the init context, set row path, fetch row etc operations
so json ones can be plugged in later

Currently the common part is not too big - just the Node related part - I am
not sure about necessity of two patches.

The problem is that the common part is all mixed in with the
XMLTABLE-specific part, so it's not at all clear it can be common with
something else.

I am agree, there is missing some
TableExpBuilder, where can be better isolated the XML part.

Yeah, that's sort of what I'm getting at.

execEvalTableExpr seems to be defined twice, with a difference in
case. This is probably not going to fly:

+static Datum
+execEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

+static Datum
+ExecEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

It looks like you've split the function into a "guts" and "wrapper"
part, with the error handling PG_TRY / PG_CATCH block in the wrapper.
That seems reasonable for readability, but the naming isn't.

I invite any idea how these functions should be named.

Definitely not how they are ;) . They really can't differ in a single
character's case.

I'm not sure if PostgreSQL has any formal convention for this. Some
places use _impl e.g. pg_read_barrier_impl() but that's in the
context of an interface-vs-implementation separation, which isn't the
case here.

Some places use _internal, like AlterObjectRename_internal(...), but
that's where there's an associated public/external part, which isn't
the case here.

Some places use _guts e.g. pg_logical_slot_get_changes_guts(...),
largely where there's common use by several callers.

This is a fairly arbitrary function split for readability/length. Is
it actually useful to split this function up at all?

Anyone else have an opinion?

A comment is needed to explain what ExecEvalTableExpr is / does. If
it's XMLTABLE specific (which it looks like based on the code), its
name should reflect that. This pattern is repeated elsewhere; e.g.
TableExprState is really the state for an XMLTABLE expression. But
PostgreSQL actually has TABLE statements, and in future we might want
to support table-expressions, so I don't think this naming is
appropriate. This is made worse by the lack of comments on things like
the definition of TableExprState. Please use something that makes it
clear it's for XMLTABLE and add appropriate comments.

I understand, so using TableExpr can be strange (for XMLTABLE function). But
when we will have JSON_TABLE function, then it will have a sense.

It's pretty hard to review that as shared infrastructure when it's
still tangled up in xmltable specifics, though.

"TableExprState" is consistent with "TableExpr".

Any idea how it should be changed?

I think if you want it to be shareable infrasructure, you need to
write it so it can be used as shared infrastructure. Not just name it
that way but then make it XMLTABLE specific in actual functionality.

/* when typmod is not valid, refresh it */
if (te->typmod == -1)

Is this a cache? How is it valid or not valid and when? The comment
(thanks!) on TableExprGetTupleDesc says:

/*
* When we skip transform stage (in view), then TableExpr's
* TupleDesc should not be valid. Refresh is necessary.
*/

but I'm not really grasping what you're trying to explain here. What
transform stage? What view? This could well be my ignorance of this
part of the code; if it should be understandable by a reader who is
appropriately familiar with the executor that's fine, but if it's
specific to how XMLTABLE works some more explanation would be good.

This is most difficult part of this patch, and I am not sure it it is fully
correctly implemented. I use TupleDesc cache. The TupleDesc is created in
parser/transform stage. When the XMLTABLE is used in some view, then the
transformed parser tree is materialized - and when the view is used in
query, then this tree is loaded and the parser/transform stage is "skipped".
I'll check this code against implementation of ROW constructor and I'll try
to do more comments there.

Thanks. It would be good to highlight when this does and does not
happen and why. Why is it necessary at all?

What happens if XMLTABLE is used in a query directly, not part of a
view? if XMLTABLE is used in a view? If XMLTABLE is used in a prepared
statement / plpgsql statement / etc? What about a CTE term?

Not necessarily list all these cases one by one, just explain what
happens, when and why. Especially if it's complex, so other readers
can understand it and don't have to study it in detail to understand
what is going on. It does not need to be good public-facing
documentation, and details of wording, grammar etc can be fixed up
later, it's the ideas that matter.

Is this similar to other logic elsewhere? If so, reference that other
logic so readers know where to look. That way if they're
changing/bugfixing/etc one place they know there's another place that
might need changing.

I don't know this area of the code well enough to give a solid review
of the actual functionality, and I don't yet understand what it's
trying to do so it's hard to review it by studying what it actually
does vs what it claims to do. Maybe Peter E can help, he said he was
thinking of looking at this patch too. But more information on what
it's trying to do would be a big help.

PostgreSQL already has XPATH support in the form of xmlexists(...)
etc. Why is getXPathToken() etc needed? What re-use is possible here?
There's no explanation in the patch header or comments. Should the new
xpath parser be re-used by the existing xpath stuff? Why can't we use
libxml's facilities? etc. This at least needs explaining in the
submission, and some kind of hint as to why we have two different ways
to do it is needed in the code. If we do need a new XML parser, should
it be bundled in adt/xml.c along with a lot of user-facing
functionality, or a separate file?

libxml2 and our XPATH function doesn't support default namespace (
http://plasmasturm.org/log/259/ ). This is pretty useful feature - so I
implemented.

OK, that makes sense.

For the purpose of getting this patch in, is it a _necessary_ feature?
Can XMLTABLE be usefully implemented without it, and if so, can it be
added in a subsequent patch? It would be nice to simplify this by
using existing libxml2 functionality in the first version rather than
adding a whole new xpath as well!

This is the mayor issue of libxml2 library. Another difference
between XPATH function and XMLTABLE function is using two phase searching
and implicit prefix "./" and suffix ("/text()") in XMLTABLE. XMLTABLE using
two XPATH expressions - for row data cutting and next for column data
cutting (from row data). The our XPATH functions is pretty simple mapped to
libxml2 XPATH API. But it is not possible with XMLTABLE function - due
design of this function in standard (it is more user friendly and doesn't
require exactly correct xpath expressions).

So you can't use existing libxml2 xpath support to implement XMLTABLE,
even without default namespaces?

I didn't find any API in libxml2 for a work with parsed xpath expressions -
I need some info about the first and last token of xpath expression - it is
base for decision about using prefix or suffix.

This functionality (xpath expression parser) cannot be used for our XPATH
function now - maybe default namespace in future.

So we'll have two different XPATH implementations for different
places, with different limitations, different possible bugs, etc?

What would be needed to make the new XPATH work for our built-in xpath
functions too?

How does XmlTableGetValue(...) and XmlTableGetRowValue(...) relate to
this? It doesn't look like they're intended to be called directly by
the user, and they're not documented (or commented).

Probably I used wrong names. XMLTABLE function is running in two different
modes - with explicitly defined columns (XmlTableGetValue is used), and
without explicitly defined columns - so result is one XML column and only
one one step searching is used (there are not column related xpath
expressions) ( XmlTableGetRowValue is used). The function XmlTableGetValue
is used for getting one column value, the function XmlTableGetRowValue is
used for getting one value too, but in special case, when there are not any
other value.

So both are internal implementation of the parser-level XMLTABLE(...)
construct and are not intended to be called directly by users - right?

Comments please! A short comment on the function saying this would be
a big help.

Regarding naming, do we already have a convention for functions that
are internal implementation of something the user "spells"
differently? Where it's transformed by the parser? I couldn't find
one. I don't much care about the names so long as there are comments
explaining what calls the functions and what the user-facing interface
that matches the function is.

Is it safe for users to call these directly? What happens if they do
so incorrectly?

Why are they not in pg_proc.h? Do they need to be?

+/*
+ * There are different requests from XMLTABLE, JSON_TABLE functions
+ * on passed data than has CREATE TABLE command. It is reason for
+ * introduction special structure instead using ColumnDef.
+ */
+typedef struct TableExprRawCol
+{
+    NodeTag     type;
+    char       *colname;
+    TypeName   *typeName;
+    bool        for_ordinality;
+    bool        is_not_null;
+    Node       *path_expr;
+    Node       *default_expr;
+    int         location;
+} TableExprRawCol;

I am sorry. It is my fault. Now we have very similar node ColumnDef. This
node is designed for usage in utility commands - and it is not designed for
usage inside a query.

Makes sense.

I had to decide between enhancing ColumnDef node or
introduction new special node. Because there are more special attributes and
it is hard to serialize current ColumnDef, I decided to use new node.

Seems reasonable. The summary is "this is the parse node for a column
of an XMLTABLE expression".

Suggested comment:

/*
* This is the parsenode for a column definition in a table-expression
like XMLTABLE.
*
* We can't re-use ColumnDef here; the utility command column
definition has all the
* wrong attributes for use in table-expressions and just doesn't make
sense here.
*/
typedef struct TableExprColumn
{
...
};

?

Why "RawCol" ? What does it become when it's not "raw" anymore? Is
that a reference to ColumnDef's raw_default and cooked_default for
untransformed vs transformed parse-trees?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-09-07 5:03 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

On 7 September 2016 at 04:13, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

Overall, I think this needs to be revised with appropriate comments.
Whitespace/formatting needs fixing since it's all over the place.
Documentation is insufficient (per notes below).

I am not able to write documentation in English language :( - This

function

is pretty complex - so I hope so anybody with better language skills can
help with this. It respects standard and it respects little bit different
Oracle's behave too (different order of DEFAULT and PATH parts).

OK, no problem. It can't be committed without more comprehensive docs
though, especially for new and nontrivial functionality.

Is there some reference material you can point to so someone else can
help with docs? And can you describe what differences there are
between your implementation and the reference?

Alternately, if you document it in Czech, do you know of anyone who
could assist in translating to English for the main documentation?

Re identifier naming, some of this code uses XmlTable naming patterns,
some uses TableExpr prefixes. Is that intended to indicate a bounary
between things re-usable for other structured data ingesting
functions? Do you expect a "JSONEXPR" or similar in future? That's
alluded to by

This structure should be reused by JSON_TABLE function. Now, it is little
bit strange, because there is only XMLTABLE implementation - and I have

to

choose between a) using two different names now, b) renaming some part in
future.

OK. Are you planning on writing this JSON_TABLE or are you leaving
room for future growth? Either way is fine, just curious.

And although XMLTABLE and JSON_TABLE functions are pretty similar - share
90% of data (input value, path, columns definitions), these functions has
different syntax - so only middle level code should be shared.

That makes sense.

I think it would be best if you separated out the TableExpr
infrastructure from the XMLTABLE implementation though, so we can
review the first level infrastrcture separately and make this a
2-patch series. Most importantly, doing it that way will help you find
places where TableExpr code calls directly into XMLTABLE code. If
TableExpr is supposed to be reusable for json etc, it probably
shouldn't be calling XmlTable stuff directly.

That also means somewhat smaller simpler patches, which probably isn't bad.

I don't necessarily think this needs to be fully pluggable with
callbacks etc. It doesn't sound like you expect this to be used by
extensions or to have a lot of users, right? So it probably just needs
clearer separation of the infrastructure layer from the xmltable
layer. I think splitting the patch will make that easier to see and
make it easier to find problems.

My biggest complaint at the moment is that execEvalTableExpr calls
initXmlTableContext(...) directly, is aware of XML namespaces
directly, calls XmlTableSetRowPath() directly, calls
XmlTableFetchRow() directly, etc. It is in no way generic/reusable for
some later JSONTABLE feature. That needs to be fixed by:

* Renaming it so it's clearly only for XMLTABLE; or
* Abstracting the init context, set row path, fetch row etc operations
so json ones can be plugged in later

Currently the common part is not too big - just the Node related part -

I am

not sure about necessity of two patches.

The problem is that the common part is all mixed in with the
XMLTABLE-specific part, so it's not at all clear it can be common with
something else.

I am agree, there is missing some
TableExpBuilder, where can be better isolated the XML part.

Yeah, that's sort of what I'm getting at.

execEvalTableExpr seems to be defined twice, with a difference in
case. This is probably not going to fly:

+static Datum
+execEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

+static Datum
+ExecEvalTableExpr(TableExprState *tstate,
+                        ExprContext *econtext,
+                        bool *isNull, ExprDoneCond *isDone)
+{

It looks like you've split the function into a "guts" and "wrapper"
part, with the error handling PG_TRY / PG_CATCH block in the wrapper.
That seems reasonable for readability, but the naming isn't.

I invite any idea how these functions should be named.

Definitely not how they are ;) . They really can't differ in a single
character's case.

I'm not sure if PostgreSQL has any formal convention for this. Some
places use _impl e.g. pg_read_barrier_impl() but that's in the
context of an interface-vs-implementation separation, which isn't the
case here.

Some places use _internal, like AlterObjectRename_internal(...), but
that's where there's an associated public/external part, which isn't
the case here.

Some places use _guts e.g. pg_logical_slot_get_changes_guts(...),
largely where there's common use by several callers.

This is a fairly arbitrary function split for readability/length. Is
it actually useful to split this function up at all?

Anyone else have an opinion?

A comment is needed to explain what ExecEvalTableExpr is / does. If
it's XMLTABLE specific (which it looks like based on the code), its
name should reflect that. This pattern is repeated elsewhere; e.g.
TableExprState is really the state for an XMLTABLE expression. But
PostgreSQL actually has TABLE statements, and in future we might want
to support table-expressions, so I don't think this naming is
appropriate. This is made worse by the lack of comments on things like
the definition of TableExprState. Please use something that makes it
clear it's for XMLTABLE and add appropriate comments.

I understand, so using TableExpr can be strange (for XMLTABLE function).

But

when we will have JSON_TABLE function, then it will have a sense.

It's pretty hard to review that as shared infrastructure when it's
still tangled up in xmltable specifics, though.

"TableExprState" is consistent with "TableExpr".

Any idea how it should be changed?

I think if you want it to be shareable infrasructure, you need to
write it so it can be used as shared infrastructure. Not just name it
that way but then make it XMLTABLE specific in actual functionality.

/* when typmod is not valid, refresh it */
if (te->typmod == -1)

Is this a cache? How is it valid or not valid and when? The comment
(thanks!) on TableExprGetTupleDesc says:

/*
* When we skip transform stage (in view), then TableExpr's
* TupleDesc should not be valid. Refresh is necessary.
*/

but I'm not really grasping what you're trying to explain here. What
transform stage? What view? This could well be my ignorance of this
part of the code; if it should be understandable by a reader who is
appropriately familiar with the executor that's fine, but if it's
specific to how XMLTABLE works some more explanation would be good.

This is most difficult part of this patch, and I am not sure it it is

fully

correctly implemented. I use TupleDesc cache. The TupleDesc is created in
parser/transform stage. When the XMLTABLE is used in some view, then the
transformed parser tree is materialized - and when the view is used in
query, then this tree is loaded and the parser/transform stage is

"skipped".

I'll check this code against implementation of ROW constructor and I'll

try

to do more comments there.

Thanks. It would be good to highlight when this does and does not
happen and why. Why is it necessary at all?

What happens if XMLTABLE is used in a query directly, not part of a
view? if XMLTABLE is used in a view? If XMLTABLE is used in a prepared
statement / plpgsql statement / etc? What about a CTE term?

Not necessarily list all these cases one by one, just explain what
happens, when and why. Especially if it's complex, so other readers
can understand it and don't have to study it in detail to understand
what is going on. It does not need to be good public-facing
documentation, and details of wording, grammar etc can be fixed up
later, it's the ideas that matter.

Is this similar to other logic elsewhere? If so, reference that other
logic so readers know where to look. That way if they're
changing/bugfixing/etc one place they know there's another place that
might need changing.

I don't know this area of the code well enough to give a solid review
of the actual functionality, and I don't yet understand what it's
trying to do so it's hard to review it by studying what it actually
does vs what it claims to do. Maybe Peter E can help, he said he was
thinking of looking at this patch too. But more information on what
it's trying to do would be a big help.

PostgreSQL already has XPATH support in the form of xmlexists(...)
etc. Why is getXPathToken() etc needed? What re-use is possible here?
There's no explanation in the patch header or comments. Should the new
xpath parser be re-used by the existing xpath stuff? Why can't we use
libxml's facilities? etc. This at least needs explaining in the
submission, and some kind of hint as to why we have two different ways
to do it is needed in the code. If we do need a new XML parser, should
it be bundled in adt/xml.c along with a lot of user-facing
functionality, or a separate file?

libxml2 and our XPATH function doesn't support default namespace (
http://plasmasturm.org/log/259/ ). This is pretty useful feature - so I
implemented.

OK, that makes sense.

For the purpose of getting this patch in, is it a _necessary_ feature?
Can XMLTABLE be usefully implemented without it, and if so, can it be
added in a subsequent patch? It would be nice to simplify this by
using existing libxml2 functionality in the first version rather than
adding a whole new xpath as well!

This is not a xpath implementation - it is preprocessing of xpath
expression. without it, a users have to set explicitly PATH clause with
explicit prefix "./" and explicit suffix "/text()". The usability will be
significantly lower, and what is worst - the examples from internet should
not work. Although is is lot of lines, this code is necessary.

This is the mayor issue of libxml2 library. Another difference
between XPATH function and XMLTABLE function is using two phase searching
and implicit prefix "./" and suffix ("/text()") in XMLTABLE. XMLTABLE

using

two XPATH expressions - for row data cutting and next for column data
cutting (from row data). The our XPATH functions is pretty simple mapped

to

libxml2 XPATH API. But it is not possible with XMLTABLE function - due
design of this function in standard (it is more user friendly and doesn't
require exactly correct xpath expressions).

So you can't use existing libxml2 xpath support to implement XMLTABLE,
even without default namespaces?

I didn't find any API in libxml2 for a work with parsed xpath

expressions -

I need some info about the first and last token of xpath expression - it

is

base for decision about using prefix or suffix.

This functionality (xpath expression parser) cannot be used for our XPATH
function now - maybe default namespace in future.

So we'll have two different XPATH implementations for different
places, with different limitations, different possible bugs, etc?

It is just preprocessing. The evaluation of xpath expression is part of
libxml2 and it is shared.

Our XPATH function is not short, but the reason is reading namespaces data
from 2D array. The evaluation of xpath expression is on few lines.

What would be needed to make the new XPATH work for our built-in xpath
functions too?

How does XmlTableGetValue(...) and XmlTableGetRowValue(...) relate to
this? It doesn't look like they're intended to be called directly by
the user, and they're not documented (or commented).

Probably I used wrong names. XMLTABLE function is running in two

different

modes - with explicitly defined columns (XmlTableGetValue is used), and
without explicitly defined columns - so result is one XML column and only
one one step searching is used (there are not column related xpath
expressions) ( XmlTableGetRowValue is used). The function

XmlTableGetValue

is used for getting one column value, the function XmlTableGetRowValue is
used for getting one value too, but in special case, when there are not

any

other value.

So both are internal implementation of the parser-level XMLTABLE(...)
construct and are not intended to be called directly by users - right?

No - it is called from executor - and it should not be called differently.
I have to do better separation from executor, and these functions will be
private.

Comments please! A short comment on the function saying this would be
a big help.

Regarding naming, do we already have a convention for functions that
are internal implementation of something the user "spells"
differently? Where it's transformed by the parser? I couldn't find
one. I don't much care about the names so long as there are comments
explaining what calls the functions and what the user-facing interface
that matches the function is.

Is it safe for users to call these directly? What happens if they do
so incorrectly?

Why are they not in pg_proc.h? Do they need to be?

+/*
+ * There are different requests from XMLTABLE, JSON_TABLE functions
+ * on passed data than has CREATE TABLE command. It is reason for
+ * introduction special structure instead using ColumnDef.
+ */
+typedef struct TableExprRawCol
+{
+    NodeTag     type;
+    char       *colname;
+    TypeName   *typeName;
+    bool        for_ordinality;
+    bool        is_not_null;
+    Node       *path_expr;
+    Node       *default_expr;
+    int         location;
+} TableExprRawCol;

I am sorry. It is my fault. Now we have very similar node ColumnDef. This
node is designed for usage in utility commands - and it is not designed

for

usage inside a query.

Makes sense.

I had to decide between enhancing ColumnDef node or
introduction new special node. Because there are more special attributes

and

it is hard to serialize current ColumnDef, I decided to use new node.

Seems reasonable. The summary is "this is the parse node for a column
of an XMLTABLE expression".

Suggested comment:

/*
* This is the parsenode for a column definition in a table-expression
like XMLTABLE.
*
* We can't re-use ColumnDef here; the utility command column
definition has all the
* wrong attributes for use in table-expressions and just doesn't make
sense here.
*/
typedef struct TableExprColumn
{
...
};

?

Why "RawCol" ? What does it become when it's not "raw" anymore? Is
that a reference to ColumnDef's raw_default and cooked_default for
untransformed vs transformed parse-trees?

TableExprColumn is better

Regards

Pavel

Show quoted text

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

Suggested comment:

/*
* This is the parsenode for a column definition in a table-expression
like XMLTABLE.
*
* We can't re-use ColumnDef here; the utility command column
definition has all the
* wrong attributes for use in table-expressions and just doesn't make
sense here.
*/
typedef struct TableExprColumn
{
...
};

?

Why "RawCol" ? What does it become when it's not "raw" anymore? Is
that a reference to ColumnDef's raw_default and cooked_default for
untransformed vs transformed parse-trees?

My previous reply was wrong - it is used by parser only and holds TypeName
field. The analogy with ColumnDef raw_default is perfect.

Regards

Pavel

Show quoted text

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

On 7 September 2016 at 14:44, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Suggested comment:

/*
* This is the parsenode for a column definition in a table-expression
like XMLTABLE.
*
* We can't re-use ColumnDef here; the utility command column
definition has all the
* wrong attributes for use in table-expressions and just doesn't make
sense here.
*/
typedef struct TableExprColumn
{
...
};

?

Why "RawCol" ? What does it become when it's not "raw" anymore? Is
that a reference to ColumnDef's raw_default and cooked_default for
untransformed vs transformed parse-trees?

My previous reply was wrong - it is used by parser only and holds TypeName
field. The analogy with ColumnDef raw_default is perfect.

Cool, lets just comment that then.

I'll wait on an updated patch per discussion to date. Hopefully
somebody else with more of a clue than me can offer better review of
the executor/view/caching part you specifically called out as complex.
Otherwise maybe it'll be clearer in a revised version.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

Regards

Pavel

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

new update

more regress tests

Regards

Pavel

Show quoted text

Regards

Pavel

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I'll take a closer read-through shortly.

Missing file. You omitted executor/tableexpr.h from the patch, so I
can't compile.

I've expanded and copy-edited the docs. Some of it is guesswork based
on the references you sent and a glance at the code. Please check my
changes carefully. I found a few surprises, like the fact that DEFAULT
isn't a normal literal, it's an xpath expression evaluated at the same
time as the rowexpression.

Updated patch attached as XMLTABLE-v3 includes the docs changes. Note
that it's missing tableexpr.h. For convenient review or to apply to
your working tree I also attach a diff of just my docs changes as
proposed-docs-changes.diff.

Docs:

- Can you send the sample data used to generate the example output?
I'd like to include at least a cut down part of it in the docs to make
it clear how the input correlates with output, and preferably put the
whole thing in an appendix.

- How does it decide what subset of the document to iterate over?
That's presumably rowexpr, which is xpath in postgresql? (I added this
to docs).

- xmlnamespaces clause in docs needs an example for a non-default namespace.

- What effect does xmlnamespaces clause have? Does supplying it allow
you to reference qualified names in xpath? What happens if you don't
specify it for a document that has namespaces or don't define all the
namespaces? What if you reference an undefined namespace in xpath?
What about if an undefined namespace isn't referenced by xpath, but is
inside a node selected by an xpath expression?

- What are the rules for converting the matched XML node into a value?
If the matched node is not a simple text node or lacks a text node as
its single child, what happens?

- What happens if the matched has multiple text node children? This
can happen if, for example, you have something like

<matchedNode>
some text <!-- comment splits up text node --> other text
</matchedNode>

- Is there a way to get an attribute as a value? If so, an example
should show this because it's going to be a common need. Presumably
you want node/@attrname ?

- What happens if COLUMNS is not specified at all? It looks like it
returns a single column result set with the matched entries as 'xml'
type, so added to docs, please verify.

- PASSING clause isn't really defined. You can specify one PASSING
entry as a literal/colref/expression, and it's the argument xml
document, right? The external docs you referred to say that PASSING
may have a BY VALUE keyword, alias its argument with AS, and may have
expressions, e.g.

PASSING BY VALUE '<x/>' AS a, '<y/>' AS b

Neither aliases nor multiple entries are supported by the code or
grammar. Should this be documented as a restriction? Do you know if
that's an extension by the other implementation or if it's SQL/XML
standard? (I've drafted a docs update to cover this in the updated
patch).

- What does BY REF mean? Should this just be mentioned with a "see
xmlexists(...)" since it seems to be compatibility noise? Is there a
corresponding BY VALUE or similar?

- The parser definitions re-use xmlexists_argument . I don't mind
that, but it's worth noting here in case others do.

- Why do the expression arguments take c_expr (anything allowed in
a_expr or b_expr), not b_expr (restricted expression) ?

- Column definitions are underdocumented. The grammar says they can be
NOT NULL, for example, but I don't see that in any of the references
you mailed me nor in the docs. What behaviour is expected for a NOT
NULL column? I've documented my best guess (not checked closely
against the code, please verify).

-

Test suggestions:

- Coverage of multiple text() node children of an element, where split
up by comment or similar

- Coverage of xpath that matches a node with child element nodes

More to come. Please review my docs changes in the mean time. I'm
spending a lot more time on this than I expected so I might have to
get onto other things for a while too.

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

On 12 September 2016 at 12:28, Craig Ringer <craig@2ndquadrant.com> wrote:

I'll take a closer read-through shortly.

DEFAULT
isn't a normal literal, it's an xpath expression evaluated at the same
time as the rowexpression.

Sorry for the spam, but turns out that's not the case as implemented
here. The docs you referenced say it should be an xpath expression,
but the implementation here is of a literal value, and examples
elsewhere on the Internet show a literal value. Unclear if the
referenced docs are wrong or what and I don't have anything to test
with.

Feel free to fix/trim the DEFAULT related changes in above docs patch as needed.

Also, tests/docs should probably cover what happens when PATH matches
more than one element, i.e. produces a list of more than one match.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2016-09-12 6:36 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

On 12 September 2016 at 12:28, Craig Ringer <craig@2ndquadrant.com> wrote:

I'll take a closer read-through shortly.

DEFAULT
isn't a normal literal, it's an xpath expression evaluated at the same
time as the rowexpression.

Sorry for the spam, but turns out that's not the case as implemented
here. The docs you referenced say it should be an xpath expression,
but the implementation here is of a literal value, and examples
elsewhere on the Internet show a literal value. Unclear if the
referenced docs are wrong or what and I don't have anything to test
with.

It is not spam. The previous comment was not correct. DEFAULT is a
expression - result of this expression is used, when data is missing.

In standard, and some others implementation, this is literal only. It is
similar to DEFAULT clause in CREATE STATEMENT. Postgres allows expression
there. Usually Postgres allows expressions everywhere when it has sense,
and when it is allowed by bizon parser.

Show quoted text

Feel free to fix/trim the DEFAULT related changes in above docs patch as
needed.

Also, tests/docs should probably cover what happens when PATH matches
more than one element, i.e. produces a list of more than one match.

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

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

Show quoted text

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

2016-09-12 6:28 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

I'll take a closer read-through shortly.

Missing file. You omitted executor/tableexpr.h from the patch, so I
can't compile.

I've expanded and copy-edited the docs. Some of it is guesswork based
on the references you sent and a glance at the code. Please check my
changes carefully. I found a few surprises, like the fact that DEFAULT
isn't a normal literal, it's an xpath expression evaluated at the same
time as the rowexpression.

Updated patch attached as XMLTABLE-v3 includes the docs changes. Note
that it's missing tableexpr.h. For convenient review or to apply to
your working tree I also attach a diff of just my docs changes as
proposed-docs-changes.diff.

Docs:

- Can you send the sample data used to generate the example output?
I'd like to include at least a cut down part of it in the docs to make
it clear how the input correlates with output, and preferably put the
whole thing in an appendix.

it is in regress tests.

- How does it decide what subset of the document to iterate over?
That's presumably rowexpr, which is xpath in postgresql? (I added this
to docs).

- xmlnamespaces clause in docs needs an example for a non-default
namespace.

- What effect does xmlnamespaces clause have? Does supplying it allow
you to reference qualified names in xpath? What happens if you don't
specify it for a document that has namespaces or don't define all the
namespaces? What if you reference an undefined namespace in xpath?
What about if an undefined namespace isn't referenced by xpath, but is
inside a node selected by an xpath expression?

All this is under libxml2 control - when you use undefined namespace, then
libxml2 raises a error. The namespaces in document and in XPath queries are
absolutely independent - the relation is a URI. When you use bad URI
(referenced by name), then the result will be empty set. When you use
undefined name, then you will get a error.

- What are the rules for converting the matched XML node into a value?
If the matched node is not a simple text node or lacks a text node as
its single child, what happens?

This process is described and controlled by "XML SQL mapping". The Postgres
has minimalistic implementation without possibility of external control and
without schema support. The my implementation is simple. When user doesn't
specify result target like explicit using of text() function, then the
text() function is used implicitly when target type is not XML. Then I dump
result to string and I enforce related input functions for target types.

- What happens if the matched has multiple text node children? This
can happen if, for example, you have something like

<matchedNode>
some text <!-- comment splits up text node --> other text
</matchedNode>

depends on target type - it is allowed in XML, and it is disallowed for
other types. I though about support of a arrays - but the patch will be
much more complex - there can be recursion - so I disallowed it. When the
user have to solve this issue, then he can use nested XMLTABLE functions
and nested function is working with XML type.

Just for record - This issue is solved in JSON_TABLE functions - it allows
nested PATHS. But XMLTABLE doesn't allow it.

- Is there a way to get an attribute as a value? If so, an example
should show this because it's going to be a common need. Presumably
you want node/@attrname ?

you can use reference to current node "." - so "./@attname" should to work
- a example is in regress tests

- What happens if COLUMNS is not specified at all? It looks like it
returns a single column result set with the matched entries as 'xml'
type, so added to docs, please verify.

sure, that is it

- PASSING clause isn't really defined. You can specify one PASSING
entry as a literal/colref/expression, and it's the argument xml
document, right? The external docs you referred to say that PASSING
may have a BY VALUE keyword, alias its argument with AS, and may have
expressions, e.g.

PASSING BY VALUE '<x/>' AS a, '<y/>' AS b

Neither aliases nor multiple entries are supported by the code or
grammar. Should this be documented as a restriction? Do you know if
that's an extension by the other implementation or if it's SQL/XML
standard? (I've drafted a docs update to cover this in the updated
patch).

The ANSI allows to pass more documents - and then do complex queries with
XQuery. Passing more than one document has not sense in libxml2 based
implementation, so I didn't supported it. The referenced names can be
implemented later - but it needs to changes in XPATH function too.

- What does BY REF mean? Should this just be mentioned with a "see
xmlexists(...)" since it seems to be compatibility noise? Is there a
corresponding BY VALUE or similar?

When the XML document is stored as serialized DOM, then by ref means link
on this DOM. It has not sense in Postgres - because we store XML documents
by value only.

- The parser definitions re-use xmlexists_argument . I don't mind
that, but it's worth noting here in case others do.

It is one clause - see SQL/XML doc PASSING <XML table argument passing
mechanism>

- Why do the expression arguments take c_expr (anything allowed in
a_expr or b_expr), not b_expr (restricted expression) ?

I don't know - I expect the problems with parser - because PASSING is
restricted keyword in ANSI/SQL and unreserved keyword in Postgres.

- Column definitions are underdocumented. The grammar says they can be
NOT NULL, for example, but I don't see that in any of the references
you mailed me nor in the docs. What behaviour is expected for a NOT
NULL column? I've documented my best guess (not checked closely
against the code, please verify).

yes - some other databases allows it - I am thinking so it is useful.

-

Test suggestions:

- Coverage of multiple text() node children of an element, where split
up by comment or similar

- Coverage of xpath that matches a node with child element nodes

I'll do it.

Show quoted text

More to come. Please review my docs changes in the mean time. I'm
spending a lot more time on this than I expected so I might have to
get onto other things for a while too.

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

2016-09-12 6:36 GMT+02:00 Craig Ringer <craig@2ndquadrant.com>:

On 12 September 2016 at 12:28, Craig Ringer <craig@2ndquadrant.com> wrote:

I'll take a closer read-through shortly.

DEFAULT
isn't a normal literal, it's an xpath expression evaluated at the same
time as the rowexpression.

Sorry for the spam, but turns out that's not the case as implemented
here. The docs you referenced say it should be an xpath expression,
but the implementation here is of a literal value, and examples
elsewhere on the Internet show a literal value. Unclear if the
referenced docs are wrong or what and I don't have anything to test
with.

Feel free to fix/trim the DEFAULT related changes in above docs patch as
needed.

Also, tests/docs should probably cover what happens when PATH matches
more than one element, i.e. produces a list of more than one match.

It is there for case, when this is allowed. When you change the target
type to any non XML type, then a error is raised.

I didn't write a negative test cases until the text of messages will be
final (or checked by native speaker).

Regards

Pavel

Show quoted text

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