Re: DSM robustness failure (was Re: Peripatus/failures)

On Thu, Oct 18, 2018 at 6:30 AM Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>
> On Thu, Oct 18, 2018 at 11:08 AM Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> > On Thu, Oct 18, 2018 at 9:43 AM Thomas Munro
> > <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> > > On Thu, Oct 18, 2018 at 9:00 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > > I would argue that both dsm_postmaster_shutdown and dsm_postmaster_startup
> > > > are broken here; the former because it makes no attempt to unmap
> > > > the old control segment (which it oughta be able to do no matter how badly
> > > > broken the contents are), and the latter because it should not let
> > > > garbage old state prevent it from establishing a valid new segment.
> > >
> > > Looking.
> >
> > (CCing Amit Kapila)
> >
> > To reproduce this, I attached lldb to a backend and did "mem write
> > &dsm_control->magic 42", and then delivered SIGKILL to the backend.
> > Here's one way to fix it. I think we have no choice but to leak the
> > referenced segments, but we can free the control segment. See
> > comments in the attached patch for rationale.
>
> I realised that the nearly identical code in dsm_postmaster_shutdown()
> might as well destroy a corrupted control segment too. New version
> attached.
>

The below code seems to be problemetic:
dsm_cleanup_using_control_segment()
{
..
if (!dsm_control_segment_sane(old_control, mapped_size))
{
dsm_impl_op(DSM_OP_DETACH, old_control_handle, 0, &impl_private,
&mapped_address, &mapped_size, LOG);
..
}

Here, don't we need to use dsm_control_* variables instead of local
variable mapped_* variables? Do we need to reset the dsm_control_*
variables in dsm_cleanup_for_mmap?

@@ -336,13 +333,14 @@ dsm_postmaster_shutdown(int code, Datum arg)
* stray shared memory segments, but there's not much we can do about that
* if the metadata is gone.
*/
- nitems = dsm_control->nitems;
if (!dsm_control_segment_sane(dsm_control, dsm_control_mapped_size))
{
ereport(LOG,
(errmsg("dynamic shared memory control segment is corrupt")));
- return;
+ nitems = 0;

The comment above this code says:
/*
* 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.
*/

Don't we need to adjust this comment, if we want to go with what you
have proposed? BTW, it doesn't appear insane to me that if the
control segment got corrupted, then we leave it as it is rather than
trying to destroy it. I am not sure, but if it is corrupted, then is
it guaranteed that destroy will always succeed?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com