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

Hello

related to http://www.postgresql.org/message-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?

Regards

Pavel

On Sun, Jan 13, 2013 at 07:54:11AM +0100, Pavel Stehule wrote:
> related to http://www.postgresql.org/message-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

2013/2/16 Noah Misch <noah@leadboat.com>:
> On Sun, Jan 13, 2013 at 07:54:11AM +0100, Pavel Stehule wrote:
>> related to http://www.postgresql.org/message-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

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 http://www.postgresql.org/message-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

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
http://www.postgresql.org/message-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

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

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

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.

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

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