Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2017-03-08 17:32 GMT+01:00 Alvaro Herrera :

> > > For daily work the default schema support is much more interesting.
> >
> > Let's see that one, then.  It was part of the original submission so
> > depending on how the patch we looks can still cram it in.  But other
> > patches have priority for me now.
> 
> It is theme for 11

Ah, great.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-08 17:32 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2017-03-08 17:01 GMT+01:00 Alvaro Herrera :
>
> > > I didn't add the change you proposed here to keep COLUMNS optional;
> > > instead, I just made COLUMNS mandatory.  I think what you propose here
> > > is not entirely out of the question, but you left out ruleutils.c
> > > support for it, so I decided to leave it aside for now so that I could
> > > get this patch out of my plate once and for all.  If you really want
> > > that feature, you can submit another patch for it and discuss with the
> > > RMT whether it belongs in PG10 or not.
> >
> > It is interesting feature - because it replaces XPATH function, but not
> > important enough.
>
> OK.
>
> > For daily work the default schema support is much more interesting.
>
> Let's see that one, then.  It was part of the original submission so
> depending on how the patch we looks can still cram it in.  But other
> patches have priority for me now.
>

It is theme for 11

Thank you very much

Pavel


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2017-03-08 17:01 GMT+01:00 Alvaro Herrera :

> > I didn't add the change you proposed here to keep COLUMNS optional;
> > instead, I just made COLUMNS mandatory.  I think what you propose here
> > is not entirely out of the question, but you left out ruleutils.c
> > support for it, so I decided to leave it aside for now so that I could
> > get this patch out of my plate once and for all.  If you really want
> > that feature, you can submit another patch for it and discuss with the
> > RMT whether it belongs in PG10 or not.
> 
> It is interesting feature - because it replaces XPATH function, but not
> important enough.

OK.

> For daily work the default schema support is much more interesting.

Let's see that one, then.  It was part of the original submission so
depending on how the patch we looks can still cram it in.  But other
patches have priority for me now.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-08 17:01 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > Hi
> >
> > I used your idea about special columns when COLUMNS are not explicitly
> > defined.
> >
> > All lines that you are dislike removed.
>
> I just pushed XMLTABLE, after some additional changes.  Please test it
> thoroughly and report any problems.
>

Thank you

>
> I didn't add the change you proposed here to keep COLUMNS optional;
> instead, I just made COLUMNS mandatory.  I think what you propose here
> is not entirely out of the question, but you left out ruleutils.c
> support for it, so I decided to leave it aside for now so that I could
> get this patch out of my plate once and for all.  If you really want
> that feature, you can submit another patch for it and discuss with the
> RMT whether it belongs in PG10 or not.
>

It is interesting feature - because it replaces XPATH function, but not
important enough.

For daily work the default schema support is much more interesting.


>
> Some changes I made:
> * I added some pg_stat_statements support.  It works fine for simple
> tests, but deeper testing of it would be appreciated.
>
> * I removed the "buildercxt" memory context.  It seemed mostly
> pointless, and I was disturbed by the MemoryContextResetOnly().
> Per-value memory still uses the per-value memory context, but the rest
> of the stuff is in the per-query context, which should be pretty much
> the same.
>
> * Desultory stylistic changes
>

ok

Regards

Pavel

>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> Hi
> 
> I used your idea about special columns when COLUMNS are not explicitly
> defined.
> 
> All lines that you are dislike removed.

I just pushed XMLTABLE, after some additional changes.  Please test it
thoroughly and report any problems.

I didn't add the change you proposed here to keep COLUMNS optional;
instead, I just made COLUMNS mandatory.  I think what you propose here
is not entirely out of the question, but you left out ruleutils.c
support for it, so I decided to leave it aside for now so that I could
get this patch out of my plate once and for all.  If you really want
that feature, you can submit another patch for it and discuss with the
RMT whether it belongs in PG10 or not.

Some changes I made:
* I added some pg_stat_statements support.  It works fine for simple
tests, but deeper testing of it would be appreciated.

* I removed the "buildercxt" memory context.  It seemed mostly
pointless, and I was disturbed by the MemoryContextResetOnly().
Per-value memory still uses the per-value memory context, but the rest
of the stuff is in the per-query context, which should be pretty much
the same.

* Desultory stylistic changes

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

I used your idea about special columns when COLUMNS are not explicitly
defined.

All lines that you are dislike removed.

Now, almost all code, related to this behave, is in next few lines.

+   /*
+* Use implicit column when it is necessary. The COLUMNS clause is
optional
+* on Oracle and DB2. In this case a result is complete row of XML type.
+*/
+   if (rtf->columns == NIL)
+   {
+   RangeTableFuncCol *fc = makeNode(RangeTableFuncCol);
+   A_Const *n = makeNode(A_Const);
+
+   fc->colname = "xmltable";
+   fc->typeName = makeTypeNameFromOid(XMLOID, -1);
+   n->val.type = T_String;
+   n->val.val.str = ".";
+   n->location = -1;
+
+   fc->colexpr = (Node *) n;
+   rtf->columns = list_make1(fc);
+   }

all regress tests passing.

Regards

Pavel


xmltable-50.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-03 21:04 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2017-03-03 19:15 GMT+01:00 Alvaro Herrera :
>
> > > 2. As I've complained many times, I find the way we manage an empty
> > > COLUMNS clause pretty bad.  The standard doesn't require that syntax
> > > (COLUMNS is required), and I don't like the implementation, so why not
> > > provide the feature in a different way?  My proposal is to change the
> > > column options in gram.y to be something like this:
> >
> > The clause COLUMNS is optional on Oracle and DB2
> >
> > So I prefer a Oracle, DB2 design. If you are strongly against it, then we
> > can remove it to be ANSI/SQL only.
> >
> > I am don't see an good idea to introduce third syntax.
>
> OK.  I think trying to be syntax compatible with DB2 or Oracle is a lost
> cause, because the syntax used in the XPath expressions seems different
> -- I think Oracle uses XQuery (which we don't support) and DB2 uses ...
> not sure what it is, but it doesn't work in our implementation
> (stuff like '$d/employees/emp' in the row expression.)
>

100% compatibility is not possible - but XPath is subset of XQuery and in
reality - the full XQuery examples of XMLTABLE is not often.

Almost all examples of usage XMLTABLE, what I found in blogs, uses XPath
only


>
> In existing applications using those Oracle/DB2, is it common to omit
> the COLUMNS clause?  I searched for "xmltable oracle" and had a look at
> the first few hits outside of the oracle docs:
> http://viralpatel.net/blogs/oracle-xmltable-tutorial/
> http://www.dba-oracle.com/t_xmltable.htm
> http://stackoverflow.com/questions/12690868/how-to-use-xmltable-in-oracle
> https://asktom.oracle.com/pls/asktom/f?p=100:11:0P11_QUESTION_ID:
> 9533111800346252295
> http://stackoverflow.com/questions/1222570/what-is-an-xmltable
> https://community.oracle.com/thread/3955198
>
> Not a single one of these omit the COLUMNS clause (though the second one
> mentions that the clause can be omitted).
>
> I also looked at a few samples with DB2 -- same thing; it is possible,
> but is it common?
>

I don't think so it is common - it is corner case - and I can live without
it well


>
> Anyway, I noticed that "xml PATH '.'" can be used to obtain the full XML
> of the row, which I think is the feature I wanted, so I think we're
> covered and we can omit the case with no COLUMNS, since we already have
> the feature in another way.  No need to implement anything further, and
> we can rip out the special case I don't like.  Example:
>

yes,


>
> CREATE TABLE EMPLOYEES
> (
>id integer,
>data   XML
> );
> INSERT INTO EMPLOYEES
>  VALUES (1, '
> 
> John
> Watson
> 30
> johnwat...@sh.com
> 
> 
> Sherlock
> Homes
> 32
> sherl...@sh.com
> 
> 
> Jim
> Moriarty
> 52
> j...@sh.com
> 
> 
> Mycroft
> Holmes
> 41
> mycr...@sh.com
> 
> ');
>
> This is with COLUMNS omitted:
>
> alvherre=# select xmltable.* from employees,
> xmltable('/Employees/Employee' passing data);
>  xmltable
> ──
> ↵
>  John ↵
>  Watson ↵
>  30   ↵
>  johnwat...@sh.com↵
>  
> ↵
>  Sherlock ↵
>  Homes  ↵
>  32   ↵
>  sherl...@sh.com  ↵
>  
>  ↵
>  Jim  ↵
>  Moriarty   ↵
>  52   ↵
>  j...@sh.com   ↵
>  
>  ↵
>  Mycroft  ↵
>  Holmes ↵
>  41   ↵
>  mycr...@sh.com   ↵
>  
>
> and this is what you get with "xml PATH '.'" (I threw in ORDINALITY just
> for fun):
>
> alvherre=# select xmltable.* from employees,
> xmltable('/Employees/Employee' passing data columns row_number for
> ordinality, emp xml path '.');
>  row_number │   emp
> ┼──
>   1 │↵
> │ John ↵
> │ Watson ↵
> │ 30   ↵
> │ johnwat...@sh.com↵
> │ 
>   2 │↵
> │ Sherlock ↵
> │ Homes  ↵
> │ 32   ↵
> │ sherl...@sh.com  ↵
> │ 
>   3 │ ↵
> │ Jim  ↵
> │ Moriarty   ↵
> │ 52   ↵
> │ j...@sh.com   ↵
> │ 
>   4 │ ↵
> │ Mycroft  ↵
> │ Holmes ↵
> │ 41   ↵
> │ mycr...@sh.com   ↵
> │ 
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> 

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2017-03-03 19:15 GMT+01:00 Alvaro Herrera :

> > 2. As I've complained many times, I find the way we manage an empty
> > COLUMNS clause pretty bad.  The standard doesn't require that syntax
> > (COLUMNS is required), and I don't like the implementation, so why not
> > provide the feature in a different way?  My proposal is to change the
> > column options in gram.y to be something like this:
> 
> The clause COLUMNS is optional on Oracle and DB2
> 
> So I prefer a Oracle, DB2 design. If you are strongly against it, then we
> can remove it to be ANSI/SQL only.
> 
> I am don't see an good idea to introduce third syntax.

OK.  I think trying to be syntax compatible with DB2 or Oracle is a lost
cause, because the syntax used in the XPath expressions seems different
-- I think Oracle uses XQuery (which we don't support) and DB2 uses ...
not sure what it is, but it doesn't work in our implementation
(stuff like '$d/employees/emp' in the row expression.)

In existing applications using those Oracle/DB2, is it common to omit
the COLUMNS clause?  I searched for "xmltable oracle" and had a look at
the first few hits outside of the oracle docs:
http://viralpatel.net/blogs/oracle-xmltable-tutorial/
http://www.dba-oracle.com/t_xmltable.htm
http://stackoverflow.com/questions/12690868/how-to-use-xmltable-in-oracle
https://asktom.oracle.com/pls/asktom/f?p=100:11:0P11_QUESTION_ID:9533111800346252295
http://stackoverflow.com/questions/1222570/what-is-an-xmltable
https://community.oracle.com/thread/3955198

Not a single one of these omit the COLUMNS clause (though the second one
mentions that the clause can be omitted).

I also looked at a few samples with DB2 -- same thing; it is possible,
but is it common?

Anyway, I noticed that "xml PATH '.'" can be used to obtain the full XML
of the row, which I think is the feature I wanted, so I think we're
covered and we can omit the case with no COLUMNS, since we already have
the feature in another way.  No need to implement anything further, and
we can rip out the special case I don't like.  Example:

CREATE TABLE EMPLOYEES
(
   id integer,
   data   XML
);
INSERT INTO EMPLOYEES
 VALUES (1, '

John
Watson
30
johnwat...@sh.com


Sherlock
Homes
32
sherl...@sh.com


Jim
Moriarty
52
j...@sh.com


Mycroft
Holmes
41
mycr...@sh.com

');

This is with COLUMNS omitted:

alvherre=# select xmltable.* from employees, xmltable('/Employees/Employee' 
passing data);
 xmltable 
──
↵
 John ↵
 Watson ↵
 30   ↵
 johnwat...@sh.com↵
 
↵
 Sherlock ↵
 Homes  ↵
 32   ↵
 sherl...@sh.com  ↵
 
 ↵
 Jim  ↵
 Moriarty   ↵
 52   ↵
 j...@sh.com   ↵
 
 ↵
 Mycroft  ↵
 Holmes ↵
 41   ↵
 mycr...@sh.com   ↵
 

and this is what you get with "xml PATH '.'" (I threw in ORDINALITY just
for fun):

alvherre=# select xmltable.* from employees, xmltable('/Employees/Employee' 
passing data columns row_number for ordinality, emp xml path '.');
 row_number │   emp
┼──
  1 │↵
│ John ↵
│ Watson ↵
│ 30   ↵
│ johnwat...@sh.com↵
│ 
  2 │↵
│ Sherlock ↵
│ Homes  ↵
│ 32   ↵
│ sherl...@sh.com  ↵
│ 
  3 │ ↵
│ Jim  ↵
│ Moriarty   ↵
│ 52   ↵
│ j...@sh.com   ↵
│ 
  4 │ ↵
│ Mycroft  ↵
│ Holmes ↵
│ 41   ↵
│ mycr...@sh.com   ↵
│ 

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-03 19:42 GMT+01:00 Pavel Stehule :

>
>
> 2017-03-03 19:15 GMT+01:00 Alvaro Herrera :
>
>> Pavel Stehule wrote:
>>
>> > attached update with fixed tests
>>
>> Heh, I noticed that you removed the libxml "context" lines that
>> differentiate xml.out from xml_2.out when doing this.  My implementation
>> emits those lines, so it was failing for me.  I restored them.
>>
>> I also changed a few things to avoid copying into TableFuncScanState
>> things that come from the TableFunc itself, since the executor state
>> node can grab them from the plan node.  Let's do that.  So instead of
>> "evalcols" the code now checks that the column list is empty; and also,
>> read the ordinality column number from the plan node.
>>
>> I have to bounce this back to you one more time, hopefully the last one
>> I hope.  Two things:
>>
>> 1. Please verify that pg_stat_statements behaves correctly.  The patch
>> doesn't have changes to contrib/ so without testing I'm guessing that it
>> doesn't work.  I think something very simple should do.
>>
>> 2. As I've complained many times, I find the way we manage an empty
>> COLUMNS clause pretty bad.  The standard doesn't require that syntax
>> (COLUMNS is required), and I don't like the implementation, so why not
>> provide the feature in a different way?  My proposal is to change the
>> column options in gram.y to be something like this:
>
>
> The clause COLUMNS is optional on Oracle and DB2
>
> So I prefer a Oracle, DB2 design. If you are strongly against it, then we
> can remove it to be ANSI/SQL only.
>
> I am don't see an good idea to introduce third syntax.
>
>
>> xmltable_column_option_el:
>> IDENT b_expr
>> { $$ = makeDefElem($1, $2, @1); }
>> | DEFAULT b_expr
>> { $$ = makeDefElem("default", $2, @1); }
>> | FULL VALUE_P
>> { $$ = makeDefElem("full_value", NULL,
>> @1); }
>> | NOT NULL_P
>> { $$ = makeDefElem("is_not_null", (Node
>> *) makeInteger(true), @1); }
>> | NULL_P
>> { $$ = makeDefElem("is_not_null", (Node
>> *) makeInteger(false), @1); }
>> ;
>>
>> Note the FULL VALUE.  Then we can process it like
>>
>> else if (strcmp(defel->defname, "full_value") ==
>> 0)
>> {
>> if (fc->colexpr != NULL)
>> ereport(ERROR,
>>
>> (errcode(ERRCODE_SYNTAX_ERROR),
>>  errmsg("FULL ROW
>> may not be specified together with PATH"),
>>
>>  parser_errposition(defel->location)));
>> fc->full_row = true;
>> }
>>
>> So if you want the full XML value of the row, you have to specify it,
>>
>>  .. XMLTABLE ( ... COLUMNS ..., whole_row xml FULL VALUE, ... )
>>
>> This has the extra feature that you can add, say, an ORDINALITY column
>> together with the XML value, something that you cannot do with the
>> current implementation.
>>
>> It doesn't have to be FULL VALUE, but I couldn't think of anything
>> better.  (I didn't want to add any new keywords for this.)  If you have
>> a better idea, let's discuss.
>>
>
> I don't see a introduction own syntax as necessary solution here - use
> Oracle, DB2 compatible syntax, or ANSI.
>
> It is partially corner case - the benefit of this case is almost bigger
> compatibility with mentioned databases.
>
>
>>
>> Code-wise, this completely removes the "else" block in
>> transformRangeTableFunc
>> which I marked with an XXX comment.  That's a good thing -- let's get
>> rid of that.  Also, it should remove the need for the separate "if
>> !columns" case in tfuncLoadRows.  All those cases would become part of
>> the normal code path instead of special cases.  I think
>> XmlTableSetColumnFilter doesn't need any change (we just don't call if
>> for the FULL VALUE row); and XmlTableGetValue needs a special case that
>> if the column filter is NULL (i.e. SetColumnFilter wasn't called for
>> that column) then return the whole row.
>>
>>
>> Of course, this opens an implementation issue: how do you annotate
>> things from parse analysis till execution?  The current TableFunc
>> structure doesn't help, because there are only lists of column names and
>> expressions; and we can't use the case of a NULL colexpr, because that
>> case is already used by the column filter being the column name (a
>> feature required by the standard).  A simple way would be to have a new
>> "colno" struct member, to store a column number for the column marked
>> FULL VALUE (just like ordinalitycol).  This means you can't have more
>> than one of those FULL VALUE columns, but that seems okay.
>>
>>
>> (Of course, this means 

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-03 19:15 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
>
> > attached update with fixed tests
>
> Heh, I noticed that you removed the libxml "context" lines that
> differentiate xml.out from xml_2.out when doing this.  My implementation
> emits those lines, so it was failing for me.  I restored them.
>
> I also changed a few things to avoid copying into TableFuncScanState
> things that come from the TableFunc itself, since the executor state
> node can grab them from the plan node.  Let's do that.  So instead of
> "evalcols" the code now checks that the column list is empty; and also,
> read the ordinality column number from the plan node.
>
> I have to bounce this back to you one more time, hopefully the last one
> I hope.  Two things:
>
> 1. Please verify that pg_stat_statements behaves correctly.  The patch
> doesn't have changes to contrib/ so without testing I'm guessing that it
> doesn't work.  I think something very simple should do.
>
> 2. As I've complained many times, I find the way we manage an empty
> COLUMNS clause pretty bad.  The standard doesn't require that syntax
> (COLUMNS is required), and I don't like the implementation, so why not
> provide the feature in a different way?  My proposal is to change the
> column options in gram.y to be something like this:


The clause COLUMNS is optional on Oracle and DB2

So I prefer a Oracle, DB2 design. If you are strongly against it, then we
can remove it to be ANSI/SQL only.

I am don't see an good idea to introduce third syntax.


> xmltable_column_option_el:
> IDENT b_expr
> { $$ = makeDefElem($1, $2, @1); }
> | DEFAULT b_expr
> { $$ = makeDefElem("default", $2, @1); }
> | FULL VALUE_P
> { $$ = makeDefElem("full_value", NULL,
> @1); }
> | NOT NULL_P
> { $$ = makeDefElem("is_not_null", (Node *)
> makeInteger(true), @1); }
> | NULL_P
> { $$ = makeDefElem("is_not_null", (Node *)
> makeInteger(false), @1); }
> ;
>
> Note the FULL VALUE.  Then we can process it like
>
> else if (strcmp(defel->defname, "full_value") == 0)
> {
> if (fc->colexpr != NULL)
> ereport(ERROR,
>
> (errcode(ERRCODE_SYNTAX_ERROR),
>  errmsg("FULL ROW
> may not be specified together with PATH"),
>
>  parser_errposition(defel->location)));
> fc->full_row = true;
> }
>
> So if you want the full XML value of the row, you have to specify it,
>
>  .. XMLTABLE ( ... COLUMNS ..., whole_row xml FULL VALUE, ... )
>
> This has the extra feature that you can add, say, an ORDINALITY column
> together with the XML value, something that you cannot do with the
> current implementation.
>
> It doesn't have to be FULL VALUE, but I couldn't think of anything
> better.  (I didn't want to add any new keywords for this.)  If you have
> a better idea, let's discuss.
>

I don't see a introduction own syntax as necessary solution here - use
Oracle, DB2 compatible syntax, or ANSI.

It is partially corner case - the benefit of this case is almost bigger
compatibility with mentioned databases.


>
> Code-wise, this completely removes the "else" block in
> transformRangeTableFunc
> which I marked with an XXX comment.  That's a good thing -- let's get
> rid of that.  Also, it should remove the need for the separate "if
> !columns" case in tfuncLoadRows.  All those cases would become part of
> the normal code path instead of special cases.  I think
> XmlTableSetColumnFilter doesn't need any change (we just don't call if
> for the FULL VALUE row); and XmlTableGetValue needs a special case that
> if the column filter is NULL (i.e. SetColumnFilter wasn't called for
> that column) then return the whole row.
>
>
> Of course, this opens an implementation issue: how do you annotate
> things from parse analysis till execution?  The current TableFunc
> structure doesn't help, because there are only lists of column names and
> expressions; and we can't use the case of a NULL colexpr, because that
> case is already used by the column filter being the column name (a
> feature required by the standard).  A simple way would be to have a new
> "colno" struct member, to store a column number for the column marked
> FULL VALUE (just like ordinalitycol).  This means you can't have more
> than one of those FULL VALUE columns, but that seems okay.
>
>
> (Of course, this means that the two cases that have no COLUMNS in the
> "xmltable" production in gram.y should go away).
>

You are commiter, and you should to decide - as first I prefer current
state, as second a remove 

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> attached update with fixed tests

Heh, I noticed that you removed the libxml "context" lines that
differentiate xml.out from xml_2.out when doing this.  My implementation
emits those lines, so it was failing for me.  I restored them.

I also changed a few things to avoid copying into TableFuncScanState
things that come from the TableFunc itself, since the executor state
node can grab them from the plan node.  Let's do that.  So instead of
"evalcols" the code now checks that the column list is empty; and also,
read the ordinality column number from the plan node.

I have to bounce this back to you one more time, hopefully the last one
I hope.  Two things:

1. Please verify that pg_stat_statements behaves correctly.  The patch
doesn't have changes to contrib/ so without testing I'm guessing that it
doesn't work.  I think something very simple should do.

2. As I've complained many times, I find the way we manage an empty
COLUMNS clause pretty bad.  The standard doesn't require that syntax
(COLUMNS is required), and I don't like the implementation, so why not
provide the feature in a different way?  My proposal is to change the
column options in gram.y to be something like this:

xmltable_column_option_el:
IDENT b_expr
{ $$ = makeDefElem($1, $2, @1); }
| DEFAULT b_expr
{ $$ = makeDefElem("default", $2, @1); }
| FULL VALUE_P
{ $$ = makeDefElem("full_value", NULL, @1); }
| NOT NULL_P
{ $$ = makeDefElem("is_not_null", (Node *) 
makeInteger(true), @1); }
| NULL_P
{ $$ = makeDefElem("is_not_null", (Node *) 
makeInteger(false), @1); }
;

Note the FULL VALUE.  Then we can process it like

else if (strcmp(defel->defname, "full_value") == 0)
{
if (fc->colexpr != NULL)
ereport(ERROR,

(errcode(ERRCODE_SYNTAX_ERROR),
 errmsg("FULL ROW may 
not be specified together with PATH"),
 
parser_errposition(defel->location)));
fc->full_row = true;
}

So if you want the full XML value of the row, you have to specify it,

 .. XMLTABLE ( ... COLUMNS ..., whole_row xml FULL VALUE, ... )

This has the extra feature that you can add, say, an ORDINALITY column
together with the XML value, something that you cannot do with the
current implementation.

It doesn't have to be FULL VALUE, but I couldn't think of anything
better.  (I didn't want to add any new keywords for this.)  If you have
a better idea, let's discuss.

Code-wise, this completely removes the "else" block in transformRangeTableFunc
which I marked with an XXX comment.  That's a good thing -- let's get
rid of that.  Also, it should remove the need for the separate "if
!columns" case in tfuncLoadRows.  All those cases would become part of
the normal code path instead of special cases.  I think
XmlTableSetColumnFilter doesn't need any change (we just don't call if
for the FULL VALUE row); and XmlTableGetValue needs a special case that
if the column filter is NULL (i.e. SetColumnFilter wasn't called for
that column) then return the whole row.


Of course, this opens an implementation issue: how do you annotate
things from parse analysis till execution?  The current TableFunc
structure doesn't help, because there are only lists of column names and
expressions; and we can't use the case of a NULL colexpr, because that
case is already used by the column filter being the column name (a
feature required by the standard).  A simple way would be to have a new
"colno" struct member, to store a column number for the column marked
FULL VALUE (just like ordinalitycol).  This means you can't have more
than one of those FULL VALUE columns, but that seems okay.


(Of course, this means that the two cases that have no COLUMNS in the
"xmltable" production in gram.y should go away).

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


xmltable-49.patch.gz
Description: application/gunzip

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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-02 22:35 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2017-03-02 19:32 GMT+01:00 Alvaro Herrera :
> >
> > > So in the old (non-executor-node) implementation, you could attach WITH
> > > ORDINALITY to the xmltable expression and it would count the output
> > > rows, regardless of which XML document it comes from.  With the new
> > > implementation, the grammar no longer accepts it.  To count output
> rows,
> > > you still need to use row_number().  Maybe this is okay.  This is the
> > > example from the docs, and I add another XML document with two more
> rows
> > > for xmltable.  Look at the three numbering columns ...
> > >
> >
> > It is expected - now tablefunc are not special case of SRF, so it lost
> all
> > SRF functionality. It is not critical lost - it supports internally FOR
> > ORDINALITY column, and classic ROW_NUMBER can be used. It can be enhanced
> > to support WITH ORDINALITY in future, but I have not any use case for it.
>
> Fine.
>
> After looking at the new executor code a bit, I noticed that we don't
> need the resultSlot anymore; we can use the ss_ScanTupleSlot instead.
> Because resultSlot was being used in the xml.c code (which already
> appeared a bit dubious to me), I changed the interface so that instead
> the things that it read from it are passed as parameters -- namely, in
> InitBuilder we pass natts, and in GetValue we pass typid and typmod.
>

I had similar feeling

>
> Secondly, I noticed we have the FetchRow routine produce a minimal
> tuple, put it in a slot; then its caller takes the slot and put the
> tuple in the tuplestore.  This is pointless; we can just have FetchRow
> put the tuple in the tuplestore directly and not bother with any slot
> manipulations there.  This simplifies the code a bit.
>
>
has sense

attached update with fixed tests

Regards

Pavel



> Here's v47 with those changes.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


xmltable-48.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2017-03-02 19:32 GMT+01:00 Alvaro Herrera :
> 
> > So in the old (non-executor-node) implementation, you could attach WITH
> > ORDINALITY to the xmltable expression and it would count the output
> > rows, regardless of which XML document it comes from.  With the new
> > implementation, the grammar no longer accepts it.  To count output rows,
> > you still need to use row_number().  Maybe this is okay.  This is the
> > example from the docs, and I add another XML document with two more rows
> > for xmltable.  Look at the three numbering columns ...
> >
> 
> It is expected - now tablefunc are not special case of SRF, so it lost all
> SRF functionality. It is not critical lost - it supports internally FOR
> ORDINALITY column, and classic ROW_NUMBER can be used. It can be enhanced
> to support WITH ORDINALITY in future, but I have not any use case for it.

Fine.

After looking at the new executor code a bit, I noticed that we don't
need the resultSlot anymore; we can use the ss_ScanTupleSlot instead.
Because resultSlot was being used in the xml.c code (which already
appeared a bit dubious to me), I changed the interface so that instead
the things that it read from it are passed as parameters -- namely, in
InitBuilder we pass natts, and in GetValue we pass typid and typmod.

Secondly, I noticed we have the FetchRow routine produce a minimal
tuple, put it in a slot; then its caller takes the slot and put the
tuple in the tuplestore.  This is pointless; we can just have FetchRow
put the tuple in the tuplestore directly and not bother with any slot
manipulations there.  This simplifies the code a bit.

Here's v47 with those changes.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


xmltable-47.patch.gz
Description: application/gunzip

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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-02 19:32 GMT+01:00 Alvaro Herrera :

> So in the old (non-executor-node) implementation, you could attach WITH
> ORDINALITY to the xmltable expression and it would count the output
> rows, regardless of which XML document it comes from.  With the new
> implementation, the grammar no longer accepts it.  To count output rows,
> you still need to use row_number().  Maybe this is okay.  This is the
> example from the docs, and I add another XML document with two more rows
> for xmltable.  Look at the three numbering columns ...
>

It is expected - now tablefunc are not special case of SRF, so it lost all
SRF functionality. It is not critical lost - it supports internally FOR
ORDINALITY column, and classic ROW_NUMBER can be used. It can be enhanced
to support WITH ORDINALITY in future, but I have not any use case for it.

Regards

Pavel



>
> CREATE TABLE xmldata AS SELECT
> xml $$
> 
>   
> AU
> Australia
>   
>   
> JP
> Japan
> Shinzo Abe
> 145935
>   
>   
> SG
> Singapore
> 697
>   
> 
> $$ AS data;
>
>  insert into xmldata values ($$
>  CL
> Chile
>  AR
> Argentina$$);
>
> SELECT ROW_NUMBER() OVER (), xmltable.*
>   FROM xmldata,
>XMLTABLE('//ROWS/ROW'
> PASSING data
> COLUMNS id int PATH '@id',
> ordinality FOR ORDINALITY,
> "COUNTRY_NAME" text,
> country_id text PATH 'COUNTRY_ID',
> size_sq_km float PATH 'SIZE[@unit = "sq_km"]',
> size_other text PATH
>  'concat(SIZE[@unit!="sq_km"], " ",
> SIZE[@unit!="sq_km"]/@unit)',
> premier_name text PATH 'PREMIER_NAME' DEFAULT 'not
> specified')
> ;
>
>  row_number │ id │ ordinality │ COUNTRY_NAME │ country_id │ size_sq_km │
> size_other  │ premier_name
> ┼┼┼──┼┼─
> ───┼──┼───
>   1 │  1 │  1 │ Australia│ AU ││
> │ not specified
>   2 │  5 │  2 │ Japan│ JP ││
> 145935 sq_mi │ Shinzo Abe
>   3 │  6 │  3 │ Singapore│ SG │697 │
> │ not specified
>   4 │  2 │  1 │ Chile│ CL ││
> │ not specified
>   5 │  3 │  2 │ Argentina│ AR ││
> │ not specified
>
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

So in the old (non-executor-node) implementation, you could attach WITH
ORDINALITY to the xmltable expression and it would count the output
rows, regardless of which XML document it comes from.  With the new
implementation, the grammar no longer accepts it.  To count output rows,
you still need to use row_number().  Maybe this is okay.  This is the
example from the docs, and I add another XML document with two more rows
for xmltable.  Look at the three numbering columns ...

CREATE TABLE xmldata AS SELECT
xml $$
  
  
AU
Australia
  
  
JP
Japan
Shinzo Abe
145935
  
  
SG
Singapore
697
  

$$ AS data;

 insert into xmldata values ($$
 CLChile
 ARArgentina$$);

SELECT ROW_NUMBER() OVER (), xmltable.*
  FROM xmldata, 
 
   XMLTABLE('//ROWS/ROW'
  
PASSING data
COLUMNS id int PATH '@id',
ordinality FOR ORDINALITY,
"COUNTRY_NAME" text,
country_id text PATH 'COUNTRY_ID',
size_sq_km float PATH 'SIZE[@unit = "sq_km"]',
size_other text PATH
 'concat(SIZE[@unit!="sq_km"], " ", 
SIZE[@unit!="sq_km"]/@unit)',
premier_name text PATH 'PREMIER_NAME' DEFAULT 'not 
specified')
;

 row_number │ id │ ordinality │ COUNTRY_NAME │ country_id │ size_sq_km │  
size_other  │ premier_name  
┼┼┼──┼┼┼──┼───
  1 │  1 │  1 │ Australia│ AU ││
  │ not specified
  2 │  5 │  2 │ Japan│ JP ││ 145935 
sq_mi │ Shinzo Abe
  3 │  6 │  3 │ Singapore│ SG │697 │
  │ not specified
  4 │  2 │  1 │ Chile│ CL ││
  │ not specified
  5 │  3 │  2 │ Argentina│ AR ││
  │ not specified


-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Dne 2. 3. 2017 18:14 napsal uživatel "Alvaro Herrera" <
alvhe...@2ndquadrant.com>:

Pavel Stehule wrote:

> It is documented already
>
> "If the PATH matches an empty tag the result is an empty
string"

Hmm, okay.  But what we have here is not an empty tag, but a tag that is
completely missing.  I don't think those two cases should be treated in
the same way ...


this information is not propagated from libxml2.


> Attached new patch
>
> cleaned documentation
> regress tests is more robust
> appended comment in src related to generating empty string for empty tag

Thanks, I incorporated those changes.  Here's v46.  I rewrote the
documentation, and fixed a couple of incorrectly copied comments
in the new executor code; I think that one looks good.  In the future we
could rewrite it to avoid the need for a tuplestore, but I think the
current approach is good enough for a pg10 implementation.

Barring serious problems, I intend to commit this later today.



thank you very much

regards

Pavel


--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> It is documented already
> 
> "If the PATH matches an empty tag the result is an empty string"

Hmm, okay.  But what we have here is not an empty tag, but a tag that is
completely missing.  I don't think those two cases should be treated in
the same way ...

> Attached new patch
> 
> cleaned documentation
> regress tests is more robust
> appended comment in src related to generating empty string for empty tag

Thanks, I incorporated those changes.  Here's v46.  I rewrote the
documentation, and fixed a couple of incorrectly copied comments
in the new executor code; I think that one looks good.  In the future we
could rewrite it to avoid the need for a tuplestore, but I think the
current approach is good enough for a pg10 implementation.

Barring serious problems, I intend to commit this later today.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


xmltable-46.patch.gz
Description: application/gunzip

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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-03-02 8:04 GMT+01:00 Pavel Stehule :

> Hi
>
> 2017-03-02 1:12 GMT+01:00 Alvaro Herrera :
>
>>
>> I've been giving this a look.  I started by tweaking the docs once
>> again, and while verifying that the example works as expected, I
>> replayed what I have in sgml:
>>
>> ... begin SGML paste ...
>> 
>>  For example, given the following XML document:
>>   
>>
>>  the following query produces the result shown below:
>>
>> 

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2017-03-02 1:12 GMT+01:00 Alvaro Herrera :

>
> I've been giving this a look.  I started by tweaking the docs once
> again, and while verifying that the example works as expected, I
> replayed what I have in sgml:
>
> ... begin SGML paste ...
> 
>  For example, given the following XML document:
>   
>
>  the following query produces the result shown below:
>
> 

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

I've been giving this a look.  I started by tweaking the docs once
again, and while verifying that the example works as expected, I
replayed what I have in sgml:

... begin SGML paste ...

 For example, given the following XML document:
  

 the following query produces the result shown below:

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2017-01-31 14:57 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2017-01-24 21:38 GMT+01:00 Alvaro Herrera :
>
> > > I think it would be good to have a more complex test case in regress --
> > > let's say there is a table with some simple XML values, then we use
> > > XMLFOREST (or maybe one of the table_to_xml functions) to generate a
> > > large document, and then XMLTABLE uses that document as input document.
> >
> > I have a 16K lines long real XML 6.MB. Probably we would not to append it
> > to regress tests.
> >
> > It is really fast - original customer implementation 20min, nested our
> > xpath implementation 10 sec, PLPython xml reader 5 sec, xmltable 400ms
>
> That's great numbers, kudos for the hard work here.  That will make for
> a nice headline in pg10 PR materials.  But what I was getting at is that
> I would like to exercise a bit more of the expression handling in
> xmltable execution, to make sure it doesn't handle just string literals.
>

done


>
> > I have a plan to create tests based on pg_proc and CTE - if all works,
> then
> > the query must be empty
> >
> > with x as (select proname, proowner, procost, pronargs,
> > array_to_string(proargnames,',') as proargnames,
> > array_to_string(proargtypes,',') as proargtypes from pg_proc), y as
> (select
> > xmlelement(name proc, xmlforest(proname, proowner, procost, pronargs,
> > proargnames, proargtypes)) as proc from x), z as (select xmltable.* from
> y,
> > lateral xmltable('/proc' passing proc columns proname name, proowner oid,
> > procost float, pronargs int, proargnames text, proargtypes text)) select
> *
> > from z except select * from x;
>
> Nice one :-)
>

please see attached patch

* enhanced regress tests
* clean memory context work

Regards

Pavel


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


xmltable-42.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-31 14:57 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2017-01-24 21:38 GMT+01:00 Alvaro Herrera :
>
> > > I think it would be good to have a more complex test case in regress --
> > > let's say there is a table with some simple XML values, then we use
> > > XMLFOREST (or maybe one of the table_to_xml functions) to generate a
> > > large document, and then XMLTABLE uses that document as input document.
> >
> > I have a 16K lines long real XML 6.MB. Probably we would not to append it
> > to regress tests.
> >
> > It is really fast - original customer implementation 20min, nested our
> > xpath implementation 10 sec, PLPython xml reader 5 sec, xmltable 400ms
>
> That's great numbers, kudos for the hard work here.  That will make for
> a nice headline in pg10 PR materials.  But what I was getting at is that
> I would like to exercise a bit more of the expression handling in
> xmltable execution, to make sure it doesn't handle just string literals.
>

I'll try to write some more dynamic examples.


>
> > I have a plan to create tests based on pg_proc and CTE - if all works,
> then
> > the query must be empty
> >
> > with x as (select proname, proowner, procost, pronargs,
> > array_to_string(proargnames,',') as proargnames,
> > array_to_string(proargtypes,',') as proargtypes from pg_proc), y as
> (select
> > xmlelement(name proc, xmlforest(proname, proowner, procost, pronargs,
> > proargnames, proargtypes)) as proc from x), z as (select xmltable.* from
> y,
> > lateral xmltable('/proc' passing proc columns proname name, proowner oid,
> > procost float, pronargs int, proargnames text, proargtypes text)) select
> *
> > from z except select * from x;
>
> Nice one :-)
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2017-01-24 21:38 GMT+01:00 Alvaro Herrera :

> > I think it would be good to have a more complex test case in regress --
> > let's say there is a table with some simple XML values, then we use
> > XMLFOREST (or maybe one of the table_to_xml functions) to generate a
> > large document, and then XMLTABLE uses that document as input document.
> 
> I have a 16K lines long real XML 6.MB. Probably we would not to append it
> to regress tests.
> 
> It is really fast - original customer implementation 20min, nested our
> xpath implementation 10 sec, PLPython xml reader 5 sec, xmltable 400ms

That's great numbers, kudos for the hard work here.  That will make for
a nice headline in pg10 PR materials.  But what I was getting at is that
I would like to exercise a bit more of the expression handling in
xmltable execution, to make sure it doesn't handle just string literals.

> I have a plan to create tests based on pg_proc and CTE - if all works, then
> the query must be empty
> 
> with x as (select proname, proowner, procost, pronargs,
> array_to_string(proargnames,',') as proargnames,
> array_to_string(proargtypes,',') as proargtypes from pg_proc), y as (select
> xmlelement(name proc, xmlforest(proname, proowner, procost, pronargs,
> proargnames, proargtypes)) as proc from x), z as (select xmltable.* from y,
> lateral xmltable('/proc' passing proc columns proname name, proowner oid,
> procost float, pronargs int, proargnames text, proargtypes text)) select *
> from z except select * from x;

Nice one :-)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-24 21:38 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
>
> > * SELECT (xmltable(..)).* + regress tests
> > * compilation and regress tests without --with-libxml
>
> Thanks.  I just realized that this is doing more work than necessary --
> I think it would be simpler to have tableexpr fill a tuplestore with the
> results, instead of just expecting function execution to apply
> ExecEvalExpr over and over to obtain the results.  So evaluating a
> tableexpr returns just the tuplestore, which function evaluation can
> return as-is.  That code doesn't use the value-per-call interface
> anyway.
>
> I also realized that the expr context callback is not called if there's
> an error, which leaves us without shutting down libxml properly.  I
> added PG_TRY around the fetchrow calls, but I'm not sure that's correct
> either, because there could be an error raised in other parts of the
> code, after we've already emitted a few rows (for example out of
> memory).  I think the right way is to have PG_TRY around the execution
> of the whole thing rather than just row at a time; and the tuplestore
> mechanism helps us with that.
>
> I think it would be good to have a more complex test case in regress --
> let's say there is a table with some simple XML values, then we use
> XMLFOREST (or maybe one of the table_to_xml functions) to generate a
> large document, and then XMLTABLE uses that document as input document.
>

I have a 16K lines long real XML 6.MB. Probably we would not to append it
to regress tests.

It is really fast - original customer implementation 20min, nested our
xpath implementation 10 sec, PLPython xml reader 5 sec, xmltable 400ms

I have a plan to create tests based on pg_proc and CTE - if all works, then
the query must be empty

with x as (select proname, proowner, procost, pronargs,
array_to_string(proargnames,',') as proargnames,
array_to_string(proargtypes,',') as proargtypes from pg_proc), y as (select
xmlelement(name proc, xmlforest(proname, proowner, procost, pronargs,
proargnames, proargtypes)) as proc from x), z as (select xmltable.* from y,
lateral xmltable('/proc' passing proc columns proname name, proowner oid,
procost float, pronargs int, proargnames text, proargtypes text)) select *
from z except select * from x;



>
> Please fix.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Michael Paquier]

On Tue, Jan 31, 2017 at 5:18 AM, Pavel Stehule  wrote:
> true, I am sorry

Last status is a new patch and no reviews. On top of that this thread
is quite active. So moved to next CF. Pavel, please be careful about
the status of the patch on the CF app, it was set to "waiting on
author"...
-- 
Michael


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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2017-01-30 20:38 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
>
> > I am sending new version - it is based on own executor scan node and
> > tuplestore.
> >
> > Some now obsolete regress tests removed, some new added.
> >
> > The executor code (memory context usage) should be cleaned little bit -
> but
> > other code should be good.
>
> I think you forgot nodeTableFuncscan.c.
>

true, I am sorry

attached

Regards

Pavel


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


xmltable-41.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> I am sending new version - it is based on own executor scan node and
> tuplestore.
> 
> Some now obsolete regress tests removed, some new added.
> 
> The executor code (memory context usage) should be cleaned little bit - but
> other code should be good.

I think you forgot nodeTableFuncscan.c.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2017-01-25 15:07 GMT+01:00 Alvaro Herrera :

> Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2017-01-24 21:32:56 -0300, Alvaro Herrera wrote:
> > >> 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.
> >
> > +1 --- we're out of the business of having simple expressions that
> > return rowsets.
>
> Well, that's it.  I'm not committing this patch against two other
> committers' opinion, plus I was already on the fence about the
> implementation anyway.  I think you should just go with the flow and
> implement this by creating nodeTableexprscan.c.  It's not even
> difficult.
>

I am sending new version - it is based on own executor scan node and
tuplestore.

Some now obsolete regress tests removed, some new added.

The executor code (memory context usage) should be cleaned little bit - but
other code should be good.

Regards

Pavel


> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


xmltable-40.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-25 15:07 GMT+01:00 Alvaro Herrera :

> Tom Lane wrote:
> > Andres Freund  writes:
> > > On 2017-01-24 21:32:56 -0300, Alvaro Herrera wrote:
> > >> 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.
> >
> > +1 --- we're out of the business of having simple expressions that
> > return rowsets.
>
> Well, that's it.  I'm not committing this patch against two other
> committers' opinion, plus I was already on the fence about the
> implementation anyway.  I think you should just go with the flow and
> implement this by creating nodeTableexprscan.c.  It's not even
> difficult.
>

I am playing with this and the patch looks about 15kB longer - just due
implementation basic scan functionality - and I didn't touch a planner.

I am not happy from this - still I have a feeling so I try to reimplement
reduced SRF.

Regards

Pavel

>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-25 23:33 GMT+01:00 Andres Freund :

> On 2017-01-25 22:51:37 +0100, Pavel Stehule wrote:
> > 2017-01-25 22:40 GMT+01:00 Andres Freund :
> > > > I afraid when I cannot to reuse a SRF infrastructure, I have to
> > > reimplement
> > > > it partially :( - mainly for usage in "ROWS FROM ()"
> > >
> >
> > The TableExpr implementation is based on SRF now. You and Alvaro propose
> > independent implementation like generic executor node. I am sceptic so
> > FunctionScan supports reading from generic executor node.
>
> Why would it need to?
>

Simply - due consistency with any other functions that can returns rows.

Maybe I don't understand to Alvaro proposal well - I have a XMLTABLE
function - TableExpr that looks like SRF function, has similar behave -
returns more rows, but should be significantly different implemented, and
should to have different limits - should not be used there and there ... It
is hard to see consistency there for me.

Regards

Pavel

Re: [HACKERS] patch: function xmltable

[Andres Freund]

On 2017-01-25 22:51:37 +0100, Pavel Stehule wrote:
> 2017-01-25 22:40 GMT+01:00 Andres Freund :
> > > I afraid when I cannot to reuse a SRF infrastructure, I have to
> > reimplement
> > > it partially :( - mainly for usage in "ROWS FROM ()"
> >
> 
> The TableExpr implementation is based on SRF now. You and Alvaro propose
> independent implementation like generic executor node. I am sceptic so
> FunctionScan supports reading from generic executor node.

Why would it need to?


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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-25 22:40 GMT+01:00 Andres Freund :

> Hi,
>
> > > > 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?
> > >
> > > targetlist SRFs were a big mistake. They cause a fair number of
> problems
> > > code-wise. They permeated for a long while into bits of both planner
> and
> > > executor, where they really shouldn't belong. Even after the recent
> > > changes there's a fair amount of uglyness associated with them.  We
> > > can't remove tSRFs for backward compatibility reasons, but that's not
> > > true for XMLTABLE
> > >
> > >
> > >
> > ok
> >
> > I afraid when I cannot to reuse a SRF infrastructure, I have to
> reimplement
> > it partially :( - mainly for usage in "ROWS FROM ()"
>

The TableExpr implementation is based on SRF now. You and Alvaro propose
independent implementation like generic executor node. I am sceptic so
FunctionScan supports reading from generic executor node.

Regards

Pavel


> Huh?
>
> Greetings,
>
> Andres Freund
>

Re: [HACKERS] patch: function xmltable

[Andres Freund]

Hi,

> > > 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?
> >
> > targetlist SRFs were a big mistake. They cause a fair number of problems
> > code-wise. They permeated for a long while into bits of both planner and
> > executor, where they really shouldn't belong. Even after the recent
> > changes there's a fair amount of uglyness associated with them.  We
> > can't remove tSRFs for backward compatibility reasons, but that's not
> > true for XMLTABLE
> >
> >
> >
> ok
> 
> I afraid when I cannot to reuse a SRF infrastructure, I have to reimplement
> it partially :( - mainly for usage in "ROWS FROM ()"

Huh?

Greetings,

Andres Freund


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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

> > >
> >
> > If you plan to hold support SRFin target list, then nothing is different.
> > In last patch is executed under nodeProjectSet.
>
> It is, because we suddenly need to call different functions - and I'm
> revamping most of execQual to have an opcode dispatch based execution
> model (which then also can be JITed).
>

> > > > 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.
>
> You're introducing a wholly separate callback system (TableExprRoutine)
> for the new functionality.  And that stuff is excruciatingly close to
> stuff that the normal executor already knows how to do.
>

These callbacks are related to isolation TableExpr infrastructure and
TableExpr implementation - This design is prepared for reusing for
JSON_TABLE function.

Any placing of TableExpr code should not impact this callback system (Or I
am absolutely out and executor is able do some work what is hidden to me).


>
>
>
> > 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?
>
> targetlist SRFs were a big mistake. They cause a fair number of problems
> code-wise. They permeated for a long while into bits of both planner and
> executor, where they really shouldn't belong. Even after the recent
> changes there's a fair amount of uglyness associated with them.  We
> can't remove tSRFs for backward compatibility reasons, but that's not
> true for XMLTABLE
>
>
>
ok

I afraid when I cannot to reuse a SRF infrastructure, I have to reimplement
it partially :( - mainly for usage in "ROWS FROM ()"



Greetings,
>
> Andres Freund
>

Re: [HACKERS] patch: function xmltable

[Andres Freund]

Hi,

On 2017-01-25 05:45:24 +0100, Pavel Stehule wrote:
> 2017-01-25 1:35 GMT+01:00 Andres Freund :
> 
> > 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.

It is, because we suddenly need to call different functions - and I'm
revamping most of execQual to have an opcode dispatch based execution
model (which then also can be JITed).


> > > 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.

You're introducing a wholly separate callback system (TableExprRoutine)
for the new functionality.  And that stuff is excruciatingly close to
stuff that the normal executor already knows how to do.



> 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?

targetlist SRFs were a big mistake. They cause a fair number of problems
code-wise. They permeated for a long while into bits of both planner and
executor, where they really shouldn't belong. Even after the recent
changes there's a fair amount of uglyness associated with them.  We
can't remove tSRFs for backward compatibility reasons, but that's not
true for XMLTABLE


Greetings,

Andres Freund


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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-01-24 21:32:56 -0300, Alvaro Herrera wrote:
> >> 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.
> 
> +1 --- we're out of the business of having simple expressions that
> return rowsets.

Well, that's it.  I'm not committing this patch against two other
committers' opinion, plus I was already on the fence about the
implementation anyway.  I think you should just go with the flow and
implement this by creating nodeTableexprscan.c.  It's not even
difficult.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-24 21:38 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
>
> > * SELECT (xmltable(..)).* + regress tests
> > * compilation and regress tests without --with-libxml
>
> Thanks.  I just realized that this is doing more work than necessary --
>

?? I don't understand?


> I think it would be simpler to have tableexpr fill a tuplestore with the
> results, instead of just expecting function execution to apply
> ExecEvalExpr over and over to obtain the results.  So evaluating a
> tableexpr returns just the tuplestore, which function evaluation can
> return as-is.  That code doesn't use the value-per-call interface
> anyway.
>

ok


> I also realized that the expr context callback is not called if there's
> an error, which leaves us without shutting down libxml properly.  I
> added PG_TRY around the fetchrow calls, but I'm not sure that's correct
> either, because there could be an error raised in other parts of the
> code, after we've already emitted a few rows (for example out of
> memory).  I think the right way is to have PG_TRY around the execution
> of the whole thing rather than just row at a time; and the tuplestore
> mechanism helps us with that.
>


ok.



>
> I think it would be good to have a more complex test case in regress --
> let's say there is a table with some simple XML values, then we use
> XMLFOREST (or maybe one of the table_to_xml functions) to generate a
> large document, and then XMLTABLE uses that document as input document.
>
> Please fix.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-25 5:45 GMT+01:00 Tom Lane :

> Andres Freund  writes:
> > On 2017-01-24 21:32:56 -0300, Alvaro Herrera wrote:
> >> 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.
>
> +1 --- we're out of the business of having simple expressions that
> return rowsets.
>

If we do decision so this kind of function will have different behave than
other SRF functions, then I remove support for this.

There are not technical reasons (maybe I don't see it) - last Andres
changes do well support for my code.

Regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2017-01-25 1:35 GMT+01:00 Andres Freund :

> 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
>

Re: [HACKERS] patch: function xmltable

[Tom Lane]

Andres Freund  writes:
> On 2017-01-24 21:32:56 -0300, Alvaro Herrera wrote:
>> 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.

+1 --- we're out of the business of having simple expressions that
return rowsets.

regards, tom lane


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

Re: [HACKERS] patch: function xmltable

[Andres Freund]

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.


> 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.

Greetings,

Andres Freund


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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

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.

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 ...

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Andres Freund]

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.

Andres


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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-19 13:35 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > Hi
> >
> > New update - rebase after yesterday changes.
> >
> > What you want to change?
>
> I think the problem might come from the still pending patch on that
> series, which Andres posted in
> https://www.postgresql.org/message-id/20170118221154.
> aldebi7yyjvds...@alap3.anarazel.de
> As far as I understand, minor details of that patch might change before
> commit, but it is pretty much in definitive form.
>

ok, we have to wait - please, check XML part if it is good for you

Regards

Pavel


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> Hi
> 
> New update - rebase after yesterday changes.
> 
> What you want to change?

I think the problem might come from the still pending patch on that
series, which Andres posted in
https://www.postgresql.org/message-id/20170118221154.aldebi7yyjvds...@alap3.anarazel.de
As far as I understand, minor details of that patch might change before
commit, but it is pretty much in definitive form.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2017-01-16 23:51 GMT+01:00 Alvaro Herrera :

> Given
> https://www.postgresql.org/message-id/20170116210019.
> a3glfwspg5lnf...@alap3.anarazel.de
> which is going to heavily change how the executor works in this area, I
> am returning this patch to you again.  I would like a few rather minor
> changes:
>
> 1. to_xmlstr can be replaced with calls to xmlCharStrdup.
> 2. don't need xml_xmlnodetostr either -- just use xml_xmlnodetoxmltype
>(which returns text*) and extract the cstring from the varlena.  It's
>a bit more wasteful in terms of cycles, but I don't think we care.
>If we do care, change the function so that it returns cstring, and
>have the callers that want text wrap it in cstring_to_text.
> 3. have a new perValueCxt memcxt in TableExprState, child of buildercxt,
>and switch to it just before GetValue() (reset it just before
>switching).  Then, don't worry about leaks in GetValue.  This way,
>the text* conversions et al don't matter.
>
> After that I think we're going to need to get this working on top of
> Andres' changes.  Which I'm afraid is going to be rather major surgery,
> but I haven't looked.
>

I'll try to clean xml part first, and then I can reflect the SRF changes. I
am not sure if I understand to all your proposed changes here, I have to
look there.

Regards

Pavel

>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

In case this still matters, I think GetValue should look more or less
like this (untested):


/*
 * Return the value for column number 'colnum' for the current row.  If column
 * -1 is requested, return representation of the whole row.
 *
 * This leaks memory, so be sure to reset often the context in which it's
 * called.
 */
static Datum
XmlTableGetValue(TableExprState *state, int colnum, bool *isnull)
{
#ifdef USE_LIBXML
XmlTableBuilderData *xtCxt;
Datum   result = (Datum) 0;
xmlNodePtr  cur;
char   *cstr = NULL;
volatile xmlXPathObjectPtr xpathobj;

xtCxt = GetXmlTableBuilderPrivateData(state, "XmlTableGetValue");

Assert(xtCxt->xpathobj &&
   xtCxt->xpathobj->type == XPATH_NODESET &&
   xtCxt->xpathobj->nodesetval != NULL);

/* Propagate context related error context to libxml2 */
xmlSetStructuredErrorFunc((void *) xtCxt->xmlerrcxt, xml_errorHandler);

cur = xtCxt->xpathobj->nodesetval->nodeTab[xtCxt->row_count - 1];
if (cur->type != XML_ELEMENT_NODE)
elog(ERROR, "unexpected xmlNode type");

/* Handle whole row case the easy way. */
if (colnum == -1)
{
text   *txt;

txt = xml_xmlnodetoxmltype(cur, xtCxt->xmlerrcxt);
result = InputFunctionCall(>in_functions[0],
   
text_to_cstring(txt),
   
state->typioparams[0],
   -1);
*isnull = false;

return result;
}

Assert(xtCxt->xpathscomp[colnum] != NULL);

xpathobj = NULL;
PG_TRY();
{
Form_pg_attribute attr;

attr = state->resultSlot->tts_tupleDescriptor->attrs[colnum];

/* Set current node as entry point for XPath evaluation */
xmlXPathSetContextNode(cur, xtCxt->xpathcxt);

/* Evaluate column path */
xpathobj = xmlXPathCompiledEval(xtCxt->xpathscomp[colnum], 
xtCxt->xpathcxt);
if (xpathobj == NULL || xtCxt->xmlerrcxt->err_occurred)
xml_ereport(xtCxt->xmlerrcxt, ERROR, 
ERRCODE_INTERNAL_ERROR,
"could not create XPath 
object");

if (xpathobj->type == XPATH_NODESET)
{
int count;
Oid targettypid = attr->atttypid;

if (xpathobj->nodesetval != NULL)
count = xpathobj->nodesetval->nodeNr;

/*
 * There are four possible cases, depending on the 
number of
 * nodes returned by the XPath expression and the type 
of the
 * target column: a) XPath returns no nodes.  b) One 
node is
 * returned, and column is of type XML.  c) One node, 
column type
 * other than XML.  d) Multiple nodes are returned.
 */
if (xpathobj->nodesetval == NULL)
{
*isnull = true;
}
else if (count == 1 && targettypid == XMLOID)
{
textstr = 
xml_xmlnodetoxmltype(xpathobj->nodesetval->nodeTab[0],

   xtCxt->xmlerrcxt);
cstr = text_to_cstring(textstr);
}
else if (count == 1)
{
xmlChar*str;

str = xmlNodeListGetString(xtCxt->doc,

   xpathobj->nodesetval->nodeTab[0]->xmlChildrenNode,

   1);
if (str)
{
PG_TRY();
{
cstr = pstrdup(str);
}
PG_CATCH();
{
xmlFree(str);
PG_RE_THROW();
}
PG_END_TRY();
xmlFree(str);
}
else
   

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Given
https://www.postgresql.org/message-id/20170116210019.a3glfwspg5lnf...@alap3.anarazel.de
which is going to heavily change how the executor works in this area, I
am returning this patch to you again.  I would like a few rather minor
changes:

1. to_xmlstr can be replaced with calls to xmlCharStrdup.
2. don't need xml_xmlnodetostr either -- just use xml_xmlnodetoxmltype
   (which returns text*) and extract the cstring from the varlena.  It's
   a bit more wasteful in terms of cycles, but I don't think we care.
   If we do care, change the function so that it returns cstring, and
   have the callers that want text wrap it in cstring_to_text.
3. have a new perValueCxt memcxt in TableExprState, child of buildercxt,
   and switch to it just before GetValue() (reset it just before
   switching).  Then, don't worry about leaks in GetValue.  This way,
   the text* conversions et al don't matter.

After that I think we're going to need to get this working on top of
Andres' changes.  Which I'm afraid is going to be rather major surgery,
but I haven't looked.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

I just realized that your new xml_xmlnodetostr is line-by-line identical
to the existing xml_xmlnodetoxmltype except for two or three lines.
I think that's wrong.  I'm going to patch the existing function so that
they can share code.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> another update - lot of cleaning

Thanks.

The more I look at this, the less I like using NameArgExpr for
namespaces.  It looks all wrong to me, and it causes ugly code all over.
Maybe I just need to look at it a bit longer.



-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> another update - lot of cleaning

Ah, the tupledesc stuff in this one appears much more reasonable to me.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-12-07 20:50 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2016-12-07 18:34 GMT+01:00 Alvaro Herrera :
>
> > > Hmm.  Now that I see how this works, by having the GetValue "guess"
> what
> > > is going on and have a special case for it, I actually don't like it
> > > very much.  It seems way too magical.  I think we should do away with
> > > the "if column is NULL" case in GetValue, and instead inject a column
> > > during transformTableExpr if columns is NIL.  This has implications on
> > > ExecInitExpr too, which currently checks for an empty column list -- it
> > > would no longer have to do so.
> >
> > I prefer this way against second described. The implementation should be
> in
> > table builder routines, not in executor.
>
> Well, given the way you have implemented it, I would prefer the original
> too.  But your v23 is not what I meant.  Essentially what you do in v23
> is to communicate the lack of COLUMNS clause in a different way --
> previously it was "ncolumns = 0", now it's "is_auto_col=true".  It's
> still "magic".  It's not an improvement.
>

is_auto_col is used primary for asserting. The table builder has
information for decision in parameter path, when path is NULL.

Hard to say, if this info should be assigned to column or to table. In both
locations has sense. But somewhere should be some flag.


>
> What I want to happen is that there is no magic at all; it's up to
> transformExpr to make sure that when COLUMNS is empty, one column
> appears and it must not be a magic column that makes the xml.c code act
> differently, but rather to xml.c it should appear that this is just a
> normal column that happens to return the entire row.  If I say "COLUMNS
> foo PATH '/'" I should be able to obtain a similar behavior (except that
> in the current code, if I ask for "COLUMNS foo XML PATH '/'" I don't get
> XML at all but rather weird text where all tags have been stripped out,
> which is very strange.  I would expect the tags to be preserved if the
> output type is XML.  Maybe the tag-stripping behavior should occur if
> the output type is some type of text.)
>

I am doing this. Just I using NULL for PATH.


>
>
> I still have to figure out how to fix the tupledesc thing.  What we have
> now is not good.
>

cannot be moved to nodefunc?

Pavel

>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2016-12-07 18:34 GMT+01:00 Alvaro Herrera :

> > Hmm.  Now that I see how this works, by having the GetValue "guess" what
> > is going on and have a special case for it, I actually don't like it
> > very much.  It seems way too magical.  I think we should do away with
> > the "if column is NULL" case in GetValue, and instead inject a column
> > during transformTableExpr if columns is NIL.  This has implications on
> > ExecInitExpr too, which currently checks for an empty column list -- it
> > would no longer have to do so.
> 
> I prefer this way against second described. The implementation should be in
> table builder routines, not in executor.

Well, given the way you have implemented it, I would prefer the original
too.  But your v23 is not what I meant.  Essentially what you do in v23
is to communicate the lack of COLUMNS clause in a different way --
previously it was "ncolumns = 0", now it's "is_auto_col=true".  It's
still "magic".  It's not an improvement.

What I want to happen is that there is no magic at all; it's up to
transformExpr to make sure that when COLUMNS is empty, one column
appears and it must not be a magic column that makes the xml.c code act
differently, but rather to xml.c it should appear that this is just a
normal column that happens to return the entire row.  If I say "COLUMNS
foo PATH '/'" I should be able to obtain a similar behavior (except that
in the current code, if I ask for "COLUMNS foo XML PATH '/'" I don't get
XML at all but rather weird text where all tags have been stripped out,
which is very strange.  I would expect the tags to be preserved if the
output type is XML.  Maybe the tag-stripping behavior should occur if
the output type is some type of text.)


I still have to figure out how to fix the tupledesc thing.  What we have
now is not good.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> I fixed two issues.
> 
> 2. there was reverse setting in NOT NULL flag

Ah-hah, that was silly, thanks.

> 1. there are not columns data when there are not any explicit column - fixed

Hmm.  Now that I see how this works, by having the GetValue "guess" what
is going on and have a special case for it, I actually don't like it
very much.  It seems way too magical.  I think we should do away with
the "if column is NULL" case in GetValue, and instead inject a column
during transformTableExpr if columns is NIL.  This has implications on
ExecInitExpr too, which currently checks for an empty column list -- it
would no longer have to do so.

Maybe this means we need an additional method, which would request "the
expr that returns the whole row", so that transformExpr can work for
XmlTable (which I think would be something like "./") and the future
JsonTable stuff (I don't know how that one would work, but I assume it's
not necessarily the same thing).

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2016-12-04 23:00 GMT+01:00 Pavel Stehule :

> > I am not sure if I understand well to your ideas - please, check attached
> > patch.

Thanks, that's what I meant, but I think you went a bit overboard
creating new functions in execQual -- seems to me it would work just
fine to have the memory switches in the same function, rather than
having a number of separate functions just to change the context then
call the method.  Please remove these shim functions.

Also, you forgot to remove the now-unused per_rowset_memcxt struct member.

Also, please rename "rc" to something more meaningful -- maybe
"rowcount" is good enough.  And "doc" would perhaps be better as
"document".

I'm not completely sure the structs are really sensible yet.  I may do
some more changes tomorrow.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> 2016-12-02 23:25 GMT+01:00 Alvaro Herrera :

> > This is looking much better now, but it still needs at least the
> > following changes.
> >
> > First, we need to fix is the per_rowset_memcxt thingy.  I think the way
> > it's currently being used is rather ugly; it looks to me like the memory
> > context does not belong into the XmlTableContext struct at all.
> > Instead, the executor code should keep the memcxt pointer in a state
> > struct of its own, and it should be the executor's responsibility to
> > change to the appropriate context before calling the table builder
> > functions.  In particular, this means that the table context can no
> > longer be a void * pointer; it needs to be a struct that's defined by
> > the executor (probably a primnodes.h one).  The void * pointer is
> > stashed inside that struct.  Also, the "routine" pointer should not be
> > part of the void * struct, but of the executor's struct.  So the
> > execQual code can switch to the memory context, and destroy it
> > appropriately.
> >
> > Second, we should make gram.y set a new "function type" value in the
> > TableExpr it creates, so that the downstream code (transformTableExpr,
> > ExecInitExpr, ruleutils.c) really knows that the given function is
> > XmlTableExpr, instead of guessing just because it's the only implemented
> > case.  Probably this "function type" is an enum (currently with a single
> > value TableExprTypeXml or something like that) in primnodes.
> 
> It has sense - I was not sure about it - because currently it is only one
> value, you mentioned it.

True.  This is a minor point.

Are you able to do the memory context change I describe?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2016-12-02 23:25 GMT+01:00 Alvaro Herrera :

> Here's version 17.  I have made significant changes here.
>
> 1. Restructure the execQual code.  Instead of a PG_TRY wrapper, I have
> split this code in three pieces; there's the main code with the PG_TRY
> wrappers and is mainly in charge of the builderCxt pointer.  In the
> previous coding there was a shim that examined builderCxt but was not
> responsible for setting it up, which was ugly.  The second part is the
> "initializer" which sets the row and column filters and does namespace
> processing.  The third part is the "FetchRow" logic.  It seems to me
> much cleaner this way.
>
> 2. rename the "builder" stuff to use the "routine" terminology.  This is
> in line with what we do for other function-pointer-filled structs, such
> as FdwRoutine, IndexAmRoutine etc.  I also cleaned up the names a bit
> more.
>
> 3. Added a magic number to the table builder context struct, so that we
> can barf appropriately.  This is in line with PgXmlErrorContext --
> mostly for future-proofing.  I didn't test this too hard.  Also, moved
> the XmlTableContext struct declaration nearer the top of the file, as is
> customary.  (We don't really need it that way, since the functions are
> all declared taking void *, but it seems cleaner to me anyway).
>
> 4. I added, edited, and fixed a large number of code comments.
>
> This is looking much better now, but it still needs at least the
> following changes.
>
> First, we need to fix is the per_rowset_memcxt thingy.  I think the way
> it's currently being used is rather ugly; it looks to me like the memory
> context does not belong into the XmlTableContext struct at all.
> Instead, the executor code should keep the memcxt pointer in a state
> struct of its own, and it should be the executor's responsibility to
> change to the appropriate context before calling the table builder
> functions.  In particular, this means that the table context can no
> longer be a void * pointer; it needs to be a struct that's defined by
> the executor (probably a primnodes.h one).  The void * pointer is
> stashed inside that struct.  Also, the "routine" pointer should not be
> part of the void * struct, but of the executor's struct.  So the
> execQual code can switch to the memory context, and destroy it
> appropriately.
>
> Second, we should make gram.y set a new "function type" value in the
> TableExpr it creates, so that the downstream code (transformTableExpr,
> ExecInitExpr, ruleutils.c) really knows that the given function is
> XmlTableExpr, instead of guessing just because it's the only implemented
> case.  Probably this "function type" is an enum (currently with a single
> value TableExprTypeXml or something like that) in primnodes.
>

It has sense - I was not sure about it - because currently it is only one
value, you mentioned it.


>
> Finally, there's the pending task of renaming and moving
> ExecTypeFromTableExpr to some better place.  Not sure that moving it
> back to nodeFuncs is a nice idea.  Looks to me like calling it from
> ExprTypmod is a rather ugly idea.
>

The code is related to prim nodes - it is used more times than in executor.

>
> Hmm, ruleutils ... not sure what to think of this one.
>

it is little bit more complex - but it is related to complexity of XMLTABLE


>
> The typedefs.list changes are just used to pgindent the affected code
> correctly.  It's not for commit.
>

The documentation is very precious. Nice

+/* XXX OK to do this?  looks a bit out of place ... */
+assign_record_type_typmod(typeInfo);

I am thinking it is ok. It is tupdesc without fixed typid, typname used in
returned value - you should to register this tupdesc in typcache.

Regards

Pavel



> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Hm, you omitted tableexpr.h from the v15 patch ...

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Haribabu Kommi]

On Thu, Dec 1, 2016 at 2:21 AM, Pavel Stehule 
wrote:

> Dne 30. 11. 2016 14:53 napsal uživatel "Pavel Stehule" <
> pavel.steh...@gmail.com>:
> >
> >
> >
> > 2016-11-30 13:38 GMT+01:00 Alvaro Herrera :
> >>
> >> Pavel Stehule wrote:
> >> > 2016-11-30 2:40 GMT+01:00 Craig Ringer :
> >> >
> >> > > On 30 November 2016 at 05:36, Pavel Stehule <
> pavel.steh...@gmail.com>
> >> > > wrote:
> >> > >
> >> > > > The problem is in unreserved keyword "PASSING" probably.
> >> > >
> >> > > Yeah, I think that's what I hit when trying to change it.
> >> > >
> >> > > Can't you just parenthesize the expression to use operators like ||
> >> > > etc? If so, not a big deal.
> >> > >
> >> > ???
> >>
> >> "'(' a_expr ')'" is a c_expr; Craig suggests that we can just tell users
> >> to manually add parens around any expressions that they want to use.
> >> That's not necessary most of the time since we've been able to use
> >> b_expr in most places.
> >
>
> still there are one c_expr, but without new reserved word there are not
> change to reduce it.
>
>
>
Moved to next CF with the same status (ready for committer).

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Dne 30. 11. 2016 14:53 napsal uživatel "Pavel Stehule" <
pavel.steh...@gmail.com>:
>
>
>
> 2016-11-30 13:38 GMT+01:00 Alvaro Herrera :
>>
>> Pavel Stehule wrote:
>> > 2016-11-30 2:40 GMT+01:00 Craig Ringer :
>> >
>> > > On 30 November 2016 at 05:36, Pavel Stehule 
>> > > wrote:
>> > >
>> > > > The problem is in unreserved keyword "PASSING" probably.
>> > >
>> > > Yeah, I think that's what I hit when trying to change it.
>> > >
>> > > Can't you just parenthesize the expression to use operators like ||
>> > > etc? If so, not a big deal.
>> > >
>> > ???
>>
>> "'(' a_expr ')'" is a c_expr; Craig suggests that we can just tell users
>> to manually add parens around any expressions that they want to use.
>> That's not necessary most of the time since we've been able to use
>> b_expr in most places.
>

still there are one c_expr, but without new reserved word there are not
change to reduce it.

>
> Now I understand
>
> Regards
>
> Pavel
>
>>
>>
>> --
>> Álvaro Herrerahttps://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-30 13:38 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > 2016-11-30 2:40 GMT+01:00 Craig Ringer :
> >
> > > On 30 November 2016 at 05:36, Pavel Stehule 
> > > wrote:
> > >
> > > > The problem is in unreserved keyword "PASSING" probably.
> > >
> > > Yeah, I think that's what I hit when trying to change it.
> > >
> > > Can't you just parenthesize the expression to use operators like ||
> > > etc? If so, not a big deal.
> > >
> > ???
>
> "'(' a_expr ')'" is a c_expr; Craig suggests that we can just tell users
> to manually add parens around any expressions that they want to use.
> That's not necessary most of the time since we've been able to use
> b_expr in most places.
>

Now I understand

Regards

Pavel


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> 2016-11-30 2:40 GMT+01:00 Craig Ringer :
> 
> > On 30 November 2016 at 05:36, Pavel Stehule 
> > wrote:
> >
> > > The problem is in unreserved keyword "PASSING" probably.
> >
> > Yeah, I think that's what I hit when trying to change it.
> >
> > Can't you just parenthesize the expression to use operators like ||
> > etc? If so, not a big deal.
> >
> ???

"'(' a_expr ')'" is a c_expr; Craig suggests that we can just tell users
to manually add parens around any expressions that they want to use.
That's not necessary most of the time since we've been able to use
b_expr in most places.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-30 2:40 GMT+01:00 Craig Ringer :

> On 30 November 2016 at 05:36, Pavel Stehule 
> wrote:
>
> > The problem is in unreserved keyword "PASSING" probably.
>
> Yeah, I think that's what I hit when trying to change it.
>
> Can't you just parenthesize the expression to use operators like ||
> etc? If so, not a big deal.
>
>
???



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

Re: [HACKERS] patch: function xmltable

[Craig Ringer]

On 30 November 2016 at 05:36, Pavel Stehule  wrote:

> The problem is in unreserved keyword "PASSING" probably.

Yeah, I think that's what I hit when trying to change it.

Can't you just parenthesize the expression to use operators like ||
etc? If so, not a big deal.




-- 
 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> Here is updated patch without default namespace support (and without XPath
> expression transformation).
> 
> Due last changes in parser
> https://github.com/postgres/postgres/commit/906bfcad7ba7cb3863fe0e2a7810be8e3cd84fbd
> I had to use c_expr on other positions ( xmlnamespace definition).
> 
> I don't think it is limit - in 99% there will be const literal.

Argh.  I can't avoid the feeling that I'm missing some parser trickery
here.  We have the XMLNAMESPACES keyword and the clause-terminating
comma to protect these clauses, there must be a way to define this piece
of the grammar so that there's no conflict, without losing the freedom
in the expressions.  But I don't see how.  Now I agree that xml
namespace definitions are going to be string literals in almost all
cases (or in extra sophisticated cases, column names) ... it's probably
better to spend the bison-fu in the document expression or the column
options, or better yet the xmlexists_argument stuff.  But I don't see
possibility of improvements in any of those places, so let's put it
aside -- we can improve later, if need arises.

In any case, it looks like we can change c_expr to b_expr in a few
places, which is good because then operators work (in particular, unless
I misread the grammar, foo||bar doesn't work with c_expr and does work
with b_expr, which seems the most useful in this case).  Also, it makes
no sense to support (in the namespaces clause) DEFAULT a_expr if the
IDENT case uses only b_expr, so let's reduce both to just b_expr.

While I'm looking at node definitions, I see a few things that could use
some naming improvement.  For example, "expr" for TableExpr is a bit
unexpressive.  We could use "document_expr" there, perhaps.  "row_path"
seems fixated on the XML case and the expression be path; let's use
"row_expr" there.  And "cols" could be "column_exprs" perhaps.  (All
those renames cause fall-out in various node-related files, so let's
think carefully to avoid renaming them multiple times.)

In primnodes, you kept the comment that says "xpath".  Please update
that to not-just-XML reality.

Please fix the comment in XmlTableAddNs; NULL is no longer a valid value.

parse_expr.c has two unused variables; please remove them.

This test in ExecEvalTableExprProtected looks weird:
if (i != tstate->for_ordinality_col - 1)
please change to comparing "i + 1" (convert array index into attribute
number), and invert the boolean expression, leaving the for_ordinality
case on top and the rest in the "else".  That seems easier to read.
Also, we customarily use post-increment (rownum++) instead of pre-incr.

In execQual.c I think it's neater to have ExecEvalTableExpr go before
its subroutine.  Actually, I wonder whether it is really necessary to
have a subroutine in the first place; you could just move the entire
contents of that subroutine to within the PG_TRY block instead.  The
only thing you lose is one indentation level.  I'm not sure about this
one, but it's worth considering.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-25 7:44 GMT+01:00 Pavel Stehule :

>
>
> 2016-11-25 3:31 GMT+01:00 Alvaro Herrera :
>
>> Michael Paquier wrote:
>>
>> > Nit: I did not look at the patch in details,
>> > but I find the size of the latest version sent, 167kB, scary as it
>> > complicates review and increases the likeliness of bugs.
>>
>> Here's the stat.  Note that removing the functionality as discussed
>> would remove all of xpath_parser.c but I think the rest of it remains
>> pretty much unchanged.  So it's clearly a large patch, but there are
>> large docs and tests too, not just code.
>>
>
> yes, lot of is regress tests (expected part is 3x) - and XMLTABLE function
> is not trivial.
>

regress tests are about 50%



>
> lot of code is mechanical - nodes related. The really complex part is only
> in xml.c. There is one longer function only - the complexity is based on
> mapping libxml2 result to PostgreSQL result (with catching exceptions due
> releasing libxml2 sources).
>
> The all changes are well isolated -  there is less risk to break some
> other.
>
>
>>
>>  doc/src/sgml/func.sgml   | 376 ++---
>>  src/backend/executor/execQual.c  | 335 +++
>>  src/backend/executor/execTuples.c|  42 +++
>>  src/backend/nodes/copyfuncs.c|  66 
>>  src/backend/nodes/equalfuncs.c   |  51 +++
>>  src/backend/nodes/nodeFuncs.c| 100 ++
>>  src/backend/nodes/outfuncs.c |  51 +++
>>  src/backend/nodes/readfuncs.c|  42 +++
>>  src/backend/optimizer/util/clauses.c |  33 ++
>>  src/backend/parser/gram.y| 181 ++-
>>  src/backend/parser/parse_coerce.c|  33 +-
>>  src/backend/parser/parse_expr.c  | 182 +++
>>  src/backend/parser/parse_target.c|   7 +
>>  src/backend/utils/adt/Makefile   |   2 +-
>>  src/backend/utils/adt/ruleutils.c| 100 ++
>>  src/backend/utils/adt/xml.c  | 610 ++
>> +
>>  src/backend/utils/adt/xpath_parser.c | 337 +++
>>  src/backend/utils/fmgr/funcapi.c |  13 +
>>  src/include/executor/executor.h  |   1 +
>>  src/include/executor/tableexpr.h |  69 
>>  src/include/funcapi.h|   1 -
>>  src/include/nodes/execnodes.h|  31 ++
>>  src/include/nodes/nodes.h|   4 +
>>  src/include/nodes/parsenodes.h   |  21 ++
>>  src/include/nodes/primnodes.h|  40 +++
>>  src/include/parser/kwlist.h  |   3 +
>>  src/include/parser/parse_coerce.h|   4 +
>>  src/include/utils/xml.h  |   2 +
>>  src/include/utils/xpath_parser.h |  24 ++
>>  src/test/regress/expected/xml.out| 415 
>>  src/test/regress/expected/xml_1.out  | 323 +++
>>  src/test/regress/expected/xml_2.out  | 414 
>>  src/test/regress/sql/xml.sql | 170 ++
>>  33 files changed, 4019 insertions(+), 64 deletions(-)
>>
>> --
>> Álvaro Herrerahttps://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>
>
>

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-25 3:31 GMT+01:00 Alvaro Herrera :

> Michael Paquier wrote:
>
> > Nit: I did not look at the patch in details,
> > but I find the size of the latest version sent, 167kB, scary as it
> > complicates review and increases the likeliness of bugs.
>
> Here's the stat.  Note that removing the functionality as discussed
> would remove all of xpath_parser.c but I think the rest of it remains
> pretty much unchanged.  So it's clearly a large patch, but there are
> large docs and tests too, not just code.
>

yes, lot of is regress tests (expected part is 3x) - and XMLTABLE function
is not trivial.

lot of code is mechanical - nodes related. The really complex part is only
in xml.c. There is one longer function only - the complexity is based on
mapping libxml2 result to PostgreSQL result (with catching exceptions due
releasing libxml2 sources).

The all changes are well isolated -  there is less risk to break some other.


>
>  doc/src/sgml/func.sgml   | 376 ++---
>  src/backend/executor/execQual.c  | 335 +++
>  src/backend/executor/execTuples.c|  42 +++
>  src/backend/nodes/copyfuncs.c|  66 
>  src/backend/nodes/equalfuncs.c   |  51 +++
>  src/backend/nodes/nodeFuncs.c| 100 ++
>  src/backend/nodes/outfuncs.c |  51 +++
>  src/backend/nodes/readfuncs.c|  42 +++
>  src/backend/optimizer/util/clauses.c |  33 ++
>  src/backend/parser/gram.y| 181 ++-
>  src/backend/parser/parse_coerce.c|  33 +-
>  src/backend/parser/parse_expr.c  | 182 +++
>  src/backend/parser/parse_target.c|   7 +
>  src/backend/utils/adt/Makefile   |   2 +-
>  src/backend/utils/adt/ruleutils.c| 100 ++
>  src/backend/utils/adt/xml.c  | 610 ++
> +
>  src/backend/utils/adt/xpath_parser.c | 337 +++
>  src/backend/utils/fmgr/funcapi.c |  13 +
>  src/include/executor/executor.h  |   1 +
>  src/include/executor/tableexpr.h |  69 
>  src/include/funcapi.h|   1 -
>  src/include/nodes/execnodes.h|  31 ++
>  src/include/nodes/nodes.h|   4 +
>  src/include/nodes/parsenodes.h   |  21 ++
>  src/include/nodes/primnodes.h|  40 +++
>  src/include/parser/kwlist.h  |   3 +
>  src/include/parser/parse_coerce.h|   4 +
>  src/include/utils/xml.h  |   2 +
>  src/include/utils/xpath_parser.h |  24 ++
>  src/test/regress/expected/xml.out| 415 
>  src/test/regress/expected/xml_1.out  | 323 +++
>  src/test/regress/expected/xml_2.out  | 414 
>  src/test/regress/sql/xml.sql | 170 ++
>  33 files changed, 4019 insertions(+), 64 deletions(-)
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Michael Paquier wrote:

> Nit: I did not look at the patch in details,
> but I find the size of the latest version sent, 167kB, scary as it
> complicates review and increases the likeliness of bugs.

Here's the stat.  Note that removing the functionality as discussed
would remove all of xpath_parser.c but I think the rest of it remains
pretty much unchanged.  So it's clearly a large patch, but there are
large docs and tests too, not just code.

 doc/src/sgml/func.sgml   | 376 ++---
 src/backend/executor/execQual.c  | 335 +++
 src/backend/executor/execTuples.c|  42 +++
 src/backend/nodes/copyfuncs.c|  66 
 src/backend/nodes/equalfuncs.c   |  51 +++
 src/backend/nodes/nodeFuncs.c| 100 ++
 src/backend/nodes/outfuncs.c |  51 +++
 src/backend/nodes/readfuncs.c|  42 +++
 src/backend/optimizer/util/clauses.c |  33 ++
 src/backend/parser/gram.y| 181 ++-
 src/backend/parser/parse_coerce.c|  33 +-
 src/backend/parser/parse_expr.c  | 182 +++
 src/backend/parser/parse_target.c|   7 +
 src/backend/utils/adt/Makefile   |   2 +-
 src/backend/utils/adt/ruleutils.c| 100 ++
 src/backend/utils/adt/xml.c  | 610 +++
 src/backend/utils/adt/xpath_parser.c | 337 +++
 src/backend/utils/fmgr/funcapi.c |  13 +
 src/include/executor/executor.h  |   1 +
 src/include/executor/tableexpr.h |  69 
 src/include/funcapi.h|   1 -
 src/include/nodes/execnodes.h|  31 ++
 src/include/nodes/nodes.h|   4 +
 src/include/nodes/parsenodes.h   |  21 ++
 src/include/nodes/primnodes.h|  40 +++
 src/include/parser/kwlist.h  |   3 +
 src/include/parser/parse_coerce.h|   4 +
 src/include/utils/xml.h  |   2 +
 src/include/utils/xpath_parser.h |  24 ++
 src/test/regress/expected/xml.out| 415 
 src/test/regress/expected/xml_1.out  | 323 +++
 src/test/regress/expected/xml_2.out  | 414 
 src/test/regress/sql/xml.sql | 170 ++
 33 files changed, 4019 insertions(+), 64 deletions(-)

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Michael Paquier]

On Fri, Nov 25, 2016 at 3:31 AM, Pavel Stehule  wrote:
> 2016-11-24 18:51 GMT+01:00 Tom Lane :
>> contrib/xml2 has always relied on libxslt for xpath functionality.
>> Can we do that here instead of writing, debugging, and documenting
>> a pile of new code?
>
> I am sorry - I don't see it. There is nothing complex manipulation with
> XPath expressions.

You are missing the point here, which is to make the implementation
footprint as light as possible, especially if the added functionality
is already present in a dependency that Postgres can be linked to. OK,
libxslt can only be linked with contrib/xml2/ now, but it would be at
least worth looking at how much the current patch can be simplified
for things like transformTableExpr or XmlTableGetValue by relying on
some existing routines. Nit: I did not look at the patch in details,
but I find the size of the latest version sent, 167kB, scary as it
complicates review and increases the likeliness of bugs.
-- 
Michael


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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-24 18:51 GMT+01:00 Tom Lane :

> Alvaro Herrera  writes:
> > Pavel Stehule wrote:
> >> 2016-11-24 0:13 GMT+01:00 Alvaro Herrera :
> >>> Oh my, I just noticed we have a new xpath preprocessor in this patch
> >>> too.  Where did this code come from -- did you write it all from
> >>> scratch?
>
> >> I wrote it from scratch - libxml2 has not any API for iteration over
> XPath
> >> expression (different than iteration over XPath expression result), and
> >> what I have info, there will not be any new API in libxml2.
>
> > Okay, I agree that the default namespace stuff looks worthwhile in the
> > long run.  But I don't have enough time to review the xpath parser stuff
> > in the current commitfest, and I think it needs at the very least a lot
> > of additional code commentary.
>
> contrib/xml2 has always relied on libxslt for xpath functionality.
> Can we do that here instead of writing, debugging, and documenting
> a pile of new code?
>

I am sorry - I don't see it. There is nothing complex manipulation with
XPath expressions.

Regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] patch: function xmltable

[Tom Lane]

Alvaro Herrera  writes:
> Pavel Stehule wrote:
>> 2016-11-24 0:13 GMT+01:00 Alvaro Herrera :
>>> Oh my, I just noticed we have a new xpath preprocessor in this patch
>>> too.  Where did this code come from -- did you write it all from
>>> scratch?

>> I wrote it from scratch - libxml2 has not any API for iteration over XPath
>> expression (different than iteration over XPath expression result), and
>> what I have info, there will not be any new API in libxml2.

> Okay, I agree that the default namespace stuff looks worthwhile in the
> long run.  But I don't have enough time to review the xpath parser stuff
> in the current commitfest, and I think it needs at the very least a lot
> of additional code commentary.

contrib/xml2 has always relied on libxslt for xpath functionality.
Can we do that here instead of writing, debugging, and documenting
a pile of new code?

regards, tom lane


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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> can me send your last work?

Sure, it's in the archives --
https://www.postgresql.org/message-id/20161123233130.oqf7jl6czehy5fiw@alvherre.pgsql

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-24 18:29 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > Hi
> >
> > 2016-11-24 0:13 GMT+01:00 Alvaro Herrera :
> >
> > > Oh my, I just noticed we have a new xpath preprocessor in this patch
> > > too.  Where did this code come from -- did you write it all from
> > > scratch?
> >
> > I wrote it from scratch - libxml2 has not any API for iteration over
> XPath
> > expression (different than iteration over XPath expression result), and
> > what I have info, there will not be any new API in libxml2.
>
> Okay, I agree that the default namespace stuff looks worthwhile in the
> long run.  But I don't have enough time to review the xpath parser stuff
> in the current commitfest, and I think it needs at the very least a lot
> of additional code commentary.
>
> However I think the rest of it can reasonably go in -- I mean the SQL
> parse of it, analysis, executor.  Let me propose this: you split the
> patch, leaving the xpath_parser.c stuff out and XMLNAMESPACES DEFAULT,
> and we introduce just the TableExpr stuff plus the XMLTABLE function.  I
> can commit that part in the current commitfest, and we leave the
> xpath_parser plus associated features for the upcoming commitfest.


> Deal?
>

ok

can me send your last work?

Regards

Pavel


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:
> Hi
> 
> 2016-11-24 0:13 GMT+01:00 Alvaro Herrera :
> 
> > Oh my, I just noticed we have a new xpath preprocessor in this patch
> > too.  Where did this code come from -- did you write it all from
> > scratch?
> 
> I wrote it from scratch - libxml2 has not any API for iteration over XPath
> expression (different than iteration over XPath expression result), and
> what I have info, there will not be any new API in libxml2.

Okay, I agree that the default namespace stuff looks worthwhile in the
long run.  But I don't have enough time to review the xpath parser stuff
in the current commitfest, and I think it needs at the very least a lot
of additional code commentary.

However I think the rest of it can reasonably go in -- I mean the SQL
parse of it, analysis, executor.  Let me propose this: you split the
patch, leaving the xpath_parser.c stuff out and XMLNAMESPACES DEFAULT,
and we introduce just the TableExpr stuff plus the XMLTABLE function.  I
can commit that part in the current commitfest, and we leave the
xpath_parser plus associated features for the upcoming commitfest.

Deal?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-24 5:52 GMT+01:00 Pavel Stehule :

> Hi
>
> 2016-11-24 0:13 GMT+01:00 Alvaro Herrera :
>
>> Oh my, I just noticed we have a new xpath preprocessor in this patch
>> too.  Where did this code come from -- did you write it all from
>> scratch?
>>
>
> I wrote it from scratch - libxml2 has not any API for iteration over XPath
> expression (different than iteration over XPath expression result), and
> what I have info, there will not be any new API in libxml2.
>
> There are two purposes:
>
> Safe manipulation with XPath expression prefixes - ANSI SQL design
> implicitly expects some prefix, but it can be used manually. The prefix
> should not be used twice and in some situations, when it can breaks the
> expression.
>

Implicit prefix for column PATH expressions is "./". An user can use it
explicitly.

In my initial patches, the manipulations with XPath expression was more
complex - now, it can be reduced - but then we lost default namespaces
support, what is nice feature, supported other providers.




>
> Second goal is support default namespaces - when we needed parser for
> first task, then the enhancing for this task was not too much lines more.
>
> This parser can be used for enhancing current XPath function - default
> namespaces are pretty nice, when you have to use namespaces.
>
>
>>
>> --
>> Álvaro Herrerahttps://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>
>
>

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-24 0:29 GMT+01:00 Alvaro Herrera :

> Here's another version.  Not there yet: need to move back the function
> to create the tupdesc, as discussed.  Not clear what's the best place,
> however.  I modified the grammar a bit (added the missing comma, removed
> PATH as an unreserved keyword and just used IDENT, removed the "Opt"
> version for column options), and reworked the comments in the transform
> phase (I tweaked the code here and there mostly to move things to nicer
> places, but it's pretty much the same code).
>
> In the new xpath_parser.c file I think we should tidy things up a bit.
> First, it needs more commentary on what the entry function actually
> does, in detail.  Also, IMO that function should be at the top of the
> file, not at the bottom, followed by all its helpers.  I would like some
> more clarity on the provenance of all this code, just to assess the
> probability of bugs; mostly as it's completely undocumented.
>
> I don't like the docs either.  I think we should have a complete
> reference to the syntax, followed by examples, rather than letting the
> examples drive the whole thing.  I fixed the synopsis so that it's not
> one very long line.
>
> If you use "PATH '/'" for a column, you get the text for all the entries
> in the whole XML, rather than the text for the particular row being
> processed.  Isn't that rather weird, or to put it differently, completely
> wrong?  I didn't find a way to obtain the whole XML row when you have
> the COLUMNS option (which is what I was hoping for with the "PATH '/'").
>

This is a libxml2 behave

Postprocessing only check result and try to push the result to expected
types.

Regards

Pavel


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2016-11-24 0:13 GMT+01:00 Alvaro Herrera :

> Oh my, I just noticed we have a new xpath preprocessor in this patch
> too.  Where did this code come from -- did you write it all from
> scratch?
>

I wrote it from scratch - libxml2 has not any API for iteration over XPath
expression (different than iteration over XPath expression result), and
what I have info, there will not be any new API in libxml2.

There are two purposes:

Safe manipulation with XPath expression prefixes - ANSI SQL design
implicitly expects some prefix, but it can be used manually. The prefix
should not be used twice and in some situations, when it can breaks the
expression.

Second goal is support default namespaces - when we needed parser for first
task, then the enhancing for this task was not too much lines more.

This parser can be used for enhancing current XPath function - default
namespaces are pretty nice, when you have to use namespaces.


>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Alvaro Herrera wrote:

> If you use "PATH '/'" for a column, you get the text for all the entries
> in the whole XML, rather than the text for the particular row being
> processed.  Isn't that rather weird, or to put it differently, completely
> wrong?  I didn't find a way to obtain the whole XML row when you have
> the COLUMNS option (which is what I was hoping for with the "PATH '/'").

Ah, apparently you need to use type XML for that column in order for
this to happen.  Example:

insert into emp values ($$

 
 
 
 John
 Doew
   
 344   

 55000
 

 
 
 Peter
 Panw
 
 216
 905-416-5004
 
 

 
 
 
 Mary
 Jonesw
 
 415
 905-403-6112
 647-504-4546
 64000
 
 

$$);

Note the weird salary_amount value here:

SELECT x.*
FROM emp, 
XMLTABLE ('//depts/dept/employee' passing doc 
 COLUMNS 
i for ordinality,
empIDint PATH '@id',
firstnamevarchar(25) PATH 'name/first' default 'FOOBAR',
lastname VARCHAR(25) PATH 'name/last',
salary xml path 'concat(salary/text(), salary/@currency)' default 'DONT 
KNOW', salary_amount xml path '/' )
   WITH ORDINALITY
   AS X (i, a, b, c)  limit 1;
 i │  a  │  b   │  c   │  salary  │ salary_amount │ ordinality 
───┼─┼──┼──┼──┼───┼
 1 │ 905 │ John │ Doew │ 55000USD │  ↵│  1
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  John↵│ 
   │ │  │  │  │  Doew↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  344 ↵│ 
   │ │  │  │  │  55000   ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  Peter   ↵│ 
   │ │  │  │  │  Panw↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  216 ↵│ 
   │ │  │  │  │  905-416-5004↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  Mary↵│ 
   │ │  │  │  │  Jonesw  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  415 ↵│ 
   │ │  │  │  │  905-403-6112↵│ 
   │ │  │  │  │  647-504-4546↵│ 
   │ │  │  │  │  64000   ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │  ↵│ 
   │ │  │  │  │   │ 
(1 fila)


If you declare salary_amount to be text instead, it doesn't happen anymore.
Apparently if you put it in a namespace, it doesn't hapen either.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Andrew Dunstan]

On 11/23/2016 06:31 PM, Alvaro Herrera wrote:

Sorry, here's the patch.  Power loss distracted me here.

By the way, the pgindent you did is slightly confused because you failed
to add the new struct types you define to typedefs.list.



Tips on how to use pgindent for developers:



cheers

andrew



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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Here's another version.  Not there yet: need to move back the function
to create the tupdesc, as discussed.  Not clear what's the best place,
however.  I modified the grammar a bit (added the missing comma, removed
PATH as an unreserved keyword and just used IDENT, removed the "Opt"
version for column options), and reworked the comments in the transform
phase (I tweaked the code here and there mostly to move things to nicer
places, but it's pretty much the same code).

In the new xpath_parser.c file I think we should tidy things up a bit.
First, it needs more commentary on what the entry function actually
does, in detail.  Also, IMO that function should be at the top of the
file, not at the bottom, followed by all its helpers.  I would like some
more clarity on the provenance of all this code, just to assess the
probability of bugs; mostly as it's completely undocumented.

I don't like the docs either.  I think we should have a complete
reference to the syntax, followed by examples, rather than letting the
examples drive the whole thing.  I fixed the synopsis so that it's not
one very long line.

If you use "PATH '/'" for a column, you get the text for all the entries
in the whole XML, rather than the text for the particular row being
processed.  Isn't that rather weird, or to put it differently, completely
wrong?  I didn't find a way to obtain the whole XML row when you have
the COLUMNS option (which is what I was hoping for with the "PATH '/'").

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Oh my, I just noticed we have a new xpath preprocessor in this patch
too.  Where did this code come from -- did you write it all from
scratch?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Alvaro Herrera wrote:

> I'm still wondering how this works.  I'll continue to explore the patch
> in order to figure out.

Ah, so it's already parsed as a "range function".  That part looks good
to me.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

I tried to see if a following RTE was able to "see" the entries created by
XMLTABLE, and sure it can:

SELECT X.*, generate_series 
FROM emp, 
XMLTABLE ('//depts/dept/employee' passing doc 
   COLUMNS 
   empIDINTEGER PATH '@id',
   firstnamevarchar(25) PATH 'name/first',
   lastname VARCHAR(25) PATH 'name/last') AS X,
generate_series(900, empid);

 empid │ firstname │ lastname │ generate_series 
───┼───┼──┼─
   901 │ John  │ Doe  │ 900
   901 │ John  │ Doe  │ 901
   902 │ Peter │ Pan  │ 900
   902 │ Peter │ Pan  │ 901
   902 │ Peter │ Pan  │ 902
   903 │ Mary  │ Jones│ 900
   903 │ Mary  │ Jones│ 901
   903 │ Mary  │ Jones│ 902
   903 │ Mary  │ Jones│ 903
(9 filas)

Cool.

I'm still wondering how this works.  I'll continue to explore the patch
in order to figure out.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-22 21:47 GMT+01:00 Alvaro Herrera :

> I found the whole TableExprGetTupleDesc() function a bit odd in
> nodeFuncs.c, so I renamed it to ExecTypeFromTableExpr() and moved it to
> execTuples.c -- but only because that's where ExecTypeFromTL and others
> already live.  I would have liked to move it to tupdesc.c instead, but
> it requires knowledge of executor nodes, which is probably the reason
> that ExecTypeFromTL is in execTuples.  I think we'd eat that bit of
> ugliness only because we're not the first.  But anyway I quickly ran
> into another problem.
>
> I noticed that ExecTypeFromTableExpr is being called from the transform
> phase, which is much earlier than the executor.  I noticed because of
> the warning that the above movement added to nodeFuncs.c,
> src/backend/nodes/nodeFuncs.c:509:5: warning: implicit declaration of
> function 'ExecTypeFromTableExpr' [-Wimplicit-function-declaration]
>

The tuple descriptor should not be serialized.

When xmltable is called directly, then living tuple descriptor is used -
created in transform time. Another situation is when xmltable is used from
view, where transform time is skipped.

Originally I serialized generated type - but I had the problems with record
types - the current infrastructure expects serialization only real types.

My solution is a recheck of tuple descriptor in executor time. It is small
overhead - once per query - but it allows use xmltable from views without
necessity to specify returned columns explicitly.


>
> so I thought, hm, is it okay to have parse analysis run an executor
> function?  (I suppose this is the reason you put it in nodeFuncs in the
> first place).  For fun, I tried this query under GDB, with a breakpoint
> on exprTypmod():
>
> SELECT X.*
> FROM emp,
> XMLTABLE ('//depts/dept/employee' passing doc
>COLUMNS
>empIDINTEGER PATH '@id',
>firstnameint PATH 'name/first',
>lastname VARCHAR(25) PATH 'name/last') AS X;
>
> and sure enough, the type is resolved during parse analysis:
>
> Breakpoint 1, exprTypmod (expr=expr@entry=0x1d23ad8)
> at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
> 283 if (!expr)
> (gdb) print *expr
> $2 = {type = T_TableExpr}
> (gdb) bt
> #0  exprTypmod (expr=expr@entry=0x1d23ad8)
> at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
> #1  0x0080c500 in get_expr_result_type (expr=0x1d23ad8,
> resultTypeId=0x7ffd482bfdb4, resultTupleDesc=0x7ffd482bfdb8)
> at /pgsql/source/master/src/backend/utils/fmgr/funcapi.c:247
> #2  0x0056de1b in expandRTE (rte=rte@entry=0x1d6b800, rtindex=2,
> sublevels_up=0, location=location@entry=7,
> include_dropped=include_dropped@entry=0 '\000',
> colnames=colnames@entry=0x7ffd482bfe10, colvars=0x7ffd482bfe18)
> at /pgsql/source/master/src/backend/parser/parse_relation.c:2052
> #3  0x0056e131 in expandRelAttrs (pstate=pstate@entry=0x1d238a8,
> rte=rte@entry=0x1d6b800, rtindex=,
> sublevels_up=, location=location@entry=7)
> at /pgsql/source/master/src/backend/parser/parse_relation.c:2435
> #4  0x0056fa64 in ExpandSingleTable (pstate=pstate@entry=
> 0x1d238a8,
> rte=rte@entry=0x1d6b800, location=7,
> make_target_entry=make_target_entry@entry=1 '\001')
> at /pgsql/source/master/src/backend/parser/parse_target.c:1266
> #5  0x0057135b in ExpandColumnRefStar (pstate=pstate@entry=
> 0x1d238a8,
> cref=0x1d22720, make_target_entry=make_target_entry@entry=1 '\001')
> at /pgsql/source/master/src/backend/parser/parse_target.c:1158
> #6  0x005716f9 in transformTargetList (pstate=0x1d238a8,
> targetlist=, exprKind=EXPR_KIND_SELECT_TARGET)
>
> This seems fine I guess, and it seems to say that we ought to move the
> code that generates the tupdesc to back parse analysis rather than
> executor.  Okay, fine.  But let's find a better place than nodeFuncs.
>
> But if I move the XMLTABLE() call to the target list instead, the type
> is resolved at planner time:
>
> SELECT
> XMLTABLE ('/dept/employee' passing $$
> 
> 
> Mary
> Jones
> 
> 415
> 905-403-6112
> 647-504-4546
> 64000
> 
> $$
>COLUMNS
>empIDINTEGER PATH '@id',
>firstnamevarchar(4) PATH 'name/first',
>lastname VARCHAR(25) PATH 'name/last') AS X;
>
> Breakpoint 1, exprTypmod (expr=expr@entry=0x1d6bed8)
> at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
> 283 if (!expr)
> (gdb) bt
> #0  exprTypmod (expr=expr@entry=0x1d6bed8)
> at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
> #1  0x00654058 in set_pathtarget_cost_width (root=0x1d6bc68,
> target=0x1d6c728)
> at /pgsql/source/master/src/backend/optimizer/path/costsize.c:4729
> #2  0x0066c197 in grouping_planner (root=0x1d6bc68,
> inheritance_update=40 '(', inheritance_update@entry=0 '\000',
>   

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

I found the whole TableExprGetTupleDesc() function a bit odd in
nodeFuncs.c, so I renamed it to ExecTypeFromTableExpr() and moved it to
execTuples.c -- but only because that's where ExecTypeFromTL and others
already live.  I would have liked to move it to tupdesc.c instead, but
it requires knowledge of executor nodes, which is probably the reason
that ExecTypeFromTL is in execTuples.  I think we'd eat that bit of
ugliness only because we're not the first.  But anyway I quickly ran
into another problem.

I noticed that ExecTypeFromTableExpr is being called from the transform
phase, which is much earlier than the executor.  I noticed because of
the warning that the above movement added to nodeFuncs.c,
src/backend/nodes/nodeFuncs.c:509:5: warning: implicit declaration of function 
'ExecTypeFromTableExpr' [-Wimplicit-function-declaration]

so I thought, hm, is it okay to have parse analysis run an executor
function?  (I suppose this is the reason you put it in nodeFuncs in the
first place).  For fun, I tried this query under GDB, with a breakpoint
on exprTypmod():

SELECT X.* 
FROM emp, 
XMLTABLE ('//depts/dept/employee' passing doc 
   COLUMNS 
   empIDINTEGER PATH '@id',
   firstnameint PATH 'name/first',
   lastname VARCHAR(25) PATH 'name/last') AS X;

and sure enough, the type is resolved during parse analysis:

Breakpoint 1, exprTypmod (expr=expr@entry=0x1d23ad8)
at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
283 if (!expr)
(gdb) print *expr
$2 = {type = T_TableExpr}
(gdb) bt
#0  exprTypmod (expr=expr@entry=0x1d23ad8)
at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
#1  0x0080c500 in get_expr_result_type (expr=0x1d23ad8, 
resultTypeId=0x7ffd482bfdb4, resultTupleDesc=0x7ffd482bfdb8)
at /pgsql/source/master/src/backend/utils/fmgr/funcapi.c:247
#2  0x0056de1b in expandRTE (rte=rte@entry=0x1d6b800, rtindex=2, 
sublevels_up=0, location=location@entry=7, 
include_dropped=include_dropped@entry=0 '\000', 
colnames=colnames@entry=0x7ffd482bfe10, colvars=0x7ffd482bfe18)
at /pgsql/source/master/src/backend/parser/parse_relation.c:2052
#3  0x0056e131 in expandRelAttrs (pstate=pstate@entry=0x1d238a8, 
rte=rte@entry=0x1d6b800, rtindex=, 
sublevels_up=, location=location@entry=7)
at /pgsql/source/master/src/backend/parser/parse_relation.c:2435
#4  0x0056fa64 in ExpandSingleTable (pstate=pstate@entry=0x1d238a8, 
rte=rte@entry=0x1d6b800, location=7, 
make_target_entry=make_target_entry@entry=1 '\001')
at /pgsql/source/master/src/backend/parser/parse_target.c:1266
#5  0x0057135b in ExpandColumnRefStar (pstate=pstate@entry=0x1d238a8, 
cref=0x1d22720, make_target_entry=make_target_entry@entry=1 '\001')
at /pgsql/source/master/src/backend/parser/parse_target.c:1158
#6  0x005716f9 in transformTargetList (pstate=0x1d238a8, 
targetlist=, exprKind=EXPR_KIND_SELECT_TARGET)

This seems fine I guess, and it seems to say that we ought to move the
code that generates the tupdesc to back parse analysis rather than
executor.  Okay, fine.  But let's find a better place than nodeFuncs.

But if I move the XMLTABLE() call to the target list instead, the type
is resolved at planner time:

SELECT  
XMLTABLE ('/dept/employee' passing $$ 


Mary
Jones

415
905-403-6112
647-504-4546
64000

$$
   COLUMNS 
   empIDINTEGER PATH '@id',
   firstnamevarchar(4) PATH 'name/first',
   lastname VARCHAR(25) PATH 'name/last') AS X;

Breakpoint 1, exprTypmod (expr=expr@entry=0x1d6bed8)
at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
283 if (!expr)
(gdb) bt
#0  exprTypmod (expr=expr@entry=0x1d6bed8)
at /pgsql/source/master/src/backend/nodes/nodeFuncs.c:283
#1  0x00654058 in set_pathtarget_cost_width (root=0x1d6bc68, 
target=0x1d6c728)
at /pgsql/source/master/src/backend/optimizer/path/costsize.c:4729
#2  0x0066c197 in grouping_planner (root=0x1d6bc68, 
inheritance_update=40 '(', inheritance_update@entry=0 '\000', 
tuple_fraction=0.01, tuple_fraction@entry=0)
at /pgsql/source/master/src/backend/optimizer/plan/planner.c:1745
#3  0x0066ef64 in subquery_planner (glob=glob@entry=0x1d6bbd0, 
parse=parse@entry=0x1d23818, parent_root=parent_root@entry=0x0, 
hasRecursion=hasRecursion@entry=0 '\000', 
tuple_fraction=tuple_fraction@entry=0)
at /pgsql/source/master/src/backend/optimizer/plan/planner.c:795
#4  0x0066fe5e in standard_planner (parse=0x1d23818, 
cursorOptions=256, boundParams=)
at /pgsql/source/master/src/backend/optimizer/plan/planner.c:307

This is surprising, but I'm not sure it's wrong.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list 

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-21 21:16 GMT+01:00 Tom Lane :

> Alvaro Herrera  writes:
> > Something I just noticed is that transformTableExpr takes a TableExpr
> > node and returns another TableExpr node.  That's unlike what we do in
> > other places, where the node returned is of a different type than the
> > input node.  I'm not real clear what happens if you try to re-transform
> > a node that was already transformed, but it seems worth thinking about.
>
> We're not 100% consistent on that --- there are cases such as RowExpr
> and CaseExpr where the same struct type is used for pre-parse-analysis
> and post-parse-analysis nodes.  I think it's okay as long as the
> information content isn't markedly different, ie the transformation
> just consists of transforming all the sub-nodes.
>
> Being able to behave sanely on a re-transformation used to be an
> issue, but we no longer expect transformExpr to support that.
>

I was not sure in this case - using new node was more clear for me -
safeguard against some uninitialized or untransformed value. There in only
few bytes memory more overhead.

regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] patch: function xmltable

[Tom Lane]

Alvaro Herrera  writes:
> Something I just noticed is that transformTableExpr takes a TableExpr
> node and returns another TableExpr node.  That's unlike what we do in
> other places, where the node returned is of a different type than the
> input node.  I'm not real clear what happens if you try to re-transform
> a node that was already transformed, but it seems worth thinking about.

We're not 100% consistent on that --- there are cases such as RowExpr
and CaseExpr where the same struct type is used for pre-parse-analysis
and post-parse-analysis nodes.  I think it's okay as long as the
information content isn't markedly different, ie the transformation
just consists of transforming all the sub-nodes.

Being able to behave sanely on a re-transformation used to be an
issue, but we no longer expect transformExpr to support that.

regards, tom lane


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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Something I just noticed is that transformTableExpr takes a TableExpr
node and returns another TableExpr node.  That's unlike what we do in
other places, where the node returned is of a different type than the
input node.  I'm not real clear what happens if you try to re-transform
a node that was already transformed, but it seems worth thinking about.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-19 5:19 GMT+01:00 Pavel Stehule :

>
>
> 2016-11-19 0:42 GMT+01:00 Alvaro Herrera :
>
>> The SQL standard seems to require a comma after the XMLNAMESPACES
>> clause:
>>
>>  ::=
>>   XMLTABLE 
>>   [   ]
>> 
>>   [  ]
>> COLUMNS  
>>
>> I don't understand the reason for that, but I have added it:
>>
>> | XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList
>> ')' ',' c_expr xmlexists_argument ')'
>> {
>> TableExpr *n =
>> makeNode(TableExpr);
>> n->row_path = $8;
>> n->expr = $9;
>> n->cols = NIL;
>> n->namespaces = $5;
>> n->location = @1;
>> $$ = (Node *)n;
>> }
>> | XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList
>> ')' ',' c_expr xmlexists_argument COLUMNS TableExprColList ')'
>> {
>> TableExpr *n =
>> makeNode(TableExpr);
>> n->row_path = $8;
>> n->expr = $9;
>> n->cols = $11;
>> n->namespaces = $5;
>> n->location = @1;
>> $$ = (Node *)n;
>> }
>> ;
>>
>>
> yes, looks my oversight - it is better
>
>
>
>> Another thing I did was remove the TableExprColOptionsOpt production; in
>> its place I added a third rule in TableExprCol for "ColId Typename
>> IsNotNull" (i.e. no options).  This seems to reduce the size of the
>> generated gram.c a good dozen kB.
>>
>
> If I remember well - this was required by better compatibility with Oracle
>
> ANSI SQL: colname type DEFAULT PATH
> Oracle: colname PATH DEFAULT
>
> My implementation allows both combinations - there are two reasons: 1. one
> less issue when people does port from Oracle, 2. almost all examples of
> XMLTABLE on a net are from Oracle - it can be unfriendly, when these
> examples would not work on PG - there was discussion about this issue in
> this mailing list
>
>
>>
>>
>> I didn't like much the use of c_expr in all these productions.  As I
>> understand it, c_expr is mostly an implementation artifact and we should
>> be using a_expr or b_expr almost everywhere.  I see that XMLEXISTS
>> already expanded the very limited use of c_expr there was; I would
>> prefer to fix that one too rather than replicate it here.  TBH I'm not
>> sure I like that XMLTABLE is re-using xmlexists_argument.
>>
>
> There are two situations: c_expr as document content, and c_expr after
> DEFAULT and PATH keywords. First probably can be fixed, second not, because
> "PATH" is unreserved keyword only.
>

It is not possible PASSING is unreserved keyword too.

Regards

Pavel

>
>
>>
>> Actually, is the existing XMLEXISTS production correct?  What I see in
>> the standard is
>>
>>  ::= 
>>
>>  ::=
>>PASSING  
>>  [ {   }... ]
>>
>>  ::= 
>>
>>  ::=  [ {
>>   }... ]
>>
>>  ::=
>> 
>>   | 
>>
>>  ::=  FOR ORDINALITY
>>
>>  ::=
>>   [  ]
>>   [  ]
>>   [ PATH  ]
>>
>>  ::= 
>>
>> so I think this resolves "PASSING BY {REF,VALUE} ",
>> but what
>> we have in gram.y is:
>>
>> /* We allow several variants for SQL and other compatibility. */
>> xmlexists_argument:
>> PASSING c_expr
>> {
>> $$ = $2;
>> }
>> | PASSING c_expr BY REF
>> {
>> $$ = $2;
>> }
>> | PASSING BY REF c_expr
>> {
>> $$ = $4;
>> }
>> | PASSING BY REF c_expr BY REF
>> {
>> $$ = $4;
>> }
>> ;
>>
>> I'm not sure why we allow "PASSING c_expr" at all.  Maybe if BY VALUE/BY
>> REF is not specified, we should just not have PASSING at all?
>>
>>
> If we extended this for XMLEXISTS for compatibility with some other
>> product, perhaps we should look into what that product supports for
>> XMLTABLE; maybe XMLTABLE does not need all the same options as
>> XMLEXISTS.
>>
>>
> The reason is a compatibility with other products - DB2. XMLTABLE uses
> same options like XMLEXISTS. These options has zero value for Postgres, but
> its are important - compatibility, and workable examples.
>
>
>> The fourth 

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-11-19 0:42 GMT+01:00 Alvaro Herrera :

> The SQL standard seems to require a comma after the XMLNAMESPACES
> clause:
>
>  ::=
>   XMLTABLE 
>   [   ]
> 
>   [  ]
> COLUMNS  
>
> I don't understand the reason for that, but I have added it:
>
> | XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList
> ')' ',' c_expr xmlexists_argument ')'
> {
> TableExpr *n = makeNode(TableExpr);
> n->row_path = $8;
> n->expr = $9;
> n->cols = NIL;
> n->namespaces = $5;
> n->location = @1;
> $$ = (Node *)n;
> }
> | XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList
> ')' ',' c_expr xmlexists_argument COLUMNS TableExprColList ')'
> {
> TableExpr *n = makeNode(TableExpr);
> n->row_path = $8;
> n->expr = $9;
> n->cols = $11;
> n->namespaces = $5;
> n->location = @1;
> $$ = (Node *)n;
> }
> ;
>
>
yes, looks my oversight - it is better



> Another thing I did was remove the TableExprColOptionsOpt production; in
> its place I added a third rule in TableExprCol for "ColId Typename
> IsNotNull" (i.e. no options).  This seems to reduce the size of the
> generated gram.c a good dozen kB.
>

If I remember well - this was required by better compatibility with Oracle

ANSI SQL: colname type DEFAULT PATH
Oracle: colname PATH DEFAULT

My implementation allows both combinations - there are two reasons: 1. one
less issue when people does port from Oracle, 2. almost all examples of
XMLTABLE on a net are from Oracle - it can be unfriendly, when these
examples would not work on PG - there was discussion about this issue in
this mailing list


>
>
> I didn't like much the use of c_expr in all these productions.  As I
> understand it, c_expr is mostly an implementation artifact and we should
> be using a_expr or b_expr almost everywhere.  I see that XMLEXISTS
> already expanded the very limited use of c_expr there was; I would
> prefer to fix that one too rather than replicate it here.  TBH I'm not
> sure I like that XMLTABLE is re-using xmlexists_argument.
>

There are two situations: c_expr as document content, and c_expr after
DEFAULT and PATH keywords. First probably can be fixed, second not, because
"PATH" is unreserved keyword only.


>
> Actually, is the existing XMLEXISTS production correct?  What I see in
> the standard is
>
>  ::= 
>
>  ::=
>PASSING  
>  [ {   }... ]
>
>  ::= 
>
>  ::=  [ {
>   }... ]
>
>  ::=
> 
>   | 
>
>  ::=  FOR ORDINALITY
>
>  ::=
>   [  ]
>   [  ]
>   [ PATH  ]
>
>  ::= 
>
> so I think this resolves "PASSING BY {REF,VALUE} ",
> but what
> we have in gram.y is:
>
> /* We allow several variants for SQL and other compatibility. */
> xmlexists_argument:
> PASSING c_expr
> {
> $$ = $2;
> }
> | PASSING c_expr BY REF
> {
> $$ = $2;
> }
> | PASSING BY REF c_expr
> {
> $$ = $4;
> }
> | PASSING BY REF c_expr BY REF
> {
> $$ = $4;
> }
> ;
>
> I'm not sure why we allow "PASSING c_expr" at all.  Maybe if BY VALUE/BY
> REF is not specified, we should just not have PASSING at all?
>
>
If we extended this for XMLEXISTS for compatibility with some other
> product, perhaps we should look into what that product supports for
> XMLTABLE; maybe XMLTABLE does not need all the same options as
> XMLEXISTS.
>
>
The reason is a compatibility with other products - DB2. XMLTABLE uses same
options like XMLEXISTS. These options has zero value for Postgres, but its
are important - compatibility, and workable examples.


> The fourth option seems very dubious to me.  This was added by commit
> 641459f26, submitted here:
> https://www.postgresql.org/message-id/4c0f6dbf.9000...@mlfowler.com
>
> ... Hm, actually some perusal of the XMLEXISTS predicate in the standard
> shows that it's quite a different thing from XMLTABLE.  Maybe we
> shouldn't 

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

The SQL standard seems to require a comma after the XMLNAMESPACES
clause:

 ::=
  XMLTABLE 
  [   ]

  [  ]
COLUMNS  

I don't understand the reason for that, but I have added it:

| XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList ')' 
',' c_expr xmlexists_argument ')'
{
TableExpr *n = makeNode(TableExpr);
n->row_path = $8;
n->expr = $9;
n->cols = NIL;
n->namespaces = $5;
n->location = @1;
$$ = (Node *)n;
}
| XMLTABLE '(' XMLNAMESPACES '(' XmlNamespaceList ')' 
',' c_expr xmlexists_argument COLUMNS TableExprColList ')'
{
TableExpr *n = makeNode(TableExpr);
n->row_path = $8;
n->expr = $9;
n->cols = $11;
n->namespaces = $5;
n->location = @1;
$$ = (Node *)n;
}
;

Another thing I did was remove the TableExprColOptionsOpt production; in
its place I added a third rule in TableExprCol for "ColId Typename
IsNotNull" (i.e. no options).  This seems to reduce the size of the
generated gram.c a good dozen kB.


I didn't like much the use of c_expr in all these productions.  As I
understand it, c_expr is mostly an implementation artifact and we should
be using a_expr or b_expr almost everywhere.  I see that XMLEXISTS
already expanded the very limited use of c_expr there was; I would
prefer to fix that one too rather than replicate it here.  TBH I'm not
sure I like that XMLTABLE is re-using xmlexists_argument.  

Actually, is the existing XMLEXISTS production correct?  What I see in
the standard is

 ::= 

 ::=
   PASSING  
 [ {   }... ]

 ::= 

 ::=  [ {  
 }... ]

 ::=

  | 

 ::=  FOR ORDINALITY

 ::=
  [  ]
  [  ]
  [ PATH  ]

 ::= 

so I think this resolves "PASSING BY {REF,VALUE} ", but what
we have in gram.y is:

/* We allow several variants for SQL and other compatibility. */
xmlexists_argument:
PASSING c_expr
{
$$ = $2;
}
| PASSING c_expr BY REF
{
$$ = $2;
}
| PASSING BY REF c_expr
{
$$ = $4;
}
| PASSING BY REF c_expr BY REF
{
$$ = $4;
}
;

I'm not sure why we allow "PASSING c_expr" at all.  Maybe if BY VALUE/BY
REF is not specified, we should just not have PASSING at all?

If we extended this for XMLEXISTS for compatibility with some other
product, perhaps we should look into what that product supports for
XMLTABLE; maybe XMLTABLE does not need all the same options as
XMLEXISTS.

The fourth option seems very dubious to me.  This was added by commit
641459f26, submitted here:
https://www.postgresql.org/message-id/4c0f6dbf.9000...@mlfowler.com

... Hm, actually some perusal of the XMLEXISTS predicate in the standard
shows that it's quite a different thing from XMLTABLE.  Maybe we
shouldn't reuse xmlexists_argument here at all.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

Pavel Stehule wrote:

> 2016-11-17 19:22 GMT+01:00 Alvaro Herrera :
> 
> > Next, looking at struct TableExprBuilder I noticed that the comments are
> > already obsolete, as they talk about function params that do not exist
> > (missing_columns) and they fail to mention the ones that do exist.
> > Also, function member SetContent is not documented at all.  Overall,
> > these comments do not convey a lot -- apparently, whoever reads them is
> > already supposed to know how it works: "xyz sets a row generating
> > filter" doesn't tell me anything.  Since this is API documentation, it
> > needs to be much clearer.
> >
> > ExecEvalTableExpr and ExecEvalTableExprProtected have no comments
> > whatsoever.  Needs fixed.
> 
> I am sending the patch with more comments - but it needs a care someone
> with good English skills.

Thanks, I can help with that.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

2016-11-17 19:22 GMT+01:00 Alvaro Herrera :

> I've been going over this patch.  I think it'd be better to restructure
> the  before adding the docs for this new function; I already
> split it out, so don't do anything about this.
>
> Next, looking at struct TableExprBuilder I noticed that the comments are
> already obsolete, as they talk about function params that do not exist
> (missing_columns) and they fail to mention the ones that do exist.
> Also, function member SetContent is not documented at all.  Overall,
> these comments do not convey a lot -- apparently, whoever reads them is
> already supposed to know how it works: "xyz sets a row generating
> filter" doesn't tell me anything.  Since this is API documentation, it
> needs to be much clearer.
>
> ExecEvalTableExpr and ExecEvalTableExprProtected have no comments
> whatsoever.  Needs fixed.
>

I am sending the patch with more comments - but it needs a care someone
with good English skills.


>
> I wonder if it'd be a good idea to install TableExpr first without the
> implementing XMLTABLE, so that it's clearer what is API and what is
> implementation.
>

I am not sure about this step - the API is clean from name. In this moment,
for this API is not any other tests than XMLTABLE implementation.


>
> The number of new keywords in this patch is depressing.  I suppose
> there's no way around that -- as I understand, this is caused by the SQL
> standard's definition of the syntax for this feature.
>
> Have to go now for a bit -- will continue looking afterwards.  Please
> submit delta patches on top of your latest v12 to fix the comments I
> mentioned.
>
>
Regards

Pavel


> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index c386be1..76c2ec8 100644
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -4505,8 +4505,18 @@ ExecEvalCurrentOfExpr(ExprState *exprstate, ExprContext *econtext,
 }
 
 /* 
- *		ExecEvalTableExpr
+ *		ExecEvalTableExprProtected and ExecEvalTableExpr
  *
+ * Evaluate a TableExpr node. ExecEvalTableExprProtected is called
+ * from ExecEvalTableExpr from PG_TRY(), PG_CATCH() block be ensured
+ * all allocated memory be released.
+ *
+ * It creates TableExprBuilder object, does all necessary settings and
+ * reads all tuples from this object. Is possible to go over this
+ * cycle more times per query - when LATERAL JOIN is used. On the end
+ * of cycle, the TableExprBuilder object is destroyed, state pointer
+ * to this object is cleaned, and related memory context is resetted.
+ * New call starts new cycle.
  * 
  */
 static Datum
@@ -4682,6 +4692,12 @@ ExecEvalTableExprProtected(TableExprState * tstate,
 	return result;
 }
 
+/*
+ * ExecEvalTableExpr - this is envelop of ExecEvalTableExprProtected() function.
+ *
+ * This function ensures releasing all TableBuilder context and related
+ * memory context, when ExecEvalTableExprProtected fails on exception.
+ */
 static Datum
 ExecEvalTableExpr(TableExprState * tstate,
   ExprContext *econtext,
diff --git a/src/include/executor/tableexpr.h b/src/include/executor/tableexpr.h
index ad5d8e2..3056317 100644
--- a/src/include/executor/tableexpr.h
+++ b/src/include/executor/tableexpr.h
@@ -20,27 +20,37 @@
  * for generating content of table-expression functions like
  * XMLTABLE.
  *
- * CreateContext is called before execution and it does query level
- * initialization. Returns pointer to private TableExprBuilder data
- * (context). A query context should be active in call time. A param
- * missing_columns is true, when a user doesn't specify returned
- * columns.
+ * The TableBuilder is initialized by calling CreateContext function
+ * at evaluation time. First parameter - tuple descriptor describes
+ * produced (expected) table. in_functions is a array of FmgrInfo input
+ * functions for types of columns of produced table. The typioparams
+ * is a array of typio Oids for types of columns of produced table.
+ * The created context is living in special memory context passed
+ * as last parameter.
  *
- * AddNs add namespace info when namespaces are is supported.
- * then passed namespace is default namespace. Namespaces should be
- * passed before Row/Column Paths setting. When name is NULL, then
- * related uri is default namespace.
+ * The SetContent function is used for passing input document to
+ * table builder. The table builder handler knows expected format
+ * and it can do some additional transformations that are not propagated
+ * out from table builder.
  *
- * SetRowPath sets a row generating filter.
+ * The AddNs add namespace info when namespaces are supported.
+ * Namespaces should be passed before Row/Column Paths 

Re: [HACKERS] patch: function xmltable

[Alvaro Herrera]

I've been going over this patch.  I think it'd be better to restructure
the  before adding the docs for this new function; I already
split it out, so don't do anything about this.

Next, looking at struct TableExprBuilder I noticed that the comments are
already obsolete, as they talk about function params that do not exist
(missing_columns) and they fail to mention the ones that do exist.
Also, function member SetContent is not documented at all.  Overall,
these comments do not convey a lot -- apparently, whoever reads them is
already supposed to know how it works: "xyz sets a row generating
filter" doesn't tell me anything.  Since this is API documentation, it
needs to be much clearer.

ExecEvalTableExpr and ExecEvalTableExprProtected have no comments
whatsoever.  Needs fixed.

I wonder if it'd be a good idea to install TableExpr first without the
implementing XMLTABLE, so that it's clearer what is API and what is
implementation.

The number of new keywords in this patch is depressing.  I suppose
there's no way around that -- as I understand, this is caused by the SQL
standard's definition of the syntax for this feature.

Have to go now for a bit -- will continue looking afterwards.  Please
submit delta patches on top of your latest v12 to fix the comments I
mentioned.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

new update - only doc

+
+ Only XPath query language is supported. PostgreSQL doesn't support
XQuery
+ language. Then the syntax of xmltable doesn't
+ allow to use XQuery related functionality - the name of xml expression
+ (clause AS) is not allowed, and only one xml
expression
+ should be passed to xmltable function as
parameter.
+

Regards

Pavel


xmltable-12.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] patch: function xmltable

[Michael Paquier]

On Tue, Sep 27, 2016 at 2:29 PM, Pavel Stehule  wrote:
> 2016-09-27 5:53 GMT+02:00 Craig Ringer :
>>
>> [...]
>> ... so please delete that text. I thought I'd tested it but the state
>> of my tests dir says I just got distracted by another task at the
>> wrong time.

Moved patch to next CF with same status: ready for committer.
-- 
Michael


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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-09-27 5:53 GMT+02:00 Craig Ringer :

> On 24 September 2016 at 14:01, Pavel Stehule 
> wrote:
>
> >> Did some docs copy-editing and integrated some examples. Explained how
> >> nested elements work, that multiple top level elements is an error,
> >> etc. Explained the time-of-evaluation stuff. Pointed out that you can
> >> refer to prior output columns in PATH and DEFAULT, since that's weird
> >> and unusual compared to normal SQL. Documented handling of multiple
> >> node matches, including the surprising results of somepath/text() on
> >> xy. Documented handling of nested
> >> elements. Documented that xmltable works only on XML documents, not
> >> fragments/forests.
> >
> >
> > I don't understand to this sentence: "It is possible for a PATH
> expression
> > to reference output columns that appear before it in the column-list, so
> > paths may be dynamically constructed based on other parts of the XML
> > document:"
>
> This was based on a misunderstanding of something you said earlier. I
> thought the idea was to allow this to work:
>
> SELECT * FROM xmltable('/x' PASSING
> 'avalue' COLUMNS elemName text,
> extractedValue text PATH elemName);
>
> ... but it doesn't:
>
>
> SELECT * FROM xmltable('/x' PASSING
> 'avalue' COLUMNS elemName text,
> extractedValue text PATH elemName);
> ERROR:  column "elemname" does not exist
> LINE 1: ...' COLUMNS elemName text, extractedValue text PATH elemName);
>
> ... so please delete that text. I thought I'd tested it but the state
> of my tests dir says I just got distracted by another task at the
> wrong time.
>

deleted

Regards

Pavel

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


xmltable-11.patch.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] patch: function xmltable

[Craig Ringer]

On 24 September 2016 at 14:01, Pavel Stehule  wrote:

>> Did some docs copy-editing and integrated some examples. Explained how
>> nested elements work, that multiple top level elements is an error,
>> etc. Explained the time-of-evaluation stuff. Pointed out that you can
>> refer to prior output columns in PATH and DEFAULT, since that's weird
>> and unusual compared to normal SQL. Documented handling of multiple
>> node matches, including the surprising results of somepath/text() on
>> xy. Documented handling of nested
>> elements. Documented that xmltable works only on XML documents, not
>> fragments/forests.
>
>
> I don't understand to this sentence: "It is possible for a PATH expression
> to reference output columns that appear before it in the column-list, so
> paths may be dynamically constructed based on other parts of the XML
> document:"

This was based on a misunderstanding of something you said earlier. I
thought the idea was to allow this to work:

SELECT * FROM xmltable('/x' PASSING
'avalue' COLUMNS elemName text,
extractedValue text PATH elemName);

... but it doesn't:


SELECT * FROM xmltable('/x' PASSING
'avalue' COLUMNS elemName text,
extractedValue text PATH elemName);
ERROR:  column "elemname" does not exist
LINE 1: ...' COLUMNS elemName text, extractedValue text PATH elemName);

... so please delete that text. I thought I'd tested it but the state
of my tests dir says I just got distracted by another task at the
wrong time.

-- 
 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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

2016-09-27 3:34 GMT+02:00 Craig Ringer :

> On 24 September 2016 at 14:01, Pavel Stehule 
> wrote:
>
> >> Did some docs copy-editing and integrated some examples. Explained how
> >> nested elements work, that multiple top level elements is an error,
> >> etc. Explained the time-of-evaluation stuff. Pointed out that you can
> >> refer to prior output columns in PATH and DEFAULT, since that's weird
> >> and unusual compared to normal SQL. Documented handling of multiple
> >> node matches, including the surprising results of somepath/text() on
> >> xy. Documented handling of nested
> >> elements. Documented that xmltable works only on XML documents, not
> >> fragments/forests.
> >
> >
> > I don't understand to this sentence: "It is possible for a PATH
> expression
> > to reference output columns that appear before it in the column-list, so
> > paths may be dynamically constructed based on other parts of the XML
> > document:"
>
>
>
> >> The docs and tests don't seem to cover XML entities. What's the
> >> behaviour there? Core XML only defines one entity, but if a schema
> >> defines more how are they processed? The tests need to cover the
> >> predefined entities and  at least.
> >
> >
> > I don't understand, what you are propose here. ?? Please, can you send
> some
> > examples.
>
> Per below - handling of DTD  declarations, and the builtin
> entity tests I already added tests for.
>
>
> >> It doesn't seem to cope with internal DTDs at all (libxml2 limitation?):
> >>
> >> SELECT * FROM xmltable('/' PASSING $XML$ >> standalone="yes" ?>
> >>  >>   
> >>   
> >> ]>
> >> Hello .
> >> $XML$ COLUMNS foo text);
> >>
> >> + ERROR:  invalid XML content
> >> + LINE 1: SELECT * FROM xmltable('/' PASSING $XML$ ...
> >> +^
> >> + DETAIL:  line 2: StartTag: invalid element name
> >> +  >> +  ^
> >> + line 3: StartTag: invalid element name
> >> +   
> >> +^
> >> + line 4: StartTag: invalid element name
> >> +   
> >> +^
> >> + line 6: Entity 'pg' not defined
> >> + Hello .
> >> +^
> >>
> >
> > It is rejected before XMLTABLE function call
> >
> > postgres=# select $XML$
> > postgres$#  > postgres$#   
> > postgres$#   
> > postgres$# ]>
> > postgres$# Hello .
> > postgres$# $XML$::xml;
> > ERROR:  invalid XML content
> > LINE 1: select $XML$
> >^
> > DETAIL:  line 2: StartTag: invalid element name
> >  [snip]
>
> > It is disabled by default in libxml2. I found a function
> > xmlSubstituteEntitiesDefault http://www.xmlsoft.org/entities.html
> > http://www.xmlsoft.org/html/libxml-parser.html#
> xmlSubstituteEntitiesDefault
> >
> > The default behave should be common for all PostgreSQL's libxml2 based
> > function - and then it is different topic - maybe part for PostgreSQL
> ToDo?
> > But I don't remember any user requests related to this issue.
>
>
> OK, so it's not xmltable specific. Fine by me.
>
> Somebody who cares can deal with it. There's clearly nobody breaking
> down the walls wanting the feature.
>
> > I removed this tests - it is not related to XMLTABLE function, but to
> > generic XML processing/validation.
>
>
> Good plan.
>
> >> In +ExecEvalTableExpr, shouldn't you be using PG_ENSURE_ERROR_CLEANUP
> >> instead of a PG_TRY() / PG_CATCH() block?
> >
> >
> > If I understand to doc, the PG_ENSURE_ERROR_CLEANUP should be used, when
> you
> > want to catch FATAL errors (and when you want to clean shared memory).
> > XMLTABLE doesn't use shared memory, and doesn't need to catch fatal
> errors.
>
> Ok, makes sense.
>
>
> >> I don't have the libxml knowledge or remaining brain to usefully
> >> evaluate the xpath and xml specifics in xpath.c today. It does strike
> >> me that the new xpath parser should probably live in its own file,
> >> though.
> >
> > moved
>
> Thanks.
>
>
> > new version is attached
>
>
> Great.
>
> I'm marking this ready for committer at this point.
>

Thank you very much

Regards

Pavel


>
> I think the XML parser likely needs a more close reading, so I'll ping
> Peter E to see if he'll have a chance to check that bit out. But by
> and large I think the issues have been ironed out - in terms of
> functionality, structure and clarity I think it's looking solid.
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] patch: function xmltable

[Craig Ringer]

On 24 September 2016 at 14:01, Pavel Stehule  wrote:

>> Did some docs copy-editing and integrated some examples. Explained how
>> nested elements work, that multiple top level elements is an error,
>> etc. Explained the time-of-evaluation stuff. Pointed out that you can
>> refer to prior output columns in PATH and DEFAULT, since that's weird
>> and unusual compared to normal SQL. Documented handling of multiple
>> node matches, including the surprising results of somepath/text() on
>> xy. Documented handling of nested
>> elements. Documented that xmltable works only on XML documents, not
>> fragments/forests.
>
>
> I don't understand to this sentence: "It is possible for a PATH expression
> to reference output columns that appear before it in the column-list, so
> paths may be dynamically constructed based on other parts of the XML
> document:"



>> The docs and tests don't seem to cover XML entities. What's the
>> behaviour there? Core XML only defines one entity, but if a schema
>> defines more how are they processed? The tests need to cover the
>> predefined entities and  at least.
>
>
> I don't understand, what you are propose here. ?? Please, can you send some
> examples.

Per below - handling of DTD  declarations, and the builtin
entity tests I already added tests for.


>> It doesn't seem to cope with internal DTDs at all (libxml2 limitation?):
>>
>> SELECT * FROM xmltable('/' PASSING $XML$> standalone="yes" ?>
>> >   
>>   
>> ]>
>> Hello .
>> $XML$ COLUMNS foo text);
>>
>> + ERROR:  invalid XML content
>> + LINE 1: SELECT * FROM xmltable('/' PASSING $XML$> +^
>> + DETAIL:  line 2: StartTag: invalid element name
>> + > +  ^
>> + line 3: StartTag: invalid element name
>> +   
>> +^
>> + line 4: StartTag: invalid element name
>> +   
>> +^
>> + line 6: Entity 'pg' not defined
>> + Hello .
>> +^
>>
>
> It is rejected before XMLTABLE function call
>
> postgres=# select $XML$
> postgres$#  postgres$#   
> postgres$#   
> postgres$# ]>
> postgres$# Hello .
> postgres$# $XML$::xml;
> ERROR:  invalid XML content
> LINE 1: select $XML$
>^
> DETAIL:  line 2: StartTag: invalid element name
>  It is disabled by default in libxml2. I found a function
> xmlSubstituteEntitiesDefault http://www.xmlsoft.org/entities.html
> http://www.xmlsoft.org/html/libxml-parser.html#xmlSubstituteEntitiesDefault
>
> The default behave should be common for all PostgreSQL's libxml2 based
> function - and then it is different topic - maybe part for PostgreSQL ToDo?
> But I don't remember any user requests related to this issue.


OK, so it's not xmltable specific. Fine by me.

Somebody who cares can deal with it. There's clearly nobody breaking
down the walls wanting the feature.

> I removed this tests - it is not related to XMLTABLE function, but to
> generic XML processing/validation.


Good plan.

>> In +ExecEvalTableExpr, shouldn't you be using PG_ENSURE_ERROR_CLEANUP
>> instead of a PG_TRY() / PG_CATCH() block?
>
>
> If I understand to doc, the PG_ENSURE_ERROR_CLEANUP should be used, when you
> want to catch FATAL errors (and when you want to clean shared memory).
> XMLTABLE doesn't use shared memory, and doesn't need to catch fatal errors.

Ok, makes sense.


>> I don't have the libxml knowledge or remaining brain to usefully
>> evaluate the xpath and xml specifics in xpath.c today. It does strike
>> me that the new xpath parser should probably live in its own file,
>> though.
>
> moved

Thanks.


> new version is attached


Great.

I'm marking this ready for committer at this point.

I think the XML parser likely needs a more close reading, so I'll ping
Peter E to see if he'll have a chance to check that bit out. But by
and large I think the issues have been ironed out - in terms of
functionality, structure and clarity I think it's looking solid.

-- 
 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