Re: [HACKERS] Last minute mini-proposal (I know, I know) for PQexecf()
My apologies for the late reply... Tom Lane wrote: [EMAIL PROTECTED] 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. The advantage of using stock sprintf commands is that most compilers understand them these days, and can check that the arguments given match the format string. If you go with your own format specifiers, this is no longer true. Brian
[HACKERS] Last minute mini-proposal (I know, I know) for PQexecf()
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 [EMAIL PROTECTED] http://www.enterprisedb.com PGresult * PQexecf(PGconn *conn, const char *fmt, ...) { static PQExpBuffer str; va_listargs; 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; intnprinted; 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); }
Re: [HACKERS] Last minute mini-proposal (I know, I know) for PQexecf()
[EMAIL PROTECTED] 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
Re: [HACKERS] Last minute mini-proposal (I know, I know) for PQexecf()
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