On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
<thomas.mu...@enterprisedb.com> wrote:
> On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane <t...@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:

+ /* 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.

+ 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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to