Re: dynamic shared memory

On Thu, Aug 29, 2013 at 8:12 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
> On 8/13/13 8:09 PM, Robert Haas wrote:
>> is removed, the segment automatically goes away (we could allow for
>> server-lifespan segments as well with only trivial changes, but I'm
>> not sure whether there are compelling use cases for that).
>
> To clarify... you're talking something that would intentionally survive
> postmaster restart? I don't see use for that either...

No, I meant something that would live as long as the postmaster and
die when it dies.

> Ignorant question... is ResourceOwner related to memory contexts? If not,
> would memory contexts be a better way to handle memory segment cleanup?

Nope. :-)

>> There are quite a few problems that this patch does not solve. First,
>
> It also doesn't provide any mechanism for notifying backends of a new
> segment. Arguably that's beyond the scope of dsm.c, but ISTM that it'd be
> useful to have a standard method or three of doing that; perhaps just some
> convenience functions wrapping the methods mentioned in comments.

I don't see that as being generally useful. Backends need to know
more than "there's a new segment", and in fact most backends won't
care about most new segments. A background worker needs to know about
the new segment *that it should attach*, but we have bgw_main_arg. If
we end up using this facility for any system-wide purposes, I imagine
we'll do that by storing the segment ID in the main shared memory
segment someplace.

> Thanks to you and the rest of the folks at EnterpriseDB... dynamic shared
> memory is something we've needed forever! :)

Thanks.

> Other comments...
>
> + * If the state file is empty or the contents are garbled, it probably
> means
> + * that the operating system rebooted before the data written by the
> previous
> + * postmaster made it to disk. In that case, we can just ignore it; any
> shared
> + * memory from before the reboot should be gone anyway.
>
> I'm a bit concerned about this; I know it was possible in older versions for
> the global shared memory context to be left behind after a crash and needing
> to clean it up by hand. Dynamic shared mem potentially multiplies that by
> 100 or more. I think it'd be worth changing dsm_write_state_file so it
> always writes a new file and then does an atomic mv (or something similar).

I agree that the possibilities for leftover shared memory segments are
multiplied with this new facility, and I've done my best to address
that. However, I don't agree that writing the state file in a
different way would improve anything.

> + * If some other backend exited uncleanly, it might have corrupted
> the
> + * control segment while it was dying. In that case, we warn and
> ignore
> + * the contents of the control segment. This may end up leaving
> behind
> + * stray shared memory segments, but there's not much we can do
> about
> + * that if the metadata is gone.
>
> Similar concern... in this case, would it be possible to always write
> updates to an un-used slot and then atomically update a pointer? This would
> be more work than what I suggested above, so maybe just a TODO for now...
>
> Though... is there anything a dying backend could do that would corrupt the
> control segment to the point that it would screw up segments allocated by
> other backends and not related to the dead backend? Like marking a slot as
> not used when it is still in use and isn't associated with the dead backend?

Sure. A messed-up backend can clobber the control segment just as it
can clobber anything else in shared memory. There's really no way
around that problem. If the control segment has been overwritten by a
memory stomp, we can't use it to clean up. There's no way around that
problem except to not the control segment, which wouldn't be better.

> Should this (in dsm_attach)
>
> + * If you're hitting this error, you probably want to use attempt to
>
> be
>
> + * If you're hitting this error, you probably want to attempt to
>
> ?

Good point.

> Should dsm_impl_op sanity check the arguments after op? I didn't notice
> checks in the type-specific code but I also didn't read all of it... are we
> just depending on the OS to sanity-check?

Sanity-check for what?

> Also, does the GUC code enforce that the GUC must always be something that's
> supported? If not then the error in dsm_impl_op should be more
> user-friendly.

Yes.

> I basically stopped reading after dsm_impl_op... the rest of the stuff was
> rather over my head.

:-)

Thanks for your interest!

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