dsm_unpin_segment
Hi hackers,
DSM segments have a concept of 'pinning'. Normally, segments are
destroyed when they are no longer mapped by any backend, using a
reference counting scheme. If you call dsm_pin_segment(segment), that
is disabled so that the segment won't be destroyed until the cluster
is shut down. It works by incrementing the reference count an extra
time.
Please find attached a patch to add a corresponding operation
'dsm_unpin_segment'. This gives you a way to ask for the segment to
survive only until you decide to unpin it, at which point the usual
reference counting semantics apply again. It decrements the reference
count, undoing the effect of dsm_pin_segment and destroying the
segment if appropriate.
I think this is very useful for any core or extension code that wants
to store data in dynamic shared memory that survives even when no
backends are running, without having to commit to keeping the segment
forever. We have several projects in the 10.x pipeline which make use
of DSM segments, and we ran into the need for this finer grained
control of their lifetime.
Thanks to my colleague Amit Kapila for the Windows part, and also to
Robert Haas for internal feedback on an earlier version. The Windows
implementation has an extra quirk: Windows has its own reference
counting scheme that destroys mapped memory when there are no attached
processes, so pinning is implemented by sending a duplicate of the
handle into the Postmaster process to keep the segment alive. This
patch needs to close that handle when unpinning. Amazingly, that can
be done without any cooperation from the postmaster.
I'd be grateful for any feedback or thoughts, and will add this to the
commitfest.
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
dsm-unpin-segment.patchapplication/octet-stream; name=dsm-unpin-segment.patchDownload
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 47f2bea..ad1425a5 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -81,6 +81,7 @@ struct dsm_segment
typedef struct dsm_control_item
{
dsm_handle handle;
+ void *impl_private_pm_handle; /* only needed on Windows */
uint32 refcnt; /* 2+ = active, 1 = moribund, 0 = gone */
} dsm_control_item;
@@ -491,6 +492,7 @@ dsm_create(Size size, int flags)
dsm_control->item[i].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[i].refcnt = 2;
+ dsm_control->item[i].impl_private_pm_handle = NULL;
seg->control_slot = i;
LWLockRelease(DynamicSharedMemoryControlLock);
return seg;
@@ -520,6 +522,7 @@ dsm_create(Size size, int flags)
dsm_control->item[nitems].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[nitems].refcnt = 2;
+ dsm_control->item[nitems].impl_private_pm_handle = NULL;
seg->control_slot = nitems;
dsm_control->nitems++;
LWLockRelease(DynamicSharedMemoryControlLock);
@@ -830,11 +833,13 @@ dsm_unpin_mapping(dsm_segment *seg)
}
/*
- * Keep a dynamic shared memory segment until postmaster shutdown.
+ * Keep a dynamic shared memory segment until postmaster shutdown, or until
+ * dsm_unpin_segment is called.
*
- * This function should not be called more than once per segment;
- * on Windows, doing so will create unnecessary handles which will
- * consume system resources to no benefit.
+ * This function should not be called more than once per segment, unless the
+ * segment is explicitly unpinned with dsm_unpin_segment in between calls; on
+ * Windows, doing so will create unnecessary handles which will consume system
+ * resources to no benefit.
*
* Note that this function does not arrange for the current process to
* keep the segment mapped indefinitely; if that behavior is desired,
@@ -844,16 +849,97 @@ dsm_unpin_mapping(dsm_segment *seg)
void
dsm_pin_segment(dsm_segment *seg)
{
+ void *handle;
+
+ dsm_impl_pin_segment(seg->handle, seg->impl_private, &handle);
+
/*
* Bump reference count for this segment in shared memory. This will
* ensure that even if there is no session which is attached to this
- * segment, it will remain until postmaster shutdown.
+ * segment, it will remain until postmaster shutdown or an explicit call
+ * to unpin.
*/
LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
dsm_control->item[seg->control_slot].refcnt++;
+ dsm_control->item[seg->control_slot].impl_private_pm_handle = handle;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+}
+
+/*
+ * Unpin a dynamic shared memory segment that was previously pinned with
+ * dsm_pin_segment. This function should not be called unless dsm_pin_segment
+ * was previously called for this segment.
+ *
+ * The argument is a dsm_handle rather than a dsm_segment in case you want
+ * to unpin a segment to which you haven't attached. This turns out to be
+ * useful if, for example, a reference to one shared memory segment is stored
+ * within another shared memory segment. You might want to unpin the
+ * referenced segment before destroying the referencing segment.
+ */
+void
+dsm_unpin_segment(dsm_handle handle)
+{
+ int control_slot = -1;
+ bool destroy = false;
+ int i;
+
+ /* Find the control slot for the given handle. */
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ for (i = 0; i < dsm_control->nitems; ++i)
+ {
+ /* Skip unused slots. */
+ if (dsm_control->item[i].refcnt == 0)
+ continue;
+
+ /* If we've found our handle, we can stop searching. */
+ if (dsm_control->item[i].handle == handle)
+ {
+ control_slot = i;
+ break;
+ }
+ }
+
+ /*
+ * We should definitely have found the slot, and it should not already be
+ * in the process of going away, because this function should only be
+ * called on a segment which is pinned.
+ */
+ Assert(control_slot != -1);
+ Assert(dsm_control->item[control_slot].refcnt > 1);
+
+ /* Note that 1 means no references (0 means unused slot). */
+ if (--dsm_control->item[i].refcnt == 1)
+ destroy = true;
+
+ /*
+ * Allow implementation-specific code to run. We have to do this before
+ * releasing the lock, because impl_private_pm_handle may get modified by
+ * dsm_impl_unpin_segment.
+ */
+ if (control_slot >= 0)
+ dsm_impl_unpin_segment(handle,
+ &dsm_control->item[control_slot].impl_private_pm_handle);
+
+ /* Now we can release the lock. */
LWLockRelease(DynamicSharedMemoryControlLock);
- dsm_impl_pin_segment(seg->handle, seg->impl_private);
+ /* Clean up resources if that was the last reference. */
+ if (destroy)
+ {
+ void *junk_mapped_address = NULL;
+ void *junk_impl_private = NULL;
+ Size junk_mapped_size = 0;
+
+ if (!dsm_impl_op(DSM_OP_DESTROY, handle, 0, &junk_impl_private,
+ &junk_mapped_address, &junk_mapped_size, WARNING))
+ return;
+
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ Assert(dsm_control->item[control_slot].handle == handle);
+ Assert(dsm_control->item[control_slot].refcnt == 1);
+ dsm_control->item[control_slot].refcnt = 0;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+ }
}
/*
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 173b982..c07a5c6 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -987,8 +987,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
#endif
/*
- * Implementation-specific actions that must be performed when a segment
- * is to be preserved until postmaster shutdown.
+ * Implementation-specific actions that must be performed when a segment is to
+ * be preserved even when no backend has it attached.
*
* Except on Windows, we don't need to do anything at all. But since Windows
* cleans up segments automatically when no references remain, we duplicate
@@ -996,7 +996,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
* do anything to receive the handle; Windows transfers it automatically.
*/
void
-dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
+dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle)
{
switch (dynamic_shared_memory_type)
{
@@ -1018,6 +1019,56 @@ dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
errmsg("could not duplicate handle for \"%s\": %m",
name)));
}
+
+ /*
+ * Here, we remember the handle that we created in the
+ * postmaster process. This handle isn't actually usable in
+ * any process other than the postmaster, but that doesn't
+ * matter. We're just holding onto it so that, if the segment
+ * is unpinned, dsm_impl_unpin_segment can close it.
+ */
+ *impl_private_pm_handle = hmap;
+ break;
+ }
+#endif
+ default:
+ break;
+ }
+}
+
+/*
+ * Implementation-specific actions that must be performed when a segment is no
+ * longer to be preserved, so that it will be cleaned up when all backends
+ * have detached from it.
+ *
+ * Except on Windows, we don't need to do anything at all. For Windows, we
+ * close the extra handle that dsm_impl_pin_segment created in the
+ * postmaster's process space.
+ */
+void
+dsm_impl_unpin_segment(dsm_handle handle, void **impl_private)
+{
+ switch (dynamic_shared_memory_type)
+ {
+#ifdef USE_DSM_WINDOWS
+ case DSM_IMPL_WINDOWS:
+ {
+ if (*impl_private &&
+ !DuplicateHandle(PostmasterHandle, *impl_private,
+ NULL, NULL, 0, FALSE,
+ DUPLICATE_CLOSE_SOURCE))
+ {
+ char name[64];
+
+ snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
+ _dosmaperr(GetLastError());
+ ereport(ERROR,
+ (errcode_for_dynamic_shared_memory(),
+ errmsg("could not duplicate handle for \"%s\": %m",
+ name)));
+ }
+
+ *impl_private = NULL;
break;
}
#endif
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 86ede7a..8be7c9a 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -41,6 +41,7 @@ extern void dsm_detach(dsm_segment *seg);
extern void dsm_pin_mapping(dsm_segment *seg);
extern void dsm_unpin_mapping(dsm_segment *seg);
extern void dsm_pin_segment(dsm_segment *seg);
+extern void dsm_unpin_segment(dsm_handle h);
extern dsm_segment *dsm_find_mapping(dsm_handle h);
/* Informational functions. */
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index ec05e22..e44b477 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -73,6 +73,8 @@ extern bool dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
extern bool dsm_impl_can_resize(void);
/* Implementation-dependent actions required to keep segment until shutdown. */
-extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private);
+extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle);
+extern void dsm_impl_unpin_segment(dsm_handle handle, void **impl_private);
#endif /* DSM_IMPL_H */
Thomas Munro <thomas.munro@enterprisedb.com> writes:
DSM segments have a concept of 'pinning'. Normally, segments are
destroyed when they are no longer mapped by any backend, using a
reference counting scheme. If you call dsm_pin_segment(segment), that
is disabled so that the segment won't be destroyed until the cluster
is shut down. It works by incrementing the reference count an extra
time.
Please find attached a patch to add a corresponding operation
'dsm_unpin_segment'. This gives you a way to ask for the segment to
survive only until you decide to unpin it, at which point the usual
reference counting semantics apply again. It decrements the reference
count, undoing the effect of dsm_pin_segment and destroying the
segment if appropriate.
What happens if dsm_unpin_segment is called more times than
dsm_pin_segment? Seems like you could try to destroy a segment that
still has processes attached.
I don't object to the concept, but you need a less half-baked
implementation if you want to add this. I'd suggest separate counters for
process attaches and pin requests, with code in dsm_unpin_segment to
disallow decrementing the pin request count below zero, and segment
destruction only when both counters go to zero.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 8, 2016 at 7:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
DSM segments have a concept of 'pinning'. Normally, segments are
destroyed when they are no longer mapped by any backend, using a
reference counting scheme. If you call dsm_pin_segment(segment), that
is disabled so that the segment won't be destroyed until the cluster
is shut down. It works by incrementing the reference count an extra
time.Please find attached a patch to add a corresponding operation
'dsm_unpin_segment'. This gives you a way to ask for the segment to
survive only until you decide to unpin it, at which point the usual
reference counting semantics apply again. It decrements the reference
count, undoing the effect of dsm_pin_segment and destroying the
segment if appropriate.What happens if dsm_unpin_segment is called more times than
dsm_pin_segment? Seems like you could try to destroy a segment that
still has processes attached.
Calling dsm_pin_segment more than once is not supported and has never
been supported. As the comments explain:
* This function should not be called more than once per segment;
* on Windows, doing so will create unnecessary handles which will
* consume system resources to no benefit.
Therefore, I don't see the problem. You can pin a segment that is not
pinned, and you can unpin a segment that is pinned. You may not
re-pin a segment that is already pinned, nor unpin a segment that is
not pinned. If you try to do so, you are using the API contrary to
specification, and if it breaks (as it will) you get to keep both
pieces.
We could add the reference counting behavior for which you are asking,
but that seems to be an entirely new feature for which I know of no
demand.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 9, 2016 at 12:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Aug 8, 2016 at 7:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Please find attached a patch to add a corresponding operation
'dsm_unpin_segment'. This gives you a way to ask for the segment to
survive only until you decide to unpin it, at which point the usual
reference counting semantics apply again. It decrements the reference
count, undoing the effect of dsm_pin_segment and destroying the
segment if appropriate.What happens if dsm_unpin_segment is called more times than
dsm_pin_segment? Seems like you could try to destroy a segment that
still has processes attached.Calling dsm_pin_segment more than once is not supported and has never
been supported. As the comments explain:* This function should not be called more than once per segment;
* on Windows, doing so will create unnecessary handles which will
* consume system resources to no benefit.Therefore, I don't see the problem. You can pin a segment that is not
pinned, and you can unpin a segment that is pinned. You may not
re-pin a segment that is already pinned, nor unpin a segment that is
not pinned. If you try to do so, you are using the API contrary to
specification, and if it breaks (as it will) you get to keep both
pieces.We could add the reference counting behavior for which you are asking,
but that seems to be an entirely new feature for which I know of no
demand.
Yeah, I was considering unbalanced pin/unpin requests to be a
programming error. To be more defensive about that, how about I add a
boolean 'pinned' to dsm_control_item, and elog(ERROR, ...) if it's not
in the expected state when you try to pin or unpin?
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Yeah, I was considering unbalanced pin/unpin requests to be a
programming error. To be more defensive about that, how about I add a
boolean 'pinned' to dsm_control_item, and elog(ERROR, ...) if it's not
in the expected state when you try to pin or unpin?
Well, what you have there is a one-bit-wide pin request counter.
I do not see why that's better than an actual counter, but if that's
what you want to do, fine.
The larger picture here is that Robert is exhibiting a touching but
unfounded faith that extensions using this feature will contain zero bugs.
IMO there needs to be some positive defense against mistakes in using the
pin/unpin API. As things stand, multiple pin requests don't have any
fatal consequences (especially not on non-Windows), so I have little
confidence that it's not happening in the field. I have even less
confidence that there wouldn't be too many unpin requests. What exactly
is an extension going to be doing to ensure that it doesn't do too many of
one or the other?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The larger picture here is that Robert is exhibiting a touching but
unfounded faith that extensions using this feature will contain zero bugs.
IMO there needs to be some positive defense against mistakes in using the
pin/unpin API. As things stand, multiple pin requests don't have any
fatal consequences (especially not on non-Windows), so I have little
confidence that it's not happening in the field. I have even less
confidence that there wouldn't be too many unpin requests.
Ok, here is a version that defends against invalid sequences of
pin/unpin calls. I had to move dsm_impl_pin_segment into the block
protected by DynamicSharedMemoryControlLock, so that it could come
after the already-pinned check, but before updating any state, since
it makes a Windows syscall that can fail. That said, I've only tested
on Unix and will need to ask someone to test on Windows.
What exactly
is an extension going to be doing to ensure that it doesn't do too many of
one or the other?
An extension that manages segment lifetimes like this needs a
carefully designed protocol to get it right, probably involving state
in another shared memory area and some interlocking, not to mention a
lot of thought about cleanup.
Here's one use case: I have a higher level object, a
multi-segment-backed shared memory allocator, which owns any number of
segments that together form a shared memory area. The protocol is
that the allocator always pins segments when it needs to create new
ones, because they need to survive as long as the control segment,
even though no one backend is guaranteed to have all of the auxiliary
segments mapped in (since they're created and attached on demand).
But when the control segment is detached by all backends and is due to
be destroyed, then we need to unpin all the auxiliary segments so they
can also be destroyed, and that can be done from an on_dsm_detach
callback on the control segment. So I'm riding on the coat tails of
the existing cleanup mechanism for the control segment, while making
sure that the auxiliary segments get pinned and unpinned exactly once.
I'll have more to say about that when I post that patch...
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
dsm-unpin-segment-v2.patchapplication/octet-stream; name=dsm-unpin-segment-v2.patchDownload
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 47f2bea..420ff62 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -82,6 +82,8 @@ typedef struct dsm_control_item
{
dsm_handle handle;
uint32 refcnt; /* 2+ = active, 1 = moribund, 0 = gone */
+ void *impl_private_pm_handle; /* only needed on Windows */
+ bool pinned;
} dsm_control_item;
/* Layout of the dynamic shared memory control segment. */
@@ -491,6 +493,8 @@ dsm_create(Size size, int flags)
dsm_control->item[i].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[i].refcnt = 2;
+ dsm_control->item[i].impl_private_pm_handle = NULL;
+ dsm_control->item[i].pinned = false;
seg->control_slot = i;
LWLockRelease(DynamicSharedMemoryControlLock);
return seg;
@@ -520,6 +524,8 @@ dsm_create(Size size, int flags)
dsm_control->item[nitems].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[nitems].refcnt = 2;
+ dsm_control->item[nitems].impl_private_pm_handle = NULL;
+ dsm_control->item[nitems].pinned = false;
seg->control_slot = nitems;
dsm_control->nitems++;
LWLockRelease(DynamicSharedMemoryControlLock);
@@ -760,6 +766,9 @@ dsm_detach(dsm_segment *seg)
/* If new reference count is 1, try to destroy the segment. */
if (refcnt == 1)
{
+ /* A pinned segment should never reach 1. */
+ Assert(!dsm_control->item[control_slot].pinned);
+
/*
* If we fail to destroy the segment here, or are killed before we
* finish doing so, the reference count will remain at 1, which
@@ -830,11 +839,11 @@ dsm_unpin_mapping(dsm_segment *seg)
}
/*
- * Keep a dynamic shared memory segment until postmaster shutdown.
+ * Keep a dynamic shared memory segment until postmaster shutdown, or until
+ * dsm_unpin_segment is called.
*
- * This function should not be called more than once per segment;
- * on Windows, doing so will create unnecessary handles which will
- * consume system resources to no benefit.
+ * This function should not be called more than once per segment, unless the
+ * segment is explicitly unpinned with dsm_unpin_segment in between calls.
*
* Note that this function does not arrange for the current process to
* keep the segment mapped indefinitely; if that behavior is desired,
@@ -844,16 +853,102 @@ dsm_unpin_mapping(dsm_segment *seg)
void
dsm_pin_segment(dsm_segment *seg)
{
+ void *handle;
+
/*
* Bump reference count for this segment in shared memory. This will
* ensure that even if there is no session which is attached to this
- * segment, it will remain until postmaster shutdown.
+ * segment, it will remain until postmaster shutdown or an explicit call
+ * to unpin.
*/
LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ if (dsm_control->item[seg->control_slot].pinned)
+ elog(ERROR, "cannot pin a segment that is already pinned");
+ dsm_impl_pin_segment(seg->handle, seg->impl_private, &handle);
+ dsm_control->item[seg->control_slot].pinned = true;
dsm_control->item[seg->control_slot].refcnt++;
+ dsm_control->item[seg->control_slot].impl_private_pm_handle = handle;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+}
+
+/*
+ * Unpin a dynamic shared memory segment that was previously pinned with
+ * dsm_pin_segment. This function should not be called unless dsm_pin_segment
+ * was previously called for this segment.
+ *
+ * The argument is a dsm_handle rather than a dsm_segment in case you want
+ * to unpin a segment to which you haven't attached. This turns out to be
+ * useful if, for example, a reference to one shared memory segment is stored
+ * within another shared memory segment. You might want to unpin the
+ * referenced segment before destroying the referencing segment.
+ */
+void
+dsm_unpin_segment(dsm_handle handle)
+{
+ int control_slot = -1;
+ bool destroy = false;
+ int i;
+
+ /* Find the control slot for the given handle. */
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ for (i = 0; i < dsm_control->nitems; ++i)
+ {
+ /* Skip unused slots. */
+ if (dsm_control->item[i].refcnt == 0)
+ continue;
+
+ /* If we've found our handle, we can stop searching. */
+ if (dsm_control->item[i].handle == handle)
+ {
+ control_slot = i;
+ break;
+ }
+ }
+
+ /*
+ * We should definitely have found the slot, and it should not already be
+ * in the process of going away, because this function should only be
+ * called on a segment which is pinned.
+ */
+ Assert(control_slot != -1);
+ Assert(dsm_control->item[control_slot].refcnt > 1);
+ if (!dsm_control->item[i].pinned)
+ elog(ERROR, "cannot unpin a segment that is not pinned");
+ dsm_control->item[i].pinned = false;
+
+ /* Note that 1 means no references (0 means unused slot). */
+ if (--dsm_control->item[i].refcnt == 1)
+ destroy = true;
+
+ /*
+ * Allow implementation-specific code to run. We have to do this before
+ * releasing the lock, because impl_private_pm_handle may get modified by
+ * dsm_impl_unpin_segment.
+ */
+ if (control_slot >= 0)
+ dsm_impl_unpin_segment(handle,
+ &dsm_control->item[control_slot].impl_private_pm_handle);
+
+ /* Now we can release the lock. */
LWLockRelease(DynamicSharedMemoryControlLock);
- dsm_impl_pin_segment(seg->handle, seg->impl_private);
+ /* Clean up resources if that was the last reference. */
+ if (destroy)
+ {
+ void *junk_mapped_address = NULL;
+ void *junk_impl_private = NULL;
+ Size junk_mapped_size = 0;
+
+ if (!dsm_impl_op(DSM_OP_DESTROY, handle, 0, &junk_impl_private,
+ &junk_mapped_address, &junk_mapped_size, WARNING))
+ return;
+
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ Assert(dsm_control->item[control_slot].handle == handle);
+ Assert(dsm_control->item[control_slot].refcnt == 1);
+ dsm_control->item[control_slot].refcnt = 0;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+ }
}
/*
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 173b982..c07a5c6 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -987,8 +987,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
#endif
/*
- * Implementation-specific actions that must be performed when a segment
- * is to be preserved until postmaster shutdown.
+ * Implementation-specific actions that must be performed when a segment is to
+ * be preserved even when no backend has it attached.
*
* Except on Windows, we don't need to do anything at all. But since Windows
* cleans up segments automatically when no references remain, we duplicate
@@ -996,7 +996,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
* do anything to receive the handle; Windows transfers it automatically.
*/
void
-dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
+dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle)
{
switch (dynamic_shared_memory_type)
{
@@ -1018,6 +1019,56 @@ dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
errmsg("could not duplicate handle for \"%s\": %m",
name)));
}
+
+ /*
+ * Here, we remember the handle that we created in the
+ * postmaster process. This handle isn't actually usable in
+ * any process other than the postmaster, but that doesn't
+ * matter. We're just holding onto it so that, if the segment
+ * is unpinned, dsm_impl_unpin_segment can close it.
+ */
+ *impl_private_pm_handle = hmap;
+ break;
+ }
+#endif
+ default:
+ break;
+ }
+}
+
+/*
+ * Implementation-specific actions that must be performed when a segment is no
+ * longer to be preserved, so that it will be cleaned up when all backends
+ * have detached from it.
+ *
+ * Except on Windows, we don't need to do anything at all. For Windows, we
+ * close the extra handle that dsm_impl_pin_segment created in the
+ * postmaster's process space.
+ */
+void
+dsm_impl_unpin_segment(dsm_handle handle, void **impl_private)
+{
+ switch (dynamic_shared_memory_type)
+ {
+#ifdef USE_DSM_WINDOWS
+ case DSM_IMPL_WINDOWS:
+ {
+ if (*impl_private &&
+ !DuplicateHandle(PostmasterHandle, *impl_private,
+ NULL, NULL, 0, FALSE,
+ DUPLICATE_CLOSE_SOURCE))
+ {
+ char name[64];
+
+ snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
+ _dosmaperr(GetLastError());
+ ereport(ERROR,
+ (errcode_for_dynamic_shared_memory(),
+ errmsg("could not duplicate handle for \"%s\": %m",
+ name)));
+ }
+
+ *impl_private = NULL;
break;
}
#endif
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 86ede7a..8be7c9a 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -41,6 +41,7 @@ extern void dsm_detach(dsm_segment *seg);
extern void dsm_pin_mapping(dsm_segment *seg);
extern void dsm_unpin_mapping(dsm_segment *seg);
extern void dsm_pin_segment(dsm_segment *seg);
+extern void dsm_unpin_segment(dsm_handle h);
extern dsm_segment *dsm_find_mapping(dsm_handle h);
/* Informational functions. */
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index ec05e22..e44b477 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -73,6 +73,8 @@ extern bool dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
extern bool dsm_impl_can_resize(void);
/* Implementation-dependent actions required to keep segment until shutdown. */
-extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private);
+extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle);
+extern void dsm_impl_unpin_segment(dsm_handle handle, void **impl_private);
#endif /* DSM_IMPL_H */
On Mon, Aug 8, 2016 at 8:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Yeah, I was considering unbalanced pin/unpin requests to be a
programming error. To be more defensive about that, how about I add a
boolean 'pinned' to dsm_control_item, and elog(ERROR, ...) if it's not
in the expected state when you try to pin or unpin?Well, what you have there is a one-bit-wide pin request counter.
I do not see why that's better than an actual counter, but if that's
what you want to do, fine.The larger picture here is that Robert is exhibiting a touching but
unfounded faith that extensions using this feature will contain zero bugs.
That's an overstatement of my position. I think it is quite likely
that extensions using this feature will have bugs, because essentially
all code has bugs, but whether they are likely have the specific bug
of unpinning a segment that is already unpinned is not quite so clear.
That's not to say I object to Thomas's v2 patch, which will catch that
mistake if it happens. Defensive programming never killed anybody, as
far as I know. However, I don't see the need for a full-blown request
counter here; we've had this API for several releases now and to my
knowledge nobody has complained about the fact that you aren't
supposed to call dsm_pin_segment() multiple times for the same
segment. Therefore, I think the evidence supports the contention that
it's not broken and doesn't need to be fixed. If we do decide it
needs to be fixed, I think that's material for a separate patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 8/9/16 1:01 PM, Robert Haas wrote:
However, I don't see the need for a full-blown request
counter here; we've had this API for several releases now and to my
knowledge nobody has complained about the fact that you aren't
supposed to call dsm_pin_segment() multiple times for the same
segment.
Could a couple of static variables be used to ensure multiple pin/unpin
and attach/detach calls throw an assert() (or become a no-op if asserts
are disabled)? It would be nice if we could protect users from this.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 10, 2016 at 10:38 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 8/9/16 1:01 PM, Robert Haas wrote:
However, I don't see the need for a full-blown request
counter here; we've had this API for several releases now and to my
knowledge nobody has complained about the fact that you aren't
supposed to call dsm_pin_segment() multiple times for the same
segment.Could a couple of static variables be used to ensure multiple pin/unpin and
attach/detach calls throw an assert() (or become a no-op if asserts are
disabled)? It would be nice if we could protect users from this.
The can't be static, they need to be in shared memory, because we also
want to protect against two *different* backends pinning it. The v2
patch protects users from this, by adding 'pinned' flag to the
per-segment control object that is visible everywhere, and then doing:
+ if (dsm_control->item[seg->control_slot].pinned)
+ elog(ERROR, "cannot pin a segment that is already pinned");... and:
+ if (!dsm_control->item[i].pinned)
+ elog(ERROR, "cannot unpin a segment that is not pinned");Those checks could arguably be assertions rather than errors, but I
don't think that pin/unpin operations will ever be high frequency
enough for it to be worth avoiding those instructions in production
builds. The whole reason for pinning segments is likely to be able to
reuse them repeatedly, so nailing it down is likely something you'd
want to do immediately after creating it.
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 8/9/16 6:14 PM, Thomas Munro wrote:
Could a couple of static variables be used to ensure multiple pin/unpin and
attach/detach calls throw an assert() (or become a no-op if asserts are
disabled)? It would be nice if we could protect users from this.The can't be static, they need to be in shared memory, because we also
want to protect against two *different* backends pinning it.
Right, this would strictly protect from it happening within a single
backend. Perhaps it's pointless for pin/unpin, but it seems like it
would be a good thing to have for attach/detach.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 10, 2016 at 2:43 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
On 8/9/16 6:14 PM, Thomas Munro wrote:
The can't be static, they need to be in shared memory, because we also
want to protect against two *different* backends pinning it.Right, this would strictly protect from it happening within a single
backend. Perhaps it's pointless for pin/unpin, but it seems like it would be
a good thing to have for attach/detach.
Double attach already gets you:
elog(ERROR, "can't attach the same segment more than once");
Double detach is conceptually like double free, and in fact actually
leads to a double pfree of the backend-local dsm_segment object. It
doesn't seem like the kind of thing it's easy or reasonable to try to
defend against, since you have no space left in which to store the
state you need to detect double-frees after you've done it once!
--
Thomas Munro
http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The larger picture here is that Robert is exhibiting a touching but
unfounded faith that extensions using this feature will contain zero bugs.
IMO there needs to be some positive defense against mistakes in using the
pin/unpin API. As things stand, multiple pin requests don't have any
fatal consequences (especially not on non-Windows), so I have little
confidence that it's not happening in the field. I have even less
confidence that there wouldn't be too many unpin requests.Ok, here is a version that defends against invalid sequences of
pin/unpin calls. I had to move dsm_impl_pin_segment into the block
protected by DynamicSharedMemoryControlLock, so that it could come
after the already-pinned check, but before updating any state, since
it makes a Windows syscall that can fail. That said, I've only tested
on Unix and will need to ask someone to test on Windows.
Few review comments:
1.
+ /* Note that 1 means no references (0 means unused slot). */
+ if (--dsm_control->item[i].refcnt == 1)
+ destroy = true;
+
+ /*
+ * Allow implementation-specific code to run. We have to do this before
+ * releasing the lock, because impl_private_pm_handle may get modified by
+ * dsm_impl_unpin_segment.
+ */
+ if (control_slot >= 0)
+ dsm_impl_unpin_segment(handle,
+ &dsm_control->item[control_slot].impl_private_pm_handle);If there is an error in dsm_impl_unpin_segment(), then we don't need
to decrement the reference count. Isn't it better to do it after the
dsm_impl_unpin_segment() is successful. Similarly, I think pinned
should be set to false after dsm_impl_unpin_segment().
Do you need a check if (control_slot >= 0)? In the code just above
you have as Assert to ensure that it is >=0.
2.
+ if (dsm_control->item[seg->control_slot].pinned)
+ elog(ERROR, "cannot pin a segment that is already pinned");Shouldn't this be a user facing error (which means we should use ereport)?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
2. + if (dsm_control->item[seg->control_slot].pinned) + elog(ERROR, "cannot pin a segment that is already pinned");Shouldn't this be a user facing error (which means we should use ereport)?
Uh, certainly not. This can't happen because of SQL the user enters;
it can only happen because of a C coding error. elog() is the
appropriate tool for that case.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Aug 20, 2016 at 6:13 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
2. + if (dsm_control->item[seg->control_slot].pinned) + elog(ERROR, "cannot pin a segment that is already pinned");Shouldn't this be a user facing error (which means we should use ereport)?
Uh, certainly not. This can't happen because of SQL the user enters;
it can only happen because of a C coding error. elog() is the
appropriate tool for that case.
Okay, your point makes sense to me, but in that case why not use an
Assert here? I think elog() could also be used here as well, but it
seems to me elog() is most appropriate for the cases where it is not
straightforward to tell the behaviour of API or value of variable like
when PageAddItem() is not successful.
void
dsm_pin_segment(dsm_segment *seg)
+void
+dsm_unpin_segment(dsm_handle handle)
Another point, I want to highlight here is that pin/unpin API's have
different type of arguments. The comments on top of function
dsm_unpin_segment() explains the need of using dsm_handle which seems
reasonable. I just pointed out to see if anybody else has a different
view.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The larger picture here is that Robert is exhibiting a touching but
unfounded faith that extensions using this feature will contain zero bugs.
IMO there needs to be some positive defense against mistakes in using the
pin/unpin API. As things stand, multiple pin requests don't have any
fatal consequences (especially not on non-Windows), so I have little
confidence that it's not happening in the field. I have even less
confidence that there wouldn't be too many unpin requests.Ok, here is a version that defends against invalid sequences of
pin/unpin calls. I had to move dsm_impl_pin_segment into the block
protected by DynamicSharedMemoryControlLock, so that it could come
after the already-pinned check, but before updating any state, since
it makes a Windows syscall that can fail. That said, I've only tested
on Unix and will need to ask someone to test on Windows.Few review comments:
Thanks for the review!
1. + /* Note that 1 means no references (0 means unused slot). */ + if (--dsm_control->item[i].refcnt == 1) + destroy = true; + + /* + * Allow implementation-specific code to run. We have to do this before + * releasing the lock, because impl_private_pm_handle may get modified by + * dsm_impl_unpin_segment. + */ + if (control_slot >= 0) + dsm_impl_unpin_segment(handle, + &dsm_control->item[control_slot].impl_private_pm_handle);If there is an error in dsm_impl_unpin_segment(), then we don't need
to decrement the reference count. Isn't it better to do it after the
dsm_impl_unpin_segment() is successful. Similarly, I think pinned
should be set to false after dsm_impl_unpin_segment().
Hmm. Yeah, OK. Things are in pretty bad shape if you fail to unpin
despite having run the earlier checks, but you're right, it's better
that way. New version attached.
Do you need a check if (control_slot >= 0)? In the code just above
you have as Assert to ensure that it is >=0.
Right. Furthermore, the code was using "i" in some places and
"control_slot" in others. We might as well use control_slot
everywhere.
On Sun, Aug 21, 2016 at 5:45 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Aug 20, 2016 at 6:13 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
2. + if (dsm_control->item[seg->control_slot].pinned) + elog(ERROR, "cannot pin a segment that is already pinned");Shouldn't this be a user facing error (which means we should use ereport)?
Uh, certainly not. This can't happen because of SQL the user enters;
it can only happen because of a C coding error. elog() is the
appropriate tool for that case.Okay, your point makes sense to me, but in that case why not use an
Assert here? I think elog() could also be used here as well, but it
seems to me elog() is most appropriate for the cases where it is not
straightforward to tell the behaviour of API or value of variable like
when PageAddItem() is not successful.
Here's the rationale I'm using: if it's helpful to programmers of
client code, especially client code that might include extensions, and
nowhere near a hot code path, then why not use elog rather than
Assert? I took inspiration for that from the pre-existing "debugging
cross-check" in dsm_attach that does elog(ERROR, "can't attach the
same segment more than once"). On that basis, this new version
retains the elog you mentioned, and now also uses elog for the
you-tried-to-unpin-a-handle-I-couldn't-find case. But I kept Assert
in places that detect bugs in *this* code, rather than client code.
void
dsm_pin_segment(dsm_segment *seg)+void
+dsm_unpin_segment(dsm_handle handle)Another point, I want to highlight here is that pin/unpin API's have
different type of arguments. The comments on top of function
dsm_unpin_segment() explains the need of using dsm_handle which seems
reasonable. I just pointed out to see if anybody else has a different
view.
Now that I have posted the DSA patch[1]/messages/by-id/CAEepm=1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA@mail.gmail.com, it probably makes sense to
explain the path by which I arrived at the conclusion that unpin
should take a handle. In an earlier version, dsm_unpin_segment took a
dsm_segment *, mirroring the pin function. But then Robert complained
privately that my dsa_area destroy code path was sometimes having to
attach segments just to unpin them and then detach, which might fail
due to lack of virtual memory. He was right to complain: that created
a fairly nasty failure mode where I'd unpinned some but not all of the
DSM segments that belong together. So I concluded that it should be
possible to unpin a segment even when you haven't got it attached, and
rewrote it this way. In general, any structure built from multiple
DSM segments which point to each other using handles could run into
this problem. I think the function's comment covers it, but hopefully
this concrete example is convincing.
[1]: /messages/by-id/CAEepm=1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA@mail.gmail.com
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
dsm-unpin-segment-v3.patchapplication/octet-stream; name=dsm-unpin-segment-v3.patchDownload
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 47f2bea..6fc64db 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -82,6 +82,8 @@ typedef struct dsm_control_item
{
dsm_handle handle;
uint32 refcnt; /* 2+ = active, 1 = moribund, 0 = gone */
+ void *impl_private_pm_handle; /* only needed on Windows */
+ bool pinned;
} dsm_control_item;
/* Layout of the dynamic shared memory control segment. */
@@ -491,6 +493,8 @@ dsm_create(Size size, int flags)
dsm_control->item[i].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[i].refcnt = 2;
+ dsm_control->item[i].impl_private_pm_handle = NULL;
+ dsm_control->item[i].pinned = false;
seg->control_slot = i;
LWLockRelease(DynamicSharedMemoryControlLock);
return seg;
@@ -520,6 +524,8 @@ dsm_create(Size size, int flags)
dsm_control->item[nitems].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[nitems].refcnt = 2;
+ dsm_control->item[nitems].impl_private_pm_handle = NULL;
+ dsm_control->item[nitems].pinned = false;
seg->control_slot = nitems;
dsm_control->nitems++;
LWLockRelease(DynamicSharedMemoryControlLock);
@@ -760,6 +766,9 @@ dsm_detach(dsm_segment *seg)
/* If new reference count is 1, try to destroy the segment. */
if (refcnt == 1)
{
+ /* A pinned segment should never reach 1. */
+ Assert(!dsm_control->item[control_slot].pinned);
+
/*
* If we fail to destroy the segment here, or are killed before we
* finish doing so, the reference count will remain at 1, which
@@ -830,11 +839,11 @@ dsm_unpin_mapping(dsm_segment *seg)
}
/*
- * Keep a dynamic shared memory segment until postmaster shutdown.
+ * Keep a dynamic shared memory segment until postmaster shutdown, or until
+ * dsm_unpin_segment is called.
*
- * This function should not be called more than once per segment;
- * on Windows, doing so will create unnecessary handles which will
- * consume system resources to no benefit.
+ * This function should not be called more than once per segment, unless the
+ * segment is explicitly unpinned with dsm_unpin_segment in between calls.
*
* Note that this function does not arrange for the current process to
* keep the segment mapped indefinitely; if that behavior is desired,
@@ -844,16 +853,102 @@ dsm_unpin_mapping(dsm_segment *seg)
void
dsm_pin_segment(dsm_segment *seg)
{
+ void *handle;
+
/*
* Bump reference count for this segment in shared memory. This will
* ensure that even if there is no session which is attached to this
- * segment, it will remain until postmaster shutdown.
+ * segment, it will remain until postmaster shutdown or an explicit call
+ * to unpin.
*/
LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ if (dsm_control->item[seg->control_slot].pinned)
+ elog(ERROR, "cannot pin a segment that is already pinned");
+ dsm_impl_pin_segment(seg->handle, seg->impl_private, &handle);
+ dsm_control->item[seg->control_slot].pinned = true;
dsm_control->item[seg->control_slot].refcnt++;
+ dsm_control->item[seg->control_slot].impl_private_pm_handle = handle;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+}
+
+/*
+ * Unpin a dynamic shared memory segment that was previously pinned with
+ * dsm_pin_segment. This function should not be called unless dsm_pin_segment
+ * was previously called for this segment.
+ *
+ * The argument is a dsm_handle rather than a dsm_segment in case you want
+ * to unpin a segment to which you haven't attached. This turns out to be
+ * useful if, for example, a reference to one shared memory segment is stored
+ * within another shared memory segment. You might want to unpin the
+ * referenced segment before destroying the referencing segment.
+ */
+void
+dsm_unpin_segment(dsm_handle handle)
+{
+ int control_slot = -1;
+ bool destroy = false;
+ int i;
+
+ /* Find the control slot for the given handle. */
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ for (i = 0; i < dsm_control->nitems; ++i)
+ {
+ /* Skip unused slots. */
+ if (dsm_control->item[i].refcnt == 0)
+ continue;
+
+ /* If we've found our handle, we can stop searching. */
+ if (dsm_control->item[i].handle == handle)
+ {
+ control_slot = i;
+ break;
+ }
+ }
+
+ /*
+ * We should definitely have found the slot, and it should not already be
+ * in the process of going away, because this function should only be
+ * called on a segment which is pinned.
+ */
+ if (control_slot == -1)
+ elog(ERROR, "cannot unpin unknown segment handle");
+ if (!dsm_control->item[control_slot].pinned)
+ elog(ERROR, "cannot unpin a segment that is not pinned");
+ Assert(dsm_control->item[control_slot].refcnt > 1);
+
+ /*
+ * Allow implementation-specific code to run. We have to do this before
+ * releasing the lock, because impl_private_pm_handle may get modified by
+ * dsm_impl_unpin_segment.
+ */
+ dsm_impl_unpin_segment(handle,
+ &dsm_control->item[control_slot].impl_private_pm_handle);
+
+ /* Note that 1 means no references (0 means unused slot). */
+ if (--dsm_control->item[control_slot].refcnt == 1)
+ destroy = true;
+ dsm_control->item[control_slot].pinned = false;
+
+ /* Now we can release the lock. */
LWLockRelease(DynamicSharedMemoryControlLock);
- dsm_impl_pin_segment(seg->handle, seg->impl_private);
+ /* Clean up resources if that was the last reference. */
+ if (destroy)
+ {
+ void *junk_mapped_address = NULL;
+ void *junk_impl_private = NULL;
+ Size junk_mapped_size = 0;
+
+ if (!dsm_impl_op(DSM_OP_DESTROY, handle, 0, &junk_impl_private,
+ &junk_mapped_address, &junk_mapped_size, WARNING))
+ return;
+
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ Assert(dsm_control->item[control_slot].handle == handle);
+ Assert(dsm_control->item[control_slot].refcnt == 1);
+ dsm_control->item[control_slot].refcnt = 0;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+ }
}
/*
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 173b982..c07a5c6 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -987,8 +987,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
#endif
/*
- * Implementation-specific actions that must be performed when a segment
- * is to be preserved until postmaster shutdown.
+ * Implementation-specific actions that must be performed when a segment is to
+ * be preserved even when no backend has it attached.
*
* Except on Windows, we don't need to do anything at all. But since Windows
* cleans up segments automatically when no references remain, we duplicate
@@ -996,7 +996,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
* do anything to receive the handle; Windows transfers it automatically.
*/
void
-dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
+dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle)
{
switch (dynamic_shared_memory_type)
{
@@ -1018,6 +1019,56 @@ dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
errmsg("could not duplicate handle for \"%s\": %m",
name)));
}
+
+ /*
+ * Here, we remember the handle that we created in the
+ * postmaster process. This handle isn't actually usable in
+ * any process other than the postmaster, but that doesn't
+ * matter. We're just holding onto it so that, if the segment
+ * is unpinned, dsm_impl_unpin_segment can close it.
+ */
+ *impl_private_pm_handle = hmap;
+ break;
+ }
+#endif
+ default:
+ break;
+ }
+}
+
+/*
+ * Implementation-specific actions that must be performed when a segment is no
+ * longer to be preserved, so that it will be cleaned up when all backends
+ * have detached from it.
+ *
+ * Except on Windows, we don't need to do anything at all. For Windows, we
+ * close the extra handle that dsm_impl_pin_segment created in the
+ * postmaster's process space.
+ */
+void
+dsm_impl_unpin_segment(dsm_handle handle, void **impl_private)
+{
+ switch (dynamic_shared_memory_type)
+ {
+#ifdef USE_DSM_WINDOWS
+ case DSM_IMPL_WINDOWS:
+ {
+ if (*impl_private &&
+ !DuplicateHandle(PostmasterHandle, *impl_private,
+ NULL, NULL, 0, FALSE,
+ DUPLICATE_CLOSE_SOURCE))
+ {
+ char name[64];
+
+ snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
+ _dosmaperr(GetLastError());
+ ereport(ERROR,
+ (errcode_for_dynamic_shared_memory(),
+ errmsg("could not duplicate handle for \"%s\": %m",
+ name)));
+ }
+
+ *impl_private = NULL;
break;
}
#endif
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 86ede7a..8be7c9a 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -41,6 +41,7 @@ extern void dsm_detach(dsm_segment *seg);
extern void dsm_pin_mapping(dsm_segment *seg);
extern void dsm_unpin_mapping(dsm_segment *seg);
extern void dsm_pin_segment(dsm_segment *seg);
+extern void dsm_unpin_segment(dsm_handle h);
extern dsm_segment *dsm_find_mapping(dsm_handle h);
/* Informational functions. */
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index ec05e22..e44b477 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -73,6 +73,8 @@ extern bool dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
extern bool dsm_impl_can_resize(void);
/* Implementation-dependent actions required to keep segment until shutdown. */
-extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private);
+extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle);
+extern void dsm_impl_unpin_segment(dsm_handle handle, void **impl_private);
#endif /* DSM_IMPL_H */
On Sun, Aug 21, 2016 at 7:54 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
Here's the rationale I'm using: if it's helpful to programmers of
client code, especially client code that might include extensions, and
nowhere near a hot code path, then why not use elog rather than
Assert? I took inspiration for that from the pre-existing "debugging
cross-check" in dsm_attach that does elog(ERROR, "can't attach the
same segment more than once"). On that basis, this new version
retains the elog you mentioned, and now also uses elog for the
you-tried-to-unpin-a-handle-I-couldn't-find case. But I kept Assert
in places that detect bugs in *this* code, rather than client code.
Hmm, well said. I've never thought about it in exactly that way, but
I think that is a useful distinction. I've sometimes phrased it this
way: an Assert() is good if the path is performance-critical, or if
the Assert() is checking something that is "nearby" in the code, but
an elog() is better if we're checking for a condition that could
happen as a result of some code that is far-removed from the place
where the elog() is. If an assertion fails, you're not necessarily
going to realize right away that the calling code needs to be checked
for errors. That could be mitigated, of course, by adding a comment
right before the Assert() saying "if this Assertion fails, you
probably did X, and you shouldn't do that". But an elog() can state
the exact problem right away.
Also, of course, elog() is the right tool if we want to perform the
check even in production builds where asserts are not enabled. That's
not so relevant here, but it matters in some other cases, like when
checking for a case that shouldn't happen normally but could be the
result of data corruption.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 22, 2016 at 5:24 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The larger picture here is that Robert is exhibiting a touching but
unfounded faith that extensions using this feature will contain zero bugs.
IMO there needs to be some positive defense against mistakes in using the
pin/unpin API. As things stand, multiple pin requests don't have any
fatal consequences (especially not on non-Windows), so I have little
confidence that it's not happening in the field. I have even less
confidence that there wouldn't be too many unpin requests.Ok, here is a version that defends against invalid sequences of
pin/unpin calls. I had to move dsm_impl_pin_segment into the block
protected by DynamicSharedMemoryControlLock, so that it could come
after the already-pinned check, but before updating any state, since
it makes a Windows syscall that can fail. That said, I've only tested
on Unix and will need to ask someone to test on Windows.Few review comments:
Thanks for the review!
1. + /* Note that 1 means no references (0 means unused slot). */ + if (--dsm_control->item[i].refcnt == 1) + destroy = true; + + /* + * Allow implementation-specific code to run. We have to do this before + * releasing the lock, because impl_private_pm_handle may get modified by + * dsm_impl_unpin_segment. + */ + if (control_slot >= 0) + dsm_impl_unpin_segment(handle, + &dsm_control->item[control_slot].impl_private_pm_handle);If there is an error in dsm_impl_unpin_segment(), then we don't need
to decrement the reference count. Isn't it better to do it after the
dsm_impl_unpin_segment() is successful. Similarly, I think pinned
should be set to false after dsm_impl_unpin_segment().Hmm. Yeah, OK. Things are in pretty bad shape if you fail to unpin
despite having run the earlier checks, but you're right, it's better
that way. New version attached.
+ int control_slot = -1;
...
+ if (control_slot == -1)
+ elog(ERROR, "cannot unpin unknown segment handle");Isn't it better to use INVALID_CONTROL_SLOT for control_slot and use
datatype as uint32 (same is used for dsm_segment->control_slot and
nitems)?
Apart from this, I have verified your patch on Windows using attached
dsm_demo module. Basically, by using dsm_demo_create(), I have pinned
the segment and noticed that Handle count of postmaster is incremented
by 1 and then by using dsm_demo_unpin_segment() unpinned the segment
which decrements the Handle count in Postmaster.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
dsm_demo_v1.patchapplication/octet-stream; name=dsm_demo_v1.patchDownload
diff --git a/contrib/Makefile b/contrib/Makefile
index 25263c0..9739013 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -17,6 +17,7 @@ SUBDIRS = \
dblink \
dict_int \
dict_xsyn \
+ dsm_demo \
earthdistance \
file_fdw \
fuzzystrmatch \
diff --git a/contrib/dsm_demo/Makefile b/contrib/dsm_demo/Makefile
new file mode 100644
index 0000000..dd9ea92
--- /dev/null
+++ b/contrib/dsm_demo/Makefile
@@ -0,0 +1,17 @@
+# contrib/dsm_demo/Makefile
+
+MODULES = dsm_demo
+
+EXTENSION = dsm_demo
+DATA = dsm_demo--1.0.sql
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/dsm_demo
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/dsm_demo/dsm_demo--1.0.sql b/contrib/dsm_demo/dsm_demo--1.0.sql
new file mode 100644
index 0000000..c0f1a99
--- /dev/null
+++ b/contrib/dsm_demo/dsm_demo--1.0.sql
@@ -0,0 +1,19 @@
+/* contrib/dsm_demo/dsm_demo--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION dsm_demo" to load this file. \quit
+
+CREATE FUNCTION dsm_demo_create(pg_catalog.text, pg_catalog.int4 default 0)
+RETURNS pg_catalog.int8 STRICT
+AS 'MODULE_PATHNAME'
+LANGUAGE C;
+
+CREATE FUNCTION dsm_demo_read(pg_catalog.int8)
+RETURNS pg_catalog.text STRICT
+AS 'MODULE_PATHNAME'
+LANGUAGE C;
+
+CREATE FUNCTION dsm_demo_unpin_segment(pg_catalog.int8)
+RETURNS pg_catalog.bool STRICT
+AS 'MODULE_PATHNAME'
+LANGUAGE C;
\ No newline at end of file
diff --git a/contrib/dsm_demo/dsm_demo.c b/contrib/dsm_demo/dsm_demo.c
new file mode 100644
index 0000000..7b10972
--- /dev/null
+++ b/contrib/dsm_demo/dsm_demo.c
@@ -0,0 +1,150 @@
+/* -------------------------------------------------------------------------
+ *
+ * dsm_demo.c
+ * Dynamic shared memory demonstration.
+ *
+ * Copyright (C) 2013, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/dsm_demo/dsm_demo.c
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "storage/dsm.h"
+#include "fmgr.h"
+
+PG_MODULE_MAGIC;
+
+void _PG_init(void);
+Datum dsm_demo_create(PG_FUNCTION_ARGS);
+Datum dsm_demo_read(PG_FUNCTION_ARGS);
+Datum dsm_demo_unpin_segment(PG_FUNCTION_ARGS);
+
+PG_FUNCTION_INFO_V1(dsm_demo_create);
+PG_FUNCTION_INFO_V1(dsm_demo_read);
+PG_FUNCTION_INFO_V1(dsm_demo_unpin_segment);
+
+#define DSM_DEMO_MAGIC 0x44454D4F
+
+typedef struct
+{
+ uint32 magic;
+ int32 len;
+ char data[FLEXIBLE_ARRAY_MEMBER];
+} dsm_demo_payload;
+
+/*
+ * dsm_demo_create(text, segment_lifetime int4)
+ *
+ * The first argument is the text data to be shared by using dsm, the second
+ * controls the life span of dsm segemnt. 0 indicates session lifetime, 1
+ * indicates postmaster lifetime.
+ */
+Datum
+dsm_demo_create(PG_FUNCTION_ARGS)
+{
+ text *txt = PG_GETARG_TEXT_PP(0);
+ int len = VARSIZE_ANY(txt);
+ int segment_life = PG_GETARG_INT32(1);
+ uint64 seglen;
+ dsm_segment *seg;
+ dsm_handle h;
+ dsm_demo_payload *payload;
+
+ seglen = offsetof(dsm_demo_payload, data) + len;
+ seg = dsm_create(seglen, DSM_CREATE_NULL_IF_MAXSEGMENTS);
+
+ if (segment_life == 0)
+ dsm_pin_mapping(seg);
+ else if (segment_life == 1)
+ dsm_pin_segment(seg);
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ (errmsg("value of segment lifetime can be either 0 or 1"))));
+
+ payload = dsm_segment_address(seg);
+ payload->magic = DSM_DEMO_MAGIC;
+ payload->len = len;
+ memcpy(payload->data, txt, len);
+
+ h = dsm_segment_handle(seg);
+
+ dsm_detach(seg);
+
+ PG_RETURN_INT64(h);
+}
+
+Datum
+dsm_demo_read(PG_FUNCTION_ARGS)
+{
+ dsm_handle h = PG_GETARG_INT64(0);
+ dsm_segment *seg;
+ bool needs_detach = false;
+ text *txt = NULL;
+ dsm_demo_payload *payload;
+
+ /*
+ * We could be called from the same sesion that called dsm_demo_create(),
+ * so search for an existing mapping. If we don't find one, attach the
+ * segment.
+ */
+ seg = dsm_find_mapping(h);
+ if (seg == NULL)
+ {
+ seg = dsm_attach(h);
+ if (!seg)
+ PG_RETURN_NULL();
+ needs_detach = true;
+ }
+
+ /* Extract data, after checking magic number. */
+ payload = dsm_segment_address(seg);
+ if (payload->magic == DSM_DEMO_MAGIC)
+ {
+ txt = palloc(payload->len);
+ memcpy(txt, payload->data, payload->len);
+ }
+
+ /* Detach, if there was no existing mapping. */
+ if (needs_detach)
+ dsm_detach(seg);
+
+ if (txt == NULL)
+ PG_RETURN_NULL();
+
+ PG_RETURN_TEXT_P(txt);
+}
+
+Datum
+dsm_demo_unpin_segment(PG_FUNCTION_ARGS)
+{
+ dsm_handle h = PG_GETARG_INT64(0);
+ dsm_segment *seg;
+ bool needs_detach = false;
+
+ /*
+ * We could be called from the same sesion that called dsm_demo_create(),
+ * so search for an existing mapping. If we don't find one, attach the
+ * segment.
+ */
+ seg = dsm_find_mapping(h);
+ if (seg == NULL)
+ {
+ seg = dsm_attach(h);
+ if (!seg)
+ PG_RETURN_BOOL(false);
+ needs_detach = true;
+ }
+
+ dsm_unpin_segment(h);
+
+ /* Detach, if there was no existing mapping. */
+ if (needs_detach)
+ dsm_detach(seg);
+
+ PG_RETURN_BOOL(true);
+}
diff --git a/contrib/dsm_demo/dsm_demo.control b/contrib/dsm_demo/dsm_demo.control
new file mode 100644
index 0000000..4060791
--- /dev/null
+++ b/contrib/dsm_demo/dsm_demo.control
@@ -0,0 +1,5 @@
+# dsm_demo extension
+comment = 'Dynamic shared memory demonstration'
+default_version = '1.0'
+module_pathname = 'dsm_demo'
+relocatable = true
On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
+ int control_slot = -1; ... + if (control_slot == -1) + elog(ERROR, "cannot unpin unknown segment handle");Isn't it better to use INVALID_CONTROL_SLOT for control_slot and use
datatype as uint32 (same is used for dsm_segment->control_slot and
nitems)?
Yes, it is better. New version attached.
Apart from this, I have verified your patch on Windows using attached
dsm_demo module. Basically, by using dsm_demo_create(), I have pinned
the segment and noticed that Handle count of postmaster is incremented
by 1 and then by using dsm_demo_unpin_segment() unpinned the segment
which decrements the Handle count in Postmaster.
Thanks!
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
dsm-unpin-segment-v4.patchapplication/octet-stream; name=dsm-unpin-segment-v4.patchDownload
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 47f2bea..b7836f1 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -82,6 +82,8 @@ typedef struct dsm_control_item
{
dsm_handle handle;
uint32 refcnt; /* 2+ = active, 1 = moribund, 0 = gone */
+ void *impl_private_pm_handle; /* only needed on Windows */
+ bool pinned;
} dsm_control_item;
/* Layout of the dynamic shared memory control segment. */
@@ -491,6 +493,8 @@ dsm_create(Size size, int flags)
dsm_control->item[i].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[i].refcnt = 2;
+ dsm_control->item[i].impl_private_pm_handle = NULL;
+ dsm_control->item[i].pinned = false;
seg->control_slot = i;
LWLockRelease(DynamicSharedMemoryControlLock);
return seg;
@@ -520,6 +524,8 @@ dsm_create(Size size, int flags)
dsm_control->item[nitems].handle = seg->handle;
/* refcnt of 1 triggers destruction, so start at 2 */
dsm_control->item[nitems].refcnt = 2;
+ dsm_control->item[nitems].impl_private_pm_handle = NULL;
+ dsm_control->item[nitems].pinned = false;
seg->control_slot = nitems;
dsm_control->nitems++;
LWLockRelease(DynamicSharedMemoryControlLock);
@@ -760,6 +766,9 @@ dsm_detach(dsm_segment *seg)
/* If new reference count is 1, try to destroy the segment. */
if (refcnt == 1)
{
+ /* A pinned segment should never reach 1. */
+ Assert(!dsm_control->item[control_slot].pinned);
+
/*
* If we fail to destroy the segment here, or are killed before we
* finish doing so, the reference count will remain at 1, which
@@ -830,11 +839,11 @@ dsm_unpin_mapping(dsm_segment *seg)
}
/*
- * Keep a dynamic shared memory segment until postmaster shutdown.
+ * Keep a dynamic shared memory segment until postmaster shutdown, or until
+ * dsm_unpin_segment is called.
*
- * This function should not be called more than once per segment;
- * on Windows, doing so will create unnecessary handles which will
- * consume system resources to no benefit.
+ * This function should not be called more than once per segment, unless the
+ * segment is explicitly unpinned with dsm_unpin_segment in between calls.
*
* Note that this function does not arrange for the current process to
* keep the segment mapped indefinitely; if that behavior is desired,
@@ -844,16 +853,102 @@ dsm_unpin_mapping(dsm_segment *seg)
void
dsm_pin_segment(dsm_segment *seg)
{
+ void *handle;
+
/*
* Bump reference count for this segment in shared memory. This will
* ensure that even if there is no session which is attached to this
- * segment, it will remain until postmaster shutdown.
+ * segment, it will remain until postmaster shutdown or an explicit call
+ * to unpin.
*/
LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ if (dsm_control->item[seg->control_slot].pinned)
+ elog(ERROR, "cannot pin a segment that is already pinned");
+ dsm_impl_pin_segment(seg->handle, seg->impl_private, &handle);
+ dsm_control->item[seg->control_slot].pinned = true;
dsm_control->item[seg->control_slot].refcnt++;
+ dsm_control->item[seg->control_slot].impl_private_pm_handle = handle;
LWLockRelease(DynamicSharedMemoryControlLock);
+}
+
+/*
+ * Unpin a dynamic shared memory segment that was previously pinned with
+ * dsm_pin_segment. This function should not be called unless dsm_pin_segment
+ * was previously called for this segment.
+ *
+ * The argument is a dsm_handle rather than a dsm_segment in case you want
+ * to unpin a segment to which you haven't attached. This turns out to be
+ * useful if, for example, a reference to one shared memory segment is stored
+ * within another shared memory segment. You might want to unpin the
+ * referenced segment before destroying the referencing segment.
+ */
+void
+dsm_unpin_segment(dsm_handle handle)
+{
+ uint32 control_slot = INVALID_CONTROL_SLOT;
+ bool destroy = false;
+ uint32 i;
- dsm_impl_pin_segment(seg->handle, seg->impl_private);
+ /* Find the control slot for the given handle. */
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ for (i = 0; i < dsm_control->nitems; ++i)
+ {
+ /* Skip unused slots. */
+ if (dsm_control->item[i].refcnt == 0)
+ continue;
+
+ /* If we've found our handle, we can stop searching. */
+ if (dsm_control->item[i].handle == handle)
+ {
+ control_slot = i;
+ break;
+ }
+ }
+
+ /*
+ * We should definitely have found the slot, and it should not already be
+ * in the process of going away, because this function should only be
+ * called on a segment which is pinned.
+ */
+ if (control_slot == INVALID_CONTROL_SLOT)
+ elog(ERROR, "cannot unpin unknown segment handle");
+ if (!dsm_control->item[control_slot].pinned)
+ elog(ERROR, "cannot unpin a segment that is not pinned");
+ Assert(dsm_control->item[control_slot].refcnt > 1);
+
+ /*
+ * Allow implementation-specific code to run. We have to do this before
+ * releasing the lock, because impl_private_pm_handle may get modified by
+ * dsm_impl_unpin_segment.
+ */
+ dsm_impl_unpin_segment(handle,
+ &dsm_control->item[control_slot].impl_private_pm_handle);
+
+ /* Note that 1 means no references (0 means unused slot). */
+ if (--dsm_control->item[control_slot].refcnt == 1)
+ destroy = true;
+ dsm_control->item[control_slot].pinned = false;
+
+ /* Now we can release the lock. */
+ LWLockRelease(DynamicSharedMemoryControlLock);
+
+ /* Clean up resources if that was the last reference. */
+ if (destroy)
+ {
+ void *junk_mapped_address = NULL;
+ void *junk_impl_private = NULL;
+ Size junk_mapped_size = 0;
+
+ if (!dsm_impl_op(DSM_OP_DESTROY, handle, 0, &junk_impl_private,
+ &junk_mapped_address, &junk_mapped_size, WARNING))
+ return;
+
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ Assert(dsm_control->item[control_slot].handle == handle);
+ Assert(dsm_control->item[control_slot].refcnt == 1);
+ dsm_control->item[control_slot].refcnt = 0;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+ }
}
/*
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 173b982..c07a5c6 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -987,8 +987,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
#endif
/*
- * Implementation-specific actions that must be performed when a segment
- * is to be preserved until postmaster shutdown.
+ * Implementation-specific actions that must be performed when a segment is to
+ * be preserved even when no backend has it attached.
*
* Except on Windows, we don't need to do anything at all. But since Windows
* cleans up segments automatically when no references remain, we duplicate
@@ -996,7 +996,8 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
* do anything to receive the handle; Windows transfers it automatically.
*/
void
-dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
+dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle)
{
switch (dynamic_shared_memory_type)
{
@@ -1018,6 +1019,56 @@ dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
errmsg("could not duplicate handle for \"%s\": %m",
name)));
}
+
+ /*
+ * Here, we remember the handle that we created in the
+ * postmaster process. This handle isn't actually usable in
+ * any process other than the postmaster, but that doesn't
+ * matter. We're just holding onto it so that, if the segment
+ * is unpinned, dsm_impl_unpin_segment can close it.
+ */
+ *impl_private_pm_handle = hmap;
+ break;
+ }
+#endif
+ default:
+ break;
+ }
+}
+
+/*
+ * Implementation-specific actions that must be performed when a segment is no
+ * longer to be preserved, so that it will be cleaned up when all backends
+ * have detached from it.
+ *
+ * Except on Windows, we don't need to do anything at all. For Windows, we
+ * close the extra handle that dsm_impl_pin_segment created in the
+ * postmaster's process space.
+ */
+void
+dsm_impl_unpin_segment(dsm_handle handle, void **impl_private)
+{
+ switch (dynamic_shared_memory_type)
+ {
+#ifdef USE_DSM_WINDOWS
+ case DSM_IMPL_WINDOWS:
+ {
+ if (*impl_private &&
+ !DuplicateHandle(PostmasterHandle, *impl_private,
+ NULL, NULL, 0, FALSE,
+ DUPLICATE_CLOSE_SOURCE))
+ {
+ char name[64];
+
+ snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
+ _dosmaperr(GetLastError());
+ ereport(ERROR,
+ (errcode_for_dynamic_shared_memory(),
+ errmsg("could not duplicate handle for \"%s\": %m",
+ name)));
+ }
+
+ *impl_private = NULL;
break;
}
#endif
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 86ede7a..8be7c9a 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -41,6 +41,7 @@ extern void dsm_detach(dsm_segment *seg);
extern void dsm_pin_mapping(dsm_segment *seg);
extern void dsm_unpin_mapping(dsm_segment *seg);
extern void dsm_pin_segment(dsm_segment *seg);
+extern void dsm_unpin_segment(dsm_handle h);
extern dsm_segment *dsm_find_mapping(dsm_handle h);
/* Informational functions. */
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index ec05e22..e44b477 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -73,6 +73,8 @@ extern bool dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
extern bool dsm_impl_can_resize(void);
/* Implementation-dependent actions required to keep segment until shutdown. */
-extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private);
+extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
+ void **impl_private_pm_handle);
+extern void dsm_impl_unpin_segment(dsm_handle handle, void **impl_private);
#endif /* DSM_IMPL_H */
On Mon, Aug 22, 2016 at 6:06 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
+ int control_slot = -1; ... + if (control_slot == -1) + elog(ERROR, "cannot unpin unknown segment handle");Isn't it better to use INVALID_CONTROL_SLOT for control_slot and use
datatype as uint32 (same is used for dsm_segment->control_slot and
nitems)?Yes, it is better. New version attached.
This version of patch looks good to me. I have marked it as Ready For
Committer.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Aug 22, 2016 at 10:04 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Aug 22, 2016 at 6:06 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
+ int control_slot = -1; ... + if (control_slot == -1) + elog(ERROR, "cannot unpin unknown segment handle");Isn't it better to use INVALID_CONTROL_SLOT for control_slot and use
datatype as uint32 (same is used for dsm_segment->control_slot and
nitems)?Yes, it is better. New version attached.
This version of patch looks good to me. I have marked it as Ready For
Committer.
Committed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers