Win32 semaphore patch

In PGSemaphoreLock, should't ImmediateInterruptOK be reset in all cases?
It's not reset if we get the semaphore now. Though I won't claim to
entirely know what that flag does ;-)

(btw, I got double newlines all over)

//Magnus

Qingqing Zhou <zhouqq@cs.toronto.edu> writes:

Attached is a patch for Win32 semaphore reimplementation per discussion
with Tom and Magnus.

The trickiest part of the sysv and posix implementations is making sure
that any kernel resources represented by the semaphores will go away at
appropriate times. There are two cases to worry about:

1. Backend crash while postmaster stays alive: postmaster will get to
execute the on_shmem_exit() callback after all the backends are gone,
and that can clean 'em up.

2. postmaster crashes: we'd like the semas to go away automatically when
the last backend goes away. If this doesn't happen, then there has to
be logic to recycle leftover semas when the postmaster is next started.

Most of the ugliness in sysv_sema.c is because it has to do "manual"
cleanup per #2.

I dunno much about Windows, and it may be that your code is already OK
on this score. I'm just pointing it out as something that has to be
considered. Some comments in the code about how this works wouldn't
be a bad idea.

regards, tom lane

Attached is a patch for Win32 semaphore reimplementation per
discussion with Tom and Magnus.

The trickiest part of the sysv and posix implementations is
making sure that any kernel resources represented by the
semaphores will go away at appropriate times. There are two
cases to worry about:

1. Backend crash while postmaster stays alive: postmaster
will get to execute the on_shmem_exit() callback after all
the backends are gone, and that can clean 'em up.

2. postmaster crashes: we'd like the semas to go away
automatically when the last backend goes away. If this
doesn't happen, then there has to be logic to recycle
leftover semas when the postmaster is next started.

Most of the ugliness in sysv_sema.c is because it has to do "manual"
cleanup per #2.

I dunno much about Windows, and it may be that your code is
already OK on this score. I'm just pointing it out as
something that has to be considered. Some comments in the
code about how this works wouldn't be a bad idea.

For #2, yes, the semaphores will go away when the last process holding a
HANDLE to it goes away. For #1, the code seems to handle that right?

//Magnus

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

For #2, yes, the semaphores will go away when the last process holding a
HANDLE to it goes away.

Well, that raises an interesting point: exactly where in this code does
ownership of the HANDLEs get propagated to the child processes? As
written, the HANDLEs seem to belong only to the postmaster --- will the
kernel calls even work in the child processes? According to what
someone was telling me the other day, HANDLEs are process-local, so just
storing them in shared memory doesn't seem like it should work.

regards, tom lane

For #2, yes, the semaphores will go away when the last

process holding

a HANDLE to it goes away.

Well, that raises an interesting point: exactly where in this
code does ownership of the HANDLEs get propagated to the
child processes? As written, the HANDLEs seem to belong only
to the postmaster --- will the kernel calls even work in the
child processes? According to what someone was telling me
the other day, HANDLEs are process-local, so just storing
them in shared memory doesn't seem like it should work.

They're inherited down. I haven't looked at the calling path, but if
they're all created in the postmaster *before* the backends are forked,
it's not a problem. The code specifically sets them to inheritable, and
if you do that you can use them in a child process.

//Magnus

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

Well, that raises an interesting point: exactly where in this
code does ownership of the HANDLEs get propagated to the
child processes?

The code specifically sets them to inheritable,

Oh, I see. Seems like it should work then, but all this stuff should be
mentioned in the file header comment.

regards, tom lane

""Magnus Hagander"" <mha@sollentuna.net> wrote

1. Backend crash while postmaster stays alive: postmaster
will get to execute the on_shmem_exit() callback after all
the backends are gone, and that can clean 'em up.

2. postmaster crashes: we'd like the semas to go away
automatically when the last backend goes away. If this
doesn't happen, then there has to be logic to recycle
leftover semas when the postmaster is next started.

Most of the ugliness in sysv_sema.c is because it has to do "manual"
cleanup per #2.

For #2, yes, the semaphores will go away when the last process holding a
HANDLE to it goes away. For #1, the code seems to handle that right?

I intentionally use *unnamed* semaphores to avoid these problems -- even if
the semaphores didn't go away (as Magus pointed out, if all processes can
exit gracefully, this won't happen), we won't worry about them -- Creating
semahpores will still succeed because there is no existent same named
semaphores will bother it.

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:

I intentionally use *unnamed* semaphores to avoid these problems -- even if
the semaphores didn't go away (as Magus pointed out, if all processes can
exit gracefully, this won't happen), we won't worry about them -- Creating
semahpores will still succeed because there is no existent same named
semaphores will bother it.

Except that eventually you run the kernel out of resources. We were
forced to confront that point very early when dealing with the SysV
API, because of the remarkably low resource limits it traditionally
has, but long-term resource leaks are never a good idea in any software.

Or are you designing this according to the widespread view that Windows
system uptimes are measured in small numbers of days anyway?

regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> wrote

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:

I intentionally use *unnamed* semaphores to avoid these problems -- even

if

the semaphores didn't go away (as Magus pointed out, if all processes

can

exit gracefully, this won't happen), we won't worry about them --

Creating

semahpores will still succeed because there is no existent same named
semaphores will bother it.

Except that eventually you run the kernel out of resources. We were
forced to confront that point very early when dealing with the SysV
API, because of the remarkably low resource limits it traditionally
has, but long-term resource leaks are never a good idea in any software.

Or are you designing this according to the widespread view that Windows
system uptimes are measured in small numbers of days anyway?

/* BTW: I should use "evently" instead of "gracefully" in the above
sentence. */

Maybe I missed the point here: If we really run out of kernel resources, I
don't think we can do much even with named semaphores - because the resource
leaked may not belong to any Postgres processes and we can't clean them up.

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:

Maybe I missed the point here: If we really run out of kernel resources, I
don't think we can do much even with named semaphores - because the resource
leaked may not belong to any Postgres processes and we can't clean them up.

It's certainly not our business to defend the kernel against misbehavior
of other applications. But IMHO it *is* our business to defend against
our own misbehavior. Postgres should not be the weakest link.

Note I am not saying there's anything wrong with the code you posted;
based on discussion to date it seems to be solid. What I am taking
issue with is the attitude you seem to have that it's not our problem
if we leak resources. It is our problem.

regards, tom lane

For #2, yes, the semaphores will go away when the last

process holding

a HANDLE to it goes away. For #1, the code seems to handle

that right?

I intentionally use *unnamed* semaphores to avoid these
problems -- even if the semaphores didn't go away (as Magus
pointed out, if all processes can exit gracefully, this won't
happen), we won't worry about them -- Creating semahpores
will still succeed because there is no existent same named
semaphores will bother it.

Just a point - they will get automatically cleaned up even if the
process doesn't exit *gracefully*, as long as it exits. Only if it's
hung and won't actually exit will the handles not get cleaned up.
This goes for both named and unnamed ones.

//Magnus

"Tom Lane" <tgl@sss.pgh.pa.us> wrote

Note I am not saying there's anything wrong with the code you posted;
based on discussion to date it seems to be solid. What I am taking
issue with is the attitude you seem to have that it's not our problem
if we leak resources. It is our problem.

Right, I totoally agree that we should try our best to prevent resource
leakage and related problems.

Regards,
Qingqing

""Magnus Hagander"" <mha@sollentuna.net> wrote

Just a point - they will get automatically cleaned up even if the
process doesn't exit *gracefully*, as long as it exits. Only if it's
hung and won't actually exit will the handles not get cleaned up.
This goes for both named and unnamed ones.

Sorry, I realized that I should not write "gracefully" and corrected it in
the above thread :-).

Regards,
Qingqing

I intentionally use *unnamed* semaphores to avoid these problems --
even if the semaphores didn't go away (as Magus pointed out, if all
processes can exit gracefully, this won't happen), we won't worry
about them -- Creating semahpores will still succeed

because there is

no existent same named semaphores will bother it.

Except that eventually you run the kernel out of resources.
We were forced to confront that point very early when dealing
with the SysV API, because of the remarkably low resource
limits it traditionally has, but long-term resource leaks are
never a good idea in any software.

Most definitly.
Windows will easily handle thousands, or tends of thousands, of
semaphores, but leaking them is never acceptable behaviour. But the
bottom line is that the objects are automatiaclly destroyed when all
processes holding open handles to them exit - which IIRC is not the case
with SysV ones. And if we leak *processes* that don't exit, that's a
much bigger problem.

Or are you designing this according to the widespread view
that Windows system uptimes are measured in small numbers of
days anyway?

Widespread misunderstanding, I'd say.
Today, a properly configured windows db server shuold normally have
uptimes well exceeding half a year, often over a year. No, that's not as
good as a properly configured unix box, but it's certainly not few
enough days not to care about leakage. So it's definitly not something
we should even consider as an excuse.

(note that "properly configured" includes "properly firewalled so you
don't need to apply the monthly set of patches every time")

//Magnus

Patch applied, and I added a mention that anonymous semaphores were
used, and are deleted on last reference.

---------------------------------------------------------------------------

Qingqing Zhou wrote:

Attached is a patch for Win32 semaphore reimplementation per discussion
with Tom and Magnus. Basically it implements the interfaces defined in
pg_sema.h using unnamed semaphores in Win32. Change in configure.in is
needed to support this modification.

Regards,
Qingqing

Content-Description:

[ Attachment, skipping... ]

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +