Thread: Re: [COMMITTERS] pgsql: Close previously open holes for invalidly encoded data to enter

adunstan@postgresql.org (Andrew Dunstan) writes:
> Log Message:
> The two argument form of convert() is gone,

Um ... so that means CONVERT(c USING y) now fails entirely?  That might
be going a bit far.  If we do want to get rid of that syntax then I'm
noting a lack of parser changes in this patch.
        regards, tom lane

Tom Lane wrote:
> adunstan@postgresql.org (Andrew Dunstan) writes:
>   
>> Log Message:
>> The two argument form of convert() is gone,
>>     
>
> Um ... so that means CONVERT(c USING y) now fails entirely?  That might
> be going a bit far.  If we do want to get rid of that syntax then I'm
> noting a lack of parser changes in this patch.
>
>             
>   

No, that's still there. The only change there is that it returns a 
bytea. I forgot to put that in the commit notes. :-( And the fact that 
it's still there is confirmed by the fact that we don't have regression 
failures ;-)

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
>> Um ... so that means CONVERT(c USING y) now fails entirely?  That might
>> be going a bit far.  If we do want to get rid of that syntax then I'm
>> noting a lack of parser changes in this patch.

> No, that's still there. The only change there is that it returns a 
> bytea. I forgot to put that in the commit notes. :-(

The SQL99 spec is pretty definite that it returns text.  I think we have
a problem here.
        <form-of-use conversion> ::=             CONVERT <left paren> <character value expression>
USING<form-of-use conversion name> <right paren>
 
           a) <form-of-use conversion> shall be simply contained in a             <value expression> that is
immediatelycontained in a             <derived column> that is immediately contained in a <select             sublist>
orshall immediately contain either a <simply value             specification> that is a <host parameter name> or a
<value            specification> that is a <host parameter specification>.
 
           b) A <form-of-use conversion name> shall identify a form-of-use             conversion.
           c) The declared type of the result is variable-length character             string with
implementation-definedmaximum length. The             character set of the result is the same as the character
  repertoire of the <character value expression> and form-of-             use determined by the form-of-use conversion
identifiedby             the <form-of-use conversion name>. Let CR be that character             repertoire. The result
hasthe Implicit coercibility             characteristic and its collating sequence is the default             collating
sequenceof CR.
 

OTOH we may be talking at cross-purposes --- on looking into gram.y
I see that this syntax is transformed to a call of convert_using(),
which may mean it has nothing to do with your changes.
        regards, tom lane

Tom Lane wrote:
> OTOH we may be talking at cross-purposes --- on looking into gram.y
> I see that this syntax is transformed to a call of convert_using(),
> which may mean it has nothing to do with your changes.
>
>     
>   

No, I changed convert_using - 
http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/pg_conversion.c?r1=1.36&r2=1.37

We can revert that if necessary. It will open up a hole, though. Take 
your pick - spec compliance or validly coded data.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> We can revert that if necessary. It will open up a hole, though. Take 
> your pick - spec compliance or validly coded data.

I would rather take CONVERT USING out altogether, than have an
implementation that so clearly disregards the spec as to not even return
a compatible datatype.

Other than the fact that it's supposed to return varchar, the spec's
description of what it converts to what seems about as clear as mud.
I suspect however that it can't really be implemented properly without
support for per-value (or at least per-column) encoding, which is
something we're nowhere near having.  Maybe we *should* take it out
instead of using spec-defined syntax for a behavior that we made up
out of whole cloth.
        regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>   
>> We can revert that if necessary. It will open up a hole, though. Take 
>> your pick - spec compliance or validly coded data.
>>     
>
> I would rather take CONVERT USING out altogether, than have an
> implementation that so clearly disregards the spec as to not even return
> a compatible datatype.
>
> Other than the fact that it's supposed to return varchar, the spec's
> description of what it converts to what seems about as clear as mud.
> I suspect however that it can't really be implemented properly without
> support for per-value (or at least per-column) encoding, which is
> something we're nowhere near having.  Maybe we *should* take it out
> instead of using spec-defined syntax for a behavior that we made up
> out of whole cloth.
>
>     
>   

Works for me. If there's no objection I'll start on that in a few days.

cheers

andrew

Andrew Dunstan wrote:
> 
> 
> Tom Lane wrote:
> > OTOH we may be talking at cross-purposes --- on looking into gram.y
> > I see that this syntax is transformed to a call of convert_using(),
> > which may mean it has nothing to do with your changes.
> >
> >     
> >   
> 
> No, I changed convert_using - 
> http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/pg_conversion.c?r1=1.36&r2=1.37
> 
> We can revert that if necessary. It will open up a hole, though. Take 
> your pick - spec compliance or validly coded data.

Is this a TODO?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Andrew Dunstan wrote:
>   
>> Tom Lane wrote:
>>     
>>> OTOH we may be talking at cross-purposes --- on looking into gram.y
>>> I see that this syntax is transformed to a call of convert_using(),
>>> which may mean it has nothing to do with your changes.
>>>
>>>     
>>>   
>>>       
>> No, I changed convert_using - 
>> http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/pg_conversion.c?r1=1.36&r2=1.37
>>
>> We can revert that if necessary. It will open up a hole, though. Take 
>> your pick - spec compliance or validly coded data.
>>     
>
> Is this a TODO?
>   

No, we're long past this point. We've dropped 'convert ... using' entirely.

cheers

andrew

Andrew Dunstan wrote:
> 
> 
> Bruce Momjian wrote:
> > Andrew Dunstan wrote:
> >   
> >> Tom Lane wrote:
> >>     
> >>> OTOH we may be talking at cross-purposes --- on looking into gram.y
> >>> I see that this syntax is transformed to a call of convert_using(),
> >>> which may mean it has nothing to do with your changes.
> >>>
> >>>     
> >>>   
> >>>       
> >> No, I changed convert_using - 
> >> http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/catalog/pg_conversion.c?r1=1.36&r2=1.37
> >>
> >> We can revert that if necessary. It will open up a hole, though. Take 
> >> your pick - spec compliance or validly coded data.
> >>     
> >
> > Is this a TODO?
> >   
> 
> No, we're long past this point. We've dropped 'convert ... using' entirely.

The question is whether re-adding it should be a TODO.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://postgres.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Andrew Dunstan wrote:
>> No, we're long past this point. We've dropped 'convert ... using' entirely.

> The question is whether re-adding it should be a TODO.

Not unless someone wants it and can explain the spec convincingly.
        regards, tom lane

Bruce Momjian wrote:
>>>
>>> Is this a TODO?
>>>   
>>>       
>> No, we're long past this point. We've dropped 'convert ... using' entirely.
>>     
>
> The question is whether re-adding it should be a TODO.
>   

One of the reasons we dropped it was that the spec didn't seem to make 
sense. So if there's a proposal, first it would have to wait until we 
can get sane multi-locale support and then it would have to explain how 
it actually does what the spec says.

I don't think there's a definite TODO in that.

We don't seem to have had a single squawk since we dropped it, so I 
suspect it will be little mourned.

cheers

andrew