Thread: Re: [HACKERS] quote_literal with NULL
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@sss.pgh.pa.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
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@2ndquadrant.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 Mon, 2007-10-15 at 12:52 +1000, Brendan Jurd wrote: > On 10/12/07, Simon Riggs <simon@2ndquadrant.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@2ndquadrant.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
On 10/12/07, Simon Riggs <simon@2ndquadrant.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
Attachment
"Brendan Jurd" <direvus@gmail.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@sss.pgh.pa.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@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
"Brendan Jurd" <direvus@gmail.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@sss.pgh.pa.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