Thread: Concurrent psql patch
Fixed the major omissions that made it incomplete. - Added sgml documentation and \? usage - Added basic mvcc regression tests using new functionality - Fixed cursor-mode (FETCH_COUNT) functionality - Removed \cwait in favour of psql variable ASYNC_DELAY I'm still not sure it's quite polished enough to commit but if there's any feedback I'll be happy to fix up anything that's not acceptable. Also, if anyone has any better ideas for names than \cswitch and \cnowait now's the time. I had intended them only as placeholders because I couldn't think of anything better but it doesn't sound like anyone else has any better ideas either. If not then we're going to be stuck with them. More or less, it's explicitly described as an "experimental" feature in the docs so I suppose we could always change them later. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Attachment
On May 11, 2007, at 10:55 AM, Gregory Stark wrote: > Also, if anyone has any better ideas for names than \cswitch and > \cnowait > now's the time. I had intended them only as placeholders because I > couldn't > think of anything better but it doesn't sound like anyone else has > any better > ideas either. If not then we're going to be stuck with them. More > or less, > it's explicitly described as an "experimental" feature in the docs > so I > suppose we could always change them later. I don't see how we could make the names shorter without moving away from a backslash command (which I'm guessing would be painful). Assuming we're stuck with a backslash command \cs[witch] and \cn [owait] seem to be about as good as we could get. -- Jim Nasby jim@nasby.net EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
"Jim Nasby" <decibel@decibel.org> writes: > I don't see how we could make the names shorter without moving away from a > backslash command (which I'm guessing would be painful). > > Assuming we're stuck with a backslash command \cs[witch] and \cn > [owait] seem to be about as good as we could get. I don't have \cs or \cn set up as abbreviations. I was originally thinking \c1, \c2, ... for \cswitch and \c& for \cnowait. I'm not sure if going for cryptic short commands is better or worse here. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Sun, May 13, 2007 at 02:39:45PM +0100, Gregory Stark wrote: > "Jim Nasby" <decibel@decibel.org> writes: > > > I don't see how we could make the names shorter without moving > > away from a backslash command (which I'm guessing would be > > painful). > > > > Assuming we're stuck with a backslash command \cs[witch] and \cn > > [owait] seem to be about as good as we could get. > > I don't have \cs or \cn set up as abbreviations. > > I was originally thinking \c1, \c2, ... for \cswitch and \c& for > \cnowait. I'm not sure if going for cryptic short commands is better > or worse here. +1 for \c1, \c2, etc. What's the reasoning behind \c&? Does it "send things into the background" the way & does in the shell? Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ phone: +1 415 235 3778 AIM: dfetter666 Skype: davidfetter Remember to vote! Consider donating to PostgreSQL: http://www.postgresql.org/about/donate
"David Fetter" <david@fetter.org> writes: >> I was originally thinking \c1, \c2, ... for \cswitch and \c& for >> \cnowait. I'm not sure if going for cryptic short commands is better >> or worse here. > > +1 for \c1, \c2, etc. > > What's the reasoning behind \c&? Does it "send things into the > background" the way & does in the shell? Sort of. It sends the *subsequent* command to the background... And unlike the shell you can't usefully do anything more in the current session while the command is in the background, you have to manually switch sessions before issuing subsequent commands. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Sun, May 13, 2007 at 02:39:45PM +0100, Gregory Stark wrote: > "Jim Nasby" <decibel@decibel.org> writes: > > > I don't see how we could make the names shorter without moving away from a > > backslash command (which I'm guessing would be painful). > > > > Assuming we're stuck with a backslash command \cs[witch] and \cn > > [owait] seem to be about as good as we could get. > > I don't have \cs or \cn set up as abbreviations. > > I was originally thinking \c1, \c2, ... for \cswitch and \c& for \cnowait. I'm > not sure if going for cryptic short commands is better or worse here. Would \c# limit us to 9 concurrent connections? Might want \cs[witch] [session] which would switch to the specified session. If none specified, it would switch back to whatever session was previously active. \c& sounds fine (as do \c1...\c9). \g& would probably be helpful as well (send query buffer to server in nowait mode). -- Jim Nasby decibel@decibel.org EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
"Jim C. Nasby" <decibel@decibel.org> writes: > On Sun, May 13, 2007 at 02:39:45PM +0100, Gregory Stark wrote: >> >> I was originally thinking \c1, \c2, ... for \cswitch and \c& for \cnowait. I'm >> not sure if going for cryptic short commands is better or worse here. > > \c& sounds fine (as do \c1...\c9). \g& would probably be helpful as well > (send query buffer to server in nowait mode). Er, I just realized I typed the wrong thing there. It can't be \c& since I do assign a meaning to that "make a new connection to the same place as this one". I meant \& -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> "David Fetter" <david@fetter.org> writes:
>> What's the reasoning behind \c&? Does it "send things into the
>> background" the way & does in the shell?
> Sort of. It sends the *subsequent* command to the background...
That sounds just bizarre. Existing backslash commands that do something
to a SQL command are typed *after* the command they affect (\g for
instance). I don't think you should randomly change that.
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > Gregory Stark <stark@enterprisedb.com> writes: >> "David Fetter" <david@fetter.org> writes: >>> What's the reasoning behind \c&? Does it "send things into the >>> background" the way & does in the shell? > >> Sort of. It sends the *subsequent* command to the background... > > That sounds just bizarre. Existing backslash commands that do something > to a SQL command are typed *after* the command they affect (\g for > instance). I don't think you should randomly change that. So would you prefer \g& as Jim Nasby suggested? I hadn't even considered that previously since I'm not accustomed to using \g but it does seem kind of pretty. I normally use ; but I suppose there's nothing wrong with just declaring that asynchronous commands must be issued using \g& rather than use the semicolon to fire them off. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> So would you prefer \g& as Jim Nasby suggested? I hadn't even considered that
> previously since I'm not accustomed to using \g but it does seem kind of
> pretty. I normally use ; but I suppose there's nothing wrong with just
> declaring that asynchronous commands must be issued using \g& rather than use
> the semicolon to fire them off.
It makes sense to me... but what is the state of the session afterward?
Should this be combined with switching to another connection?
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > Gregory Stark <stark@enterprisedb.com> writes: >> So would you prefer \g& as Jim Nasby suggested? I hadn't even considered that >> previously since I'm not accustomed to using \g but it does seem kind of >> pretty. I normally use ; but I suppose there's nothing wrong with just >> declaring that asynchronous commands must be issued using \g& rather than use >> the semicolon to fire them off. > > It makes sense to me... but what is the state of the session afterward? > Should this be combined with switching to another connection? It's an interesting idea since you'll inevitably have to switch connections. If you issue a second query it'll forces the session to wait for the results. (It doesn't seem like there's any point in keeping a queue of pending queries per session.) However we do still need a command to switch back anyways so there doesn't seem to be any advantage in combining the two. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
"Jim C. Nasby" <decibel@decibel.org> writes: > Would \c# limit us to 9 concurrent connections? Might want > > \cs[witch] [session] Hm, we kind of have a choice with \c#. Either we treat it as part of the command in which case the way to connect to an integer-named database is to include a space. We could even have it magically connect to a database if the connection isn't already active. But these kinds of inconsistent behaviours can be traps for users. It means "\c1" and "\c 1" do different things even though "\cpostgres" and \c postgres" do the same thing. And it means "\c1" might connect to a database named "1" today but switch sessions tomorrow. Or we treat it as the first argument in which case even "\c 9" switches to session 9. I would prefer to do that but I fear there may be people with databases named "9". -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <stark@enterprisedb.com> writes:
> But these kinds of inconsistent behaviours can be traps for users. It means
> "\c1" and "\c 1" do different things even though "\cpostgres" and \c postgres"
> do the same thing. And it means "\c1" might connect to a database named "1"
> today but switch sessions tomorrow.
The real problem here is trying to overload an existing command name
with too many different meanings. You need to pick some other name
besides \c.
If you were willing to think of it as "switch session" instead of "connect",
then \S is available ...
regards, tom lane
On Mon, May 14, 2007 at 12:51:39PM +0100, Gregory Stark wrote: > "Tom Lane" <tgl@sss.pgh.pa.us> writes: > > > Gregory Stark <stark@enterprisedb.com> writes: > >> So would you prefer \g& as Jim Nasby suggested? I hadn't even considered that > >> previously since I'm not accustomed to using \g but it does seem kind of > >> pretty. I normally use ; but I suppose there's nothing wrong with just > >> declaring that asynchronous commands must be issued using \g& rather than use > >> the semicolon to fire them off. > > > > It makes sense to me... but what is the state of the session afterward? > > Should this be combined with switching to another connection? > > It's an interesting idea since you'll inevitably have to switch connections. > If you issue a second query it'll forces the session to wait for the results. > (It doesn't seem like there's any point in keeping a queue of pending queries > per session.) > > However we do still need a command to switch back anyways so there doesn't > seem to be any advantage in combining the two. I'd thought about this, and the question I came up with was: what connection should we switch to? First thought was to switch back to whatever connection we'd been using before this one, but then you'd quickly have 2 connections tied up... then what? If someone could come up with a logical session to connect to automatically that'd be great. In the meantime, what about allowing \g& accept a connection number to switch to? Also, I'd really love it if we could also do ';&'... I didn't mention it before because I'm assuming it's essentially not possible, but I'd like to be wrong... -- Jim Nasby decibel@decibel.org EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
On Mon, May 14, 2007 at 11:03:52AM -0400, Tom Lane wrote: > Gregory Stark <stark@enterprisedb.com> writes: > > But these kinds of inconsistent behaviours can be traps for users. It means > > "\c1" and "\c 1" do different things even though "\cpostgres" and \c postgres" > > do the same thing. And it means "\c1" might connect to a database named "1" > > today but switch sessions tomorrow. > > The real problem here is trying to overload an existing command name > with too many different meanings. You need to pick some other name > besides \c. > > If you were willing to think of it as "switch session" instead of "connect", > then \S is available ... Since this command will be getting used very frequently by anyone using concurrent connections interactively, it'd be nice if it was lower-case. It looks like that limits us to j, k, m, n, v, and y. In unix this idea is about jobs, what about using \j? -- Jim Nasby decibel@decibel.org EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
"Jim C. Nasby" <decibel@decibel.org> writes: > Since this command will be getting used very frequently by anyone using > concurrent connections interactively, it'd be nice if it was lower-case. > It looks like that limits us to j, k, m, n, v, and y. In unix this idea > is about jobs, what about using \j? Well currently it's not really terribly interesting to use interactively since you could always just start a second shell and run a second instance of psql. I really only have regression tests in mind for it. That's why I don't find it a problem at all to only extend \g and not semicolon handling. That said, I think a next step for this for interactive use would be to handle C-z to "background" the currently running query. So perhaps it does make sense to keep use cases like that when deciding on command names now. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Mon, May 14, 2007 at 06:26:42PM +0100, Gregory Stark wrote: > > "Jim C. Nasby" <decibel@decibel.org> writes: > > > Since this command will be getting used very frequently by anyone using > > concurrent connections interactively, it'd be nice if it was lower-case. > > It looks like that limits us to j, k, m, n, v, and y. In unix this idea > > is about jobs, what about using \j? > > Well currently it's not really terribly interesting to use interactively since > you could always just start a second shell and run a second instance of psql. > I really only have regression tests in mind for it. That's why I don't find it > a problem at all to only extend \g and not semicolon handling. > > That said, I think a next step for this for interactive use would be to handle > C-z to "background" the currently running query. So perhaps it does make sense > to keep use cases like that when deciding on command names now. Yeah, I think having the ability to open up another connection within psql will turn out to be very useful from an interactive standpoint; \c& (or whatever command we use to duplicate the current connection) is going to be a lot easier to enter than actually starting up a new psql in many production environments. -- Jim Nasby decibel@decibel.org EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
On Mon, May 14, 2007 at 11:55:07AM -0500, Jim C. Nasby wrote: > On Mon, May 14, 2007 at 11:03:52AM -0400, Tom Lane wrote: > > Gregory Stark <stark@enterprisedb.com> writes: > > > But these kinds of inconsistent behaviours can be traps for users. It means > > > "\c1" and "\c 1" do different things even though "\cpostgres" and \c postgres" > > > do the same thing. And it means "\c1" might connect to a database named "1" > > > today but switch sessions tomorrow. > > > > The real problem here is trying to overload an existing command name > > with too many different meanings. You need to pick some other name > > besides \c. > > > > If you were willing to think of it as "switch session" instead of "connect", > > then \S is available ... > > Since this command will be getting used very frequently by anyone using > concurrent connections interactively, it'd be nice if it was lower-case. > It looks like that limits us to j, k, m, n, v, and y. In unix this idea > is about jobs, what about using \j? I suppose there is some reason the bash/csh job control characters: %- %+ %1 won't work? -dg -- David Gould daveg@sonic.net If simplicity worked, the world would be overrun with insects.
So based on the feedback and suggestions here this is the interface I suggest:
\connect& - to open a new connection keeping the existing one
\g& - to submit a command asynchronously (like & in the shell)
\S [Sess#] - to _S_witch to a different _S_ession
- if no connection # specified list available _S_essions
\D - _D_isconnect from current session (like ^D in the shell)
This leaves no way to submit an asynchronous command without using \g but I'm
really not too concerned with that. I don't want to start messing with psql's
semicolon parsing behaviour and I'm mainly only concerned with this for
regression tests.
Another thought I had for the future is a \C command to simulate C-c and send
a query cancel. That would let us have regression tests that query
cancellation worked. The tests would presumably have to be written using
pg_sleep() to ensure they ran for long enough but even then there would be no
way to control exactly when the interrupt arrived.
Attached is an updated patch.
I also found and fixed some missing ResetCancelConn()s. I think I got them all
and the behaviour seems correct in practice when cancelling various
combinations of synchronous queries, asynchronous queries, and backslash
commands. The one thing I wonder about is that I'm a bit concerned I may have
introduced an assumption about how many resultsets arrive from a single query.
I'll be offline for a few days but I'll be back Monday.
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Gregory Stark wrote:
>
> So based on the feedback and suggestions here this is the interface I suggest:
>
> \connect& - to open a new connection keeping the existing one
> \g& - to submit a command asynchronously (like & in the shell)
> \S [Sess#] - to _S_witch to a different _S_ession
> - if no connection # specified list available _S_essions
> \D - _D_isconnect from current session (like ^D in the shell)
>
> This leaves no way to submit an asynchronous command without using \g but I'm
> really not too concerned with that. I don't want to start messing with psql's
> semicolon parsing behaviour and I'm mainly only concerned with this for
> regression tests.
>
> Another thought I had for the future is a \C command to simulate C-c and send
> a query cancel. That would let us have regression tests that query
> cancellation worked. The tests would presumably have to be written using
> pg_sleep() to ensure they ran for long enough but even then there would be no
> way to control exactly when the interrupt arrived.
>
> Attached is an updated patch.
>
> I also found and fixed some missing ResetCancelConn()s. I think I got them all
> and the behaviour seems correct in practice when cancelling various
> combinations of synchronous queries, asynchronous queries, and backslash
> commands. The one thing I wonder about is that I'm a bit concerned I may have
> introduced an assumption about how many resultsets arrive from a single query.
>
> I'll be offline for a few days but I'll be back Monday.
>
[ Attachment, skipping... ]
>
>
> --
> Gregory Stark
> EnterpriseDB http://www.enterprisedb.com
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
--
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. +
Hi Greg,
I looked at the patch briefly. I couldn't spot any issues and it looks good
to me. I've just couple of comments here.
The mvcc regression test files are missing in the patch.
--- 1179,1189 ----
dbname, user, password);
/* We can immediately discard the password -- no longer needed */
! if (password)
! {
! memset(password, '\0', strlen(password));
free(password);
+ }
Any reason why we do this ? "password" is anyways freed. I think you
might have left it behind after some debugging exercise.
--- 25,37 ----
#include "mb/pg_wchar.h"
#include "mbprint.h"
+ #if 0
+ #include "libpq-int.h" /* For PG_ASYNC */
+ #endif
+
This looks redundant..
Apart from that I really like consistent coding style. For example, to me,
"for (i = 0; i < 10; i++)" looks much better than "for (i=0;i<10; i++)"
This is not comment on your patch and neither I am saying
we should follow a specific coding style (though I wish we could have done
so) because we have already so many different styles. So its best to
stick to the coding style already followed in that particular file. But few
simple rules like having a single space around operators like '<', '+', '='
etc really makes the code more readable. Other examples are using
parenthesis in a right manner to improve code readability.
flag = (pointer == NULL); is more readable than
flag = pointer == NULL;
Thanks,
Pavan
--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com
Pavan Deolasee wrote:
> --- 1179,1189 ----
> dbname, user, password);
>
> /* We can immediately discard the password -- no longer needed */
> ! if (password)
> ! {
> ! memset(password, '\0', strlen(password));
> free(password);
> + }
>
>
> Any reason why we do this ? "password" is anyways freed. I think you
> might have left it behind after some debugging exercise.
I believe it's for security reasons. If that memory page is for example
swapped to disk after freeing the field, the password would be written
to the swapfile. Someone who steals your laptop would be able to recover
it from there. Clearing passwords from memory when they're no longer
needed is a good practice.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > Attached is an updated patch. > This patch appears to add a nonexistent test to the regression schedules. cheers andrew
"Andrew Dunstan" <andrew@dunslane.net> writes: > Gregory Stark wrote: >> Attached is an updated patch. >> > > This patch appears to add a nonexistent test to the regression schedules. I must have forgotten to cvs add it. Sorry. Also, I forgot to mention previously there is an unrelated trivial hunk in here. I noticed we free the password early, presumably for security reasons, but don't actually overwrite the memory at that point. I added a memset in there, otherwise the free seems kind of pointless. I normally don't bother with "security" features like that since they don't really add any security but as long as it's there it may as well do something vaguely useful. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Attachment
Am Montag, 21. Mai 2007 15:21 schrieb Gregory Stark: > Also, I forgot to mention previously there is an unrelated trivial hunk in > here. I noticed we free the password early, presumably for security > reasons, No, to save memory. -- Peter Eisentraut http://developer.postgresql.org/~petere/
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Gregory Stark wrote:
> "Andrew Dunstan" <andrew@dunslane.net> writes:
>
> > Gregory Stark wrote:
> >> Attached is an updated patch.
> >>
> >
> > This patch appears to add a nonexistent test to the regression schedules.
>
> I must have forgotten to cvs add it. Sorry.
>
> Also, I forgot to mention previously there is an unrelated trivial hunk in
> here. I noticed we free the password early, presumably for security reasons,
> but don't actually overwrite the memory at that point. I added a memset in
> there, otherwise the free seems kind of pointless. I normally don't bother
> with "security" features like that since they don't really add any security
> but as long as it's there it may as well do something vaguely useful.
>
[ Attachment, skipping... ]
>
> --
> Gregory Stark
> EnterpriseDB http://www.enterprisedb.com
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
--
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. +
Gregory Stark wrote: > "Andrew Dunstan" <andrew@dunslane.net> writes: > > >> Gregory Stark wrote: >> >>> Attached is an updated patch. >>> >>> >> This patch appears to add a nonexistent test to the regression schedules. >> > > I must have forgotten to cvs add it. Sorry. > > Also, I forgot to mention previously there is an unrelated trivial hunk in > here. I noticed we free the password early, presumably for security reasons, > but don't actually overwrite the memory at that point. I added a memset in > there, otherwise the free seems kind of pointless. I normally don't bother > with "security" features like that since they don't really add any security > but as long as it's there it may as well do something vaguely useful. > > Greg, In general this looks quite reasonable. It's a very large patch for a feature of this size, but luckily it's mostly changes due to the new pset structure rather than new code. There are some places that it doesn't use PG style (e.g. no newline before brace after if) and some comments that need to be fixed (e.g. /* XXX get result */ ) If you can fix that I'll apply the patch - I especially want to get it tested widely via the buildfarm. I am leaving town for about 5 or 6 days on Friday morning, and my availability will be somewhat restricted during that time, so if this isn't fixed pronto it will possibly have to wait till my return or till another reviewer takes pity on you :-) cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> There are some places that it doesn't use PG style (e.g. no newline
> before brace after if) and some comments that need to be fixed (e.g. /*
> XXX get result */ )
Of course you both realize pgindent will take care of the former ;-).
But yes, bogus comments are worth the trouble to fix now.
regards, tom lane
"Andrew Dunstan" <andrew@dunslane.net> writes: > Greg, > > In general this looks quite reasonable. It's a very large patch for a feature > of this size, but luckily it's mostly changes due to the new pset structure > rather than new code. Really? I expected there would be a few issues raised. For one about the need to use PG_ASYNC from libpq-int.h. Perhaps what I should do it split it into two patches, one which just adds \connect& and \S (switch connection) which will be the larger patch because it has to break out the connection structure like you mention. And a second which does the asynchronous response handling which is where there may be some questions about how to handle things. > There are some places that it doesn't use PG style (e.g. no newline before > brace after if) and some comments that need to be fixed (e.g. /* XXX get result > */ ) Ah, but it's not just the comment, if I put an XXX in it's definitely because there's something I'm not certain about. In this case now that I look at it again I think it's usually ok to ignore the result but in a session with ON_ERROR_STOP set (such as one running a script) we would want to exit I think. Another XXX is next to the include of libpq-int.h and a third one is where I have the pg_sleep loop instead of a select/poll loop. It occurs to me now that that loop should check cancel_pressed too. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote: > I expected there would be a few issues raised. For one about the need > to use PG_ASYNC from libpq-int.h. > Hmm, yes. Maybe we need to export that. I also see: + #if 0 + #include "libpq-int.h" /* For PG_ASYNC */ + #endif + which needs to disappear. If we're going to include libpq-int.h maybe we need to put it in common.h. Is there a reason that our own client programs shouldn't use our own library internals? > Perhaps what I should do it split it into two patches, one which just adds > \connect& and \S (switch connection) which will be the larger patch because it > has to break out the connection structure like you mention. And a second which > does the asynchronous response handling which is where there may be some > questions about how to handle things. > I don't think that's necessary. > >> There are some places that it doesn't use PG style (e.g. no newline before >> brace after if) and some comments that need to be fixed (e.g. /* XXX get result >> */ ) >> > > Ah, but it's not just the comment, if I put an XXX in it's definitely because > there's something I'm not certain about. In this case now that I look at it > again I think it's usually ok to ignore the result but in a session with > ON_ERROR_STOP set (such as one running a script) we would want to exit I > think. > Seems fair. > Another XXX is next to the include of libpq-int.h and a third one is where I > have the pg_sleep loop instead of a select/poll loop. It occurs to me now that > that loop should check cancel_pressed too. > > Are you talking about polling the connection? Using select/poll except on a socket is forbidden, because it breaks on Windows. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> If we're going to include libpq-int.h maybe we need to put it in
> common.h. Is there a reason that our own client programs shouldn't use
> our own library internals?
Seems like a really bad idea to me. I know I've cursed mysql more than
once for doing the equivalent. Also, if psql needs more than is
currently exported as official API, why wouldn't other programs need it
too? If there's more needed here, let's see an official API change,
not a hack.
regards, tom lane
Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> If we're going to include libpq-int.h maybe we need to put it in
>> common.h. Is there a reason that our own client programs shouldn't use
>> our own library internals?
>>
>
> Seems like a really bad idea to me. I know I've cursed mysql more than
> once for doing the equivalent. Also, if psql needs more than is
> currently exported as official API, why wouldn't other programs need it
> too? If there's more needed here, let's see an official API change,
> not a hack.
>
>
>
Well, I guess the obvious API would be something like:
PGAsyncStatusType PQasyncStatus(const PGconn *conn);
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> If there's more needed here, let's see an official API change,
>> not a hack.
> Well, I guess the obvious API would be something like:
> PGAsyncStatusType PQasyncStatus(const PGconn *conn);
That would mean exposing PGAsyncStatusType, which doesn't seem like an
amazingly good idea either. What is it that the patch actually wants
to do?
regards, tom lane
Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> Tom Lane wrote:
>>
>>> If there's more needed here, let's see an official API change,
>>> not a hack.
>>>
>
>
>> Well, I guess the obvious API would be something like:
>> PGAsyncStatusType PQasyncStatus(const PGconn *conn);
>>
>
> That would mean exposing PGAsyncStatusType, which doesn't seem like an
> amazingly good idea either. What is it that the patch actually wants
> to do?
>
The relevant snippet in command.c says:
/* If we have async_delay set then we need to allow up to that many
* milliseconds for any responses to come in on *either* connection.
* This ensures that results are printed in a relatively deterministic
* order for regression tests. Note that we only have to allow for n
* milliseconds total between the two connections so we don't reset the
* timer for the second wait.
*
* XXX this should maybe be changed to a select/poll loop instead
*/
if (pset.async_delay > 0 && pset.c->db)
{
GETTIMEOFDAY(&start);
do {
if (pset.c->db->asyncStatus != PGASYNC_BUSY)
{
break;
}
if (CheckQueryResults())
{
ReadQueryResults();
break;
}
pg_usleep(10000);
GETTIMEOFDAY(&now);
} while (DIFF_MSEC(&now, &start) < pset.async_delay);
}
pset.c = &cset[slot-1];
printf(_("[%d] You are now connected to database \"%s\"\n"), slot,
PQdb(pset.c->db));
if (pset.async_delay > 0 && pset.c->db)
{
do {
if (pset.c->db->asyncStatus != PGASYNC_BUSY)
break;
if (CheckQueryResults()) {
ReadQueryResults();
break;
}
pg_usleep(10000);
GETTIMEOFDAY(&now);
} while (DIFF_MSEC(&now, &start) < pset.async_delay);
}
and in mainloop.c it's used like this:
if (pset.c->db && pset.c->db->asyncStatus != PGASYNC_IDLE &&
CheckQueryResults()) {
ReadQueryResults();
/* If we processed a query cancellation cancel_pressed may
still be
* set and will interrupt the processing of the next command
unless
* we start the main loop over to reset it. */
if (cancel_pressed)
break;
}
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> if (pset.c->db->asyncStatus != PGASYNC_BUSY)
> {
> break;
> }
There already is a defined API for this, namely PQisBusy().
In any case, I rather concur with the XXX comment: busy-waiting like
this sucks. The correct way to do this is to get the socket numbers for
the connections (via PQsocket), wait for any of them to be read-ready
according to select() (or for the timeout to elapse, assuming that we
think that behavior is good), then cycle through PQconsumeInput() and
PQisBusy() on each connection. See
http://www.postgresql.org/docs/8.2/static/libpq-async.html
regards, tom lane
Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> if (pset.c->db->asyncStatus != PGASYNC_BUSY)
>> {
>> break;
>> }
>>
>
> There already is a defined API for this, namely PQisBusy().
>
> In any case, I rather concur with the XXX comment: busy-waiting like
> this sucks. The correct way to do this is to get the socket numbers for
> the connections (via PQsocket), wait for any of them to be read-ready
> according to select() (or for the timeout to elapse, assuming that we
> think that behavior is good), then cycle through PQconsumeInput() and
> PQisBusy() on each connection. See
> http://www.postgresql.org/docs/8.2/static/libpq-async.html
>
>
>
In that case I guess Greg has some work to do :-) . Looks like there
are about five such calls in toto, so it's not a huge tragedy.
cheers
andrew
"Andrew Dunstan" <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>
>>> if (pset.c->db->asyncStatus != PGASYNC_BUSY)
>>> {
>>> break;
>>> }
>>>
>>
>> There already is a defined API for this, namely PQisBusy().
Oh, now I remember why I'm including libpq-int.h. It's not for PGASYNC_BUSY as
above. The case above can indeed be fixed using PQIsBusy() (and select/poll
looping).
The missing interface is for PGASYNC_IDLE. The problem is that I didn't really
want to have psql go through the trouble to check all the connections for
waiting output if it didn't have any queries pending.
This could be fixed by having psql track which connections are waiting for
query results. It's a bit annoying to have two state bits that hold the same
data at two different levels of abstraction though.
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
"Tom Lane" <tgl@sss.pgh.pa.us> writes:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> if (pset.c->db->asyncStatus != PGASYNC_BUSY)
>> {
>> break;
>> }
>
> There already is a defined API for this, namely PQisBusy().
>
> In any case, I rather concur with the XXX comment: busy-waiting like
> this sucks. The correct way to do this is to get the socket numbers for
> the connections (via PQsocket), wait for any of them to be read-ready
> according to select() (or for the timeout to elapse, assuming that we
> think that behavior is good), then cycle through PQconsumeInput() and
> PQisBusy() on each connection. See
> http://www.postgresql.org/docs/8.2/static/libpq-async.html
Huh, so it turns out we already have code that does exactly this in
pqSocketPoll and pqSocketCheck. Except that they have too little resolution
because they work with time_t which means we would have to wait at least 1-2
seconds.
And pqSocketCheck keeps looping when it gets an EINTR which doesn't seem like
the right thing for psql to do.
It would be nice to use these functions though because:
a) They get the SSL case right in that that they check the SSL buffer before
calling select/poll.
b) They use poll if available and fall back to select
c) they would keep the select/poll system code out of psql where there's none
of it currently.
So would I be better off adding a PQSocketPollms() which works in milliseconds
instead of seconds? Or should I just copy all this code into psql?
--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Gregory Stark wrote:
> "Andrew Dunstan" <andrew@dunslane.net> writes:
>
>
>> Tom Lane wrote:
>>
>>> Andrew Dunstan <andrew@dunslane.net> writes:
>>>
>>>
>>>> if (pset.c->db->asyncStatus != PGASYNC_BUSY)
>>>> {
>>>> break;
>>>> }
>>>>
>>>>
>>> There already is a defined API for this, namely PQisBusy().
>>>
>
> Oh, now I remember why I'm including libpq-int.h. It's not for PGASYNC_BUSY as
> above. The case above can indeed be fixed using PQIsBusy() (and select/poll
> looping).
>
> The missing interface is for PGASYNC_IDLE. The problem is that I didn't really
> want to have psql go through the trouble to check all the connections for
> waiting output if it didn't have any queries pending.
>
> This could be fixed by having psql track which connections are waiting for
> query results. It's a bit annoying to have two state bits that hold the same
> data at two different levels of abstraction though.
>
>
Unless you have a better solution I think we'd better do that, though.
I notice that the header is also included in command.c even though it
doesn't use PGASYNC_IDLE.
Are you going to make these fixes?
cheers
andrew
"Andrew Dunstan" <andrew@dunslane.net> writes: > Unless you have a better solution I think we'd better do that, though. > > I notice that the header is also included in command.c even though it doesn't > use PGASYNC_IDLE. > > Are you going to make these fixes? Yes, sorry, I started to already but got distracted by some tests I've been running. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Author unresponsive to request for updated patch.
This patch has been saved for the 8.4 release:
http://momjian.postgresql.org/cgi-bin/pgpatches_hold
---------------------------------------------------------------------------
Gregory Stark wrote:
> "Andrew Dunstan" <andrew@dunslane.net> writes:
>
> > Unless you have a better solution I think we'd better do that, though.
> >
> > I notice that the header is also included in command.c even though it doesn't
> > use PGASYNC_IDLE.
> >
> > Are you going to make these fixes?
>
> Yes, sorry, I started to already but got distracted by some tests I've been running.
>
>
> --
> Gregory Stark
> EnterpriseDB http://www.enterprisedb.com
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
--
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. +
This has been saved for the next commit-fest:
http://momjian.postgresql.org/cgi-bin/pgpatches_hold
---------------------------------------------------------------------------
Gregory Stark wrote:
> "Andrew Dunstan" <andrew@dunslane.net> writes:
>
> > Gregory Stark wrote:
> >> Attached is an updated patch.
> >>
> >
> > This patch appears to add a nonexistent test to the regression schedules.
>
> I must have forgotten to cvs add it. Sorry.
>
> Also, I forgot to mention previously there is an unrelated trivial hunk in
> here. I noticed we free the password early, presumably for security reasons,
> but don't actually overwrite the memory at that point. I added a memset in
> there, otherwise the free seems kind of pointless. I normally don't bother
> with "security" features like that since they don't really add any security
> but as long as it's there it may as well do something vaguely useful.
>
[ Attachment, skipping... ]
>
> --
> Gregory Stark
> EnterpriseDB http://www.enterprisedb.com
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes: > This has been saved for the next commit-fest: > http://momjian.postgresql.org/cgi-bin/pgpatches_hold Er, why "saved"? Until there's a new patch submission there's not going to be more work to do on this in the next fest. I think maybe you need to think a bit harder about the distinction between your TODO-items-in-waiting list and the commit fest queue. I was willing to wade through a pile of TODO-items-in-waiting this time, because I pressed you to make the list available before having sorted through it; but I'm not going to be pleased to see those same threads in the fest queue next time, unless someone's done some actual work in between. regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > This has been saved for the next commit-fest: > > http://momjian.postgresql.org/cgi-bin/pgpatches_hold > > Er, why "saved"? Until there's a new patch submission there's not going > to be more work to do on this in the next fest. > > I think maybe you need to think a bit harder about the distinction > between your TODO-items-in-waiting list and the commit fest queue. > I was willing to wade through a pile of TODO-items-in-waiting this > time, because I pressed you to make the list available before having > sorted through it; but I'm not going to be pleased to see those same > threads in the fest queue next time, unless someone's done some actual > work in between. It is in the next fest so I will remember to ask if people have done any work on them --- if not they are either deleted or moved to the next commit fest. Are you suggesting we just delete the threads and let them die if they don't submit a new version? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
> Are you suggesting we just delete the threads and let them die if they
> don't submit a new version?
I am suggesting that they are not material for another commit fest until
some new work has been done. (And the appearance of that new work would
trigger an entry being made in the pending-commit-fest list.)
I've surely got no objection to you hanging on to such threads in your
personal almost-TODO list, and prodding people when appropriate. But
the commit fest queue is not for that purpose.
regards, tom lane
Bruce Momjian wrote: > Tom Lane wrote: > >> Bruce Momjian <bruce@momjian.us> writes: >> >>> This has been saved for the next commit-fest: >>> http://momjian.postgresql.org/cgi-bin/pgpatches_hold >>> >> Er, why "saved"? Until there's a new patch submission there's not going >> to be more work to do on this in the next fest. >> >> I think maybe you need to think a bit harder about the distinction >> between your TODO-items-in-waiting list and the commit fest queue. >> I was willing to wade through a pile of TODO-items-in-waiting this >> time, because I pressed you to make the list available before having >> sorted through it; but I'm not going to be pleased to see those same >> threads in the fest queue next time, unless someone's done some actual >> work in between. >> > > It is in the next fest so I will remember to ask if people have done any > work on them --- if not they are either deleted or moved to the next > commit fest. > > Are you suggesting we just delete the threads and let them die if they > don't submit a new version? > > My understanding was that all items in a commit-fest have one of these three dispositions: . committed . rejected . referred back to author for more work We're really only interested in the third one here, and so, yes, the ball should be in the author's court, not yours. I don't see any reason for you to move items from one queue to another like that. It just looks like it's making work. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> My understanding was that all items in a commit-fest have one of these
> three dispositions:
> . committed
> . rejected
> . referred back to author for more work
Right. But Bruce's personal queue has got a different lifecycle:
items get removed when they are resolved by a committed patch, or
by being rejected as not wanted, or by being summarized on the public
TODO list. For what he's doing that's a very good definition ---
things don't get forgotten just because nothing has happened lately.
But it's becoming clearer to me that the commit-fest queue has to be
a separate animal. We used Bruce's queue as the base this time around,
because we had no other timely-available source of the raw data.
Seems like it's time to split them, though.
If we do split them then there is going to be some added effort to
maintain the commit fest queue. Bruce has made it pretty clear
that he doesn't want to put in any extra cycles here. So someone
else has to step up to the plate if this is going to work.
Any volunteers out there?
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > My understanding was that all items in a commit-fest have one of these > > three dispositions: > > > . committed > > . rejected > > . referred back to author for more work > > Right. But Bruce's personal queue has got a different lifecycle: > items get removed when they are resolved by a committed patch, or > by being rejected as not wanted, or by being summarized on the public > TODO list. For what he's doing that's a very good definition --- > things don't get forgotten just because nothing has happened lately. > But it's becoming clearer to me that the commit-fest queue has to be > a separate animal. We used Bruce's queue as the base this time around, > because we had no other timely-available source of the raw data. > Seems like it's time to split them, though. Right, if the patch author stops working on it, but it is a feature we want, the thread goes on the TODO list (or we complete the patch), so yes, it is a different life-cycle. > If we do split them then there is going to be some added effort to > maintain the commit fest queue. Bruce has made it pretty clear > that he doesn't want to put in any extra cycles here. So someone > else has to step up to the plate if this is going to work. > Any volunteers out there? I assumed the wiki was going to be the official patch list from now on and my web pages were just going to be a public display of things I was tracking. Frankly, I haven't been putting anything on the queue for the next commit fest now except stuff that was already in-process for this commit fest. The ideas is that we can commit stuff that has appeared since the commit fest started before the next commit fest starts. I also moved the emails to the next commit fest queue because that preserves the comments made too. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Andrew Dunstan wrote: > > Are you suggesting we just delete the threads and let them die if they > > don't submit a new version? > > > > > > My understanding was that all items in a commit-fest have one of these > three dispositions: > > . committed > . rejected > . referred back to author for more work > > We're really only interested in the third one here, and so, yes, the > ball should be in the author's court, not yours. I don't see any reason > for you to move items from one queue to another like that. It just looks > like it's making work. True. I could move the emails back to my private mailbox and just track them there too. I moved them so it would be visible we were waiting for some people. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +