Thread: Last minute mini-proposal (I know, I know) for PQexecf()
While cleaning up some pg_migrator code (<a href="http://pgfoundry.org/projects/pg-migrator/)">http://pgfoundry.org/projects/pg-migrator/)</a>it occurred to me thata typical libpq client application spends a lot of code constructing SQL commands. The code typically looks like this:<br/><br /> a) allocate enough room to hold the command <br /> b) sprintf( command, text, argument,argument, argument, ... )<br /> c) PQexec( conn, command )<br /> d) free( command )<br /><br /> In mostcases, the amount of memory that you allocate in step a) is just an educated guess. It's typically more room than youneed, occassionally less room than you need (and you get a buffer overflow exploit), and it's rarely maintained properlywhen you modify the command text (or the argument list).<br /><br /> I'd like to see a new variant on PQexec():<br/><br /> PGresult * PQexecf(PGconn *conn, const char *fmt, ...);<br /><br /> PQexecf() simply performs stepsa, b, c, and d for you. And you call it like this:<br /><br /> PQexecf( conn, text, argument, argument, argument,... )<br /><br /> PQexecf() is just a wrapper around the already existing createPQExpBuffer(), enlargePQExpBuffer(),printfPQExpBuffer(), and PQexec() so it introduces no new code (other than assembling the wrapper) anddoesn't change any existing code. PQexecf() is similar to PQexecParams() but it much simpler to use (and should be veryfamiliar to C programmers). PQexecf() is not intended as a replacement for PQprepare() and PQexecPrepared() - you shoulduse prepare/exec when you want to execute a command many times.<br /><br /> I could eliminate a lot of client-sidecode if PQexecf() were available - and the code that I could remove is the code that's most likely to be buggyand least likely to be properly maintained.<br /><br /> I've thrown together an UNTESTED prototype (below), just toget the idea across - you'll recognize that most of this code is identical to printPQExpBuffer(). In the prototype, I'mkeeping a static PQExpBuffer that grows to the hold the largest string ever required by the client application - thatpart seems to be a point for discussion, but since the detail is hidden in the implementation, we could adjust the codelater if necessary (without changing the interface).<br /><br /> Of course, I could include an implementation of PQexecf()in each of my client applications if it were not available in libpq, but that would be silly and I'd have to inventmy own createPQExpBuffer() / enlargePQExpBuffer() code since those functions are not an official part of libpq (andwon't even be available to a Win32 client application).<br /><br /> Is it just too late to even think about this for8.3? (Bruce laughed at me when I suggested the idea :-) <br /><br /> -- Korry<br /> <a href="mailto:korryd@enterprisedb.com">korryd@enterprisedb.com</a><br/> <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/><br /><hr noshade="NOSHADE" /><br /> PGresult *<br/> PQexecf(PGconn *conn, const char *fmt, ...)<br /> {<br /> static PQExpBuffer str;<br /> va_list args;<br /><br/> if (str == NULL)<br /> str = createPQExpBuffer();<br /><br /> for (;;)<br /> {<br /> /*<br /> * Try to formatthe given string into the available space; but if<br /> * there's hardly any space, don't bother trying, just fallthrough to<br /> * enlarge the buffer first.<br /> */<br /> if (str->maxlen > str->len + 16)<br /> {<br /> size_tavail = str->maxlen - str->len - 1;<br /> int nprinted;<br /><br /> va_start(args, fmt);<br /> nprinted =vsnprintf(str->data + str->len, avail, fmt, args);<br /> va_end(args);<br /><br /> /*<br /> * Note: some versionsof vsnprintf return the number of chars<br /> * actually stored, but at least one returns -1 on failure. Be<br />* conservative about believing whether the print worked.<br /> */<br /> if (nprinted >= 0 && nprinted < (int)avail - 1)<br /> {<br /> /* Success. Note nprinted does not include trailing null. */<br /> str->len += nprinted;<br/> break;<br /> }<br /> }<br /> /* Double the buffer size and try again. */<br /> if (!enlargePQExpBuffer(str,str->maxlen))<br /> return PQmakeEmptyPGresult(conn, PGRES_FATAL_ERROR); /* oops, out of memory*/<br /> }<br /><br /> return PQexec(conn, str->data);<br /> }<br /><br />
<korryd@enterprisedb.com> writes:
> I'd like to see a new variant on PQexec():
> PGresult * PQexecf(PGconn *conn, const char *fmt, ...);
Way too late for 8.3 --- if we were going to do something like this,
we should think first and program later. In particular, blindly
adopting the sprintf format string definition doesn't seem very helpful.
The sorts of escapes I'd want to have are "properly quoted SQL
identifier", "properly quoted SQL literal", etc. A large fraction of
what sprintf knows about is more or less irrelevant to the task of
creating SQL commands.
Also, how does this interact with parameterized or prepared commands?
If we wanted PQexecf we'd soon want PQexecParamsf, etc. I don't think
we really want so much duplicate logic there --- it'd be better to
decouple the string-building functionality from the query-sending
functionality. Probably better to consider something like
PQformatQuery() that passes back a malloc'd string.
regards, tom lane
Hm, my first thought was that you should just be using bind parameters instead
of interpolating variables directly into the query.
But the more I think about it the more I like your idea. It's true that using
parameters takes away most of the use cases for the kind of interface you
suggest. But there are still cases that remain. And in those cases it would be
possible to do it more cleanly and conveniently than with a stock sprintf.
In particular cases like when I want to insert one of a small number of
constants and want to be sure the planner plans and caches separate plans for
each value; or when I want to insert entirely different subexpressions
depending on some parameter; or most commonly of all I want to vary the order
of the ORDER BY expressions and still have every chance of using indexes.
Aside from the convenience I think it would be interesting from an
injection-safety point of view. We can offer a %-escape for "string with SQL
quoting" and a separate %-escape for unquoted SQL text which is documented as
being the wrong thing to use for user-provided data. And we can ensure that
all escapes except for this "raw SQL" escape are all injection-safe.
But anything you provide you should provide both in PQexec form and PQprepare
form as well (and I suppose in PQexecParams form). This might seem pointless,
if you're interpolating some values why not interpolate them all? The answer
is that you quite often want to interpolate a few specific values, often
values that don't have many possible values and might affect the plan, but
definitely don't want to interpolate user-provided values that have many
possible values.
A typical example might be something like:
SELECT * FROM invoices WHERE customer_id = ? ORDER BY { order_by_clauses[column_selected] }
You certainly don't want to a plan a new query for every possible user, but
you don't mind caching 5 different plans for the five display columns
depending on which the user has clicked on.
-- Gregory Stark EnterpriseDB http://www.enterprisedb.com
<blockquote type="CITE"><pre> <font color="#000000">Way too late for 8.3 --- if we were going to do something like this,</font> <font color="#000000">we should think first and program later. In particular, blindly</font> <font color="#000000">adopting the sprintf format string definition doesn't seem very helpful.</font> <font color="#000000">The sorts of escapes I'd want to have are "properly quoted SQL</font> <font color="#000000">identifier", "properly quoted SQL literal", etc. A large fraction of</font> <font color="#000000">what sprintf knows about is more or less irrelevant to the task of</font> <font color="#000000">creating SQL commands.</font> <font color="#000000">Also, how does this interact with parameterized or prepared commands?</font> <font color="#000000">If we wanted PQexecf we'd soon want PQexecParamsf, etc. I don't think</font> <font color="#000000">we really want so much duplicate logic there --- it'd be better to</font> <font color="#000000">decouple the string-building functionality from the query-sending</font> <font color="#000000">functionality. Probably better to consider something like</font> <font color="#000000">PQformatQuery() that passes back a malloc'd string.</font> </pre></blockquote><br /> I agree with almost everything that you said. I <i>really</i> like the idea of new format specifiersfor "properly quoted stuff".<br /><br /> I like the idea of adding a new PQformatQuery() function. And once youhave PQformatQuery(), you can easily change the implementation of PQexecf() without affecting any client applications.I'm not sure that we would need a PQexecParamsf(), but I'm willing to accept that we might (it seems more likelythat we would want PQpreparef()).<br /><br /> But we would want those features in addition to PQexecf(), not insteadof PQexecf().<br /><br /> PQexecf() would be useful today, even without all of those extras - just look at the thesource code for pg_dump to see how much code we would eliminate with a simple PQexecf().<br /><br /> There are two reasonsI threw out this idea now.<br /><br /> 1) I didn't think of it until a few days ago...<br /><br /> 2) It's importantto get the <i>interface</i> into a near-future release so that client applications can start using it soon. Wecan add features and refactor the implementation later. I assume that my prototype is not horrible (it's based on existingcode).<br /><br /> -- Korry<br /><table cellpadding="0" cellspacing="0" width="100%"><tr><td><br /><br/> --<br /> Korry Douglas <a href="mailto:korryd@enterprisedb.com">korryd@enterprisedb.com</a><br /> EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a></td></tr></table>
<korryd@enterprisedb.com> writes: > 2) It's important to get the interface into a near-future release so > that client applications can start using it soon. It's important to get the *right* interface into the first release that has it. The day before feature freeze is way too late for blue-sky design IMHO. I note that the nominal schedule http://www.postgresql.org/developer/roadmap says that all major proposals should have been made and reviewed at least a month ago. While we're still busy resolving details for a lot of things, I don't believe there is anything currently under serious consideration that hadn't at least been talked about before the beginning of March. regards, tom lane
<blockquote type="CITE"><pre> <font color="#000000">It's important to get the *right* interface into the first release</font> <font color="#000000">that has it. </font> </pre></blockquote><br /> Agreed, that's why I proposed the right interface to begin with :-)<br /><br /><blockquote type="CITE"><pre> <font color="#000000">The day before feature freeze is way too late for</font> <font color="#000000">blue-sky design IMHO.</font> </pre></blockquote><br /> Ok, I can certainly bring this up again in the next release cycle. And I can include my own privateimplementation in any client applications until we have something similar in libpq.<br /><br /><blockquote type="CITE"><pre> <font color="#000000">I note that the nominal schedule</font> <font color="#000000"><a href="http://www.postgresql.org/developer/roadmap">http://www.postgresql.org/developer/roadmap</a></font> <font color="#000000">says that all major proposals should have been made and reviewed at</font> <font color="#000000">least a month ago. </font> </pre></blockquote><br /> Consider me spanked... (and quit giggling Bruce).<br /><br /> -- Korry<br /><br /><tablecellpadding="0" cellspacing="0" width="100%"><tr><td><br /><br /> --<br /> Korry Douglas <a href="mailto:korryd@enterprisedb.com">korryd@enterprisedb.com</a><br/> EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a></td></tr></table>
korryd@enterprisedb.com wrote: > > I note that the nominal schedule > > http://www.postgresql.org/developer/roadmap > > says that all major proposals should have been made and reviewed at > > least a month ago. > > > Consider me spanked... (and quit giggling Bruce). Awe, you got me. :-) FYI, I sung "Dream On" to Korry when he first suggested this to me: http://en.wikipedia.org/wiki/Dream_On_(Aerosmith_song) -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
korryd@enterprisedb.com wrote: >> It's important to get the *right* interface into the first release >> that has it. >> > > Agreed, that's why I proposed the right interface to begin with :-) > Maybe the first thing we might usefully do would be to document PQExpBuffer. And you can send in a patch for that for 8.3 :-) cheers andrew
Andrew Dunstan wrote: > korryd@enterprisedb.com wrote: > >> It's important to get the *right* interface into the first release > >> that has it. > >> > > > > Agreed, that's why I proposed the right interface to begin with :-) > > > > Maybe the first thing we might usefully do would be to document > PQExpBuffer. And you can send in a patch for that for 8.3 :-) The big question is whether these functions are for external use. We do export them using libpq/exports.txt, but I assume it was only for psql use and not for general usage. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
<blockquote type="CITE"><pre> <font color="#000000"><a href="mailto:korryd@enterprisedb.com">korryd@enterprisedb.com</a> wrote:</font> <font color="#000000">> > I note that the nominal schedule</font> <font color="#000000">> > <a href="http://www.postgresql.org/developer/roadmap">http://www.postgresql.org/developer/roadmap</a></font> <font color="#000000">> > says that all major proposals should have been made and reviewed at</font> <font color="#000000">> > least a month ago. </font> <font color="#000000">> </font> <font color="#000000">> </font> <font color="#000000">> Consider me spanked... (and quit giggling Bruce).</font> <font color="#000000">Awe, you got me. :-)</font> <font color="#000000">FYI, I sung "Dream On" to Korry when he first suggested this to me:</font> </pre></blockquote><br /> I tried to forget the singing... it was your evil laughter that still haunts my dreams.<br /><br/> -- Korry
<korryd@enterprisedb.com> writes:
>> It's important to get the *right* interface into the first release
>> that has it.
> Agreed, that's why I proposed the right interface to begin with :-)
I don't necessarily object to PQexecf() as a shortcut for some
multi-step operation, but I don't think you've got the format string
semantics down yet.
regards, tom lane
<blockquote type="CITE"><pre> <font color="#000000">I don't necessarily object to PQexecf() as a shortcut for some</font> <font color="#000000">multi-step operation, but I don't think you've got the format string</font> <font color="#000000">semantics down yet.</font> </pre></blockquote><br /> I'm thinking that we could start with the "standard" conversion specifiers - those are well understoodand would be expected by just about any C developer.<br /><br /> In particular, the %d, %u, %e, and %f format specifiersare immediately useful.<br /><br /> If we start with the "standard" set, you can start to use PQexecf() immediatelyand we could promise to maintain *at least* that set.<br /><br /> We can add more specifiers (for proper quotingand such) later - we can't break existing client applications if we just add to the set of supported specifiers; thefunction gets more useful as time goes by.<br /><br /><br /> -- Korry
<korryd@enterprisedb.com> writes:
>> I don't necessarily object to PQexecf() as a shortcut for some
>> multi-step operation, but I don't think you've got the format string
>> semantics down yet.
> I'm thinking that we could start with the "standard" conversion
> specifiers - those are well understood and would be expected by just
> about any C developer.
> In particular, the %d, %u, %e, and %f format specifiers are immediately
> useful.
> If we start with the "standard" set, you can start to use PQexecf()
> immediately and we could promise to maintain *at least* that set.
That's exactly the approach I don't want to take. To implement our
quoting-escape additions, we'll have to stop relying on sprintf and
implement for ourselves whatever "standard C" escapes we want to
support. Then we'd have a backwards compatibility problem anywhere that
the local sprintf() implements escapes that go beyond the standard.
That means we'd be buying into *at least* as much complexity as is in
src/port/snprintf.c, probably rather more, plus ongoing portability
headaches while we find out what people happen to have depended on.
And that's before we've added any value at all.
I think it's simply not sane to start off with an sprintf-based
implementation when we fully intend to have custom code later.
We need a small, tightly specified set of escapes so that we aren't
forced to support a pile of stuff that has little if any use for
SQL-query construction. As an example, I see the use for %d but
not the use for %-012.6d, to say nothing of $-reordering. But shipping
a stopgap version of PQexecf would lock us into supporting all of that
cruft.
regards, tom lane
<blockquote type="CITE"><pre> <font color="#000000">That's exactly the approach I don't want to take. To implement our</font> <font color="#000000">quoting-escape additions, we'll have to stop relying on sprintf and</font> <font color="#000000">implement for ourselves whatever "standard C" escapes we want to</font> <font color="#000000">support. </font> </pre></blockquote><br /> Ok - then it seems like it might make sense to implement PQexecf() in terms of src/port/snprintf.c(and enhance that family of functions to support the quoting conversion specifiers that we want).<br /><br/> Let's just pick up this discussion in the next release cycle.<br /><br /> Bruce - can you add a TODO for this topic? Thanks.<br /><br /><br /> -- Korry<br /><table cellpadding="0" cellspacing="0" width="100%"><tr><td><br /><br/> --<br /> Korry Douglas <a href="mailto:korryd@enterprisedb.com">korryd@enterprisedb.com</a><br /> EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a></td></tr></table>
On Sat, Mar 31, 2007 at 07:16:19PM -0400, Bruce Momjian wrote: > Andrew Dunstan wrote: > > korryd@enterprisedb.com wrote: > > >> It's important to get the *right* interface into the first release > > >> that has it. > > >> > > > > > > Agreed, that's why I proposed the right interface to begin with :-) > > > > > > > Maybe the first thing we might usefully do would be to document > > PQExpBuffer. And you can send in a patch for that for 8.3 :-) > > The big question is whether these functions are for external use. We > do export them using libpq/exports.txt, but I assume it was only for > psql use and not for general usage. There was discussion about this before, and the conclusion then was that they're not a part of the public interface, and only intended to be used by "our own" frontends. Doesn't mean we can't put it out there if we think it's a good interface for people to use though ;-) //Magnus
Added to TODO:
o Add PQexecf() that allows complex parameter substitution
http://archives.postgresql.org/pgsql-hackers/2007-03/msg01803.php
---------------------------------------------------------------------------
korryd@enterprisedb.com wrote:
> > That's exactly the approach I don't want to take. To implement our
> > quoting-escape additions, we'll have to stop relying on sprintf and
> > implement for ourselves whatever "standard C" escapes we want to
> > support.
>
>
> Ok - then it seems like it might make sense to implement PQexecf() in
> terms of src/port/snprintf.c (and enhance that family of functions to
> support the quoting conversion specifiers that we want).
>
> Let's just pick up this discussion in the next release cycle.
>
> Bruce - can you add a TODO for this topic? Thanks.
>
>
> -- Korry
>
>
> --
> Korry Douglas korryd@enterprisedb.com
> EnterpriseDB http://www.enterprisedb.com
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
My apologies for the late reply...<br /><br /> Tom Lane wrote:<br /><blockquote cite="mid26530.1175301022@sss.pgh.pa.us"type="cite"><pre wrap=""><a class="moz-txt-link-rfc2396E" href="mailto:korryd@enterprisedb.com"><korryd@enterprisedb.com></a>writes: </pre><blockquote type="cite"><pre wrap="">I'dlike to see a new variant on PQexec(): PGresult * PQexecf(PGconn *conn, const char *fmt, ...); </pre></blockquote><prewrap=""> Way too late for 8.3 --- if we were going to do something like this, we should think first and program later. In particular, blindly adopting the sprintf format string definition doesn't seem very helpful. The sorts of escapes I'd want to have are "properly quoted SQL identifier", "properly quoted SQL literal", etc. A large fraction of what sprintf knows about is more or less irrelevant to the task of creating SQL commands. </pre></blockquote> The advantage of using stock sprintf commands is that most compilers understand them these days, andcan check that the arguments given match the format string. If you go with your own format specifiers, this is no longertrue.<br /><br /> Brian<br /><br />