While cleaning up some pg_migrator code
(http://pgfoundry.org/projects/pg-migrator/) it occurred to me that a
typical libpq client application spends a lot of code constructing SQL
commands. The code typically looks like this:

a) allocate enough room to hold the command
b) sprintf( command, text, argument, argument, argument, ... )
c) PQexec( conn, command )
d) free( command )

In most cases, the amount of memory that you allocate in step a) is just
an educated guess. It's typically more room than you need,
occassionally less room than you need (and you get a buffer overflow
exploit), and it's rarely maintained properly when you modify the
command text (or the argument list).

I'd like to see a new variant on PQexec():

PGresult * PQexecf(PGconn *conn, const char *fmt, ...);

PQexecf() simply performs steps a, b, c, and d for you. And you call it
like this:

PQexecf( conn, text, argument, argument, argument, ... )

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) and doesn't change any existing code. PQexecf() is similar to
PQexecParams() but it much simpler to use (and should be very familiar
to C programmers). PQexecf() is not intended as a replacement for
PQprepare() and PQexecPrepared() - you should use prepare/exec when you
want to execute a command many times.

I could eliminate a lot of client-side code if PQexecf() were available
- and the code that I could remove is the code that's most likely to be
buggy and least likely to be properly maintained.

I've thrown together an UNTESTED prototype (below), just to get the idea
across - you'll recognize that most of this code is identical to
printPQExpBuffer(). In the prototype, I'm keeping a static PQExpBuffer
that grows to the hold the largest string ever required by the client
application - that part seems to be a point for discussion, but since
the detail is hidden in the implementation, we could adjust the code
later if necessary (without changing the interface).

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 invent my own createPQExpBuffer() /
enlargePQExpBuffer() code since those functions are not an official part
of libpq (and won't even be available to a Win32 client application).

Is it just too late to even think about this for 8.3? (Bruce laughed at
me when I suggested the idea :-)

-- Korry
korryd@enterprisedb.com
http://www.enterprisedb.com


__________________________________________________ ______________________
PGresult *
PQexecf(PGconn *conn, const char *fmt, ...)
{
static PQExpBuffer str;
va_list args;

if (str == NULL)
str = createPQExpBuffer();

for (;
{
/*
* Try to format the given string into the available space; but if
* there's hardly any space, don't bother trying, just fall through to
* enlarge the buffer first.
*/
if (str->maxlen > str->len + 16)
{
size_t avail = str->maxlen - str->len - 1;
int nprinted;

va_start(args, fmt);
nprinted = vsnprintf(str->data + str->len, avail, fmt, args);
va_end(args);

/*
* Note: some versions of vsnprintf return the number of chars
* actually stored, but at least one returns -1 on failure. Be
* conservative about believing whether the print worked.
*/
if (nprinted >= 0 && nprinted < (int) avail - 1)
{
/* Success. Note nprinted does not include trailing null. */
str->len += nprinted;
break;
}
}
/* Double the buffer size and try again. */
if (!enlargePQExpBuffer(str, str->maxlen))
return PQmakeEmptyPGresult(conn, PGRES_FATAL_ERROR); /* oops, out of
memory */
}

return PQexec(conn, str->data);
}

<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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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


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

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

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


I agree with almost everything that you said. I really like the idea of
new format specifiers for "properly quoted stuff".

I like the idea of adding a new PQformatQuery() function. And once you
have 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 likely that we would want PQpreparef()).

But we would want those features in addition to PQexecf(), not instead
of PQexecf().

PQexecf() would be useful today, even without all of those extras - just
look at the the source code for pg_dump to see how much code we would
eliminate with a simple PQexecf().

There are two reasons I threw out this idea now.

1) I didn't think of it until a few days ago...

2) It's important to get the interface into a near-future release so
that client applications can start using it soon. We can add features
and refactor the implementation later. I assume that my prototype is
not horrible (it's based on existing code).

-- Korry


--
Korry Douglas korryd@enterprisedb.com
EnterpriseDB http://www.enterprisedb.com

<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

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

> 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 :-)


> The day before feature freeze is way too late for
> blue-sky design IMHO.


Ok, I can certainly bring this up again in the next release cycle. And
I can include my own private implementation in any client applications
until we have something similar in libpq.


> 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).

-- Korry



--
Korry Douglas korryd@enterprisedb.com
EnterpriseDB http://www.enterprisedb.com

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. +

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

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

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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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. +

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

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


I tried to forget the singing... it was your evil laughter that still
haunts my dreams.

-- Korry