On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.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:
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().
Do you need a check if (control_slot >= 0)? In the code just above
you have as Assert to ensure that it is >=0.
2.
+ if (dsm_control->item[seg->control_slot].pinned)
+ elog(ERROR, "cannot pin a segment that is already pinned");
Shouldn't this be a user facing error (which means we should use ereport)?
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
