Thread

1. quote_literal(integer) does not exist

   Andreas 'ads' Scherbaum <adsmail@wars-nicht.de> — 2007-11-25T02:05:46Z

   Hello all,
   
   testing 8.3b3, i found out an interesting thing:
   
   we have some plpgsql functions which use quote_literal() regardless of
   the data type. With Beta 3 this does not work anymore[1].
   
   Given the fact, that some functions do a lot of work, you (or at least
   we) don't want to look, if the data you just moving around is from type
   integer, text or something else. So in the past we just quoted
   everything which worked fine.
   
   I can understand, that enforcing a strict type checking is a fine
   thing. But given the fact, that PG did a lot of implicit typecasting in
   the past, removing this is not a real world solution. This will surely
   prevent some more people from upgrading to 8.3 because the previous
   fine-working applications will stop working on 8.3.
   
   A quote_literal() which can cope with any data type, maybe combined
   with a warning, would be a better way for a smooth upgrade.
   
   
   Kind regards
   
   
   1: http://archives.postgresql.org/pgsql-hackers/2007-08/msg00697.php
   
   -- 
   				Andreas 'ads' Scherbaum
   PostgreSQL User Group Germany
   
   

2. Re: quote_literal(integer) does not exist

   Tom Lane <tgl@sss.pgh.pa.us> — 2007-11-25T02:17:39Z

   "Andreas 'ads' Scherbaum" <adsmail@wars-nicht.de> writes:
   > we have some plpgsql functions which use quote_literal() regardless of
   > the data type. With Beta 3 this does not work anymore[1].
   
   If you're unwilling to fix your application, you can hack around that
   for yourself.
   
   regression=# select quote_literal(42);   
   ERROR:  function quote_literal(integer) does not exist
   LINE 1: select quote_literal(42);
                  ^
   HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
   
   regression=# create function quote_literal(anyelement) returns text as $$
   regression$# select pg_catalog.quote_literal($1 :: pg_catalog.text)
   regression$# $$ language sql;
   CREATE FUNCTION
   
   regression=# select quote_literal(42);
    quote_literal 
   ---------------
    '42'
   (1 row)
   
   
   			regards, tom lane
   
   

3. Re: quote_literal(integer) does not exist

   Andreas 'ads' Scherbaum <adsmail@wars-nicht.de> — 2007-11-25T12:51:56Z

   On Sat, 24 Nov 2007 21:17:39 -0500 Tom Lane wrote:
   
   > "Andreas 'ads' Scherbaum" <adsmail@wars-nicht.de> writes:
   > > we have some plpgsql functions which use quote_literal() regardless of
   > > the data type. With Beta 3 this does not work anymore[1].
   > 
   > If you're unwilling to fix your application, you can hack around that
   > for yourself.
   > 
   > regression=# select quote_literal(42);   
   > ERROR:  function quote_literal(integer) does not exist
   > LINE 1: select quote_literal(42);
   >                ^
   > HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
   > 
   > regression=# create function quote_literal(anyelement) returns text as $$
   > regression$# select pg_catalog.quote_literal($1 :: pg_catalog.text)
   > regression$# $$ language sql;
   > CREATE FUNCTION
   > 
   > regression=# select quote_literal(42);
   >  quote_literal 
   > ---------------
   >  '42'
   > (1 row)
   
   Already had a similar function in my test case, but yours is more
   elegant. I also think, that we will fix our applications or at least
   most of them.
   
   But that's not the point: more people will run into this problem and
   this looks like a showstopper for updating to 8.3.
   
   By the way, the function is named quote_literal(), not quote_text().
   From my point of view i expect to get everything correctly quoted,
   what's feeded as input into this function. Given the fact, that
   previous versions accepted every input without notice about the
   implicit cast, i don't see not so much blame in the application.
   
   
   Kind regards
   
   -- 
   				Andreas 'ads' Scherbaum
   PostgreSQL User Group Germany
   
   

4. Re: quote_literal(integer) does not exist

   Brendan Jurd <direvus@gmail.com> — 2007-11-25T13:19:17Z

   On Nov 25, 2007 11:51 PM, Andreas 'ads' Scherbaum <adsmail@wars-nicht.de> wrote:
   > But that's not the point: more people will run into this problem and
   > this looks like a showstopper for updating to 8.3.
   >
   > By the way, the function is named quote_literal(), not quote_text().
   > From my point of view i expect to get everything correctly quoted,
   > what's feeded as input into this function. Given the fact, that
   > previous versions accepted every input without notice about the
   > implicit cast, i don't see not so much blame in the application.
   
   I had a similar experience to Andreas when I first migrated my app to
   8.3b1 for testing.  It wasn't hard to fix, but did seem like an
   unnecessary barrier to upgrading.
   
   I agree that the name of the function (and its behavior up till now)
   encourage users to think of it as a "quote whatever I throw at you"
   function, which is indeed what you would want/expect in the context of
   building a dynamic query.
   
   Having quote_literal take its argument as text was fine whilst we were
   automatically casting arguments, but in 8.3 it seems that
   quote_literal(anyelement) makes a lot more sense than
   quote_literal(text).
   
   So, I wonder why we don't just adapt the internal function to take
   anyelement?  It would save a lot of apps from being broken by the move
   to 8.3, and make the function more convenient.
   
   Regards,
   BJ
   
   

5. Re: quote_literal(integer) does not exist

   Pavel Stehule <pavel.stehule@gmail.com> — 2007-11-25T14:02:31Z

   Hello
   
   >
   > Having quote_literal take its argument as text was fine whilst we were
   > automatically casting arguments, but in 8.3 it seems that
   > quote_literal(anyelement) makes a lot more sense than
   > quote_literal(text).
   >
   > So, I wonder why we don't just adapt the internal function to take
   > anyelement?  It would save a lot of apps from being broken by the move
   > to 8.3, and make the function more convenient.
   >
   
   It's good idea. I'll look on it.
   
   It needs change of pgproc :(
   
   Pavel
   
   

6. Re: quote_literal(integer) does not exist

   Pavel Stehule <pavel.stehule@gmail.com> — 2007-11-25T17:17:26Z

   Hello
   
   I sent patch to pg_patches.
   
   Regards
   Pavel Stehule
   
   

7. Re: quote_literal(integer) does not exist

   Tom Lane <tgl@sss.pgh.pa.us> — 2007-11-25T18:23:34Z

   "Brendan Jurd" <direvus@gmail.com> writes:
   > So, I wonder why we don't just adapt the internal function to take
   > anyelement?
   
   The main argument against is the "slippery slope" one: once we accept
   this, what else?  The entire point of that change was to make people
   be aware of where they are inducing text coercions, and deciding to
   hide that again on the basis of individual complaints is no way to
   design a system.
   
   As a not-too-far-away example, I see that the proposed patch Pavel
   sent in arbitrarily decides to change quote_ident() too, which was
   not asked for and has got much less justification than changing
   quote_literal().  That sort of cowboy approach to semantics is not
   the way to proceed.
   
   Another issue is that changing pg_proc.h without forcing initdb
   is not good practice.  We are far enough along in the beta cycle
   that we shouldn't force initdb lightly, and I definitely *don't*
   want to do it again next week when someone else comes up with
   some other "must have" auto-coercing function.  If anyone wants
   to make a serious argument for this, look through the whole of
   pg_proc.h, see what else needs to be changed at the same time,
   and make a coherent proposal.
   
   			regards, tom lane
   
   

8. Re: quote_literal(integer) does not exist

   Pavel Stehule <pavel.stehule@gmail.com> — 2007-11-25T19:31:11Z

   On 25/11/2007, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > "Brendan Jurd" <direvus@gmail.com> writes:
   > > So, I wonder why we don't just adapt the internal function to take
   > > anyelement?
   >
   > The main argument against is the "slippery slope" one: once we accept
   > this, what else?  The entire point of that change was to make people
   > be aware of where they are inducing text coercions, and deciding to
   > hide that again on the basis of individual complaints is no way to
   > design a system.
   >
   
   I know so is too late now. This patch can be saved for 8.4.
   
   There is one reason for patching. It is consistence with || operator.
   That's all. This patch doesn't change casting rules, and these rules
   are strict still (what is good). But quote_literal is more natural
   with anyelement parameter, than only text element. There isn't any
   reason for some limit. This discus is not about change rules, but
   about change of definition of some functions.
   
   > As a not-too-far-away example, I see that the proposed patch Pavel
   > sent in arbitrarily decides to change quote_ident() too, which was
   > not asked for and has got much less justification than changing
   > quote_literal().  That sort of cowboy approach to semantics is not
   > the way to proceed.
   >
   
   Reason is same. Consistence with || operator. But, equivalent in SQL is:
   
   CREATE OR REPLACE FUNCTION quote_ident(anyelement)
   RETURNS text AS
   SELECT CASE when $1::text LIKE '% %' <- first problem
                 THEN '"' || $1 || '"'
                 ELSE $1::text END             <- second problem
   $$ LANGUAGE SQL;
   
   so, I see, with quote_ident I was wrong. I belive so with anyelement
   can be more usable, but it is different than quote_literal.
   
   > Another issue is that changing pg_proc.h without forcing initdb
   > is not good practice.  We are far enough along in the beta cycle
   > that we shouldn't force initdb lightly, and I definitely *don't*
   > want to do it again next week when someone else comes up with
   > some other "must have" auto-coercing function.  If anyone wants
   > to make a serious argument for this, look through the whole of
   > pg_proc.h, see what else needs to be changed at the same time,
   > and make a coherent proposal.
   >
   
   probably quote_literal is more important than others. It is wide used
   with dynamic selects. On other side, any similar problem can be simple
   solved with custom wrapper (with some note in release notes).
   
   I found more important problem. I cannot simply use literal in
   polymorphic function. I cannot call anyfce('literal') what is
   acceptable in SQL or plpgsql languages, but not in C language. Sure,
   this topic is for 8.4.
   
   nice a day
   Pavel Stehule
   
   >                         regards, tom lane
   >
   > ---------------------------(end of broadcast)---------------------------
   > TIP 1: if posting/reading through Usenet, please send an appropriate
   >        subscribe-nomail command to majordomo@postgresql.org so that your
   >        message can get through to the mailing list cleanly
   >
   
   

9. Re: quote_literal(integer) does not exist

   Brendan Jurd <direvus@gmail.com> — 2007-11-25T19:35:20Z

   On Nov 26, 2007 5:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
   > "Brendan Jurd" <direvus@gmail.com> writes:
   > > So, I wonder why we don't just adapt the internal function to take
   > > anyelement?
   >
   > The main argument against is the "slippery slope" one: once we accept
   > this, what else?  The entire point of that change was to make people
   > be aware of where they are inducing text coercions, and deciding to
   > hide that again on the basis of individual complaints is no way to
   > design a system.
   
   I'm all for the idea of making people conscious of text coercions in
   general, but in the *particular* case of quote_literal, having it only
   accept text is undesirable, unintuitive and most importantly, it will
   break apps which otherwise may have been able to enjoy a smooth
   transition to 8.3.
   
   I would argue that quote_literal should have been set up to accept
   anyelement in the very first place, and I'd guess that the original
   choice of text as an argument type was partially driven by the
   understanding that everything gets coerced to text, making it a de
   facto "anyelement" substitute.  Or maybe anyelement wasn't available
   when it was introduced.  Either way, if quote_literal() is all about
   safely stuffing variables into dynamic queries, the new behaviour is a
   regression.  In context, it makes perfect sense to throw integers,
   numerics and whatever else at quote_literal and expect it to Just
   Work.
   
   My feeling is that the change in text coercion behaviour has well
   illuminated that the text argument type for quote_literal isn't ideal.
    Great!  Let's fix it.
   
   > As a not-too-far-away example, I see that the proposed patch Pavel
   > sent in arbitrarily decides to change quote_ident() too, which was
   > not asked for and has got much less justification than changing
   > quote_literal().  That sort of cowboy approach to semantics is not
   > the way to proceed.
   
   I'd pass on changing quote_ident.  It seems natural for it to take a
   text argument.  I can imagine a lot of people using, say,
   quote_literal(int) in the field; I can't imagine the same for
   quote_ident.
   
   > Another issue is that changing pg_proc.h without forcing initdb
   > is not good practice.  We are far enough along in the beta cycle
   > that we shouldn't force initdb lightly, and I definitely *don't*
   > want to do it again next week when someone else comes up with
   > some other "must have" auto-coercing function.  If anyone wants
   > to make a serious argument for this, look through the whole of
   > pg_proc.h, see what else needs to be changed at the same time,
   > and make a coherent proposal.
   
   I took your suggestion and looked through all the procs that take a
   text argument.  I honestly didn't see anything else I thought needed
   to change.
   
   So my proposal is to add your quote_literal(anyelement) SQL function
   to pg_proc and be done with it.
   
   I can see your reluctance to force an initdb, but what's the greater
   mischief; forcing initdb in beta, or breaking applications on release?
    My personal perspective is that it's an easy choice ... avoid
   breaking the apps, that's what betas are for.
   
   Thanks for your time,
   BJ
   
   

10. Re: quote_literal(integer) does not exist

    Andreas 'ads' Scherbaum <adsmail@wars-nicht.de> — 2007-11-25T22:35:46Z

    On Mon, 26 Nov 2007 06:35:20 +1100 Brendan Jurd wrote:
    
    > On Nov 26, 2007 5:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    > 
    > I'm all for the idea of making people conscious of text coercions in
    > general, but in the *particular* case of quote_literal, having it only
    > accept text is undesirable, unintuitive and most importantly, it will
    > break apps which otherwise may have been able to enjoy a smooth
    > transition to 8.3.
    > 
    > I would argue that quote_literal should have been set up to accept
    > anyelement in the very first place, and I'd guess that the original
    > choice of text as an argument type was partially driven by the
    > understanding that everything gets coerced to text, making it a de
    > facto "anyelement" substitute.  Or maybe anyelement wasn't available
    > when it was introduced.  Either way, if quote_literal() is all about
    > safely stuffing variables into dynamic queries, the new behaviour is a
    > regression.  In context, it makes perfect sense to throw integers,
    > numerics and whatever else at quote_literal and expect it to Just
    > Work.
    
    The problem for me is: we expect and encourage people to do safe
    programming and now they have to debug their programs and remove 
    some of the "safe" parts just to make PostgreSQL happy.
    As you said, that is not, what the average programmer expect.
    
    
    
    > My feeling is that the change in text coercion behaviour has well
    > illuminated that the text argument type for quote_literal isn't ideal.
    >  Great!  Let's fix it.
    
    Yes, Tom Lane is right that the current behavior is broken. But the
    solution cannot be to exclude anything beside text but instead we
    should move forward to accept anything (at least, if it's possible).
    
    
    
    > > As a not-too-far-away example, I see that the proposed patch Pavel
    > > sent in arbitrarily decides to change quote_ident() too, which was
    > > not asked for and has got much less justification than changing
    > > quote_literal().  That sort of cowboy approach to semantics is not
    > > the way to proceed.
    > 
    > I'd pass on changing quote_ident.  It seems natural for it to take a
    > text argument.  I can imagine a lot of people using, say,
    > quote_literal(int) in the field; I can't imagine the same for
    > quote_ident.
    
    True. You can't even create a table who's name is just an integer or
    where the name starts with an integer, so in any way you already have
    to use quotes and you are aware of the problem.
    
    
    > I can see your reluctance to force an initdb, but what's the greater
    > mischief; forcing initdb in beta, or breaking applications on release?
    >  My personal perspective is that it's an easy choice ... avoid
    > breaking the apps, that's what betas are for.
    
    Yeah, that's what a beta is for. We don't expect to have people running
    production systems with beta software so it needs an reinstall anyway
    after the release.
    
    
    Kind regards
    
    -- 
    				Andreas 'ads' Scherbaum
    PostgreSQL User Group Germany
    
    

11. Re: quote_literal(integer) does not exist

    Tom Lane <tgl@sss.pgh.pa.us> — 2007-11-26T01:55:45Z

    "Brendan Jurd" <direvus@gmail.com> writes:
    > On Nov 26, 2007 5:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    >> ... If anyone wants
    >> to make a serious argument for this, look through the whole of
    >> pg_proc.h, see what else needs to be changed at the same time,
    >> and make a coherent proposal.
    
    > I took your suggestion and looked through all the procs that take a
    > text argument.  I honestly didn't see anything else I thought needed
    > to change.
    
    > So my proposal is to add your quote_literal(anyelement) SQL function
    > to pg_proc and be done with it.
    
    I did the same search.  Ignoring cases where it's fairly obvious that
    you're trying to apply a textual operation to something non-textual
    (eg, LIKE and btrim(), though both of these have been complained of
    since beta started...), it seems that quote_literal() has a good case,
    and you could also make an argument for allowing a non-text second
    argument for set_config(), since these things used to work:
    
    regression=# select set_config('work_mem', 1000, false);
     set_config 
    ------------
     1000kB
    (1 row)
    
    regression=# select set_config('random_page_cost', 2.5, false);
     set_config 
    ------------
     2.5
    (1 row)
    
    I don't find that amazingly compelling, but it's not silly either.
    If we were to do this I think I'd introduce set_config(text,float8,bool)
    rather than going all the way with anyelement, though.  That would be
    enough to cover both the int and float cases, as well as anyone who
    likes to spell booleans as 1 and 0.
    
    I don't offhand see anything else I'd consider weakening the casting
    rules for.  If anyone else is interested, I took
    
    select p.oid::regprocedure as regprocedure, oprname from pg_proc p left
    join pg_operator o on (oprcode = p.oid) where 25 = any (proargtypes)
    
    and removed duplicates and obviously-internal functions such as textout,
    leaving me with this list:
    
                           regprocedure                        | oprname 
    -----------------------------------------------------------+---------
     array_to_string(anyarray,text)                            | 
     btrim(text)                                               | 
     btrim(text,text)                                          | 
     character_length(text)                                    | 
     convert_to(text,name)                                     | 
     initcap(text)                                             | 
     length(text)                                              | 
     lower(text)                                               | 
     lpad(text,integer)                                        | 
     lpad(text,integer,text)                                   | 
     ltrim(text)                                               | 
     ltrim(text,text)                                          | 
     md5(text)                                                 | 
     octet_length(text)                                        | 
     overlay(text,text,integer)                                | 
     overlay(text,text,integer,integer)                        | 
     position(text,text)                                       | 
     quote_ident(text)                                         | 
     quote_literal(text)                                       | 
     regexp_matches(text,text)                                 | 
     regexp_matches(text,text,text)                            | 
     regexp_replace(text,text,text)                            | 
     regexp_replace(text,text,text,text)                       | 
     regexp_split_to_array(text,text)                          | 
     regexp_split_to_array(text,text,text)                     | 
     regexp_split_to_table(text,text)                          | 
     regexp_split_to_table(text,text,text)                     | 
     repeat(text,integer)                                      | 
     replace(text,text,text)                                   | 
     rpad(text,integer)                                        | 
     rpad(text,integer,text)                                   | 
     rtrim(text)                                               | 
     rtrim(text,text)                                          | 
     set_config(text,text,boolean)                             | 
     split_part(text,text,integer)                             | 
     string_to_array(text,text)                                | 
     strpos(text,text)                                         | 
     substr(text,integer)                                      | 
     substr(text,integer,integer)                              | 
     substring(text,integer)                                   | 
     substring(text,integer,integer)                           | 
     substring(text,text)                                      | 
     substring(text,text,text)                                 | 
     texticlike(text,text)                                     | ~~*
     texticnlike(text,text)                                    | !~~*
     texticregexeq(text,text)                                  | ~*
     texticregexne(text,text)                                  | !~*
     textlike(text,text)                                       | ~~
     textnlike(text,text)                                      | !~~
     textregexeq(text,text)                                    | ~
     textregexne(text,text)                                    | !~
     upper(text)                                               | 
    
    
    Thoughts?
    
    			regards, tom lane
    
    

12. Re: quote_literal(integer) does not exist

    Gregory Stark <stark@enterprisedb.com> — 2007-11-26T03:15:18Z

    "Tom Lane" <tgl@sss.pgh.pa.us> writes:
    
    > Thoughts?
    
    I wouldn't have thought set_config was frequently enough used to warrant a
    catalog change but quote_literal does seem like a common enough use case to
    warrant it and once we're doing it there...
    
    I started to think md5(bytea) would be reasonable but then I checked and we do
    in fact already have that. Perhaps it would be interesting to have some of the
    other functions like overlay or replace for bytea as well in the future but I
    hardly think anyone is going to complain urgently given that using them via
    free casts to text would never have really worked that well previously.
    
    -- 
      Gregory Stark
      EnterpriseDB          http://www.enterprisedb.com
      Ask me about EnterpriseDB's RemoteDBA services!
    
    

13. Re: quote_literal(integer) does not exist

    Andrew Dunstan <andrew@dunslane.net> — 2007-11-26T04:58:51Z

    
    Tom Lane wrote:
    
    [...]
    >  it seems that quote_literal() has a good case,
    > and you could also make an argument for allowing a non-text second
    > argument for set_config()
    >
    >   
    [...]
    > Thoughts?
    >
    > 	
    >   
    
    I think there is just enough of  a case for quote_literal(), although in 
    my experience the vast majority of places where it is used in fact 
    should be replaced by using placeholders. I find that I need to use it 
    very rarely indeed.
    
    The case for set_config() is less compelling.
    
    cheers
    
    andrew
    
    

14. Re: quote_literal(integer) does not exist

    Michael Paesold <mpaesold@gmx.at> — 2007-11-29T08:57:19Z

    Tom Lane wrote:
    > I don't offhand see anything else I'd consider weakening the casting
    > rules for.  If anyone else is interested, I took
    ...
    >  substring(text,integer)                                   | 
    >  substring(text,integer,integer)                           | 
    >  substring(text,text)                                      | 
    >  substring(text,text,text)                                 | 
    >  texticlike(text,text)                                     | ~~*
    >  texticnlike(text,text)                                    | !~~*
    >  texticregexeq(text,text)                                  | ~*
    >  texticregexne(text,text)                                  | !~*
    >  textlike(text,text)                                       | ~~
    >  textnlike(text,text)                                      | !~~
    >  textregexeq(text,text)                                    | ~
    >  textregexne(text,text)                                    | !~
    >  upper(text)                                               | 
    > 
    > 
    > Thoughts?
    
    In one of our applications, we have some "numbers" (e.g. product 
    numbers) that have meaning attached to individual digits. Product 
    numbers usually contain letters, too, but for historical reasons they do 
    not in this application. So we put them into integer columns for 
    efficiency. We still want to run queries like product_no LIKE '51%' on them.
    
    Well, for the application, I don't see much of a problem here. This will 
    probably cost 3-5 lines of code in the whole application. It will just 
    cause some inconvenience when working with psql interactively.
    
    And I have not yet seen another DBMS that does not accept almost any 
    input type for the typical string operations such as substring or LIKE. 
    It feels a little bit strange that I will have to do all that 
    typecasting now.
    
    Just my $0.02.
    
    Best Regards
    Michael Paesold