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...8/msg00697.php

--
Andreas 'ads' Scherbaum
PostgreSQL User Group Germany

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"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

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

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

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

Hello

I sent patch to pg_patches.

Regards
Pavel Stehule

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

"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

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

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match