Thread: dsm_unpin_segment
Hi hackers, DSM segments have a concept of 'pinning'. Normally, segments are destroyed when they are no longer mapped by any backend, using a reference counting scheme. If you call dsm_pin_segment(segment), that is disabled so that the segment won't be destroyed until the cluster is shut down. It works by incrementing the reference count an extra time. Please find attached a patch to add a corresponding operation 'dsm_unpin_segment'. This gives you a way to ask for the segment to survive only until you decide to unpin it, at which point the usual reference counting semantics apply again. It decrements the reference count, undoing the effect of dsm_pin_segment and destroying the segment if appropriate. I think this is very useful for any core or extension code that wants to store data in dynamic shared memory that survives even when no backends are running, without having to commit to keeping the segment forever. We have several projects in the 10.x pipeline which make use of DSM segments, and we ran into the need for this finer grained control of their lifetime. Thanks to my colleague Amit Kapila for the Windows part, and also to Robert Haas for internal feedback on an earlier version. The Windows implementation has an extra quirk: Windows has its own reference counting scheme that destroys mapped memory when there are no attached processes, so pinning is implemented by sending a duplicate of the handle into the Postmaster process to keep the segment alive. This patch needs to close that handle when unpinning. Amazingly, that can be done without any cooperation from the postmaster. I'd be grateful for any feedback or thoughts, and will add this to the commitfest. -- Thomas Munro http://www.enterprisedb.com
Attachment
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> DSM segments have a concept of 'pinning'. Normally, segments are
> destroyed when they are no longer mapped by any backend, using a
> reference counting scheme. If you call dsm_pin_segment(segment), that
> is disabled so that the segment won't be destroyed until the cluster
> is shut down. It works by incrementing the reference count an extra
> time.
> Please find attached a patch to add a corresponding operation
> 'dsm_unpin_segment'. This gives you a way to ask for the segment to
> survive only until you decide to unpin it, at which point the usual
> reference counting semantics apply again. It decrements the reference
> count, undoing the effect of dsm_pin_segment and destroying the
> segment if appropriate.
What happens if dsm_unpin_segment is called more times than
dsm_pin_segment? Seems like you could try to destroy a segment that
still has processes attached.
I don't object to the concept, but you need a less half-baked
implementation if you want to add this. I'd suggest separate counters for
process attaches and pin requests, with code in dsm_unpin_segment to
disallow decrementing the pin request count below zero, and segment
destruction only when both counters go to zero.
regards, tom lane
On Mon, Aug 8, 2016 at 7:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: >> DSM segments have a concept of 'pinning'. Normally, segments are >> destroyed when they are no longer mapped by any backend, using a >> reference counting scheme. If you call dsm_pin_segment(segment), that >> is disabled so that the segment won't be destroyed until the cluster >> is shut down. It works by incrementing the reference count an extra >> time. > >> Please find attached a patch to add a corresponding operation >> 'dsm_unpin_segment'. This gives you a way to ask for the segment to >> survive only until you decide to unpin it, at which point the usual >> reference counting semantics apply again. It decrements the reference >> count, undoing the effect of dsm_pin_segment and destroying the >> segment if appropriate. > > What happens if dsm_unpin_segment is called more times than > dsm_pin_segment? Seems like you could try to destroy a segment that > still has processes attached. Calling dsm_pin_segment more than once is not supported and has never been supported. As the comments explain: * This function should not be called more than once per segment;* on Windows, doing so will create unnecessary handles whichwill* consume system resources to no benefit. Therefore, I don't see the problem. You can pin a segment that is not pinned, and you can unpin a segment that is pinned. You may not re-pin a segment that is already pinned, nor unpin a segment that is not pinned. If you try to do so, you are using the API contrary to specification, and if it breaks (as it will) you get to keep both pieces. We could add the reference counting behavior for which you are asking, but that seems to be an entirely new feature for which I know of no demand. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Aug 9, 2016 at 12:22 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Aug 8, 2016 at 7:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Thomas Munro <thomas.munro@enterprisedb.com> writes: >>> Please find attached a patch to add a corresponding operation >>> 'dsm_unpin_segment'. This gives you a way to ask for the segment to >>> survive only until you decide to unpin it, at which point the usual >>> reference counting semantics apply again. It decrements the reference >>> count, undoing the effect of dsm_pin_segment and destroying the >>> segment if appropriate. >> >> What happens if dsm_unpin_segment is called more times than >> dsm_pin_segment? Seems like you could try to destroy a segment that >> still has processes attached. > > Calling dsm_pin_segment more than once is not supported and has never > been supported. As the comments explain: > > * This function should not be called more than once per segment; > * on Windows, doing so will create unnecessary handles which will > * consume system resources to no benefit. > > Therefore, I don't see the problem. You can pin a segment that is not > pinned, and you can unpin a segment that is pinned. You may not > re-pin a segment that is already pinned, nor unpin a segment that is > not pinned. If you try to do so, you are using the API contrary to > specification, and if it breaks (as it will) you get to keep both > pieces. > > We could add the reference counting behavior for which you are asking, > but that seems to be an entirely new feature for which I know of no > demand. Yeah, I was considering unbalanced pin/unpin requests to be a programming error. To be more defensive about that, how about I add a boolean 'pinned' to dsm_control_item, and elog(ERROR, ...) if it's not in the expected state when you try to pin or unpin? -- Thomas Munro http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Yeah, I was considering unbalanced pin/unpin requests to be a
> programming error. To be more defensive about that, how about I add a
> boolean 'pinned' to dsm_control_item, and elog(ERROR, ...) if it's not
> in the expected state when you try to pin or unpin?
Well, what you have there is a one-bit-wide pin request counter.
I do not see why that's better than an actual counter, but if that's
what you want to do, fine.
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. What exactly
is an extension going to be doing to ensure that it doesn't do too many of
one or the other?
regards, tom lane
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. > What exactly > is an extension going to be doing to ensure that it doesn't do too many of > one or the other? An extension that manages segment lifetimes like this needs a carefully designed protocol to get it right, probably involving state in another shared memory area and some interlocking, not to mention a lot of thought about cleanup. Here's one use case: I have a higher level object, a multi-segment-backed shared memory allocator, which owns any number of segments that together form a shared memory area. The protocol is that the allocator always pins segments when it needs to create new ones, because they need to survive as long as the control segment, even though no one backend is guaranteed to have all of the auxiliary segments mapped in (since they're created and attached on demand). But when the control segment is detached by all backends and is due to be destroyed, then we need to unpin all the auxiliary segments so they can also be destroyed, and that can be done from an on_dsm_detach callback on the control segment. So I'm riding on the coat tails of the existing cleanup mechanism for the control segment, while making sure that the auxiliary segments get pinned and unpinned exactly once. I'll have more to say about that when I post that patch... -- Thomas Munro http://www.enterprisedb.com
Attachment
On Mon, Aug 8, 2016 at 8:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: >> Yeah, I was considering unbalanced pin/unpin requests to be a >> programming error. To be more defensive about that, how about I add a >> boolean 'pinned' to dsm_control_item, and elog(ERROR, ...) if it's not >> in the expected state when you try to pin or unpin? > > Well, what you have there is a one-bit-wide pin request counter. > I do not see why that's better than an actual counter, but if that's > what you want to do, fine. > > The larger picture here is that Robert is exhibiting a touching but > unfounded faith that extensions using this feature will contain zero bugs. That's an overstatement of my position. I think it is quite likely that extensions using this feature will have bugs, because essentially all code has bugs, but whether they are likely have the specific bug of unpinning a segment that is already unpinned is not quite so clear. That's not to say I object to Thomas's v2 patch, which will catch that mistake if it happens. Defensive programming never killed anybody, as far as I know. However, I don't see the need for a full-blown request counter here; we've had this API for several releases now and to my knowledge nobody has complained about the fact that you aren't supposed to call dsm_pin_segment() multiple times for the same segment. Therefore, I think the evidence supports the contention that it's not broken and doesn't need to be fixed. If we do decide it needs to be fixed, I think that's material for a separate patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 8/9/16 1:01 PM, Robert Haas wrote: > However, I don't see the need for a full-blown request > counter here; we've had this API for several releases now and to my > knowledge nobody has complained about the fact that you aren't > supposed to call dsm_pin_segment() multiple times for the same > segment. Could a couple of static variables be used to ensure multiple pin/unpin and attach/detach calls throw an assert() (or become a no-op if asserts are disabled)? It would be nice if we could protect users from this. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On Wed, Aug 10, 2016 at 10:38 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 8/9/16 1:01 PM, Robert Haas wrote: >> >> However, I don't see the need for a full-blown request >> counter here; we've had this API for several releases now and to my >> knowledge nobody has complained about the fact that you aren't >> supposed to call dsm_pin_segment() multiple times for the same >> segment. > > > Could a couple of static variables be used to ensure multiple pin/unpin and > attach/detach calls throw an assert() (or become a no-op if asserts are > disabled)? It would be nice if we could protect users from this. The can't be static, they need to be in shared memory, because we also want to protect against two *different* backends pinning it. The v2 patch protects users from this, by adding 'pinned' flag to the per-segment control object that is visible everywhere, and then doing: + if (dsm_control->item[seg->control_slot].pinned) + elog(ERROR, "cannot pin a segment that is already pinned"); ... and: + if (!dsm_control->item[i].pinned) + elog(ERROR, "cannot unpin a segment that is not pinned"); Those checks could arguably be assertions rather than errors, but I don't think that pin/unpin operations will ever be high frequency enough for it to be worth avoiding those instructions in production builds. The whole reason for pinning segments is likely to be able to reuse them repeatedly, so nailing it down is likely something you'd want to do immediately after creating it. -- Thomas Munro http://www.enterprisedb.com
On 8/9/16 6:14 PM, Thomas Munro wrote: >> Could a couple of static variables be used to ensure multiple pin/unpin and >> > attach/detach calls throw an assert() (or become a no-op if asserts are >> > disabled)? It would be nice if we could protect users from this. > The can't be static, they need to be in shared memory, because we also > want to protect against two *different* backends pinning it. Right, this would strictly protect from it happening within a single backend. Perhaps it's pointless for pin/unpin, but it seems like it would be a good thing to have for attach/detach. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461
On Wed, Aug 10, 2016 at 2:43 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:
> On 8/9/16 6:14 PM, Thomas Munro wrote:
>> The can't be static, they need to be in shared memory, because we also
>> want to protect against two *different* backends pinning it.
>
> Right, this would strictly protect from it happening within a single
> backend. Perhaps it's pointless for pin/unpin, but it seems like it would be
> a good thing to have for attach/detach.
Double attach already gets you:
elog(ERROR, "can't attach the same segment more than once");
Double detach is conceptually like double free, and in fact actually
leads to a double pfree of the backend-local dsm_segment object. It
doesn't seem like the kind of thing it's easy or reasonable to try to
defend against, since you have no space left in which to store the
state you need to detect double-frees after you've done it once!
--
Thomas Munro
http://www.enterprisedb.com
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
On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > 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)? Uh, certainly not. This can't happen because of SQL the user enters; it can only happen because of a C coding error. elog() is the appropriate tool for that case. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Aug 20, 2016 at 6:13 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> 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)? > > Uh, certainly not. This can't happen because of SQL the user enters; > it can only happen because of a C coding error. elog() is the > appropriate tool for that case. > Okay, your point makes sense to me, but in that case why not use an Assert here? I think elog() could also be used here as well, but it seems to me elog() is most appropriate for the cases where it is not straightforward to tell the behaviour of API or value of variable like when PageAddItem() is not successful. voiddsm_pin_segment(dsm_segment *seg) +void +dsm_unpin_segment(dsm_handle handle) Another point, I want to highlight here is that pin/unpin API's have different type of arguments. The comments on top of function dsm_unpin_segment() explains the need of using dsm_handle which seems reasonable. I just pointed out to see if anybody else has a different view. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > 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: 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. > Do you need a check if (control_slot >= 0)? In the code just above > you have as Assert to ensure that it is >=0. Right. Furthermore, the code was using "i" in some places and "control_slot" in others. We might as well use control_slot everywhere. On Sun, Aug 21, 2016 at 5:45 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Sat, Aug 20, 2016 at 6:13 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> 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)? >> >> Uh, certainly not. This can't happen because of SQL the user enters; >> it can only happen because of a C coding error. elog() is the >> appropriate tool for that case. >> > > Okay, your point makes sense to me, but in that case why not use an > Assert here? I think elog() could also be used here as well, but it > seems to me elog() is most appropriate for the cases where it is not > straightforward to tell the behaviour of API or value of variable like > when PageAddItem() is not successful. Here's the rationale I'm using: if it's helpful to programmers of client code, especially client code that might include extensions, and nowhere near a hot code path, then why not use elog rather than Assert? I took inspiration for that from the pre-existing "debugging cross-check" in dsm_attach that does elog(ERROR, "can't attach the same segment more than once"). On that basis, this new version retains the elog you mentioned, and now also uses elog for the you-tried-to-unpin-a-handle-I-couldn't-find case. But I kept Assert in places that detect bugs in *this* code, rather than client code. > void > dsm_pin_segment(dsm_segment *seg) > > +void > +dsm_unpin_segment(dsm_handle handle) > > Another point, I want to highlight here is that pin/unpin API's have > different type of arguments. The comments on top of function > dsm_unpin_segment() explains the need of using dsm_handle which seems > reasonable. I just pointed out to see if anybody else has a different > view. Now that I have posted the DSA patch[1], it probably makes sense to explain the path by which I arrived at the conclusion that unpin should take a handle. In an earlier version, dsm_unpin_segment took a dsm_segment *, mirroring the pin function. But then Robert complained privately that my dsa_area destroy code path was sometimes having to attach segments just to unpin them and then detach, which might fail due to lack of virtual memory. He was right to complain: that created a fairly nasty failure mode where I'd unpinned some but not all of the DSM segments that belong together. So I concluded that it should be possible to unpin a segment even when you haven't got it attached, and rewrote it this way. In general, any structure built from multiple DSM segments which point to each other using handles could run into this problem. I think the function's comment covers it, but hopefully this concrete example is convincing. [1] https://www.postgresql.org/message-id/CAEepm%3D1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA%40mail.gmail.com -- Thomas Munro http://www.enterprisedb.com
Attachment
On Sun, Aug 21, 2016 at 7:54 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > Here's the rationale I'm using: if it's helpful to programmers of > client code, especially client code that might include extensions, and > nowhere near a hot code path, then why not use elog rather than > Assert? I took inspiration for that from the pre-existing "debugging > cross-check" in dsm_attach that does elog(ERROR, "can't attach the > same segment more than once"). On that basis, this new version > retains the elog you mentioned, and now also uses elog for the > you-tried-to-unpin-a-handle-I-couldn't-find case. But I kept Assert > in places that detect bugs in *this* code, rather than client code. Hmm, well said. I've never thought about it in exactly that way, but I think that is a useful distinction. I've sometimes phrased it this way: an Assert() is good if the path is performance-critical, or if the Assert() is checking something that is "nearby" in the code, but an elog() is better if we're checking for a condition that could happen as a result of some code that is far-removed from the place where the elog() is. If an assertion fails, you're not necessarily going to realize right away that the calling code needs to be checked for errors. That could be mitigated, of course, by adding a comment right before the Assert() saying "if this Assertion fails, you probably did X, and you shouldn't do that". But an elog() can state the exact problem right away. Also, of course, elog() is the right tool if we want to perform the check even in production builds where asserts are not enabled. That's not so relevant here, but it matters in some other cases, like when checking for a case that shouldn't happen normally but could be the result of data corruption. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Aug 22, 2016 at 5:24 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> 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: > > 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
Attachment
On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > + 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)? Yes, it is better. New version attached. > 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. Thanks! -- Thomas Munro http://www.enterprisedb.com
Attachment
On Mon, Aug 22, 2016 at 6:06 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >> + 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)? > > Yes, it is better. New version attached. > This version of patch looks good to me. I have marked it as Ready For Committer. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Mon, Aug 22, 2016 at 10:04 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: > On Mon, Aug 22, 2016 at 6:06 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: >> On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote: >>> + 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)? >> >> Yes, it is better. New version attached. >> > > This version of patch looks good to me. I have marked it as Ready For > Committer. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company