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

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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)samurai(dot)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(at)xs4all(dot)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

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

"Jeroen T. Vermeulen" <jtv(at)xs4all(dot)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

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

"Jeroen T. Vermeulen" <jtv(at)xs4all(dot)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(at)xs4all(dot)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

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

"Jeroen T. Vermeulen" <jtv(at)xs4all(dot)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

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

"Jeroen T. Vermeulen" <jtv(at)xs4all(dot)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

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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)gmx(dot)net

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(at)gmx(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073