On Mon, Aug 22, 2016 at 5:24 AM, Thomas Munro <thomas.mu...@enterprisedb.com> wrote: > On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: >> 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: > > 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
dsm_demo_v1.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
