Re: Win32 open items
* Query cancel in psql
I see two ways of doing this. One is to take the easy way
out and just
add a console ctrl handler and let it do a standard query
cancel. The
problem with this is that PQrequestCancel() is not
thread-safe. Which
means we have a possible crash there, but only if we're either
PQfinish():ing the connection in the main thread, or if we're
accessing the error members (either reading or setting). If this is
acceptable, the fix for psql is really simple.If this is not acceptable, we have to do something along
the line of
what's done in the backend to handle signals - we need to
implement a
"signals aware" PQexec(). This function would have to call
the async
query functions, and then wait for both these and an
internal event to
look for cancel events. This is clearly the safest bet, but it's
definitly more work.Anybody have any other ideas that happen to be as simple as
the first
optino but safer? Please ;-)
I have an idea on this one. I think we need to put locking around
access to psql/common.c::cancelConn so that we are sure the
signal handler is not accessing it while it is being
modified. We already have thread locking primitives for
Win32 in libpq and we are sure the standard C functions are
thread-safe on WIn32 because there are no special thread
compile flags for Win32.
We also might need to make
cancelConn a copy of the PGconn structure rather than just a pointer.
It won't work unless you make it a copy, I think. Because you'd have to
lock the entire structure whenever you called PQexec() (since you don't
know when PQexec does things), and that's just the time when you'd need
to use it from the other thread...
As for making a copy, yes, that's the other optino I had, but completely
had lost when I wrote that mail. However, it cannot be done in just
pqsl, because psql doesn't see the contents of PGconn. We'd need at
least one new libpq function to create a duplicate PGconn. In order to
be safe, this should probably be a for-cancel-only-copy, so we shouldn't
copy the whole PGconn. But we could copy whatever fields are required to
cancel the query, which in turn would require a new cancelilng function
that accepted this limited structure.
Probably doable, but it requires API additions, I think.
I can't think of how to simulate the longjump() currently
used when cancelConn is NULL though.
Um. No need for that on win32, I think. When cancelConn is NULL (I am
now assuming we have solved the fact that cancelConn has to be a copy),
we can just do a "return" from the signal handler. The therad will then
happily end, and we're all fine. Or is it supposed to do something other
than just ignoring the cancel request if cancelconn is NULL?
//Magnus
Magnus Hagander wrote:
It won't work unless you make it a copy, I think. Because you'd have to
lock the entire structure whenever you called PQexec() (since you don't
know when PQexec does things), and that's just the time when you'd need
to use it from the other thread...As for making a copy, yes, that's the other optino I had, but completely
had lost when I wrote that mail. However, it cannot be done in just
pqsl, because psql doesn't see the contents of PGconn. We'd need at
least one new libpq function to create a duplicate PGconn. In order to
be safe, this should probably be a for-cancel-only-copy, so we shouldn't
copy the whole PGconn. But we could copy whatever fields are required to
cancel the query, which in turn would require a new cancelilng function
that accepted this limited structure.Probably doable, but it requires API additions, I think.
I thought about this. There are a lot of pointers in PGconn, and most
we don't use so I don't like the idea of adding a new API to copy the
complex PGconn structure just for Win32 psql cancel. Looking at the
PQrequestCancel() code, it only writes to errorMessage and reads from
everything else, and I don't see any of those changing (except
errorMessage) once the connection is established, so we could do a
non-deep copy of the structure and reuse all the existing structure
pointers. We would just need to create a new errorMessage structure
because PQrequestCancel() wants to write to that.
We do need to use locking while we create/destroy this structure.
I can't think of how to simulate the longjump() currently
used when cancelConn is NULL though.Um. No need for that on win32, I think. When cancelConn is NULL (I am
now assuming we have solved the fact that cancelConn has to be a copy),
we can just do a "return" from the signal handler. The therad will then
happily end, and we're all fine. Or is it supposed to do something other
than just ignoring the cancel request if cancelconn is NULL?
Agreed, we can just ignore it.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Magnus Hagander wrote:
Probably doable, but it requires API additions, I think.
I thought about this. There are a lot of pointers in PGconn, and most
we don't use so I don't like the idea of adding a new API to copy the
complex PGconn structure just for Win32 psql cancel. Looking at the
PQrequestCancel() code, it only writes to errorMessage and reads from
everything else, and I don't see any of those changing (except
errorMessage) once the connection is established, so we could do a
non-deep copy of the structure and reuse all the existing structure
pointers. We would just need to create a new errorMessage structure
because PQrequestCancel() wants to write to that.
I think Magnus had the right idea. We should invent a completely
separate opaque struct that contains *only* the fields that
PQrequestCancel actually needs (the hostname, port, and secret key;
anything else?) and make a function to create one of these from a
PQconn. This struct could then be read-only as far as the thread-safe
variant of PQrequestCancel is concerned.
The error message return convention used by PQrequestCancel leaves a lot
to be desired as well; I'd be inclined to think of something else for
the new variant of PQrequestCancel. Possibly have the caller supply a
buffer to write into.
We could probably reimplement the existing PQrequestCancel on top of the
cleaner version, or at least find some way to share code. But
basically, the API it has now is pretty bogus. (I think I can say that,
since I invented it ;-))
regards, tom lane