Re: Win32 open items

"Magnus Hagander" <mha@sollentuna.net> writes:

Here is an attempt at this. First patch contains the changes to libpq,
second patch contains changes to psql to use this API. Docs not updated
yet, pending approval of the general idea at least :)

I think it would be better to dispense with the PQgetCancelError
function and just make the signature of PQcancel be
int PQcancel(PGcancel *cancel, char *errbuf, int errbuflen);
where errbuf would normally point to a local array in the calling
function.

As-is, PQcancel is itself not thread safe because it is scribbling
on the PGcancel struct. I thought the whole point of this exercise
was to allow multiple threads to use the PGcancel struct; which seems
to me to imply that it had better be read-only to PQcancel.

We don't need the cancelConnLock if this is done properly (at least,
assuming that storing a pointer is atomic, which seems reasonable).

regards, tom lane

Here is an attempt at this. First patch contains the changes

to libpq,

second patch contains changes to psql to use this API. Docs

not updated

yet, pending approval of the general idea at least :)

I think it would be better to dispense with the PQgetCancelError
function and just make the signature of PQcancel be
int PQcancel(PGcancel *cancel, char *errbuf, int errbuflen);
where errbuf would normally point to a local array in the calling
function.

As-is, PQcancel is itself not thread safe because it is scribbling
on the PGcancel struct. I thought the whole point of this exercise
was to allow multiple threads to use the PGcancel struct; which seems
to me to imply that it had better be read-only to PQcancel.

Um. Right. I somehow had it in my head that the 1 thread <-> 1 PGcancel
would always hold, in which case this would not be a problem. Now that
you put it like this, I have no idea why I made that assumption.
I'll get a new patch together using that syntax.

We don't need the cancelConnLock if this is done properly (at least,
assuming that storing a pointer is atomic, which seems reasonable).

Are you sure about this?
Per what docs I have, storing a pointer should always be atomic.
exchanging two pointers are not, which is why at least win32 provides a
specific function to do that (InterlockedExchangePointer).

Anyway, consider this scenario. Thread A is the mainloop thread, Thread
B is the thread that handles Ctrl-C. What if Thread B starts its run and
starts reading off the pointer. Before it's done, it's pre-empted, and
Thread A starts executing. Thread A does a free() on the memory pointed
to by the pointer. When control goes back to Thread B, it will definitly
die.
The fact that Thread B makes kernel socket calls for possibly remote
communications only makes the probability of Thread B actually being
switched out a whole lot higher.

Or are you seeing a scenario that prevents this from happening? (I guess
we could just ignore free:ing the memory, but that seems excessibly
ugly)

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

We don't need the cancelConnLock if this is done properly (at least,
assuming that storing a pointer is atomic, which seems reasonable).

Anyway, consider this scenario. Thread A is the mainloop thread, Thread
B is the thread that handles Ctrl-C. What if Thread B starts its run and
starts reading off the pointer. Before it's done, it's pre-empted, and
Thread A starts executing. Thread A does a free() on the memory pointed
to by the pointer. When control goes back to Thread B, it will definitly
die.

Good point. Never mind that claim then ...

regards, tom lane

We don't need the cancelConnLock if this is done properly (at least,
assuming that storing a pointer is atomic, which seems reasonable).

Anyway, consider this scenario. Thread A is the mainloop

thread, Thread

B is the thread that handles Ctrl-C. What if Thread B starts

its run and

starts reading off the pointer. Before it's done, it's

pre-empted, and

Thread A starts executing. Thread A does a free() on the

memory pointed

to by the pointer. When control goes back to Thread B, it

will definitly

die.

Good point. Never mind that claim then ...

Ok, here we go with a new patch.

I started looking at docs, and reliased the PQrequestCancel() function
is documented under "async query processing". Is that really the correct
place? It has a mention that it's safe to use in signal handlers and can
thus be used by PQexec, but if you skip the async part because you're
not using async, you will never know you can cancel a query...

Not sure what to make of it. So here is a code-only patch. Most of the
docs would just be a copy of the old with a renamed patch anyway, but if
it should be restructured it should probably be done by someone who
knows a bit more about it.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

Ok, here we go with a new patch.

Applied with a few minor revisions.

I started looking at docs, and reliased the PQrequestCancel() function
is documented under "async query processing". Is that really the correct
place?

Probably not. I made a new subsection for it.

regards, tom lane

Magnus Hagander wrote:

We don't need the cancelConnLock if this is done properly (at least,
assuming that storing a pointer is atomic, which seems reasonable).

Are you sure about this?
Per what docs I have, storing a pointer should always be atomic.
exchanging two pointers are not, which is why at least win32 provides a
specific function to do that (InterlockedExchangePointer).

You can't even assume a pointer write is atomic to all threads if you
have multiple CPUs. Assume two CPU's. Even if CPU 1 writes the pointer
atomically, there is no guarantee that the other CPU will see the change
at the same time. To guarantee it, you need a memory barrier like a
lock/unlock. Some CPU systems guarantee memory conherency for memory
operations, but some do not.

It is temping to think that if one CPU can write a value atomically then
the other CPU will also see it at the same time, but that isn't
guaranteed.

For the hardware issues see:

http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-toolbox.html

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