Re: [HACKERS] quote_literal with NULL

Hi hackers,

I note that if you pass NULL to quote_literal(), you get NULL.

This isn't surprising, but I was thinking that the stated purpose of
quote_literal is preparing the argument for entry into a dynamic SQL
statement. In this context, it fails for NULL input.

Wouldn't it be more useful if quote_literal(NULL) yielded the text value 'NULL'?

With the current behaviour, if you want quote_literal to be "null
safe" you have to replace any such calls with
coalesce(quote_literal(foo), 'NULL')). Since the use case for
quote_literal is concatenating the result with some other text, a NULL
return seems guaranteed to be unhelpful.

Meanwhile, the string 'NULL' is the only way of representing a NULL in
SQL, so it makes sense (to me) that this is what quote_literal should
output.

Comments?

Cheers,
BJ

On Wed, 2007-10-10 at 14:57 +1000, Brendan Jurd wrote:

> Wouldn't it be more useful if quote_literal(NULL) yielded the text value 'NULL'?

I don't think you can change that now. There could be code out there
that relies on that behaviour.

It isn't very helpful to return the word NULL in many cases, since the
WHERE clause "col = NULL" does not do the same thing as "col is NULL".
So you need to know about NULL values and how to handle them in many
cases.

It might be useful to define a new text concatenation operator ||| that
treats NULL values as zero-length strings, so that
'help ' ||| NULL ||| 'me' returns 'help me'

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

On 10/10/07, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, 2007-10-10 at 14:57 +1000, Brendan Jurd wrote:
>
> > Wouldn't it be more useful if quote_literal(NULL) yielded the text value 'NULL'?
>
> I don't think you can change that now. There could be code out there
> that relies on that behaviour.
>

Bummer. But I take your point. If there's a good chance someone is
going to have their application murdered by a change here, best to
leave it alone.

I've already gotten around this in my own apps by adding a UDF
alternative to quote_literal that plays nicely with NULLs, but thought
I'd mention it here in case others were of the same mind.

> It isn't very helpful to return the word NULL in many cases, since the
> WHERE clause "col = NULL" does not do the same thing as "col is NULL".
> So you need to know about NULL values and how to handle them in many
> cases.
>

Well if you're expecting a possibly-NULL value in your dynamic query
you're going to be using something like 'WHERE foo IS NOT DISTINCT
FROM ' || quote_literal(bar) anyway.

Either way possibly-NULL values need to be anticipated and treated
specially. With the string 'NULL' you need DISTINCT FROM. With an
actual NULL you need COALESCE. It just seemed to me that the string
'NULL' result was more in line with what quote_literal was supposed to
do; and leads to less cluttered code.

> It might be useful to define a new text concatenation operator ||| that
> treats NULL values as zero-length strings, so that
> 'help ' ||| NULL ||| 'me' returns 'help me'
>

That could be cool. Not immediately practical for the dynamic query
scenario though: If I do 'WHERE foo IS NOT DISTINCT FROM ' |||
quote_literal(bar) it'll still give me an invalid query string if bar
is NULL.

Cheers,
BJ

>>> On Wed, Oct 10, 2007 at 4:57 AM, in message
<37ed240d0710100257r149a8d2cmb671b69a1673eb54(at)mail(dot)gmail(dot)com>, "Brendan Jurd"
<direvus(at)gmail(dot)com> wrote:
> On 10/10/07, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On Wed, 2007-10-10 at 14:57 +1000, Brendan Jurd wrote:
>>
>> > Wouldn't it be more useful if quote_literal(NULL) yielded the text value
> 'NULL'?
>>
>> I don't think you can change that now. There could be code out there
>> that relies on that behaviour.
>>
>
> Bummer. But I take your point. If there's a good chance someone is
> going to have their application murdered by a change here, best to
> leave it alone.

In particular, it seems like exactly what you would want for values
you're going to use in an INSERT or the SET clause of an UPDATE.

-Kevin

On Wed, 2007-10-10 at 10:12 -0500, Kevin Grittner wrote:
> >>> On Wed, Oct 10, 2007 at 4:57 AM, in message
> <37ed240d0710100257r149a8d2cmb671b69a1673eb54(at)mail(dot)gmail(dot)com>, "Brendan Jurd"
> <direvus(at)gmail(dot)com> wrote:
> > On 10/10/07, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> On Wed, 2007-10-10 at 14:57 +1000, Brendan Jurd wrote:
> >>
> >> > Wouldn't it be more useful if quote_literal(NULL) yielded the text value
> > 'NULL'?
> >>
> >> I don't think you can change that now. There could be code out there
> >> that relies on that behaviour.
> >>
> >
> > Bummer. But I take your point. If there's a good chance someone is
> > going to have their application murdered by a change here, best to
> > leave it alone.
>
> In particular, it seems like exactly what you would want for values
> you're going to use in an INSERT or the SET clause of an UPDATE.

Perhaps have quote_nullable() then as well?

You then use quote_nullable() in INSERT and UPDATE SET clauses and
quote_literal() in SELECT WHERE clauses.

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> Perhaps have quote_nullable() then as well?
>
> You then use quote_nullable() in INSERT and UPDATE SET clauses and
> quote_literal() in SELECT WHERE clauses.

I still don't see the use case. Wouldn't your app still need to check
for nullability anyway, to avoid " = NULL"? (Aside: seems to me that
SET foo = NULL; really should be SET foo TO NULL; to be consistent
with WHERE foo IS NULL;)

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200710101221
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iD8DBQFHDPwyvJuQZxSWSsgRAwGaAJ92ICR+MyclkmWvJRkC4vazIw+b0ACghpZt
WXbCxe0abFlp8jwr0ol/fac=
=oWqD
-----END PGP SIGNATURE-----

On Oct 10, 2007, at 11:24 , Greg Sabino Mullane wrote:

> (Aside: seems to me that
> SET foo = NULL; really should be SET foo TO NULL; to be consistent
> with WHERE foo IS NULL;)

The = character has different meanings in these two cases.

UPDATE foos
SET foo = NULL -- assignment
WHERE bar IS NULL -- comparison
AND foo = 'ignore me' -- comparison

Or is that what the smiley was about? :)

Michael Glaesemann
grzm seespotcode net

"Greg Sabino Mullane" <greg(at)turnstep(dot)com> writes:
>> Perhaps have quote_nullable() then as well?
>>
>> You then use quote_nullable() in INSERT and UPDATE SET clauses and
>> quote_literal() in SELECT WHERE clauses.

> I still don't see the use case. Wouldn't your app still need to check
> for nullability anyway, to avoid " = NULL"?

Well, it's clearly useful in INSERT and UPDATE. For WHERE cases, you
might or might not be able to use it, but I note that quote_nullable()
would work much more like what happens if you use a parameter symbol
and then bind NULL as the actual parameter value ...

In hindsight we should probably have done quote_literal the way the OP
suggests, but I concur that it's too late to change it. An additional
function seems a reasonable compromise.

regards, tom lane

On 10/11/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Well, it's clearly useful in INSERT and UPDATE. For WHERE cases, you
> might or might not be able to use it, but I note that quote_nullable()
> would work much more like what happens if you use a parameter symbol
> and then bind NULL as the actual parameter value ...
>
> In hindsight we should probably have done quote_literal the way the OP
> suggests, but I concur that it's too late to change it. An additional
> function seems a reasonable compromise.

quote_nullable() works for me. I'll write up a patch.

Cheers,
BJ

Hi patchers,

Per discussion on -hackers, I've implemented a new internal function
quote_nullable, as an alternative to quote_literal. The difference is
that quote_nullable returns the text value 'NULL' on NULL input, which
is suitable for insertion into an SQL statement.

The idea is that when you're writing a plpgsql function with dynamic
queries, you can use quote_nullable for values which are
possibly-null. You're still responsible for handling NULLs sensibly
within your query, but at least you get a syntactically valid SQL
statement.

I've included doc updates but no new regression tests. I did not add
tests because there are currently no tests for quote_literal and when
I recently suggested addition of tests for quote_ident [1] they were
rejected. I still don't fully understand the criteria for inclusion
of regression tests, but this is a similar situation, so I'm following
the same guidance.

Patch compiles cleanly and passes make check on x86 gentoo.

Thanks for your time,
BJ

[1] http://archives.postgresql.org/pgsql-patches/2007-10/msg00080.php

On 10/11/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Well, it's clearly useful in INSERT and UPDATE. For WHERE cases, you
> might or might not be able to use it, but I note that quote_nullable()
> would work much more like what happens if you use a parameter symbol
> and then bind NULL as the actual parameter value ...
>
> In hindsight we should probably have done quote_literal the way the OP
> suggests, but I concur that it's too late to change it. An additional
> function seems a reasonable compromise.

On Fri, 2007-10-12 at 02:11 +1000, Brendan Jurd wrote:

> Per discussion on -hackers, I've implemented a new internal function
> quote_nullable, as an alternative to quote_literal. The difference is
> that quote_nullable returns the text value 'NULL' on NULL input, which
> is suitable for insertion into an SQL statement.

Patch looks fine.

> The idea is that when you're writing a plpgsql function with dynamic
> queries, you can use quote_nullable for values which are
> possibly-null. You're still responsible for handling NULLs sensibly
> within your query, but at least you get a syntactically valid SQL
> statement.
>
> I've included doc updates but no new regression tests.

I think you should add some examples to show how we would handle an
INSERT or an UPDATE SET with quite_nullable() and a SELECT WHERE clause
with quote_literal. The difference is a subtle one, which is why nobody
mentioned it before, so it needs some better docs too.

A cross-ref to the functions page would help also.

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

On 10/12/07, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> I think you should add some examples to show how we would handle an
> INSERT or an UPDATE SET with quite_nullable() and a SELECT WHERE clause
> with quote_literal. The difference is a subtle one, which is why nobody
> mentioned it before, so it needs some better docs too.
>
> A cross-ref to the functions page would help also.

Thanks for your comments Simon. I agree about the doco, and will send
in an updated patch soon.

Looking at the patch again, I was thinking; is there actually any
point having separate underlying C functions for quote_nullable and
quote_literal? If I merged the functions together, and pointed both
pg_proc entries at the one combined function wouldn't it have the same
effect?

Perhaps having the separate function makes the intent of the code more
obvious, but looking at the patch with fresh eyes I'm thinking it's
mostly a waste of space.

Cheers,
BJ

On 10/12/07, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> I think you should add some examples to show how we would handle an
> INSERT or an UPDATE SET with quite_nullable() and a SELECT WHERE clause
> with quote_literal. The difference is a subtle one, which is why nobody
> mentioned it before, so it needs some better docs too.
>
> A cross-ref to the functions page would help also.

Alright, I've improved the documentation along the lines suggested by
Simon. There's a full example on doing a null-safe dynamic UPDATE, as
well as a brief discussion about being wary of using comparison
operators with NULLs (e.g., in WHERE clauses). Cross references
abound.

I did make a version of the patch which has the pg_proc entries for
quote_literal and quote_nullable both pointing to the same internal
function. I thought this was a tidier solution, but it failed
regression test #5 in opr_sanity; apparently two entries in pg_proc
can't have the same prosrc and differing proisstrict?

Cheers,
BJ

On Mon, 2007-10-15 at 12:52 +1000, Brendan Jurd wrote:
> On 10/12/07, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > I think you should add some examples to show how we would handle an
> > INSERT or an UPDATE SET with quite_nullable() and a SELECT WHERE clause
> > with quote_literal. The difference is a subtle one, which is why nobody
> > mentioned it before, so it needs some better docs too.
> >
> > A cross-ref to the functions page would help also.
>
> Alright, I've improved the documentation along the lines suggested by
> Simon. There's a full example on doing a null-safe dynamic UPDATE, as
> well as a brief discussion about being wary of using comparison
> operators with NULLs (e.g., in WHERE clauses). Cross references
> abound.

Cool

> I did make a version of the patch which has the pg_proc entries for
> quote_literal and quote_nullable both pointing to the same internal
> function. I thought this was a tidier solution, but it failed
> regression test #5 in opr_sanity; apparently two entries in pg_proc
> can't have the same prosrc and differing proisstrict?

Sanity prevails, I guess. :-)

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

On 10/15/07, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > I did make a version of the patch which has the pg_proc entries for
> > quote_literal and quote_nullable both pointing to the same internal
> > function. I thought this was a tidier solution, but it failed
> > regression test #5 in opr_sanity; apparently two entries in pg_proc
> > can't have the same prosrc and differing proisstrict?
>
> Sanity prevails, I guess. :-)
>

I'm all for the prevalance of sanity, but I'm not really clear on what
about the above scenario is not sane.

Suspect I'm missing something about the workings of pg_proc, but from
over here it seems like having a strict and a non-strict version of
the same function would be okay. As long as the internal function is
rigged to handle null input properly, what's the problem?

It's only tangential to the patch itself, and I'm not challenging the
regression test. Just curious about it.

Cheers,
BJ

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> I'm all for the prevalance of sanity, but I'm not really clear on what
> about the above scenario is not sane.

Well, a situation like that just calls into question whether there's
been a mistake --- in particular whether the underlying function is
really null-safe or not.

We could remove the opr_sanity test, but to me that'd be akin to
disabling a compiler warning. Our project policy has generally been
to write warning-free code, instead.

regards, tom lane

This has been saved for the 8.4 release:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Brendan Jurd wrote:
> Hi patchers,
>
> Per discussion on -hackers, I've implemented a new internal function
> quote_nullable, as an alternative to quote_literal. The difference is
> that quote_nullable returns the text value 'NULL' on NULL input, which
> is suitable for insertion into an SQL statement.
>
> The idea is that when you're writing a plpgsql function with dynamic
> queries, you can use quote_nullable for values which are
> possibly-null. You're still responsible for handling NULLs sensibly
> within your query, but at least you get a syntactically valid SQL
> statement.
>
> I've included doc updates but no new regression tests. I did not add
> tests because there are currently no tests for quote_literal and when
> I recently suggested addition of tests for quote_ident [1] they were
> rejected. I still don't fully understand the criteria for inclusion
> of regression tests, but this is a similar situation, so I'm following
> the same guidance.
>
> Patch compiles cleanly and passes make check on x86 gentoo.
>
> Thanks for your time,
> BJ
>
> [1] http://archives.postgresql.org/pgsql-patches/2007-10/msg00080.php
>
> On 10/11/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Well, it's clearly useful in INSERT and UPDATE. For WHERE cases, you
> > might or might not be able to use it, but I note that quote_nullable()
> > would work much more like what happens if you use a parameter symbol
> > and then bind NULL as the actual parameter value ...
> >
> > In hindsight we should probably have done quote_literal the way the OP
> > suggests, but I concur that it's too late to change it. An additional
> > function seems a reasonable compromise.

[ Attachment, skipping... ]

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

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> [ second version of quote_nullable patch ]

Applied with some revisions to sync it with CVS HEAD --- primarily,
since we now have a quote_literal(anyelement) function, it seemed
important to add a quote_nullable(anyelement) variant. I also
editorialized on the documentation example a bit.

regards, tom lane

On 23/03/2008, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Applied with some revisions to sync it with CVS HEAD --- primarily,
> since we now have a quote_literal(anyelement) function, it seemed
> important to add a quote_nullable(anyelement) variant. I also
> editorialized on the documentation example a bit.
>

Thanks Tom, good call on the (anyelement) version.

I like the changes to the documentation too. I didn't know that the
DISTINCT FROM operator was relatively slow.

Regards,
BJ