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

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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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

---------------------------(end of broadcast)---------------------------
TIP 7: You can help support the PostgreSQL project by donating at

http://www.postgresql.org/about/donate

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

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

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/s...bpq-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

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/s...bpq-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 majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

"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


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

"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/s...bpq-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

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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster