Re: [HACKERS] dsm_unpin_segment

[Robert Haas]

On Mon, Aug 22, 2016 at 10:04 AM, Amit Kapila  wrote:
> On Mon, Aug 22, 2016 at 6:06 PM, Thomas Munro
>  wrote:
>> On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila  
>> 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


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

Re: [HACKERS] dsm_unpin_segment

[Amit Kapila]

On Mon, Aug 22, 2016 at 6:06 PM, Thomas Munro
 wrote:
> On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila  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


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

Re: [HACKERS] dsm_unpin_segment

[Thomas Munro]

On Tue, Aug 23, 2016 at 12:07 AM, Amit Kapila  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


dsm-unpin-segment-v4.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

Re: [HACKERS] dsm_unpin_segment

[Amit Kapila]

On Mon, Aug 22, 2016 at 5:24 AM, Thomas Munro
 wrote:
> On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila  wrote:
>> On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
>>  wrote:
>>> On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane  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,
>> + _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

Re: [HACKERS] dsm_unpin_segment

[Robert Haas]

On Sun, Aug 21, 2016 at 7:54 PM, Thomas Munro
 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


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

Re: [HACKERS] dsm_unpin_segment

[Thomas Munro]

On Sat, Aug 20, 2016 at 11:37 PM, Amit Kapila  wrote:
> On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
>  wrote:
>> On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane  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,
> + _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  wrote:
> On Sat, Aug 20, 2016 at 6:13 PM, Robert Haas  wrote:
>> On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila  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 

Re: [HACKERS] dsm_unpin_segment

[Amit Kapila]

On Sat, Aug 20, 2016 at 6:13 PM, Robert Haas  wrote:
> On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila  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.

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.


-- 
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

Re: [HACKERS] dsm_unpin_segment

[Robert Haas]

On Sat, Aug 20, 2016 at 7:37 AM, Amit Kapila  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


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

Re: [HACKERS] dsm_unpin_segment

[Amit Kapila]

On Tue, Aug 9, 2016 at 10:07 AM, Thomas Munro
 wrote:
> On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane  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,
+ _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

Re: [HACKERS] dsm_unpin_segment

[Thomas Munro]

On Wed, Aug 10, 2016 at 2:43 PM, Jim Nasby  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


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

Re: [HACKERS] dsm_unpin_segment

[Jim Nasby]

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


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

Re: [HACKERS] dsm_unpin_segment

[Thomas Munro]

On Wed, Aug 10, 2016 at 10:38 AM, Jim Nasby  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


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

Re: [HACKERS] dsm_unpin_segment

[Jim Nasby]

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


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

Re: [HACKERS] dsm_unpin_segment

[Robert Haas]

On Mon, Aug 8, 2016 at 8:53 PM, Tom Lane  wrote:
> Thomas Munro  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


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

Re: [HACKERS] dsm_unpin_segment

[Thomas Munro]

On Tue, Aug 9, 2016 at 12:53 PM, Tom Lane  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


dsm-unpin-segment-v2.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

Re: [HACKERS] dsm_unpin_segment

[Tom Lane]

Thomas Munro  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


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

Re: [HACKERS] dsm_unpin_segment

[Thomas Munro]

On Tue, Aug 9, 2016 at 12:22 PM, Robert Haas  wrote:
> On Mon, Aug 8, 2016 at 7:33 PM, Tom Lane  wrote:
>> Thomas Munro  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


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

Re: [HACKERS] dsm_unpin_segment

[Robert Haas]

On Mon, Aug 8, 2016 at 7:33 PM, Tom Lane  wrote:
> Thomas Munro  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 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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] dsm_unpin_segment

[Tom Lane]

Thomas Munro  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


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

[HACKERS] dsm_unpin_segment

[Thomas Munro]

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


dsm-unpin-segment.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