Re: stats collector causes shared-memory-block leakage

Tom Lane wrote:

> While investigating Scott Goodwin's recent report of trouble on Mac OS
> X, I have realized that we have an unpleasant little misbehavior in our
> last several releases.  After a backend crash, the postmaster will
> attempt to recycle (delete and recreate) the old shared memory segment.
> However, if the stats collector is enabled, the two stats collection
> subprocesses are still attached to the old segment.  Which means it
> doesn't go away.  Instead the postmaster will set up shop with a new
> segment.
> 
> This is not so bad in a system with large SHMMAX ... but if the SHMMAX
> setting is too tight to permit a second shared memory segment to be
> created, the postmaster fails.
> 
> The attached patch fixes the problem by causing the stats collector to
> detach from shared memory, which it isn't using anyway.  Unless I hear
> objections I will apply it to 7.4 and HEAD ... and I'm seriously
> thinking of applying it to the 7.3 branch as well.

I seem to recall there once was a pipe from the postmaster to the stat's 
processes and closing that will actually get rid of them. I also seem to 
recall that this was changed someday, but I don't remember why.

Anyhow, good catch. The judgement to apply to both looks right to me.


Jan

> 
>             regards, tom lane
> 
> 
> *** src/backend/port/sysv_shmem.c.orig    Mon Oct 27 13:58:00 2003
> --- src/backend/port/sysv_shmem.c    Thu Nov  6 18:10:02 2003
> ***************
> *** 253,258 ****
> --- 253,261 ----
>           return hdr;
>       }
>   
> +     /* Make sure PGSharedMemoryAttach doesn't fail without need */
> +     UsedShmemSegAddr = NULL;
> + 
>       /* Loop till we find a free IPC key */
>       NextShmemSegID = port * 1000;
>   
> ***************
> *** 326,341 ****
>       hdr->totalsize = size;
>       hdr->freeoffset = MAXALIGN(sizeof(PGShmemHeader));
>   
> ! 
> !     if (ExecBackend && UsedShmemSegAddr == NULL && !makePrivate)
> !     {
> !         UsedShmemSegAddr = memAddress;
> !         UsedShmemSegID = NextShmemSegID;
> !     }
>   
>       return hdr;
>   }
>   
>   
>   /*
>    * Attach to shared memory and make sure it has a Postgres header
> --- 329,360 ----
>       hdr->totalsize = size;
>       hdr->freeoffset = MAXALIGN(sizeof(PGShmemHeader));
>   
> !     /* Save info for possible future use */
> !     UsedShmemSegAddr = memAddress;
> !     UsedShmemSegID = NextShmemSegID;
>   
>       return hdr;
>   }
>   
> + /*
> +  * PGSharedMemoryDetach
> +  *
> +  * Detach from the shared memory segment, if still attached.  This is not
> +  * intended for use by the process that originally created the segment
> +  * (it will have an on_shmem_exit callback registered to do that).  Rather,
> +  * this is for subprocesses that have inherited an attachment and want to
> +  * get rid of it.
> +  */
> + void
> + PGSharedMemoryDetach(void)
> + {
> +     if (UsedShmemSegAddr != NULL)
> +     {
> +         if (shmdt(UsedShmemSegAddr) < 0)
> +             elog(LOG, "shmdt(%p) failed: %m", UsedShmemSegAddr);
> +         UsedShmemSegAddr = NULL;
> +     }
> + }
>   
>   /*
>    * Attach to shared memory and make sure it has a Postgres header
> *** src/backend/postmaster/pgstat.c.orig    Thu Sep 25 10:23:09 2003
> --- src/backend/postmaster/pgstat.c    Thu Nov  6 18:10:46 2003
> ***************
> *** 44,49 ****
> --- 44,50 ----
>   #include "utils/memutils.h"
>   #include "storage/backendid.h"
>   #include "storage/ipc.h"
> + #include "storage/pg_shmem.h"
>   #include "utils/rel.h"
>   #include "utils/hsearch.h"
>   #include "utils/ps_status.h"
> ***************
> *** 399,404 ****
> --- 400,408 ----
>   
>       /* Close the postmaster's sockets, except for pgstat link */
>       ClosePostmasterPorts(false);
> + 
> +     /* Drop our connection to postmaster's shared memory, as well */
> +     PGSharedMemoryDetach();
>   
>       pgstat_main();
>   
> *** src/include/storage/pg_shmem.h.orig    Sun Aug  3 23:01:43 2003
> --- src/include/storage/pg_shmem.h    Thu Nov  6 18:09:50 2003
> ***************
> *** 44,48 ****
> --- 44,49 ----
>   extern PGShmemHeader *PGSharedMemoryCreate(uint32 size, bool makePrivate,
>                        int port);
>   extern bool PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2);
> + extern void PGSharedMemoryDetach(void);
>   
>   #endif   /* PG_SHMEM_H */
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend


-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #