Re: [HACKERS] dsm.c API tweak

2017-03-28 Thread Alvaro Herrera 
Thomas Munro wrote:

> +   if (CurrentResourceOwner)
> +   {
> +   seg->resowner = CurrentResourceOwner;
> +   ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
> +   }
> 
> You need to assign seg->resowner = CurrentResourceOwner
> unconditionally here.  Otherwise seg->resowner is uninitialised junk.

Thanks for that.  Pushed, with the comments as I suggested in my other
reply.  If you think they merit more wordsmithing, feel free to suggest
something.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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.c API tweak

2017-03-25 Thread Alvaro Herrera 
Thomas Munro wrote:

> I'd word this slightly differently:
> 
> + * If there is a CurrentResourceOwner, the new segment is born unpinned and 
> the
> + * resource owner is in charge of destroying it (and will be blamed if it
> + * doesn't).  If there's no current resource owner, then the segment starts 
> in
> + * the pinned state, so it'll stay alive until explicitely unpinned.
> 
> It's confusing that we've overloaded the term 'pin'.  When we 'pin the
> mapping', we're disassociating it from the resource owner so that it
> will remain attached for longer than the current resource owner scope.
> When we 'pin the segment' we are making it survive even if all
> backends detach (= an extra secret refcount).  What we're talking
> about here is not pinning the *segment*, but pinning the *mapping* in
> this backend.

I agree that it's unfortunate.  I think we would be happier in the long
run if we changed one of those terms.  Right now, the whole of dsm.c
appears confusing to me.  Maybe one thing that would help is to explain
the distinction between mappings and segments in the comment at the top
of the file.  This is a bigger change than I care to tackle right now,
though.

It took me a few minutes to realize that the memory allocated by
dsm_create_descriptor is backend-local, which is why dsm_attach creates
a new descriptor.

> How about: "If there is a non-NULL CurrentResourceOwner, the new
> segment is associated with it and will be automatically detached when
> the resource owner releases.  Otherwise it will remain attached until
> explicitly detached.  Creating or attaching with a NULL
> CurrentResourceOwner is equivalent to creating or attaching with a
> non-NULL CurrentResourceOwner and then calling dsm_pin_mapping()."

Sounds a lot better, but I don't think it explains the contract exactly
either, at least in the case where there is a resowner: what happens if
the resowner releases is that it logs a complaint (so what the resowner
does is act as cross-check that resources are properly handled
elsewhere, as well as ensuring sane behavior in case of error).  I think
we should stress the point that the segment must be detached before the
resowner releases in normal cases.  (Also, let's talk about segment
creation in the dsm_create comment, not attachment).

How about this:
  If there is a non-NULL CurrentResourceOwner, the new segment is
  associated with it and must be detached before the resource owner
  releases, or a warning will be logged.  If CurrentResourceOwner is
  NULL, the segment remains attached until explicitely detached or the
  session ends.
  Creating with a NULL CurrentResourceOwner is equivalent to creating
  with a non-NULL CurrentResourceOwner and then calling dsm_pin_mapping.

> Then dsm_attach() needs to say "See the note above dsm_create() about
> the CurrentResourceOwner.", since it's the same deal there.

I think trying to explain both in the comment for dsm_create() is more
confusing than helpful.  I propose to spell out the rule in both places
instead:

  If there is a non-NULL CurrentResourceOwner, the attached segment is
  associated with it and must be detached before the resource owner
  releases, or a warning will be logged.  Otherwise the segment remains
  attached until explicitely detached or the session ends.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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.c API tweak

2017-03-24 Thread Thomas Munro 
On Sat, Mar 25, 2017 at 1:59 PM, Thomas Munro
 wrote:
> On Sat, Mar 25, 2017 at 12:35 PM, Alvaro Herrera
>  wrote:
>> Alvaro Herrera wrote:
>>> Per
>>> https://postgr.es/m/CAEepm=11ma_Z1HoPxPcSCANnh5ykHORa=hca1u1v1+5s_jw...@mail.gmail.com
>>> it seems that the dsm.c API is a bit inconvenient right now.  I proposed
>>> in the first patch in that thread to change the API so that a segment is
>>> marked as "pinned" if created with no ResourceOwner set as current;
>>> which is essentially the same as creating a fake one, then marking the
>>> segment as pinned.
>>>
>>> Thomas agrees with me, so I propose this patch as preparatory work for
>>> the autovacuum/BRIN stuff I'm interested in.
>>
>> Here's the proposed patch.
>
> +1

-   seg->resowner = CurrentResourceOwner;
-   ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+   if (CurrentResourceOwner)
+   {
+   seg->resowner = CurrentResourceOwner;
+   ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+   }

You need to assign seg->resowner = CurrentResourceOwner
unconditionally here.  Otherwise seg->resowner is uninitialised junk.

-- 
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.c API tweak

2017-03-24 Thread Thomas Munro 
On Sat, Mar 25, 2017 at 12:35 PM, Alvaro Herrera
 wrote:
> Alvaro Herrera wrote:
>> Per
>> https://postgr.es/m/CAEepm=11ma_Z1HoPxPcSCANnh5ykHORa=hca1u1v1+5s_jw...@mail.gmail.com
>> it seems that the dsm.c API is a bit inconvenient right now.  I proposed
>> in the first patch in that thread to change the API so that a segment is
>> marked as "pinned" if created with no ResourceOwner set as current;
>> which is essentially the same as creating a fake one, then marking the
>> segment as pinned.
>>
>> Thomas agrees with me, so I propose this patch as preparatory work for
>> the autovacuum/BRIN stuff I'm interested in.
>
> Here's the proposed patch.

+1

I'd word this slightly differently:

+ * If there is a CurrentResourceOwner, the new segment is born unpinned and the
+ * resource owner is in charge of destroying it (and will be blamed if it
+ * doesn't).  If there's no current resource owner, then the segment starts in
+ * the pinned state, so it'll stay alive until explicitely unpinned.

It's confusing that we've overloaded the term 'pin'.  When we 'pin the
mapping', we're disassociating it from the resource owner so that it
will remain attached for longer than the current resource owner scope.
When we 'pin the segment' we are making it survive even if all
backends detach (= an extra secret refcount).  What we're talking
about here is not pinning the *segment*, but pinning the *mapping* in
this backend.

How about: "If there is a non-NULL CurrentResourceOwner, the new
segment is associated with it and will be automatically detached when
the resource owner releases.  Otherwise it will remain attached until
explicitly detached.  Creating or attaching with a NULL
CurrentResourceOwner is equivalent to creating or attaching with a
non-NULL CurrentResourceOwner and then calling dsm_pin_mapping()."
Then dsm_attach() needs to say "See the note above dsm_create() about
the CurrentResourceOwner.", since it's the same deal there.

-- 
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.c API tweak

2017-03-24 Thread Alvaro Herrera 
Alvaro Herrera wrote:
> Per 
> https://postgr.es/m/CAEepm=11ma_Z1HoPxPcSCANnh5ykHORa=hca1u1v1+5s_jw...@mail.gmail.com
> it seems that the dsm.c API is a bit inconvenient right now.  I proposed
> in the first patch in that thread to change the API so that a segment is
> marked as "pinned" if created with no ResourceOwner set as current;
> which is essentially the same as creating a fake one, then marking the
> segment as pinned.
> 
> Thomas agrees with me, so I propose this patch as preparatory work for
> the autovacuum/BRIN stuff I'm interested in.

Here's the proposed patch.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From ded46066cb634f7714706c8d347af2acf267da17 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 24 Mar 2017 19:27:22 -0300
Subject: [PATCH] Change dsm.c API so that mappings are pinned if created with
 no resowner

---
 src/backend/storage/ipc/dsm.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 54378bc..061a457 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -453,6 +453,11 @@ dsm_set_control_handle(dsm_handle h)
 
 /*
  * Create a new dynamic shared memory segment.
+ *
+ * If there is a CurrentResourceOwner, the new segment is born unpinned and the
+ * resource owner is in charge of destroying it (and will be blamed if it
+ * doesn't).  If there's no current resource owner, then the segment starts in
+ * the pinned state, so it'll stay alive until explicitely unpinned.
  */
 dsm_segment *
 dsm_create(Size size, int flags)
@@ -1095,7 +1100,8 @@ dsm_create_descriptor(void)
 {
dsm_segment *seg;
 
-   ResourceOwnerEnlargeDSMs(CurrentResourceOwner);
+   if (CurrentResourceOwner)
+   ResourceOwnerEnlargeDSMs(CurrentResourceOwner);
 
seg = MemoryContextAlloc(TopMemoryContext, sizeof(dsm_segment));
dlist_push_head(_segment_list, >node);
@@ -1106,8 +1112,11 @@ dsm_create_descriptor(void)
seg->mapped_address = NULL;
seg->mapped_size = 0;
 
-   seg->resowner = CurrentResourceOwner;
-   ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+   if (CurrentResourceOwner)
+   {
+   seg->resowner = CurrentResourceOwner;
+   ResourceOwnerRememberDSM(CurrentResourceOwner, seg);
+   }
 
slist_init(>on_detach);
 
-- 
2.1.4


-- 
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.c API tweak

2017-03-24 Thread Alvaro Herrera 
Per 
https://postgr.es/m/CAEepm=11ma_Z1HoPxPcSCANnh5ykHORa=hca1u1v1+5s_jw...@mail.gmail.com
it seems that the dsm.c API is a bit inconvenient right now.  I proposed
in the first patch in that thread to change the API so that a segment is
marked as "pinned" if created with no ResourceOwner set as current;
which is essentially the same as creating a fake one, then marking the
segment as pinned.

Thomas agrees with me, so I propose this patch as preparatory work for
the autovacuum/BRIN stuff I'm interested in.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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