Re: dynamic shared memory

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Jim Nasby <jim(at)nasby(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: dynamic shared memory
Date: 2013-09-18 17:42:02
Message-ID: 20130918174202.GD21051@awork2.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Robert, Hi Amit,

Ok, first read through the patch.

On 2013-09-13 15:32:36 -0400, Robert Haas wrote:
> -AC_CHECK_FUNCS([cbrt dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll pstat readlink setproctitle setsid sigprocmask symlink sync_file_range towlower utime utimes wcstombs wcstombs_l])
> +AC_CHECK_FUNCS([cbrt dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll pstat readlink setproctitle setsid shm_open sigprocmask symlink sync_file_range towlower utime utimes wcstombs wcstombs_l])

Maybe also check for shm_unlink or is that too absurd?

> --- /dev/null
> +++ b/src/backend/storage/ipc/dsm.c
> +#define PG_DYNSHMEM_STATE_FILE PG_DYNSHMEM_DIR "/state"
> +#define PG_DYNSHMEM_NEW_STATE_FILE PG_DYNSHMEM_DIR "/state.new"

Hm, I guess you dont't want to add it to global/ or so because of the
mmap implementation where you presumably scan the directory?

> +struct dsm_segment
> +{
> + dlist_node node; /* List link in dsm_segment_list. */
> + ResourceOwner resowner; /* Resource owner. */
> + dsm_handle handle; /* Segment name. */
> + uint32 control_slot; /* Slot in control segment. */
> + void *impl_private; /* Implementation-specific private data. */
> + void *mapped_address; /* Mapping address, or NULL if unmapped. */
> + uint64 mapped_size; /* Size of our mapping. */
> +};

Document that's backend local?

> +typedef struct dsm_control_item
> +{
> + dsm_handle handle;
> + uint32 refcnt; /* 2+ = active, 1 = moribund, 0 = gone */
> +} dsm_control_item;
> +
> +typedef struct dsm_control_header
> +{
> + uint32 magic;
> + uint32 nitems;
> + uint32 maxitems;
> + dsm_control_item item[FLEXIBLE_ARRAY_MEMBER];
> +} dsm_control_header;

And those are shared memory?

> +static void dsm_cleanup_using_control_segment(void);
> +static void dsm_cleanup_for_mmap(void);
> +static bool dsm_read_state_file(dsm_handle *h);
> +static void dsm_write_state_file(dsm_handle h);
> +static void dsm_postmaster_shutdown(int code, Datum arg);
> +static void dsm_backend_shutdown(int code, Datum arg);
> +static dsm_segment *dsm_create_descriptor(void);
> +static bool dsm_control_segment_sane(dsm_control_header *control,
> + uint64 mapped_size);
> +static uint64 dsm_control_bytes_needed(uint32 nitems);
> +
> +/* Has this backend initialized the dynamic shared memory system yet? */
> +static bool dsm_init_done = false;
> +
> +/*
> + * List of dynamic shared memory segments used by this backend.
> + *
> + * At process exit time, we must decrement the reference count of each
> + * segment we have attached; this list makes it possible to find all such
> + * segments.
> + *
> + * This list should always be empty in the postmaster. We could probably
> + * allow the postmaster to map dynamic shared memory segments before it
> + * begins to start child processes, provided that each process adjusted
> + * the reference counts for those segments in the control segment at
> + * startup time, but there's no obvious need for such a facility, which
> + * would also be complex to handle in the EXEC_BACKEND case. Once the
> + * postmaster has begun spawning children, there's an additional problem:
> + * each new mapping would require an update to the control segment,
> + * which requires locking, in which the postmaster must not be involved.
> + */
> +static dlist_head dsm_segment_list = DLIST_STATIC_INIT(dsm_segment_list);
> +
> +/*
> + * Control segment information.
> + *
> + * Unlike ordinary shared memory segments, the control segment is not
> + * reference counted; instead, it lasts for the postmaster's entire
> + * life cycle. For simplicity, it doesn't have a dsm_segment object either.
> + */
> +static dsm_handle dsm_control_handle;
> +static dsm_control_header *dsm_control;
> +static uint64 dsm_control_mapped_size = 0;
> +static void *dsm_control_impl_private = NULL;
> +
> +/*
> + * Start up the dynamic shared memory system.
> + *
> + * This is called just once during each cluster lifetime, at postmaster
> + * startup time.
> + */
> +void
> +dsm_postmaster_startup(void)
> +{
> + void *dsm_control_address = NULL;
> + uint32 maxitems;
> + uint64 segsize;
> +
> + Assert(!IsUnderPostmaster);
> +
> + /* If dynamic shared memory is disabled, there's nothing to do. */
> + if (dynamic_shared_memory_type == DSM_IMPL_NONE)
> + return;
> +
> + /*
> + * Check for, and remove, shared memory segments left behind by a dead
> + * postmaster. This isn't necessary on Windows, which always removes them
> + * when the last reference is gone.
> + */
> + switch (dynamic_shared_memory_type)
> + {
> + case DSM_IMPL_POSIX:
> + case DSM_IMPL_SYSV:
> + dsm_cleanup_using_control_segment();
> + break;
> + case DSM_IMPL_MMAP:
> + dsm_cleanup_for_mmap();
> + break;
> + case DSM_IMPL_WINDOWS:
> + /* Nothing to do. */
> + break;
> + default:
> + elog(ERROR, "unknown dynamic shared memory type: %d",
> + dynamic_shared_memory_type);
> + }
> +
> + /* Determine size for new control segment. */
> + maxitems = PG_DYNSHMEM_FIXED_SLOTS
> + + PG_DYNSHMEM_SLOTS_PER_BACKEND * MaxBackends;

It seems likely that MaxConnections would be sufficient?

> + elog(DEBUG2, "dynamic shared memory system will support %lu segments",
> + (unsigned long) maxitems);
> + segsize = dsm_control_bytes_needed(maxitems);
> +
> + /* Create new control segment. */
> + for (;;)
> + {
> + Assert(dsm_control_address == NULL);
> + Assert(dsm_control_mapped_size == 0);
> + dsm_control_handle = random();
> + if (dsm_impl_op(DSM_OP_CREATE, dsm_control_handle, segsize,
> + &dsm_control_impl_private, &dsm_control_address,
> + &dsm_control_mapped_size, ERROR))
> + break;
> + }

Comment that we loop endlessly to find a unused identifier.

Why do we create the control segment in dynamic smem and not in the
normal shmem? Presumably because this way it has the same lifetime? If
so, that should be commented upon.

> +static void
> +dsm_cleanup_using_control_segment(void)
> +{
> + /*
> + * We've managed to reattach it, but the contents might not be sane.
> + * If they aren't, we disregard the segment after all.
> + */
> + old_control = (dsm_control_header *) mapped_address;
> + 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);
> + return;
> + }

So, we leave it just hanging around... Well, it has precedent in our
normal shared memory handling.

> +static void
> +dsm_cleanup_for_mmap(void)
> +{
...
> +}

I still maintain that the extra infrastructure required isn't worth the
gain of having the mmap implementation.

> +/*
> + * Read and parse the state file.
> + *
> + * 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.
> + */
> +static bool
> +dsm_read_state_file(dsm_handle *h)
> +{
...
> + return true;
> +}

Perhaps CRC32 the content?

> +/*
> + * Write our control segment handle to the state file, so that if the
> + * postmaster is killed without running it's on_shmem_exit hooks, the
> + * next postmaster can clean things up after restart.
> + */
> +static void
> +dsm_write_state_file(dsm_handle h)
> +{
> + int statefd;
> + char statebuf[PG_DYNSHMEM_STATE_BUFSIZ];
> + int nbytes;
> +
> + /* Create or truncate the file. */
> + statefd = open(PG_DYNSHMEM_NEW_STATE_FILE, O_RDWR|O_CREAT|O_TRUNC, 0600);

Doesn't this need a | PG_BINARY? Why are you using open() and not
BasicOpenFile or even OpenTransientFile?

> + /* Write contents. */
> + snprintf(statebuf, PG_DYNSHMEM_STATE_BUFSIZ, "%lu\n",
> + (unsigned long) dsm_control_handle);

Why are we upcasting the length of dsm_control_handle here? Also,
doesn't this need the usual UINT64_FORMAT thingy?

> +/*
> + * At shutdown time, we iterate over the control segment and remove all
> + * remaining dynamic shared memory segments. We avoid throwing errors here;
> + * the postmaster is shutting down either way, and this is just non-critical
> + * resource cleanup.
> + */
> +static void
> +dsm_postmaster_shutdown(int code, Datum arg)
> +{
> + uint32 nitems;
> + uint32 i;
> + void *dsm_control_address;
> + void *junk_mapped_address = NULL;
> + void *junk_impl_private = NULL;
> + uint64 junk_mapped_size = 0;
> +
> + /* If dynamic shared memory is disabled, there's nothing to do. */
> + if (dynamic_shared_memory_type == DSM_IMPL_NONE)
> + return;

But we don't even get calld in that case, right? You're only regitering
the on_shmem_exit() handler after the same check in startup?

> + /*
> + * 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.
> + */
> + 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;
> + }

I'd rename dsm_control_segment_sane to dsm_control_segment_looks_sane ;)

> + /* Remove any remaining segments. */
> + for (i = 0; i < nitems; ++i)
> + {
> + dsm_handle handle;
> +
> + /* If the reference count is 0, the slot is actually unused. */
> + if (dsm_control->item[i].refcnt == 0)
> + continue;
> +
> + /* Log debugging information. */
> + handle = dsm_control->item[i].handle;
> + elog(DEBUG2, "cleaning up orphaned dynamic shared memory with ID %lu",
> + (unsigned long) handle);

I'd include the refcount here, might be helpful for debugging.

> + /* Destroy the segment. */
> + dsm_impl_op(DSM_OP_DESTROY, handle, 0, &junk_impl_private,
> + &junk_mapped_address, &junk_mapped_size, LOG);
> + }
> +
> + /* Remove the control segment itself. */
> + elog(DEBUG2,
> + "cleaning up dynamic shared memory control segment with ID %lu",
> + (unsigned long) dsm_control_handle);
> + dsm_control_address = dsm_control;
> + dsm_impl_op(DSM_OP_DESTROY, dsm_control_handle, 0,
> + &dsm_control_impl_private, &dsm_control_address,
> + &dsm_control_mapped_size, LOG);
> + dsm_control = dsm_control_address;
> +
> + /* And, finally, remove the state file. */
> + if (unlink(PG_DYNSHMEM_STATE_FILE) < 0)
> + ereport(LOG,
> + (errcode_for_file_access(),
> + errmsg("could not unlink file \"%s\": %m",
> + PG_DYNSHMEM_STATE_FILE)));
> +}

Not sure whether it's sensible to only LOG in these cases. After all
there's something unexpected happening. The robustness argument doesn't
count since we're already shutting down.

> +/*
> + * Prepare this backend for dynamic shared memory usage. Under EXEC_BACKEND,
> + * we must reread the state file and map the control segment; in other cases,
> + * we'll have inherited the postmaster's mapping and global variables.
> + */
> +static void
> +dsm_backend_startup(void)
> +{
> +
> +#ifdef EXEC_BACKEND
> + {
> + dsm_handle control_handle;
> + void *control_address = NULL;
> +
> + /* Read the control segment information from the state file. */
> + if (!dsm_read_state_file(&control_handle))
> + ereport(ERROR,
> + (errcode(ERRCODE_INTERNAL_ERROR),
> + errmsg("could not parse dynamic shared memory state file")));
> +
> + /* Attach control segment. */
> + dsm_impl_op(DSM_OP_ATTACH, control_handle, 0,
> + &dsm_control_impl_private, &control_address,
> + &dsm_control_mapped_size, ERROR);
> + dsm_control_handle = control_handle;
> + dsm_control = control_address;
> + /* If control segment doesn't look sane, something is badly wrong. */
> + if (!dsm_control_segment_sane(dsm_control, dsm_control_mapped_size))
> + {
> + dsm_impl_op(DSM_OP_DETACH, control_handle, 0,
> + &dsm_control_impl_private, &control_address,
> + &dsm_control_mapped_size, WARNING);
> + ereport(ERROR,
> + (errcode(ERRCODE_INTERNAL_ERROR),
> + errmsg("dynamic shared memory control segment is not valid")));
> + }

Imo that's a PANIC or at the very least a FATAL.

> +
> +/*
> + * Create a new dynamic shared memory segment.
> + */
> +dsm_segment *
> +dsm_create(uint64 size)
> +{
...
> + /* Verify that we can support an additional mapping. */
> + if (nitems >= dsm_control->maxitems)
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_RESOURCES),
> + errmsg("too many dynamic shared memory segments")));

Do we rely on being run in an environment with proper setup for lwlock
cleanup? I can imagine shared libraries doing this pretty early on...

> +dsm_segment *
> +dsm_attach(dsm_handle h)
> +{

> + /* Bump reference count for this segment in shared memory. */
> + LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
> + nitems = dsm_control->nitems;
> + for (i = 0; i < nitems; ++i)
> + {
> + /* If the reference count is 0, the slot is actually unused. */
> + if (dsm_control->item[i].refcnt == 0)
> + continue;
> +
> + /*
> + * If the reference count is 1, the slot is still in use, but the
> + * segment is in the process of going away. Treat that as if we
> + * didn't find a match.
> + */
> + if (dsm_control->item[i].refcnt == 1)
> + break;

Why can we see that state? Shouldn't locking prevent that?

> +/*
> + * Resize an existing shared memory segment.
> + *
> + * This may cause the shared memory segment to be remapped at a different
> + * address. For the caller's convenience, we return the mapped address.
> + */
> +void *
> +dsm_resize(dsm_segment *seg, uint64 size)
> +{
> + Assert(seg->control_slot != INVALID_CONTROL_SLOT);
> + dsm_impl_op(DSM_OP_RESIZE, seg->handle, size, &seg->impl_private,
> + &seg->mapped_address, &seg->mapped_size, ERROR);
> + return seg->mapped_address;
> +}

Hm. That's valid when there are other backends attached? What are the
implications for already attached ones?

Shouldn't we error out if !dsm_impl_can_resize()?

> +/*
> + * Detach from a shared memory segment, destroying the segment if we
> + * remove the last reference.
> + *
> + * This function should never fail. It will often be invoked when aborting
> + * a transaction, and a further error won't serve any purpose. It's not a
> + * complete disaster if we fail to unmap or destroy the segment; it means a
> + * resource leak, but that doesn't necessarily preclude further operations.
> + */
> +void
> +dsm_detach(dsm_segment *seg)
> +{

Why do we want to ignore errors like failing to unmap? ISTM that
indicates an actual problem...

> + /* Reduce reference count, if we previously increased it. */
> + if (seg->control_slot != INVALID_CONTROL_SLOT)
> + {
> + uint32 refcnt;
> + uint32 control_slot = seg->control_slot;
> +
> + LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
> + Assert(dsm_control->item[control_slot].handle == seg->handle);
> + Assert(dsm_control->item[control_slot].refcnt > 1);
> + refcnt = --dsm_control->item[control_slot].refcnt;
> + seg->control_slot = INVALID_CONTROL_SLOT;
> + LWLockRelease(DynamicSharedMemoryControlLock);
> +
> + /* If new reference count is 1, try to destroy the segment. */
> + if (refcnt == 1)
> + {
> + /*
> + * If we fail to destroy the segment here, or are killed before
> + * we finish doing so, the reference count will remain at 1, which
> + * will mean that nobody else can attach to the segment. At
> + * postmaster shutdown time, or when a new postmaster is started
> + * after a hard kill, another attempt will be made to remove the
> + * segment.
> + *
> + * The main case we're worried about here is being killed by
> + * a signal before we can finish removing the segment. In that
> + * case, it's important to be sure that the segment still gets
> + * removed. If we actually fail to remove the segment for some
> + * other reason, the postmaster may not have any better luck than
> + * we did. There's not much we can do about that, though.
> + */
> + if (dsm_impl_op(DSM_OP_DESTROY, seg->handle, 0, &seg->impl_private,
> + &seg->mapped_address, &seg->mapped_size, WARNING))
> + {
> + LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE);
> + Assert(dsm_control->item[control_slot].handle == seg->handle);
> + Assert(dsm_control->item[control_slot].refcnt == 1);
> + dsm_control->item[control_slot].refcnt = 0;
> + LWLockRelease(DynamicSharedMemoryControlLock);
> + }
> + }
> + }

Yuck. So that's the answer to my earlier question about the legality of
seing a refcount of 1....

> diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
> new file mode 100644
> index 0000000..8005fd9
> @@ -0,0 +1,986 @@
> +/*-------------------------------------------------------------------------
> + *
> + * dsm_impl.c
> + * manage dynamic shared memory segments
> + *
> + * This file provides low-level APIs for creating and destroying shared
> + * memory segments using several different possible techniques. We refer
> + * to these segments as dynamic because they can be created, altered, and
> + * destroyed at any point during the server life cycle. This is unlike
> + * the main shared memory segment, of which there is always exactly one
> + * and which is always mapped at a fixed address in every PostgreSQL
> + * background process.
> + *
> + * Because not all systems provide the same primitives in this area, nor
> + * do all primitives behave the saem way on all systems, we provide

*same

> + * several implementations of this facility. Many systems implement
> + * POSIX shared memory (shm_open etc.), which is well-suited to our needs
> + * in this area, with the exception that shared memory identifiers live
> + * in a flat system-wide namespace, raising the uncomfortable prospect of
> + * name collisions with other processes (including other copies of
> + * PostgreSQL) running on the same system.

Why isn't the port number part of the posix shmem identifiers? Sure, we
retry, but using a logic similar to sysv_shmem.c seems like a good idea.

> +/*
> + * Does the current dynamic shared memory implementation support resizing
> + * segments? (The answer here could be platform-dependent in the future,
> + * since AIX allows shmctl(shmid, SHM_RESIZE, &buffer), though you apparently
> + * can't resize segments to anything larger than 256MB that way. For now,
> + * we keep it simple.)
> + */
> +bool
> +dsm_impl_can_resize(void)
> +{
> + switch (dynamic_shared_memory_type)
> + {
> + case DSM_IMPL_NONE:
> + return false;
> + case DSM_IMPL_SYSV:
> + return false;
> + case DSM_IMPL_WINDOWS:
> + return false;
> + default:
> + return true;
> + }
> +}

Looks to me like the logic should be the reverse.

> +#ifdef USE_DSM_POSIX
> +/*
> + * Operating system primitives to support POSIX shared memory.
> + *
> + * POSIX shared memory segments are created and attached using shm_open()
> + * and shm_unlink(); other operations, such as sizing or mapping the
> + * segment, are performed as if the shared memory segments were files.
> + *
> + * Indeed, on some platforms, they may be implemented that way. While
> + * POSIX shared memory segments seem intended to exist in a flat namespace,
> + * some operating systems may implement them as files, even going so far
> + * to treat a request for /xyz as a request to create a file by that name
> + * in the root directory. Users of such broken platforms should select
> + * a different shared memory implementation.
> + */
> +static bool
> +dsm_impl_posix(dsm_op op, dsm_handle handle, uint64 request_size,
> + void **impl_private, void **mapped_address, uint64 *mapped_size,
> + int elevel)
> +{
> + char name[64];
> + int flags;
> + int fd;
> + char *address;
> +
> + snprintf(name, 64, "/PostgreSQL.%lu", (unsigned long) handle);

Why wider than the handle?

> + /*
> + * If we're reattaching or resizing, we must remove any existing mapping,
> + * unless we've already got the right thing mapped.
> + */
> + if (*mapped_address != NULL)
> + {
> + if (*mapped_size == request_size)
> + return true;

Hm. It could have gotten resized to the old size, or resized twice. In
that case it might not be at the same address before, so checking for
the size doesn't seem to be sufficient.

> +static bool
> +dsm_impl_sysv(dsm_op op, dsm_handle handle, uint64 request_size,
> + void **impl_private, void **mapped_address, uint64 *mapped_size,
> + int elevel)
> +{

> + /*
> + * There's one special key, IPC_PRIVATE, which can't be used. If we end
> + * up with that value by chance during a create operation, just pretend
> + * it already exists, so that caller will retry. If we run into it
> + * anywhere else, the caller has passed a handle that doesn't correspond
> + * to anything we ever created, which should not happen.
> + */
> + if (key == IPC_PRIVATE)
> + {
> + if (op != DSM_OP_CREATE)
> + elog(elevel, "System V shared memory key may not be IPC_PRIVATE");
> + errno = EEXIST;
> + return false;
> + }

Hm. You're elog(elevel) here, but the retry code in dsm_create() passes
in ERROR?

> +static bool
> +dsm_impl_mmap(dsm_op op, dsm_handle handle, uint64 request_size,
> + void **impl_private, void **mapped_address, uint64 *mapped_size,
> + int elevel)
> +{

> + /*
> + * If we're reattaching or resizing, we must remove any existing mapping,
> + * unless we've already got the right thing mapped.
> + */
> + if (*mapped_address != NULL)
> + {
> + if (*mapped_size == request_size)
> + return true;

Same think like in posix shmem.

> +static int
> +errcode_for_dynamic_shared_memory()
> +{
> + if (errno == EFBIG || errno == ENOMEM)
> + return errcode(ERRCODE_OUT_OF_MEMORY);
> + else
> + return errcode_for_file_access();
> +}

Is EFBIG guaranteed to be defined?

> +/*
> + * Forget that a temporary file is owned by a ResourceOwner
> + */
> +void
> +ResourceOwnerForgetDSM(ResourceOwner owner, dsm_segment *seg)
> +{
> + dsm_segment **dsms = owner->dsms;
> + int ns1 = owner->ndsms - 1;
> + int i;
> +
> + for (i = ns1; i >= 0; i--)
> + {
> + if (dsms[i] == seg)
> + {
> + while (i < ns1)
> + {
> + dsms[i] = dsms[i + 1];
> + i++;
> + }
> + owner->ndsms = ns1;
> + return;
> + }
> + }
> + elog(ERROR,
> + "dynamic shared memory segment %lu is not owned by resource owner %s",
> + (unsigned long) dsm_segment_handle(seg), owner->name);
> +}

Not really an issue, but this will grow owner->dsm unnecessarily because
ResourceOwnerEnlargeDSMs() will have been done previously.

Not sure yet how happy I am with the separation of concerns between
dsm.c and dsm_impl.c...

That's it for now.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fujii Masao 2013-09-18 17:49:56 Re: pg_stat_statements: calls under-estimation propagation
Previous Message Fujii Masao 2013-09-18 17:41:28 Re: pg_stat_statements: calls under-estimation propagation