Thread: psql patch
Here's some changes I made last night to psql's common.c (as found in 7.3.2). It removes some code duplication and #ifdeffing, and some unstructured ugliness such as tacky breaks and an unneeded continue. Breaks up a large function into smaller functions and reduces required nesting levels, and kills a variable or two. If this doesn't apply against CVS, I can try to redo it on the CVS version, but could anyone try it out before I go to all that overhead? Jeroen
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Jeroen T. Vermeulen wrote:
> Here's some changes I made last night to psql's common.c (as found in
> 7.3.2). It removes some code duplication and #ifdeffing, and some
> unstructured ugliness such as tacky breaks and an unneeded continue.
> Breaks up a large function into smaller functions and reduces required
> nesting levels, and kills a variable or two.
>
> If this doesn't apply against CVS, I can try to redo it on the CVS
> version, but could anyone try it out before I go to all that overhead?
>
>
> Jeroen
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied cleanly. Thanks. --------------------------------------------------------------------------- Jeroen T. Vermeulen wrote: > Here's some changes I made last night to psql's common.c (as found in > 7.3.2). It removes some code duplication and #ifdeffing, and some > unstructured ugliness such as tacky breaks and an unneeded continue. > Breaks up a large function into smaller functions and reduces required > nesting levels, and kills a variable or two. > > If this doesn't apply against CVS, I can try to redo it on the CVS > version, but could anyone try it out before I go to all that overhead? > > > Jeroen > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Thu, 2003-02-13 at 08:49, Jeroen T. Vermeulen wrote:
> Here's some changes I made last night to psql's common.c (as found in
> 7.3.2). It removes some code duplication and #ifdeffing, and some
> unstructured ugliness such as tacky breaks and an unneeded continue.
> Breaks up a large function into smaller functions and reduces required
> nesting levels, and kills a variable or two.
This patch breaks psql on my system:
(without the patch applied)
nconway=# \d
List of relations
Schema | Name | Type | Owner
--------+------+-------+---------
public | a | table | nconway
(1 row)
(with the patch applied)
nconway=> \d
nconway=>
Other slash commands (\d table, \df, etc.) are similarly broken.
Cheers,
Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
On Fri, Feb 21, 2003 at 02:18:04AM -0500, Neil Conway wrote: > > This patch breaks psql on my system: > Other slash commands (\d table, \df, etc.) are similarly broken. Oops. I thought I had only touched the query execution code... Jeroen
"Jeroen T. Vermeulen" <jtv@xs4all.nl> writes:
> On Fri, Feb 21, 2003 at 02:18:04AM -0500, Neil Conway wrote:
>> This patch breaks psql on my system:
>> Other slash commands (\d table, \df, etc.) are similarly broken.
> Oops. I thought I had only touched the query execution code...
I have backed out the patch so I could get work done ;-). Please repair
it and resubmit.
regards, tom lane
"Jeroen T. Vermeulen" <jtv@xs4all.nl> writes:
> One of the goals of my patch is to enable immediate notification of
> notifications, even when the listening process isn't issuing a query.
> Maybe it would be nice to be able to check for notifications e.g.
> whenever the input line is empty. That change would be rather messy
> without a slight restructuring.
Hmm. Once you have started to type, it would be annoying for the
thing to spit out notifications, even if you momentarily backspace
out what you've typed. I'd argue that it's only okay to print
notifications after completion of a command (where at least some
backslash constructs, eg \r, could be allowed to count as a command).
I find it kind of hard to envision situations where you'd really care
anyway. No one's going to use psql as the base for an interactive
application that depends on LISTEN/NOTIFY, surely?
regards, tom lane
"Jeroen T. Vermeulen" <jtv@xs4all.nl> writes:
> Imagine you're maintaing a program that sends out notifications, and
> you want to get a feel for when this happens. I think it would be
> nice to be able to open a shell, run psql, issue a LISTEN and just
> let it sit there and print out anything that comes along, while you
> play with the application in another window.
I'd be happier with it if there were a switch that enabled this behavior
(and it were *not* on by default). That would eliminate one of the
things that was bothering me, which was the prospect of every psql
everywhere consuming cycles to check for notifications. If you did that
it would likely also be acceptable to print notifications in the midst
of type-in.
regards, tom lane
On Mon, 24 Feb 2003, Tom Lane wrote: > "Jeroen T. Vermeulen" <jtv@xs4all.nl> writes: > > Imagine you're maintaing a program that sends out notifications, and > > you want to get a feel for when this happens. I think it would be > > nice to be able to open a shell, run psql, issue a LISTEN and just > > let it sit there and print out anything that comes along, while you > > play with the application in another window. > > I'd be happier with it if there were a switch that enabled this behavior > (and it were *not* on by default). That would eliminate one of the > things that was bothering me, which was the prospect of every psql > everywhere consuming cycles to check for notifications. If you did that > it would likely also be acceptable to print notifications in the midst > of type-in. > Wouldn't it make sense to have a separate utility for this? Something like pg_listen? Configuration of the conditions could simply be by command line switches ( or even just pg_listen <condition> [<condition> ...] ). Perhaps it's a gborg or contrib utility rather than core though. -- Nigel J. Andrews
"Jeroen T. Vermeulen" <jtv@xs4all.nl> writes:
> 3. It would be a little extra work, but perhaps not undoable, to keep
> track of whether the client is actually listening on any triggers.
Not without help from the backend --- you have no idea whether a LISTEN
command might have been executed via some user-defined function.
> Similarly, there's no need to
> poll while inside a transaction because no notifications will be
> delivered in that state.
Again, it's not all that easy to be sure if you're inside a transaction
or not. We looked at this and decided it was impractical to do without
a protocol addition.
regards, tom lane
"Jeroen T. Vermeulen" <jtv@xs4all.nl> writes:
> However I just took a look at the docs for readline and apparently it
> was designed with select() in mind. So it should be possible to
> implement this without any cost to scalability: the server doesn't
> care if the frontend is listening when it sends out the notification,
> and the frontend can sleep until either a notification or a keypress
> arrives.
Cool; if you can do it that way then the wasted-cycles objection is
gone. (I don't think it would have been very interesting to implement
the functionality only in the no-readline case, 'cuz just about everyone
seems to use readline builds.)
regards, tom lane
On Mon, Feb 24, 2003 at 01:35:15PM -0500, Tom Lane wrote: > > Cool; if you can do it that way then the wasted-cycles objection is > gone. (I don't think it would have been very interesting to implement > the functionality only in the no-readline case, 'cuz just about everyone > seems to use readline builds.) OK. I'm attaching my new patch (which is fixed for the bug in the previous attempt, and now touches a lot more code). After this, I'll get to work on introducing select(), and once that's finished we can start experimenting with asynchronous notification reports. Jeroen
Attachment
On Mon, Feb 24, 2003 at 01:35:15PM -0500, Tom Lane wrote: > > Cool; if you can do it that way then the wasted-cycles objection is > gone. (I don't think it would have been very interesting to implement > the functionality only in the no-readline case, 'cuz just about everyone > seems to use readline builds.) OK. I'm attaching my new patch (which is fixed for the bug in the previous attempt, and now touches a lot more code). After this, I'll get to work on introducing select(), and once that's finished we can start experimenting with asynchronous notification reports. Jeroen
On Mon, Feb 24, 2003 at 02:32:47PM +0000, Nigel J. Andrews wrote: > > Wouldn't it make sense to have a separate utility for this? Something like > pg_listen? Configuration of the conditions could simply be by command line > switches ( or even just pg_listen <condition> [<condition> ...] ). Perhaps it's > a gborg or contrib utility rather than core though. Sure, but would it be as practical as having an interactive shell where you could add or remove triggers on the fly and manipulate your database? Chances are, if you need the one you're going to need the other as well, with the same connect strings and in similar shell windows. Jeroen
On Mon, Feb 24, 2003 at 12:36:56PM -0500, Tom Lane wrote: > Not without help from the backend --- you have no idea whether a LISTEN > command might have been executed via some user-defined function. True. Hadn't thought of that. The backend does identify a direct LISTEN to the frontend through PQcmdStatus(), but it doesn't say anything about what happens in functions and such. And similar for transactions, I guess. > Again, it's not all that easy to be sure if you're inside a transaction > or not. We looked at this and decided it was impractical to do without > a protocol addition. I don't think I followed that discussion to its conclusion at the time, but it left me with the impression that such an addition was being considered. However I just took a look at the docs for readline and apparently it was designed with select() in mind. So it should be possible to implement this without any cost to scalability: the server doesn't care if the frontend is listening when it sends out the notification, and the frontend can sleep until either a notification or a keypress arrives. Or am I missing something basic here? Jeroen
On Mon, Feb 24, 2003 at 09:21:47AM -0500, Tom Lane wrote: > > I'd be happier with it if there were a switch that enabled this behavior > (and it were *not* on by default). That would eliminate one of the > things that was bothering me, which was the prospect of every psql > everywhere consuming cycles to check for notifications. If you did that > it would likely also be acceptable to print notifications in the midst > of type-in. So your only objection is which should be the default setting for the switch? In that case I can just go ahead and implement this as planned, with the controlling variable I mentioned earlier set to the current behaviour by default. As for cycle consumption, I think there are mitigating factors: 1. Since the backend pushes notifications out the frontend anyway, no extra backend or network cycles are used. The cost is paid entirely on the side of the client, so the problem case is where a single machine runs an enourmous number of psql clients. How many psql clients may access a single server from various remote machines does not come into the equation. (I suppose that would be the main scalability worry) 2. In the case where readline is not used, I don't think there is any need to busy-wait. It shouldn't be very hard to select() on the backend socket and the command input fd at the same time. Dunno about the readline case though; if that doesn't support some form of timeout or nonblocking operation, notifications can only be checked when input occurs anyway. 3. It would be a little extra work, but perhaps not undoable, to keep track of whether the client is actually listening on any triggers. If not, there's no need to poll anything. Similarly, there's no need to poll while inside a transaction because no notifications will be delivered in that state. Jeroen
On Sat, Feb 22, 2003 at 11:14:47AM -0500, Tom Lane wrote: > > Hmm. Once you have started to type, it would be annoying for the > thing to spit out notifications, even if you momentarily backspace > out what you've typed. I'd argue that it's only okay to print > notifications after completion of a command (where at least some > backslash constructs, eg \r, could be allowed to count as a command). I'm not saying it should be mandatory, but how about receiving notifications whenever a blank line is entered, or any time as long as the current input line is empty? Perhaps the input line could be redisplayed after the notification message, similar to how most shells handle commands being typed while another command is still executing. Even if an unexpected notification is a little annoying during an interactive session, how often does it happen? Would it be a problem to have a variable to define how often pending notifications should be checked for? > I find it kind of hard to envision situations where you'd really care > anyway. No one's going to use psql as the base for an interactive > application that depends on LISTEN/NOTIFY, surely? Surely not. I'm thinking more of developers, including first-time postgres users, who use psql to experiment with new queries, new schemas, SQL and postgres features they're not familiar with, and so on. Many of them come back to ask why their notifications aren't arriving. So I thought it would be more insightful to them if notifications could be printed straightaway. Imagine you're maintaing a program that sends out notifications, and you want to get a feel for when this happens. I think it would be nice to be able to open a shell, run psql, issue a LISTEN and just let it sit there and print out anything that comes along, while you play with the application in another window. Jeroen
On Fri, Feb 21, 2003 at 09:44:27PM -0500, Tom Lane wrote: > > I have backed out the patch so I could get work done ;-). Please repair > it and resubmit. Mea culpa, mea culpa, mea maxima maxima culpa. I misspelled the comparison operator for two clauses in a loop condition. I'll submit an updated patch once I've done some more testing (I'm not yet 100% sure that my new version if free of memory leaks). One of the goals of my patch is to enable immediate notification of notifications, even when the listening process isn't issuing a query. Maybe it would be nice to be able to check for notifications e.g. whenever the input line is empty. That change would be rather messy without a slight restructuring. Jeroen
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Jeroen T. Vermeulen wrote:
> On Mon, Feb 24, 2003 at 01:35:15PM -0500, Tom Lane wrote:
> >
> > Cool; if you can do it that way then the wasted-cycles objection is
> > gone. (I don't think it would have been very interesting to implement
> > the functionality only in the no-readline case, 'cuz just about everyone
> > seems to use readline builds.)
>
> OK. I'm attaching my new patch (which is fixed for the bug in the
> previous attempt, and now touches a lot more code).
>
> After this, I'll get to work on introducing select(), and once that's
> finished we can start experimenting with asynchronous notification
> reports.
>
>
> Jeroen
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied and attached. Thanks. I had to modify the handling of cancelConn. It wasn't properly being defined and set in the original patch. --------------------------------------------------------------------------- Jeroen T. Vermeulen wrote: > On Mon, Feb 24, 2003 at 01:35:15PM -0500, Tom Lane wrote: > > > > Cool; if you can do it that way then the wasted-cycles objection is > > gone. (I don't think it would have been very interesting to implement > > the functionality only in the no-readline case, 'cuz just about everyone > > seems to use readline builds.) > > OK. I'm attaching my new patch (which is fixed for the bug in the > previous attempt, and now touches a lot more code). > > After this, I'll get to work on introducing select(), and once that's > finished we can start experimenting with asynchronous notification > reports. > > > Jeroen > [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Don't 'kill -9' the postmaster -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/bin/psql/common.c =================================================================== RCS file: /cvsroot/pgsql-server/src/bin/psql/common.c,v retrieving revision 1.58 diff -c -c -r1.58 common.c *** src/bin/psql/common.c 20 Mar 2003 04:49:18 -0000 1.58 --- src/bin/psql/common.c 20 Mar 2003 05:57:19 -0000 *************** *** 34,39 **** --- 34,58 ---- #include "print.h" #include "mainloop.h" + + /* Workarounds for Windows */ + /* Probably to be moved up the source tree in the future, perhaps to be replaced by + * more specific checks like configure-style HAVE_GETTIMEOFDAY macros. + */ + #ifndef WIN32 + + typedef struct timeval TimevalStruct; + #define GETTIMEOFDAY(T) gettimeofday(T, NULL) + #define DIFF_MSEC(T, U) ((((T)->tv_sec - (U)->tv_sec) * 1000000.0 + (T)->tv_usec - (U)->tv_usec) / 1000.0) + + #else + + typedef struct _timeb TimevalStruct; + #define GETTIMEOFDAY(T) _ftime(&T) + #define DIFF_MSEC(T, U) ((((T)->time - (U)->time) * 1000.0 + (T)->millitm - (U)->millitm)) + + #endif + extern bool prompt_state; /* *************** *** 110,119 **** /* Direct signals */ #ifndef WIN32 ! if (pset.queryFoutPipe) ! pqsignal(SIGPIPE, SIG_IGN); ! else ! pqsignal(SIGPIPE, SIG_DFL); #endif return status; --- 129,135 ---- /* Direct signals */ #ifndef WIN32 ! pqsignal(SIGPIPE, pset.queryFoutPipe ? SIG_IGN : SIG_DFL); #endif return status; *************** *** 165,172 **** * so. We use write() to print to stdout because it's better to use simple * facilities in a signal handler. */ ! PGconn *cancelConn; ! volatile bool cancel_pressed; #ifndef WIN32 --- 181,190 ---- * so. We use write() to print to stdout because it's better to use simple * facilities in a signal handler. */ ! static PGconn *volatile cancelConn = NULL; ! ! volatile bool cancel_pressed = false; ! #ifndef WIN32 *************** *** 198,203 **** --- 216,357 ---- #endif /* not WIN32 */ + + /* ConnectionUp + * + * Returns whether our backend connection is still there. + */ + static bool + ConnectionUp() + { + return PQstatus(pset.db) != CONNECTION_BAD; + } + + + + /* CheckConnection + * + * Verify that we still have a good connection to the backend, and if not, + * see if it can be restored. + * + * Returns true if either the connection was still there, or it could be + * restored successfully; false otherwise. If, however, there was no + * connection and the session is non-interactive, this will exit the program + * with a code of EXIT_BADCONN. + */ + static bool + CheckConnection() + { + bool OK; + + OK = ConnectionUp(); + if (!OK) + { + if (!pset.cur_cmd_interactive) + { + psql_error("connection to server was lost\n"); + exit(EXIT_BADCONN); + } + + fputs(gettext("The connection to the server was lost. Attempting reset: "), stderr); + PQreset(pset.db); + OK = ConnectionUp(); + if (!OK) + { + fputs(gettext("Failed.\n"), stderr); + PQfinish(pset.db); + pset.db = NULL; + ResetCancelConn(); + SetVariable(pset.vars, "DBNAME", NULL); + SetVariable(pset.vars, "HOST", NULL); + SetVariable(pset.vars, "PORT", NULL); + SetVariable(pset.vars, "USER", NULL); + SetVariable(pset.vars, "ENCODING", NULL); + } + else + fputs(gettext("Succeeded.\n"), stderr); + } + + return OK; + } + + + + /* + * SetCancelConn + * + * Set cancelConn to point to the current database connection. + */ + static void SetCancelConn(void) + { + cancelConn = pset.db; + } + + + /* + * ResetCancelConn + * + * Set cancelConn to NULL. I don't know what this means exactly, but it saves + * having to export the variable. + */ + void ResetCancelConn(void) + { + cancelConn = NULL; + } + + + /* + * AcceptResult + * + * Checks whether a result is valid, giving an error message if necessary; + * (re)sets copy_in_state and cancelConn as needed, and ensures that the + * connection to the backend is still up. + * + * Returns true for valid result, false for error state. + */ + static bool + AcceptResult(const PGresult *result) + { + bool OK = true; + + ResetCancelConn(); + + if (!result) + { + OK = false; + } + else switch (PQresultStatus(result)) + { + case PGRES_COPY_IN: + copy_in_state = true; + break; + + case PGRES_COMMAND_OK: + case PGRES_TUPLES_OK: + /* Fine, do nothing */ + break; + + case PGRES_COPY_OUT: + /* keep cancel connection for copy out state */ + SetCancelConn(); + break; + + default: + OK = false; + break; + } + + if (!OK) + { + CheckConnection(); + psql_error("%s", PQerrorMessage(pset.db)); + } + + return OK; + } + + + /* * PSQLexec * *************** *** 239,394 **** while ((newres = PQgetResult(pset.db)) != NULL) PQclear(newres); ! cancelConn = pset.db; ! if (PQsendQuery(pset.db, query)) { ! while ((newres = PQgetResult(pset.db)) != NULL) { rstatus = PQresultStatus(newres); ! if (ignore_command_ok && rstatus == PGRES_COMMAND_OK) { ! PQclear(newres); ! continue; ! } ! PQclear(res); res = newres; ! if (rstatus != PGRES_COPY_IN && ! rstatus != PGRES_COPY_OUT) ! break; } } ! rstatus = PQresultStatus(res); ! /* keep cancel connection for copy out state */ ! if (rstatus != PGRES_COPY_OUT) ! cancelConn = NULL; ! if (rstatus == PGRES_COPY_IN) ! copy_in_state = true; ! ! if (res && (rstatus == PGRES_COMMAND_OK || ! rstatus == PGRES_TUPLES_OK || ! rstatus == PGRES_COPY_IN || ! rstatus == PGRES_COPY_OUT)) ! return res; ! else { - psql_error("%s", PQerrorMessage(pset.db)); PQclear(res); ! ! if (PQstatus(pset.db) == CONNECTION_BAD) ! { ! if (!pset.cur_cmd_interactive) ! { ! psql_error("connection to server was lost\n"); ! exit(EXIT_BADCONN); ! } ! fputs(gettext("The connection to the server was lost. Attempting reset: "), stderr); ! PQreset(pset.db); ! if (PQstatus(pset.db) == CONNECTION_BAD) ! { ! fputs(gettext("Failed.\n"), stderr); ! PQfinish(pset.db); ! pset.db = NULL; ! SetVariable(pset.vars, "DBNAME", NULL); ! SetVariable(pset.vars, "HOST", NULL); ! SetVariable(pset.vars, "PORT", NULL); ! SetVariable(pset.vars, "USER", NULL); ! SetVariable(pset.vars, "ENCODING", NULL); ! } ! else ! fputs(gettext("Succeeded.\n"), stderr); } ! return NULL; ! } } /* ! * SendQuery: send the query string to the backend ! * (and print out results) ! * ! * Note: This is the "front door" way to send a query. That is, use it to ! * send queries actually entered by the user. These queries will be subject to ! * single step mode. ! * To send "back door" queries (generated by slash commands, etc.) in a ! * controlled way, use PSQLexec(). * - * Returns true if the query executed successfully, false otherwise. */ ! bool ! SendQuery(const char *query) { - bool success = false; - PGresult *results; PGnotify *notify; - #ifndef WIN32 - struct timeval before, - after; - #else - struct _timeb before, - after; - #endif ! if (!pset.db) ! { ! psql_error("You are currently not connected to a database.\n"); ! return false; ! } ! ! if (GetVariableBool(pset.vars, "SINGLESTEP")) { ! char buf[3]; ! ! printf(gettext("***(Single step mode: Verify query)*********************************************\n" ! "%s\n" ! "***(press return to proceed or enter x and return to cancel)********************\n"), ! query); ! fflush(stdout); ! if (fgets(buf, sizeof(buf), stdin) != NULL) ! if (buf[0] == 'x') ! return false; ! } ! else ! { ! const char *var = GetVariable(pset.vars, "ECHO"); ! ! if (var && strncmp(var, "queries", strlen(var)) == 0) ! puts(query); } - cancelConn = pset.db; - - #ifndef WIN32 - if (pset.timing) - gettimeofday(&before, NULL); - results = PQexec(pset.db, query); - if (pset.timing) - gettimeofday(&after, NULL); - #else - if (pset.timing) - _ftime(&before); - results = PQexec(pset.db, query); - if (pset.timing) - _ftime(&after); - #endif - - if (PQresultStatus(results) == PGRES_COPY_IN) - copy_in_state = true; - /* keep cancel connection for copy out state */ - if (PQresultStatus(results) != PGRES_COPY_OUT) - cancelConn = NULL; ! if (results == NULL) ! { ! fputs(PQerrorMessage(pset.db), pset.queryFout); ! success = false; ! } ! else ! { ! switch (PQresultStatus(results)) ! { ! case PGRES_TUPLES_OK: /* write output to \g argument, if any */ if (pset.gfname) { --- 393,460 ---- while ((newres = PQgetResult(pset.db)) != NULL) PQclear(newres); ! SetCancelConn(); ! if (!PQsendQuery(pset.db, query)) { ! psql_error("%s", PQerrorMessage(pset.db)); ! ResetCancelConn(); ! return NULL; ! } ! ! rstatus = PGRES_EMPTY_QUERY; ! ! while (rstatus != PGRES_COPY_IN && ! rstatus != PGRES_COPY_OUT && ! (newres = PQgetResult(pset.db))) { rstatus = PQresultStatus(newres); ! if (!ignore_command_ok || rstatus != PGRES_COMMAND_OK) { ! PGresult *tempRes = res; res = newres; ! newres = tempRes; } + PQclear(newres); } ! ! if (!AcceptResult(res) && res) { PQclear(res); ! res = NULL; } ! return res; } /* ! * PrintNotifications: check for asynchronous notifications, and print them out * */ ! static void ! PrintNotifications(void) { PGnotify *notify; ! while ((notify = PQnotifies(pset.db))) { ! fprintf(pset.queryFout, gettext("Asynchronous NOTIFY '%s' from backend with pid %d received.\n"), ! notify->relname, notify->be_pid); ! free(notify); ! fflush(pset.queryFout); } + } ! /* ! * PrintQueryTuples: assuming query result is OK, print its tuples ! * ! * Returns true if successful, false otherwise. ! */ ! static bool ! PrintQueryTuples(const PGresult *results) ! { /* write output to \g argument, if any */ if (pset.gfname) { *************** *** 403,410 **** { pset.queryFout = queryFout_copy; pset.queryFoutPipe = queryFoutPipe_copy; ! success = false; ! break; } printQuery(results, &pset.popt, pset.queryFout); --- 469,475 ---- { pset.queryFout = queryFout_copy; pset.queryFoutPipe = queryFoutPipe_copy; ! return false; } printQuery(results, &pset.popt, pset.queryFout); *************** *** 417,430 **** free(pset.gfname); pset.gfname = NULL; - - success = true; } else { printQuery(results, &pset.popt, pset.queryFout); - success = true; } break; case PGRES_EMPTY_QUERY: success = true; --- 482,519 ---- free(pset.gfname); pset.gfname = NULL; } else { printQuery(results, &pset.popt, pset.queryFout); } + + return true; + } + + + + /* + * PrintQueryResults: analyze query results and print them out + * + * Note: Utility function for use by SendQuery() only. + * + * Returns true if the query executed successfully, false otherwise. + */ + static bool + PrintQueryResults(PGresult *results, + const TimevalStruct *before, + const TimevalStruct *after) + { + bool success = false; + + if (!results) + return false; + + switch (PQresultStatus(results)) + { + case PGRES_TUPLES_OK: + success = PrintQueryTuples(results); break; case PGRES_EMPTY_QUERY: success = true; *************** *** 453,516 **** pset.cur_cmd_interactive ? get_prompt(PROMPT_COPY) : NULL); break; ! case PGRES_NONFATAL_ERROR: ! case PGRES_FATAL_ERROR: ! case PGRES_BAD_RESPONSE: ! success = false; ! psql_error("%s", PQerrorMessage(pset.db)); break; } fflush(pset.queryFout); ! if (PQstatus(pset.db) == CONNECTION_BAD) ! { ! if (!pset.cur_cmd_interactive) { ! psql_error("connection to server was lost\n"); ! exit(EXIT_BADCONN); } ! fputs(gettext("The connection to the server was lost. Attempting reset: "), stderr); ! PQreset(pset.db); ! if (PQstatus(pset.db) == CONNECTION_BAD) { ! fputs(gettext("Failed.\n"), stderr); ! PQfinish(pset.db); ! PQclear(results); ! pset.db = NULL; ! SetVariable(pset.vars, "DBNAME", NULL); ! SetVariable(pset.vars, "HOST", NULL); ! SetVariable(pset.vars, "PORT", NULL); ! SetVariable(pset.vars, "USER", NULL); ! SetVariable(pset.vars, "ENCODING", NULL); return false; } else - fputs(gettext("Succeeded.\n"), stderr); - } - - /* check for asynchronous notification returns */ - while ((notify = PQnotifies(pset.db)) != NULL) { ! fprintf(pset.queryFout, gettext("Asynchronous NOTIFY '%s' from backend with pid %d received.\n"), ! notify->relname, notify->be_pid); ! free(notify); ! fflush(pset.queryFout); ! } ! if (results) ! PQclear(results); } ! /* Possible microtiming output */ ! if (pset.timing && success) ! #ifndef WIN32 ! printf(gettext("Time: %.2f ms\n"), ! ((after.tv_sec - before.tv_sec) * 1000000.0 + after.tv_usec - before.tv_usec) / 1000.0); ! #else ! printf(gettext("Time: %.2f ms\n"), ! ((after.time - before.time) * 1000.0 + after.millitm - before.millitm)); ! #endif ! return success; } --- 542,621 ---- pset.cur_cmd_interactive ? get_prompt(PROMPT_COPY) : NULL); break; ! default: break; } fflush(pset.queryFout); ! if (!CheckConnection()) return false; ! ! /* Possible microtiming output */ ! if (pset.timing && success) ! printf(gettext("Time: %.2f ms\n"), DIFF_MSEC(after, before)); ! ! return success; ! } ! ! ! ! /* ! * SendQuery: send the query string to the backend ! * (and print out results) ! * ! * Note: This is the "front door" way to send a query. That is, use it to ! * send queries actually entered by the user. These queries will be subject to ! * single step mode. ! * To send "back door" queries (generated by slash commands, etc.) in a ! * controlled way, use PSQLexec(). ! * ! * Returns true if the query executed successfully, false otherwise. ! */ ! bool ! SendQuery(const char *query) ! { ! PGresult *results; ! TimevalStruct before, after; ! bool OK; ! ! if (!pset.db) { ! psql_error("You are currently not connected to a database.\n"); ! return false; } ! ! if (GetVariableBool(pset.vars, "SINGLESTEP")) { ! char buf[3]; ! ! printf(gettext("***(Single step mode: Verify query)*********************************************\n" ! "%s\n" ! "***(press return to proceed or enter x and return to cancel)********************\n"), ! query); ! fflush(stdout); ! if (fgets(buf, sizeof(buf), stdin) != NULL) ! if (buf[0] == 'x') return false; } else { ! const char *var = GetVariable(pset.vars, "ECHO"); ! if (var && strncmp(var, "queries", strlen(var)) == 0) ! puts(query); } ! SetCancelConn(); ! if (pset.timing) ! GETTIMEOFDAY(&before); ! results = PQexec(pset.db, query); ! if (pset.timing) ! GETTIMEOFDAY(&after); ! ! OK = (AcceptResult(results) && PrintQueryResults(results, &before, &after)); ! PQclear(results); ! ! PrintNotifications(); ! return OK; } Index: src/bin/psql/common.h =================================================================== RCS file: /cvsroot/pgsql-server/src/bin/psql/common.h,v retrieving revision 1.22 diff -c -c -r1.22 common.h *** src/bin/psql/common.h 18 Mar 2003 22:15:44 -0000 1.22 --- src/bin/psql/common.h 20 Mar 2003 05:57:19 -0000 *************** *** 29,34 **** --- 29,36 ---- extern volatile bool cancel_pressed; extern PGconn *cancelConn; + extern void ResetCancelConn(void); + #ifndef WIN32 extern void handle_sigint(SIGNAL_ARGS); #endif /* not WIN32 */ Index: src/bin/psql/copy.c =================================================================== RCS file: /cvsroot/pgsql-server/src/bin/psql/copy.c,v retrieving revision 1.28 diff -c -c -r1.28 copy.c *** src/bin/psql/copy.c 19 Oct 2002 00:22:14 -0000 1.28 --- src/bin/psql/copy.c 20 Mar 2003 05:57:19 -0000 *************** *** 446,453 **** char copybuf[COPYBUFSIZ]; int ret; - assert(cancelConn); - while (!copydone) { ret = PQgetline(conn, copybuf, COPYBUFSIZ); --- 446,451 ---- *************** *** 476,482 **** } fflush(copystream); ret = !PQendcopy(conn); ! cancelConn = NULL; return ret; } --- 474,480 ---- } fflush(copystream); ret = !PQendcopy(conn); ! ResetCancelConn(); return ret; } Index: src/bin/psql/input.c =================================================================== RCS file: /cvsroot/pgsql-server/src/bin/psql/input.c,v retrieving revision 1.21 diff -c -c -r1.21 input.c *** src/bin/psql/input.c 6 Sep 2002 02:33:46 -0000 1.21 --- src/bin/psql/input.c 20 Mar 2003 05:57:19 -0000 *************** *** 20,25 **** --- 20,34 ---- #ifdef USE_READLINE static bool useReadline; static bool useHistory; + + enum histcontrol + { + hctl_none = 0, + hctl_ignorespace = 1, + hctl_ignoredups = 2, + hctl_ignoreboth = hctl_ignorespace | hctl_ignoredups + }; + #endif #ifdef HAVE_ATEXIT *************** *** 33,38 **** --- 42,76 ---- #define PSQLHISTORY ".psql_history" + #ifdef USE_READLINE + static enum histcontrol + GetHistControlConfig(void) + { + enum histcontrol HC; + const char *var; + + var = GetVariable(pset.vars, "HISTCONTROL"); + + if (!var) HC = hctl_none; + else if (strcmp(var, "ignorespace") == 0) HC = hctl_ignorespace; + else if (strcmp(var, "ignoredups") == 0) HC = hctl_ignoredups; + else if (strcmp(var, "ignoreboth") == 0) HC = hctl_ignoreboth; + else HC = hctl_none; + + return HC; + } + #endif + + + static char * + gets_basic(const char prompt[]) + { + fputs(prompt, stdout); + fflush(stdout); + return gets_fromFile(stdin); + } + + /* * gets_interactive() * *************** *** 40,84 **** * The result is malloced. */ char * ! gets_interactive(char *prompt) { char *s; - #ifdef USE_READLINE - const char *var; static char *prev_hist = NULL; - #endif - #ifdef USE_READLINE if (useReadline) s = readline(prompt); else { ! #endif ! fputs(prompt, stdout); ! fflush(stdout); ! s = gets_fromFile(stdin); ! #ifdef USE_READLINE ! } ! #endif ! #ifdef USE_READLINE ! if (useHistory && s && s[0] != '\0') { ! var = GetVariable(pset.vars, "HISTCONTROL"); ! if (!var || (var ! && !((strcmp(var, "ignorespace") == 0 || strcmp(var, "ignoreboth") == 0) && s[0] == ' ') ! && !((strcmp(var, "ignoredups") == 0 || strcmp(var, "ignoreboth") == 0) && prev_hist && strcmp(s,prev_hist) == 0) ! )) { free(prev_hist); prev_hist = strdup(s); add_history(s); } } - #endif return s; } --- 78,118 ---- * The result is malloced. */ char * ! gets_interactive(const char *prompt) { + #ifdef USE_READLINE char *s; static char *prev_hist = NULL; if (useReadline) s = readline(prompt); else + s = gets_basic(prompt); + + if (useHistory && s && s[0]) { ! enum histcontrol HC; ! HC = GetHistControlConfig(); ! ! if (((HC & hctl_ignorespace) && s[0] == ' ') || ! ((HC & hctl_ignoredups) && prev_hist && strcmp(s, prev_hist) == 0)) { ! /* Ignore this line as far as history is concerned */ ! } ! else { free(prev_hist); prev_hist = strdup(s); add_history(s); } } return s; + #else + return gets_basic(prompt); + #endif } *************** *** 126,142 **** initializeInput(int flags) { #ifdef USE_READLINE ! if (flags == 1) { useReadline = true; initialize_readline(); - } - #endif - - #ifdef USE_READLINE - if (flags == 1) - { - const char *home; useHistory = true; SetVariable(pset.vars, "HISTSIZE", "500"); --- 160,171 ---- initializeInput(int flags) { #ifdef USE_READLINE ! if (flags & 1) { + const char *home; + useReadline = true; initialize_readline(); useHistory = true; SetVariable(pset.vars, "HISTSIZE", "500"); *************** *** 172,189 **** #ifdef USE_READLINE if (useHistory && fname) { ! if (write_history(fname) != 0) ! { ! psql_error("could not save history to %s: %s\n", fname, strerror(errno)); ! return false; ! } return true; } - else - return false; - #else - return false; #endif } --- 201,214 ---- #ifdef USE_READLINE if (useHistory && fname) { ! if (write_history(fname) == 0) return true; + + psql_error("could not save history to %s: %s\n", fname, strerror(errno)); } #endif + + return false; } Index: src/bin/psql/input.h =================================================================== RCS file: /cvsroot/pgsql-server/src/bin/psql/input.h,v retrieving revision 1.18 diff -c -c -r1.18 input.h *** src/bin/psql/input.h 19 Feb 2003 04:04:04 -0000 1.18 --- src/bin/psql/input.h 20 Mar 2003 05:57:19 -0000 *************** *** 32,38 **** #endif #endif ! char *gets_interactive(char *prompt); char *gets_fromFile(FILE *source); void initializeInput(int flags); --- 32,39 ---- #endif #endif ! ! char *gets_interactive(const char *prompt); char *gets_fromFile(FILE *source); void initializeInput(int flags); Index: src/bin/psql/mainloop.c =================================================================== RCS file: /cvsroot/pgsql-server/src/bin/psql/mainloop.c,v retrieving revision 1.51 diff -c -c -r1.51 mainloop.c *** src/bin/psql/mainloop.c 12 Oct 2002 23:09:34 -0000 1.51 --- src/bin/psql/mainloop.c 20 Mar 2003 05:57:20 -0000 *************** *** 92,99 **** /* main loop to get queries and execute them */ while (1) { - #ifndef WIN32 - /* * Welcome code for Control-C */ --- 92,97 ---- *************** *** 113,118 **** --- 111,117 ---- cancel_pressed = false; } + #ifndef WIN32 if (sigsetjmp(main_loop_jmp, 1) != 0) { /* got here with longjmp */
I have applied the following patch to fix a bug in the new psql patch. It wasn't handling multi-line /* */ comments properly. Jeroen, would you review the change. The rest of the changes look very good. --------------------------------------------------------------------------- pgman wrote: > > Patch applied and attached. Thanks. > > I had to modify the handling of cancelConn. It wasn't properly being > defined and set in the original patch. > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073 Index: src/bin/psql/mainloop.c =================================================================== RCS file: /cvsroot/pgsql-server/src/bin/psql/mainloop.c,v retrieving revision 1.53 diff -c -c -r1.53 mainloop.c *** src/bin/psql/mainloop.c 20 Mar 2003 06:43:35 -0000 1.53 --- src/bin/psql/mainloop.c 20 Mar 2003 22:08:07 -0000 *************** *** 272,292 **** /* start of extended comment? */ else if (line[i] == '/' && line[i + thislen] == '*') ! { in_xcomment++; if (in_xcomment == 1) ! ADVANCE_1; ! } ! /* end of extended comment? */ ! else if (line[i] == '*' && line[i + thislen] == '/') { ! in_xcomment--; ! if (in_xcomment <= 0) { ! in_xcomment = 0; ! ADVANCE_1; ! } } /* start of quote? */ --- 272,295 ---- /* start of extended comment? */ else if (line[i] == '/' && line[i + thislen] == '*') ! { in_xcomment++; if (in_xcomment == 1) ! ADVANCE_1; ! } ! /* in or end of extended comment? */ ! else if (in_xcomment) { ! if (line[i] == '*' && line[i + thislen] == '/') { ! in_xcomment--; ! if (in_xcomment <= 0) ! { ! in_xcomment = 0; ! ADVANCE_1; ! } ! } } /* start of quote? */
OK, I applied your suggested cleanup. I don't think I can move the code
up or I would not be able to do nested /* */ comments, which the code
seems to support.
---------------------------------------------------------------------------
Jeroen T. Vermeulen wrote:
> On Thu, Mar 20, 2003 at 05:13:05PM -0500, Bruce Momjian wrote:
> >
> > I have applied the following patch to fix a bug in the new psql patch.
> > It wasn't handling multi-line /* */ comments properly. Jeroen, would
> > you review the change. The rest of the changes look very good.
>
> Yes, come to think of it I can see now that it also wouldn't deal
> with special sequences like '--' or unmatched quotes and parentheses
> very well either. Nor did the original version, from the looks of
> it, so I'd like to suggest moving the whole "if (in_xcomment)" clause
> up a little in the else-if chain, to just below the "if (in_quote)"
> one. That would make the thing ignore all special sequences except
> '*/' when inside an xcomment, and all sequences inside a quote
> (except a closing quote, of course). Plus, just in case any
> legitimate expression could give rise to the sequence "*/" ouside
> of an xcomment, that would also be handled properly.
>
> BTW your version could be simplified a little:
>
> /* [ "if (in_quote)" clause here ] */
>
> /* end of extended comment? */
> else if (in_xcomment)
> {
> if ((line[i] == '*') &&
> (line[i + thislen] == '/') &&
> !--in_xcomment)
> ADVANCE_1;
> }
>
> /* start of extended comment? */
> /* [ '/*' clause here ] */
>
>
> Lessee... I hope this says it all, but if you like I can submit
> a patch to today's CVS version.
>
>
> Jeroen
>
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Index: src/bin/psql/mainloop.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/psql/mainloop.c,v
retrieving revision 1.54
diff -c -c -r1.54 mainloop.c
*** src/bin/psql/mainloop.c 20 Mar 2003 22:08:50 -0000 1.54
--- src/bin/psql/mainloop.c 21 Mar 2003 03:26:40 -0000
***************
*** 281,295 ****
/* in or end of extended comment? */
else if (in_xcomment)
{
! if (line[i] == '*' && line[i + thislen] == '/')
! {
! in_xcomment--;
! if (in_xcomment <= 0)
! {
! in_xcomment = 0;
ADVANCE_1;
- }
- }
}
/* start of quote? */
--- 281,289 ----
/* in or end of extended comment? */
else if (in_xcomment)
{
! if (line[i] == '*' && line[i + thislen] == '/' &&
! !--in_xcomment)
ADVANCE_1;
}
/* start of quote? */
Configure already has provisions for the single-argument gettimeofday form. Please use that and remove the code below. Jeroen T. Vermeulen writes: + + /* Workarounds for Windows */ + /* Probably to be moved up the source tree in the future, perhaps to be replaced by + * more specific checks like configure-style HAVE_GETTIMEOFDAY macros. + */ + #ifndef WIN32 + + typedef struct timeval TimevalStruct; + #define GETTIMEOFDAY(T) gettimeofday(T, NULL) + #define DIFF_MSEC(T, U) ((((T)->tv_sec - (U)->tv_sec) * 1000000.0 + (T)->tv_usec - (U)->tv_usec) / 1000.0) + + #else + + typedef struct _timeb TimevalStruct; + #define GETTIMEOFDAY(T) _ftime(&T) + #define DIFF_MSEC(T, U) ((((T)->time - (U)->time) * 1000.0 + (T)->millitm - (U)->millitm)) + + #endif + + -- Peter Eisentraut peter_e@gmx.net
On Thu, Mar 20, 2003 at 05:13:05PM -0500, Bruce Momjian wrote:
>
> I have applied the following patch to fix a bug in the new psql patch.
> It wasn't handling multi-line /* */ comments properly. Jeroen, would
> you review the change. The rest of the changes look very good.
Yes, come to think of it I can see now that it also wouldn't deal
with special sequences like '--' or unmatched quotes and parentheses
very well either. Nor did the original version, from the looks of
it, so I'd like to suggest moving the whole "if (in_xcomment)" clause
up a little in the else-if chain, to just below the "if (in_quote)"
one. That would make the thing ignore all special sequences except
'*/' when inside an xcomment, and all sequences inside a quote
(except a closing quote, of course). Plus, just in case any
legitimate expression could give rise to the sequence "*/" ouside
of an xcomment, that would also be handled properly.
BTW your version could be simplified a little:
/* [ "if (in_quote)" clause here ] */
/* end of extended comment? */
else if (in_xcomment)
{
if ((line[i] == '*') &&
(line[i + thislen] == '/') &&
!--in_xcomment)
ADVANCE_1;
}
/* start of extended comment? */
/* [ '/*' clause here ] */
Lessee... I hope this says it all, but if you like I can submit
a patch to today's CVS version.
Jeroen
On Thu, Mar 20, 2003 at 10:40:03PM -0500, Bruce Momjian wrote: > > OK, I applied your suggested cleanup. I don't think I can move the code > up or I would not be able to do nested /* */ comments, which the code > seems to support. Ah, that's one I'd forgotten about. So you also need the '/*' clause above the "if (in_xcomment)" clause. Jeroen
Jeroen, did you see this request? --------------------------------------------------------------------------- Peter Eisentraut wrote: > Configure already has provisions for the single-argument gettimeofday > form. Please use that and remove the code below. > > > Jeroen T. Vermeulen writes: > > + > + /* Workarounds for Windows */ > + /* Probably to be moved up the source tree in the future, perhaps to be > replaced by > + * more specific checks like configure-style HAVE_GETTIMEOFDAY macros. > + */ > + #ifndef WIN32 > + > + typedef struct timeval TimevalStruct; > + #define GETTIMEOFDAY(T) gettimeofday(T, NULL) > + #define DIFF_MSEC(T, U) ((((T)->tv_sec - (U)->tv_sec) * 1000000.0 + > (T)->tv_usec - > (U)->tv_usec) / 1000.0) > + > + #else > + > + typedef struct _timeb TimevalStruct; > + #define GETTIMEOFDAY(T) _ftime(&T) > + #define DIFF_MSEC(T, U) ((((T)->time - (U)->time) * 1000.0 + > (T)->millitm - > (U)->millitm)) > + > + #endif > + > + > > -- > Peter Eisentraut peter_e@gmx.net > > > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faqs/FAQ.html > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073