On Sat, Feb 8, 2014 at 2:31 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Thu, Feb 6, 2014 at 3:42 PM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@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
