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

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

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

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

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

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

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

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

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

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