Re: [PATCHES] Concurrent psql patch
This has been saved for the next commit-fest: http://momjian.postgresql.org/cgi-bin/pgpatches_hold --- Gregory Stark wrote: Andrew Dunstan [EMAIL PROTECTED] 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 [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Bruce Momjian [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Bruce Momjian [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Bruce Momjian wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Andrew Dunstan [EMAIL PROTECTED] 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] 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 [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
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 [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Concurrent psql patch
Gregory Stark wrote:
Andrew Dunstan [EMAIL PROTECTED] writes:
Tom Lane wrote:
Andrew Dunstan [EMAIL PROTECTED] 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
---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Concurrent psql patch
Andrew Dunstan [EMAIL PROTECTED] 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
Re: [PATCHES] Concurrent psql patch
Tom Lane [EMAIL PROTECTED] writes:
Andrew Dunstan [EMAIL PROTECTED] 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
---(end of broadcast)---
TIP 6: explain analyze is your friend
Re: [PATCHES] Concurrent psql patch
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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Concurrent psql patch
Andrew Dunstan [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Concurrent psql patch
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
Andrew Dunstan [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Concurrent psql patch
Tom Lane wrote:
Andrew Dunstan [EMAIL PROTECTED] 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(1);
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(1);
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
---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] Concurrent psql patch
Andrew Dunstan [EMAIL PROTECTED] 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
---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at
http://www.postgresql.org/about/donate
Re: [PATCHES] Concurrent psql patch
Tom Lane wrote:
Andrew Dunstan [EMAIL PROTECTED] 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
---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] Concurrent psql patch
Gregory Stark wrote: Andrew Dunstan [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Concurrent psql patch
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Concurrent psql patch
Andrew Dunstan [EMAIL PROTECTED] 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. concurrent-psql-v9.patch.gz Description: Binary data -- Gregory Stark EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
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/ ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Concurrent psql patch
Gregory Stark wrote: Attached is an updated patch. This patch appears to add a nonexistent test to the regression schedules. cheers andrew ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Concurrent psql patch
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
---(end of broadcast)---
TIP 6: explain analyze is your friend
Re: [PATCHES] Concurrent psql patch
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;i10; 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
Re: [PATCHES] Concurrent psql patch
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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Concurrent psql patch
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. concurrent-psql-v8.patch.gz Description: Binary data -- Gregory Stark EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
Tom Lane [EMAIL PROTECTED] writes: Gregory Stark [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Concurrent psql patch
Jim C. Nasby [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Concurrent psql patch
Gregory Stark [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
On Mon, May 14, 2007 at 12:51:39PM +0100, Gregory Stark wrote: Tom Lane [EMAIL PROTECTED] writes: Gregory Stark [EMAIL PROTECTED] 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 [EMAIL PROTECTED] EnterpriseDB http://enterprisedb.com 512.569.9461 (cell) ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Concurrent psql patch
On Mon, May 14, 2007 at 11:03:52AM -0400, Tom Lane wrote: Gregory Stark [EMAIL PROTECTED] 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 [EMAIL PROTECTED] EnterpriseDB http://enterprisedb.com 512.569.9461 (cell) ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
Jim C. Nasby [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Concurrent psql patch
On Mon, May 14, 2007 at 06:26:42PM +0100, Gregory Stark wrote: Jim C. Nasby [EMAIL PROTECTED] 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 [EMAIL PROTECTED] EnterpriseDB http://enterprisedb.com 512.569.9461 (cell) ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Concurrent psql patch
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] If simplicity worked, the world would be overrun with insects. ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
Jim Nasby [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
On Sun, May 13, 2007 at 02:39:45PM +0100, Gregory Stark wrote: Jim Nasby [EMAIL PROTECTED] 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 [EMAIL PROTECTED] http://fetter.org/ phone: +1 415 235 3778AIM: dfetter666 Skype: davidfetter Remember to vote! Consider donating to PostgreSQL: http://www.postgresql.org/about/donate ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] Concurrent psql patch
David Fetter [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
On Sun, May 13, 2007 at 02:39:45PM +0100, Gregory Stark wrote: Jim Nasby [EMAIL PROTECTED] 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 [EMAIL PROTECTED] EnterpriseDB http://enterprisedb.com 512.569.9461 (cell) ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Concurrent psql patch
Jim C. Nasby [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Concurrent psql patch
Gregory Stark [EMAIL PROTECTED] writes: David Fetter [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Concurrent psql patch
Tom Lane [EMAIL PROTECTED] writes: Gregory Stark [EMAIL PROTECTED] writes: David Fetter [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Concurrent psql patch
Gregory Stark [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Concurrent psql patch
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[EMAIL PROTECTED] EnterpriseDB http://enterprisedb.com 512.569.9461 (cell) ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[PATCHES] 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. concurrent-psql-v7.patch.gz Description: Binary data -- Gregory Stark EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
