On Mon, May 9, 2016 at 4:17 PM, Michael Paquier
<[email protected]> wrote:
> On Tue, Mar 22, 2016 at 1:56 PM, Amit Kapila <[email protected]> wrote:
>> So as far as I can see there are two ways to resolve this issue, one is to
>> retry generation of dsm name if CreateFileMapping returns EACCES and second
>> is to append data_dir name to dsm name as the same is done for main shared
>> memory, that will avoid the error to occur. First approach has minor flaw
>> that if CreateFileMapping returns EACCES due to reason other then duplicate
>> dsm name which I am not sure is possible to identify, then we should report
>> error instead try to regenerate the name
>>
>> Robert and or others, can you share your opinion on what is the best way to
>> proceed for this issue.
>
> For my 2c here, the approach using GetSharedMemName to identify the
> origin of a dynamic shared memory segment looks more solid in terms of
> robustness and collision handling. Retrying a segment is never going
> to be completely water-proof.
So, I have been hacking that a bit more and finished with the
attached, which seem to address the issue here. Some of the code paths
of dsm_impl.c are done in such a way that we can fail a dsm allocation
and still continue to move on. I have taken that into account by using
palloc_extended with NO_OOM. palloc instead of malloc is a good fit
anyway to prevent leaks in error code paths.
Thoughts?
--
Michael
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 173b982..060d0cb 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -62,6 +62,7 @@
#include <sys/shm.h>
#endif
+#include "miscadmin.h"
#include "portability/mem.h"
#include "storage/dsm_impl.h"
#include "storage/fd.h"
@@ -80,6 +81,7 @@ static bool dsm_impl_sysv(dsm_op op, dsm_handle handle, Size request_size,
Size *mapped_size, int elevel);
#endif
#ifdef USE_DSM_WINDOWS
+static char *get_segment_name(dsm_handle handle);
static bool dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
void **impl_private, void **mapped_address,
Size *mapped_size, int elevel);
@@ -590,6 +592,36 @@ dsm_impl_sysv(dsm_op op, dsm_handle handle, Size request_size,
#ifdef USE_DSM_WINDOWS
/*
+ * Generate shared memory segment name, using the data directory to generate
+ * a unique identifier. This segment is stored in Global\ namespace to allow
+ * any other process to have an access to it.
+ */
+static char *
+get_segment_name(dsm_handle handle)
+{
+ char *res;
+
+ /* let enough place for prefix and handle */
+ res = palloc_extended(strlen(DataDir) + 64, MCXT_ALLOC_NO_OOM);
+
+ if (res == NULL)
+ return NULL;
+
+ /*
+ * Storing the shared memory segment in the Global\ namespace, can allow
+ * any process running in any session to access that file mapping object
+ * provided that the caller has the required access rights. But to avoid
+ * issues faced in main shared memory, we are using the naming convention
+ * similar to main shared memory. We can change here once issue mentioned
+ * in GetSharedMemName is resolved.
+ */
+ snprintf(res, strlen(DataDir) + 64, "%s.%s.%u",
+ SEGMENT_NAME_PREFIX, DataDir, handle);
+
+ return res;
+}
+
+/*
* Operating system primitives to support Windows shared memory.
*
* Windows shared memory implementation is done using file mapping
@@ -609,7 +641,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
{
char *address;
HANDLE hmap;
- char name[64];
+ char *name;
MEMORY_BASIC_INFORMATION info;
/* Resize is not supported for Windows shared memory. */
@@ -623,15 +655,13 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
if (op == DSM_OP_ATTACH && *mapped_address != NULL)
return true;
- /*
- * Storing the shared memory segment in the Global\ namespace, can allow
- * any process running in any session to access that file mapping object
- * provided that the caller has the required access rights. But to avoid
- * issues faced in main shared memory, we are using the naming convention
- * similar to main shared memory. We can change here once issue mentioned
- * in GetSharedMemName is resolved.
- */
- snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
+ name = get_segment_name(handle);
+
+ if (name == NULL)
+ {
+ elog(elevel, "could not allocate memory for shared memory segment name");
+ return false;
+ }
/*
* Handle teardown cases. Since Windows automatically destroys the object
@@ -647,6 +677,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
(errcode_for_dynamic_shared_memory(),
errmsg("could not unmap shared memory segment \"%s\": %m",
name)));
+ pfree(name);
return false;
}
if (*impl_private != NULL
@@ -657,12 +688,14 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
(errcode_for_dynamic_shared_memory(),
errmsg("could not remove shared memory segment \"%s\": %m",
name)));
+ pfree(name);
return false;
}
*impl_private = NULL;
*mapped_address = NULL;
*mapped_size = 0;
+ pfree(name);
return true;
}
@@ -693,6 +726,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
(errcode_for_dynamic_shared_memory(),
errmsg("could not create shared memory segment \"%s\": %m",
name)));
+ pfree(name);
return false;
}
_dosmaperr(GetLastError());
@@ -705,6 +739,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
* modified.
*/
CloseHandle(hmap);
+ pfree(name);
return false;
}
}
@@ -720,6 +755,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
(errcode_for_dynamic_shared_memory(),
errmsg("could not open shared memory segment \"%s\": %m",
name)));
+ pfree(name);
return false;
}
}
@@ -741,6 +777,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
(errcode_for_dynamic_shared_memory(),
errmsg("could not map shared memory segment \"%s\": %m",
name)));
+ pfree(name);
return false;
}
@@ -765,6 +802,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
(errcode_for_dynamic_shared_memory(),
errmsg("could not stat shared memory segment \"%s\": %m",
name)));
+ pfree(name);
return false;
}
@@ -772,6 +810,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
*mapped_size = info.RegionSize;
*impl_private = hmap;
+ pfree(name);
return true;
}
#endif
@@ -1009,14 +1048,15 @@ dsm_impl_pin_segment(dsm_handle handle, void *impl_private)
PostmasterHandle, &hmap, 0, FALSE,
DUPLICATE_SAME_ACCESS))
{
- char name[64];
+ char *name;
- snprintf(name, 64, "%s.%u", SEGMENT_NAME_PREFIX, handle);
_dosmaperr(GetLastError());
+ name = get_segment_name(handle);
ereport(ERROR,
(errcode_for_dynamic_shared_memory(),
errmsg("could not duplicate handle for \"%s\": %m",
- name)));
+ name ? name : "undefined")));
+ pfree(name);
}
break;
}
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
