Re: dsm_unpin_segment

On Mon, Aug 22, 2016 at 5:24 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
>> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>>> On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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