is it bug? - printing boolean domains in sql/xml function

Started by Pavel Stehulealmost 13 years ago11 messages

Pavel Stehule

pavel.stehule@gmail.com

almost 13 years ago

Hello

boolean domains is serialised to string different than boolean

postgres=# CREATE DOMAIN booldomain as bool;
CREATE DOMAIN

-- fully expected behave
postgres=# select true, true::booldomain;
bool | booldomain
------+------------
t | t
(1 row)

postgres=# select true::text, true::booldomain::text;
text | text
------+------
true | true
(1 row)

-- unexpected behave
postgres=# select xmlforest(true as bool, true::booldomain as booldomain);
xmlforest
---------------------------------------------
<bool>true</bool><booldomain>t</booldomain>
(1 row)

is it expected behave?

Regards

Pavel

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

Noah Misch

noah@leadboat.com

almost 13 years ago

In reply to: Pavel Stehule (#1)

Re: is it bug? - printing boolean domains in sql/xml function

On Sun, Jan 13, 2013 at 07:54:11AM +0100, Pavel Stehule wrote:

related to /messages/by-id/CAFj8pRDTAVfNrAzwEt+ewMfRBDZFfVa8W17Kk_E12fb6T-ZNXQ@mail.gmail.com

boolean domains is serialised to string different than boolean

postgres=# CREATE DOMAIN booldomain as bool;
CREATE DOMAIN

-- fully expected behave
postgres=# select true, true::booldomain;
bool | booldomain
------+------------
t | t
(1 row)

postgres=# select true::text, true::booldomain::text;
text | text
------+------
true | true
(1 row)

-- unexpected behave
postgres=# select xmlforest(true as bool, true::booldomain as booldomain);
xmlforest
---------------------------------------------
<bool>true</bool><booldomain>t</booldomain>
(1 row)

is it expected behave?

There is a bug here. map_sql_type_to_xmlschema_type() has special treatment
for domains, but map_sql_value_to_xml_value() and its callers have no
corresponding logic. In the same vein, this yields a schema that does not
validate its corresponding document:

set datestyle = 'sql, dmy';
create domain datedom as date;
create table t as select current_date AS a, current_date::datedom AS b;
select table_to_xml('t', true, true, '');
select table_to_xmlschema('t', true, true, '');

One could debate whether the schema generation or the data generation should
be the one to change, but I tentatively vote for the latter.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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

Pavel Stehule

pavel.stehule@gmail.com

almost 13 years ago

In reply to: Noah Misch (#2)

Re: is it bug? - printing boolean domains in sql/xml function

2013/2/16 Noah Misch <noah@leadboat.com>:

On Sun, Jan 13, 2013 at 07:54:11AM +0100, Pavel Stehule wrote:

related to /messages/by-id/CAFj8pRDTAVfNrAzwEt+ewMfRBDZFfVa8W17Kk_E12fb6T-ZNXQ@mail.gmail.com

boolean domains is serialised to string different than boolean

postgres=# CREATE DOMAIN booldomain as bool;
CREATE DOMAIN

-- fully expected behave
postgres=# select true, true::booldomain;
bool | booldomain
------+------------
t | t
(1 row)

postgres=# select true::text, true::booldomain::text;
text | text
------+------
true | true
(1 row)

-- unexpected behave
postgres=# select xmlforest(true as bool, true::booldomain as booldomain);
xmlforest
---------------------------------------------
<bool>true</bool><booldomain>t</booldomain>
(1 row)

is it expected behave?

There is a bug here. map_sql_type_to_xmlschema_type() has special treatment
for domains, but map_sql_value_to_xml_value() and its callers have no
corresponding logic. In the same vein, this yields a schema that does not
validate its corresponding document:

set datestyle = 'sql, dmy';
create domain datedom as date;
create table t as select current_date AS a, current_date::datedom AS b;
select table_to_xml('t', true, true, '');
select table_to_xmlschema('t', true, true, '');

One could debate whether the schema generation or the data generation should
be the one to change, but I tentatively vote for the latter.

yes, I am thinking so it is bug too.

if we will agree so it should be fixed I'll write fix

Regards

Pavel

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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

Pavel Stehule

pavel.stehule@gmail.com

almost 13 years ago

In reply to: Pavel Stehule (#3)

1 attachment(s)

Re: is it bug? - printing boolean domains in sql/xml function

Hello

here is patch

where it should be pushed - 9.3 or 9.4 ?

I vote 9.3 - I know a users, that should to do workarounds (he should
not to use domains) now, so early is better. And this patch is one
line size patch

Regards

Pavel

2013/2/16 Pavel Stehule <pavel.stehule@gmail.com>:

Show quoted text

2013/2/16 Noah Misch <noah@leadboat.com>:

On Sun, Jan 13, 2013 at 07:54:11AM +0100, Pavel Stehule wrote:

related to /messages/by-id/CAFj8pRDTAVfNrAzwEt+ewMfRBDZFfVa8W17Kk_E12fb6T-ZNXQ@mail.gmail.com

boolean domains is serialised to string different than boolean

postgres=# CREATE DOMAIN booldomain as bool;
CREATE DOMAIN

-- fully expected behave
postgres=# select true, true::booldomain;
bool | booldomain
------+------------
t | t
(1 row)

postgres=# select true::text, true::booldomain::text;
text | text
------+------
true | true
(1 row)

-- unexpected behave
postgres=# select xmlforest(true as bool, true::booldomain as booldomain);
xmlforest
---------------------------------------------
<bool>true</bool><booldomain>t</booldomain>
(1 row)

is it expected behave?

There is a bug here. map_sql_type_to_xmlschema_type() has special treatment
for domains, but map_sql_value_to_xml_value() and its callers have no
corresponding logic. In the same vein, this yields a schema that does not
validate its corresponding document:

set datestyle = 'sql, dmy';
create domain datedom as date;
create table t as select current_date AS a, current_date::datedom AS b;
select table_to_xml('t', true, true, '');
select table_to_xmlschema('t', true, true, '');

One could debate whether the schema generation or the data generation should
be the one to change, but I tentatively vote for the latter.

yes, I am thinking so it is bug too.

if we will agree so it should be fixed I'll write fix

Regards

Pavel

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

Attachments:

fix-xmlmap.patchapplication/octet-stream; name=fix-xmlmap.patchDownload

*** a/src/backend/utils/adt/xml.c
--- b/src/backend/utils/adt/xml.c
***************
*** 2001,2006 **** map_sql_value_to_xml_value(Datum value, Oid type, bool xml_escape_strings)
--- 2001,2009 ----
  		bool		isvarlena;
  		char	   *str;
  
+ 		/* flatten domains */
+ 		type = getBaseType(type);
+ 
  		/*
  		 * Special XSD formatting for some data types
  		 */
*** a/src/test/regress/expected/xmlmap.out
--- b/src/test/regress/expected/xmlmap.out
***************
*** 1174,1176 **** SELECT schema_to_xml_and_xmlschema('testxmlschema', true, true, 'foo');
--- 1174,1200 ----
   
  (1 row)
  
+ -- domain should be transformed correctly to - a result should be same as base types
+ CREATE DOMAIN testboolxmldomain AS bool;
+ CREATE DOMAIN testdatexmldomain AS date;
+ CREATE TABLE testxmlschema.test3 AS SELECT true c1, true::testboolxmldomain c2,
+                                            '2013-02-21'::date c3, '2013-02-21'::testdatexmldomain c4;
+ SELECT xmlforest(c1, c2, c3, c4) FROM testxmlschema.test3;
+                             xmlforest                             
+ ------------------------------------------------------------------
+  <c1>true</c1><c2>true</c2><c3>2013-02-21</c3><c4>2013-02-21</c4>
+ (1 row)
+ 
+ select table_to_xml('testxmlschema.test3', true, true, '');
+                          table_to_xml                          
+ ---------------------------------------------------------------
+  <test3 xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">+
+    <c1>true</c1>                                              +
+    <c2>true</c2>                                              +
+    <c3>2013-02-21</c3>                                        +
+    <c4>2013-02-21</c4>                                        +
+  </test3>                                                     +
+                                                               +
+  
+ (1 row)
+ 
*** a/src/test/regress/sql/xmlmap.sql
--- b/src/test/regress/sql/xmlmap.sql
***************
*** 39,41 **** SELECT schema_to_xml('testxmlschema', true, false, '');
--- 39,51 ----
  SELECT schema_to_xmlschema('testxmlschema', false, true, '');
  SELECT schema_to_xmlschema('testxmlschema', true, false, '');
  SELECT schema_to_xml_and_xmlschema('testxmlschema', true, true, 'foo');
+ 
+ -- domain should be transformed correctly to - a result should be same as base types
+ CREATE DOMAIN testboolxmldomain AS bool;
+ CREATE DOMAIN testdatexmldomain AS date;
+ 
+ CREATE TABLE testxmlschema.test3 AS SELECT true c1, true::testboolxmldomain c2,
+                                            '2013-02-21'::date c3, '2013-02-21'::testdatexmldomain c4;
+ 
+ SELECT xmlforest(c1, c2, c3, c4) FROM testxmlschema.test3;
+ select table_to_xml('testxmlschema.test3', true, true, '');

Pavel Stehule

pavel.stehule@gmail.com

almost 13 years ago

In reply to: Pavel Stehule (#4)

Re: is it bug? - printing boolean domains in sql/xml function

pink Peter as xml feature commiter.

Regards

Pavel

2013/2/21 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

here is patch

where it should be pushed - 9.3 or 9.4 ?

I vote 9.3 - I know a users, that should to do workarounds (he should
not to use domains) now, so early is better. And this patch is one
line size patch

Regards

Pavel

2013/2/16 Pavel Stehule <pavel.stehule@gmail.com>:

2013/2/16 Noah Misch <noah@leadboat.com>:

On Sun, Jan 13, 2013 at 07:54:11AM +0100, Pavel Stehule wrote:

related to /messages/by-id/CAFj8pRDTAVfNrAzwEt+ewMfRBDZFfVa8W17Kk_E12fb6T-ZNXQ@mail.gmail.com

boolean domains is serialised to string different than boolean

postgres=# CREATE DOMAIN booldomain as bool;
CREATE DOMAIN

-- fully expected behave
postgres=# select true, true::booldomain;
bool | booldomain
------+------------
t | t
(1 row)

postgres=# select true::text, true::booldomain::text;
text | text
------+------
true | true
(1 row)

-- unexpected behave
postgres=# select xmlforest(true as bool, true::booldomain as booldomain);
xmlforest
---------------------------------------------
<bool>true</bool><booldomain>t</booldomain>
(1 row)

is it expected behave?

There is a bug here. map_sql_type_to_xmlschema_type() has special treatment
for domains, but map_sql_value_to_xml_value() and its callers have no
corresponding logic. In the same vein, this yields a schema that does not
validate its corresponding document:

set datestyle = 'sql, dmy';
create domain datedom as date;
create table t as select current_date AS a, current_date::datedom AS b;
select table_to_xml('t', true, true, '');
select table_to_xmlschema('t', true, true, '');

One could debate whether the schema generation or the data generation should
be the one to change, but I tentatively vote for the latter.

yes, I am thinking so it is bug too.

if we will agree so it should be fixed I'll write fix

Regards

Pavel

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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

Tom Lane

tgl@sss.pgh.pa.us

almost 13 years ago

In reply to: Pavel Stehule (#4)

Re: is it bug? - printing boolean domains in sql/xml function

Pavel Stehule <pavel.stehule@gmail.com> writes:

here is patch

Applied, though without the regression test changes, because (a) that
didn't really seem necessary, and (b) I didn't feel like updating
xmlmap_1.out.

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

Pavel Stehule

pavel.stehule@gmail.com

almost 13 years ago

In reply to: Tom Lane (#6)

Re: is it bug? - printing boolean domains in sql/xml function

Hello

2013/3/4 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

here is patch

Applied, though without the regression test changes, because (a) that
didn't really seem necessary, and (b) I didn't feel like updating
xmlmap_1.out.

thank you for commit

in this use case I am think so some regression test is important - It
should not be mine, but missing more explicit regression test is
reason, why this bug was not detected some years.

Regards

Pavel

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

Peter Eisentraut

peter_e@gmx.net

almost 13 years ago

In reply to: Pavel Stehule (#7)

Re: is it bug? - printing boolean domains in sql/xml function

On Mon, 2013-03-04 at 08:35 +0100, Pavel Stehule wrote:

in this use case I am think so some regression test is important - It
should not be mine, but missing more explicit regression test is
reason, why this bug was not detected some years.

I've added the tests.

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

Szymon Guz

mabewlun@gmail.com

over 12 years ago

In reply to: Peter Eisentraut (#8)

Re: is it bug? - printing boolean domains in sql/xml function

On 14 March 2013 03:45, Peter Eisentraut <peter_e@gmx.net> wrote:

On Mon, 2013-03-04 at 08:35 +0100, Pavel Stehule wrote:

in this use case I am think so some regression test is important - It
should not be mine, but missing more explicit regression test is
reason, why this bug was not detected some years.

I've added the tests.

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

Hi,
how should I apply the patch from fix-xmlmap.patch? I've run out of ideas.

When I run it normally, I get:

$ patch --verbose --dry-run -p1 < fix-xmlmap.patch
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/backend/utils/adt/xml.c
|--- b/src/backend/utils/adt/xml.c
--------------------------
Patching file src/backend/utils/adt/xml.c using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 succeeded at 2002 with fuzz 2 (offset 1 line).
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
The next patch looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/test/regress/expected/xmlmap.out
|--- b/src/test/regress/expected/xmlmap.out
--------------------------
Patching file src/test/regress/expected/xmlmap.out using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 succeeded at 1201 (offset 27 lines).
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
The next patch looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/test/regress/sql/xmlmap.sql
|--- b/src/test/regress/sql/xmlmap.sql
--------------------------
Patching file src/test/regress/sql/xmlmap.sql using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 FAILED at 39.
1 out of 1 hunk FAILED -- saving rejects to file
src/test/regress/sql/xmlmap.sql.rej
done

thanks,
Szymon

#10

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Szymon Guz (#9)

Re: is it bug? - printing boolean domains in sql/xml function

Hello

you can try fresh patch

git format-patch -1 788bce13d3249ddbcdf3443ee078145f4888ab45

regards

Pavel

2013/6/24 Szymon Guz <mabewlun@gmail.com>:

On 14 March 2013 03:45, Peter Eisentraut <peter_e@gmx.net> wrote:

On Mon, 2013-03-04 at 08:35 +0100, Pavel Stehule wrote:

in this use case I am think so some regression test is important - It
should not be mine, but missing more explicit regression test is
reason, why this bug was not detected some years.

I've added the tests.

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

Hi,
how should I apply the patch from fix-xmlmap.patch? I've run out of ideas.

When I run it normally, I get:

$ patch --verbose --dry-run -p1 < fix-xmlmap.patch
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/backend/utils/adt/xml.c
|--- b/src/backend/utils/adt/xml.c
--------------------------
Patching file src/backend/utils/adt/xml.c using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 succeeded at 2002 with fuzz 2 (offset 1 line).
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
The next patch looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/test/regress/expected/xmlmap.out
|--- b/src/test/regress/expected/xmlmap.out
--------------------------
Patching file src/test/regress/expected/xmlmap.out using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 succeeded at 1201 (offset 27 lines).
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
The next patch looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/test/regress/sql/xmlmap.sql
|--- b/src/test/regress/sql/xmlmap.sql
--------------------------
Patching file src/test/regress/sql/xmlmap.sql using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 FAILED at 39.
1 out of 1 hunk FAILED -- saving rejects to file
src/test/regress/sql/xmlmap.sql.rej
done

thanks,
Szymon

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

#11

Pavel Stehule

pavel.stehule@gmail.com

over 12 years ago

In reply to: Pavel Stehule (#10)

Re: is it bug? - printing boolean domains in sql/xml function

2013/6/24 Pavel Stehule <pavel.stehule@gmail.com>:

Hello

you can try fresh patch

git format-patch -1 788bce13d3249ddbcdf3443ee078145f4888ab45

and git format-patch -1 bc61878682051678ade5f59da7bfd90ab72ce13b

regards

Pavel

2013/6/24 Szymon Guz <mabewlun@gmail.com>:

On 14 March 2013 03:45, Peter Eisentraut <peter_e@gmx.net> wrote:

On Mon, 2013-03-04 at 08:35 +0100, Pavel Stehule wrote:

in this use case I am think so some regression test is important - It
should not be mine, but missing more explicit regression test is
reason, why this bug was not detected some years.

I've added the tests.

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

Hi,
how should I apply the patch from fix-xmlmap.patch? I've run out of ideas.

When I run it normally, I get:

$ patch --verbose --dry-run -p1 < fix-xmlmap.patch
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/backend/utils/adt/xml.c
|--- b/src/backend/utils/adt/xml.c
--------------------------
Patching file src/backend/utils/adt/xml.c using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 succeeded at 2002 with fuzz 2 (offset 1 line).
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
The next patch looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/test/regress/expected/xmlmap.out
|--- b/src/test/regress/expected/xmlmap.out
--------------------------
Patching file src/test/regress/expected/xmlmap.out using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 succeeded at 1201 (offset 27 lines).
Hmm...(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
The next patch looks like a context diff to me...
The text leading up to this was:
--------------------------
|*** a/src/test/regress/sql/xmlmap.sql
|--- b/src/test/regress/sql/xmlmap.sql
--------------------------
Patching file src/test/regress/sql/xmlmap.sql using Plan A...
(Fascinating -- this is really a new-style context diff but without
the telltale extra asterisks on the *** line that usually indicate
the new style...)
Hunk #1 FAILED at 39.
1 out of 1 hunk FAILED -- saving rejects to file
src/test/regress/sql/xmlmap.sql.rej
done

thanks,
Szymon

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