Обсуждение: psql tab completion bug and possible fix
Recently I've been seeing regular but very occasional errors like the following while using psql: test=> BEGIN ; BEGIN test=> UPDATE language SET name_native = 'Français' WHERE lang_id='fr'; ERROR: current transaction is aborted, commands ignored until end of transaction block where the UPDATE statement itself is entirely correct and is executed correctly when a new transaction is started. Unfortunately I was never able to reproduce the error and thought it might be some kind of beta flakiness, until it turned up in a 7.3 installation too. The culprit is the following section of psql's tab-complete.c , around line 1248: /* WHERE */ /* Simple case of the word before the where being the table name */ else if (strcasecmp(prev_wd, "WHERE") == 0) COMPLETE_WITH_ATTR(prev2_wd); which is correct for SELECT statements. Where the line contains an UPDATE statement however, and tab is pressed after WHERE, the word before WHERE is passed to the backend via a sprintf-generated query with the %s between single quotes, i.e. in the above case AND c.relname='%s' is translated to AND c.relname=''Français'' which is causing a silent error and the transaction failure. I don't see a simple solution to cater for UPDATE syntax in this context (you'd need to keep track of whether the statement begins with SELECT or UPDATE), though it might be a good todo item. A quick (but not dirty) fix for this and other current or future potential corner cases would be to ensure any statements executed by the tab completion functions are quoted correctly, so even if the statement does not produce any results for tab completion, at least it cannot cause mysterious transaction errors (and associated doubts about PostgreSQL's stability ;-). A patch for this using PQescapeString (is there another preferred method?) is attached as a possible solution. Ian Barwick barwick@gmx.net
Вложения
Ian Barwick <barwick@gmx.net> writes: > A patch for this using PQescapeString (is there another preferred method?) is > attached as a possible solution. Surely all those replacements of \\ with \\\\ are wrong. I agree that it's insane not to be escaping the user input, though ... regards, tom lane
On Tuesday 14 October 2003 23:38, Tom Lane wrote: > Ian Barwick <barwick@gmx.net> writes: > > A patch for this using PQescapeString (is there another preferred > > method?) is attached as a possible solution. > > Surely all those replacements of \\ with \\\\ are wrong. The slash in the slash command gets escaped too...: #include <stdio.h> #include "libpq-fe.h" main() { char *s, *r; s = "\\d"; printf("%s\n", s); r = (char *) malloc(strlen(s) * 2); PQescapeString(r, s, strlen(s)); printf("%s\n", r); if(strcmp(r, "\\\\d") == 0) { printf("match\n"); } free(s); free(r); } Without \\\\ tab expansion for slash commands doesn't work. There may of course be better ways of solving this problem. Ian Barwick barwick@gmx.net
Ian Barwick <barwick@gmx.net> writes: > On Tuesday 14 October 2003 23:38, Tom Lane wrote: >> Surely all those replacements of \\ with \\\\ are wrong. > The slash in the slash command gets escaped too...: Not the way I did it. You were doing the escaping in the wrong place IMHO --- the string passed to _complete_from_query() mustn't be escaped already, because it is not only used to send a command to the backend, but also to compare against the strings returned by the backend, which won't be escaped. So the escaping needs to be done internally to _complete_from_query(), and that eliminates the side-effects elsewhere. regards, tom lane
On Wednesday 15 October 2003 22:45, Tom Lane wrote: > Ian Barwick <barwick@gmx.net> writes: > > On Tuesday 14 October 2003 23:38, Tom Lane wrote: > >> Surely all those replacements of \\ with \\\\ are wrong. > > > > The slash in the slash command gets escaped too...: > > Not the way I did it. You were doing the escaping in the wrong place > IMHO --- the string passed to _complete_from_query() mustn't be escaped > already, because it is not only used to send a command to the backend, > but also to compare against the strings returned by the backend, which > won't be escaped. So the escaping needs to be done internally to > _complete_from_query(), and that eliminates the side-effects elsewhere. Aha, sorry, I hadn't seen your patch. It works excellently for me, so I shall gripe no longer ;-). Many thanks Ian Barwick barwick@gmx.net