Thread: quote_literal(integer) does not exist
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
"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
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
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
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
Hello I sent patch to pg_patches. Regards Pavel Stehule
"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
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
>
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
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
"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
"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!
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
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