On Mon, Mar 10, 2014 at 8:18 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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.
Thank you for looking into patch. I have verified that attached patch
works fine on Windows.
One observation in new version of patch:
+ {
+ 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")));
+ }
Now, the patch doesn't use segment *name* in errmsg which makes
it and handle passed in function dsm_impl_keep_segment() redundant.
I think its better to use name in errmsg as it is used at other places
in code as well and make the error more meaningful. However if you
feel it is better otherwise, then we should remove not required variable
and parameter in function dsm_impl_keep_segment()
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com