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: 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
