On Sat, Feb 8, 2014 at 2:31 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
>> On Thu, Feb 6, 2014 at 3:42 PM, Kyotaro HORIGUCHI
>> <horiguchi.kyot...@lab.ntt.co.jp> wrote:
>>> Hello, I've understood how this works and seems working as
>>> expected.
>>>
>>>
>>> The orphan section handles on postmaster have become a matter of
>>> documentation.
>
> I had explained this in function header of dsm_keep_segment().
>
>>> Besides all above, I'd like to see a comment for the win32 code
>>> about the 'DuplicateHandle hack', specifically, description that
>>> the DuplicateHandle pushes the copy of the section handle to the
>>> postmaster so the section can retain for the postmaster lifetime.
>
> I had added a new function in dsm_impl.c for platform specific
> handling and explained about new behaviour in function header.
>
>
>> - "Global/PostgreSQL.%u" is the same literal constant with that
>> occurred in dsm_impl_windows. It should be defined as a
>> constant (or a macro).
>
> Changed to hash define.
>
>> - dms_impl_windows uses errcode_for_dynamic_shared_memory() for
>> ereport and it finally falls down to
>> errcode_for_file_access(). I think it is preferable, maybe
>
> Changed as per suggestion.
>
> Please find new version of patch attached with this mail containing
> above changes.
I took a look at this patch. It seems to me that it doesn't do a very
good job maintaining the abstraction boundary between what the dsm.c
layer knows about and what the dsm_impl.c layer knows about. However,
AFAICS, these problems are purely cosmetic, so I took a crack at
fixing them. I retitled the new implementation-layer function to
dsm_impl_keep_segment(), swapped the order of the arguments for
consistency with other code, adjusted the dsm_impl.c code slightly to
avoid assuming that only the Windows implementation works on Windows
(that's currently true, but we could probably make the mmap
implementation work there as well), and retooled some of the comments
to read better in English. I'm happy with the attached version but
don't have a Windows box to test it there.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 31e592e..c7dc971 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -886,6 +886,34 @@ dsm_keep_mapping(dsm_segment *seg)
}
/*
+ * Keep a dynamic shared memory segment until postmaster shutdown.
+ *
+ * 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.
+ */
+void
+dsm_keep_segment(dsm_segment *seg)
+{
+ /*
+ * 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.
+ */
+ LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
+ dsm_control->item[seg->control_slot].refcnt++;
+ LWLockRelease(DynamicSharedMemoryControlLock);
+
+ dsm_impl_keep_segment(seg->handle, seg->impl_private);
+
+ if (seg->resowner != NULL)
+ {
+ ResourceOwnerForgetDSM(seg->resowner, seg);
+ seg->resowner = NULL;
+ }
+}
+
+/*
* Find an existing mapping for a shared memory segment, if there is one.
*/
dsm_segment *
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index a8d8a64..221044a 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -67,6 +67,7 @@
#include "storage/fd.h"
#include "utils/guc.h"
#include "utils/memutils.h"
+#include "postmaster/postmaster.h"
#ifdef USE_DSM_POSIX
static bool dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
@@ -113,6 +114,8 @@ int dynamic_shared_memory_type;
/* Size of buffer to be used for zero-filling. */
#define ZBUFFER_SIZE 8192
+#define SEGMENT_NAME_PREFIX "Global/PostgreSQL"
+
/*------
* Perform a low-level shared memory operation in a platform-specific way,
* as dictated by the selected implementation. Each implementation is
@@ -635,7 +638,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
* convention similar to main shared memory. We can change here once
* issue mentioned in GetSharedMemName is resolved.
*/
- snprintf(name, 64, "Global/PostgreSQL.%u", handle);
+ snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
/*
* Handle teardown cases. Since Windows automatically destroys the object
@@ -982,6 +985,45 @@ 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.
+ *
+ * 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
+ * the segment handle into the postmaster process. The postmaster needn't
+ * do anything to receive the handle; Windows transfers it automatically.
+ */
+void
+dsm_impl_keep_segment(dsm_handle handle, void *impl_private)
+{
+ switch (dynamic_shared_memory_type)
+ {
+#ifdef USE_DSM_WINDOWS
+ case DSM_IMPL_WINDOWS:
+ {
+ HANDLE hmap;
+
+ if (!DuplicateHandle(GetCurrentProcess(), impl_private,
+ PostmasterHandle, &hmap, 0, FALSE,
+ DUPLICATE_SAME_ACCESS))
+ {
+ 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: %m")));
+ }
+ break;
+ }
+#endif
+ default:
+ break;
+ }
+}
+
static int
errcode_for_dynamic_shared_memory()
{
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 46d3cbd..03dd767 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -30,6 +30,7 @@ extern void dsm_detach(dsm_segment *seg);
/* Resource management functions. */
extern void dsm_keep_mapping(dsm_segment *seg);
+extern void dsm_keep_segment(dsm_segment *seg);
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 f2d0c64..fda5514 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -72,4 +72,7 @@ extern bool dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
/* Some implementations cannot resize segments. Can this one? */
extern bool dsm_impl_can_resize(void);
+/* Implementation-dependent actions required to keep segment until shudown. */
+extern void dsm_impl_keep_segment(dsm_handle handle, void *impl_private);
+
#endif /* DSM_IMPL_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
