Обсуждение: stats collector causes shared-memory-block leakage
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.
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 */
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 #
Jan Wieck <JanWieck@Yahoo.com> writes:
> Tom Lane wrote:
>> The attached patch fixes the problem by causing the stats collector to
>> detach from shared memory, which it isn't using anyway.
> 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.
Yeah, plan B would be to force the stats processes to die and be reborn
along with the backends. However that seems like a much larger
behavioral change than just detaching them from the shared memory.
If we ever need the stats collector to be able to access shared memory,
we'd need to do it that way; for now I'll take the simpler solution.
> Anyhow, good catch. The judgement to apply to both looks right to me.
Will do.
regards, tom lane