flags argument for dsm_create
Discussion on the parallel sequential scan thread has revealed the
need for a way to make dsm_create() return NULL, instead of failing,
when we hit the system-wide maximum on the number of dynamic shared
memory segments than can be created. I've developed a small patch for
this which I attach here. It adds a second argument to dsm_create(),
an integer flags argument. I would like to go ahead and commit this
more or less immediately if there are not objections.
One question I struggled with is whether to keep the existing
dsm_create() signature intact and add a new function
dsm_create_extended(). I eventually decided against it. The
dsm_create() API is relatively new at this point, so there probably
aren't too many people who will be inconvenienced by an API break now.
If we go ahead and create dsm_create_extended() now, and then need
to make another API change down the line, I doubt there will be much
support for dsm_create_extended2() or whatever. So my gut is that
it's better to just change this outright, and keep
dsm_create_extended() as an idea for the future. But I could go the
other way on that if people feel strongly about it.
Thanks,
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
dsm-create-flags.patchbinary/octet-stream; name=dsm-create-flags.patchDownload
From 38c8022d7722eaee7db147007933e2fbd10fe35e Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 19 Mar 2015 08:01:31 -0400
Subject: [PATCH 1/5] Add flags argument to dsm_create.
Right now, there's only one flag, DSM_CREATE_NULL_IF_MAXSEGMENTS,
which suppresses the error that would normally be thrown when the
maximum number of segments already exists, instead returning NULL.
It might be useful to add more flags in the future, such as one to
ignore allocation errors, but I haven't done that here.
---
src/backend/storage/ipc/dsm.c | 22 +++++++++++++++++++++-
src/include/storage/dsm.h | 4 +++-
src/test/modules/test_shm_mq/setup.c | 2 +-
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 983d0fa..53d1fcf 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -454,7 +454,7 @@ dsm_set_control_handle(dsm_handle h)
* Create a new dynamic shared memory segment.
*/
dsm_segment *
-dsm_create(Size size)
+dsm_create(Size size, int flags)
{
dsm_segment *seg = dsm_create_descriptor();
uint32 i;
@@ -466,6 +466,18 @@ dsm_create(Size size)
if (!dsm_init_done)
dsm_backend_startup();
+ /*
+ * If we've been instructed to return NULL when it's not possible to
+ * register another segment, check whether we seem to be at the limit.
+ * This allows us to avoid the overhead of creating a new segment only to
+ * immediately destroy it again. Since we don't take the lock here, the
+ * value we read might be slightly stale, but the remote possibility of
+ * an unnecessary failure here shouldn't trouble anyone too much.
+ */
+ if ((flags & DSM_CREATE_NULL_IF_MAXSEGMENTS) != 0
+ && nitems >= dsm_control->maxitems)
+ return NULL;
+
/* Loop until we find an unused segment identifier. */
for (;;)
{
@@ -496,9 +508,17 @@ dsm_create(Size size)
/* Verify that we can support an additional mapping. */
if (nitems >= dsm_control->maxitems)
+ {
+ if ((flags & DSM_CREATE_NULL_IF_MAXSEGMENTS) != 0)
+ {
+ dsm_impl_op(DSM_OP_DESTROY, seg->handle, 0, &seg->impl_private,
+ &seg->mapped_address, &seg->mapped_size, WARNING);
+ return NULL;
+ }
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_RESOURCES),
errmsg("too many dynamic shared memory segments")));
+ }
/* Enter the handle into a new array slot. */
dsm_control->item[nitems].handle = seg->handle;
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 2bf12ce..beee105 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -17,6 +17,8 @@
typedef struct dsm_segment dsm_segment;
+#define DSM_CREATE_NULL_IF_MAXSEGMENTS 0x0001
+
/* Startup and shutdown functions. */
struct PGShmemHeader; /* avoid including pg_shmem.h */
extern void dsm_cleanup_using_control_segment(dsm_handle old_control_handle);
@@ -29,7 +31,7 @@ extern void dsm_set_control_handle(dsm_handle h);
#endif
/* Functions that create, update, or remove mappings. */
-extern dsm_segment *dsm_create(Size size);
+extern dsm_segment *dsm_create(Size size, int flags);
extern dsm_segment *dsm_attach(dsm_handle h);
extern void *dsm_resize(dsm_segment *seg, Size size);
extern void *dsm_remap(dsm_segment *seg);
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 1950990..7f2f5fd 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -125,7 +125,7 @@ setup_dynamic_shared_memory(int64 queue_size, int nworkers,
segsize = shm_toc_estimate(&e);
/* Create the shared memory segment and establish a table of contents. */
- seg = dsm_create(shm_toc_estimate(&e));
+ seg = dsm_create(shm_toc_estimate(&e), 0);
toc = shm_toc_create(PG_TEST_SHM_MQ_MAGIC, dsm_segment_address(seg),
segsize);
--
1.7.9.6 (Apple Git-31.1)
Hi,
On 2015-03-19 11:21:45 -0400, Robert Haas wrote:
One question I struggled with is whether to keep the existing
dsm_create() signature intact and add a new function
dsm_create_extended(). I eventually decided against it. The
dsm_create() API is relatively new at this point, so there probably
aren't too many people who will be inconvenienced by an API break now.
If we go ahead and create dsm_create_extended() now, and then need
to make another API change down the line, I doubt there will be much
support for dsm_create_extended2() or whatever. So my gut is that
it's better to just change this outright, and keep
dsm_create_extended() as an idea for the future. But I could go the
other way on that if people feel strongly about it.
+1 for a clear API break.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund wrote:
Hi,
On 2015-03-19 11:21:45 -0400, Robert Haas wrote:
One question I struggled with is whether to keep the existing
dsm_create() signature intact and add a new function
dsm_create_extended(). I eventually decided against it. The
dsm_create() API is relatively new at this point, so there probably
aren't too many people who will be inconvenienced by an API break now.
+1 for a clear API break.
Seconded.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 19, 2015 at 11:25 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2015-03-19 11:21:45 -0400, Robert Haas wrote:
One question I struggled with is whether to keep the existing
dsm_create() signature intact and add a new function
dsm_create_extended(). I eventually decided against it. The
dsm_create() API is relatively new at this point, so there probably
aren't too many people who will be inconvenienced by an API break now.
If we go ahead and create dsm_create_extended() now, and then need
to make another API change down the line, I doubt there will be much
support for dsm_create_extended2() or whatever. So my gut is that
it's better to just change this outright, and keep
dsm_create_extended() as an idea for the future. But I could go the
other way on that if people feel strongly about it.+1 for a clear API break.
I'm slightly confused. Does that mean "just change it" or does that
mean "add dsm_create_extended instead"?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
I'm slightly confused. Does that mean "just change it" or does that
mean "add dsm_create_extended instead"?
FWIW, I vote for "just change it". We change C-level APIs all the time,
and this function has surely not got either the longevity nor the wide
usage that might argue for keeping its API sancrosanct.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2015-03-19 12:10:03 -0400, Robert Haas wrote:
On Thu, Mar 19, 2015 at 11:25 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2015-03-19 11:21:45 -0400, Robert Haas wrote:
One question I struggled with is whether to keep the existing
dsm_create() signature intact and add a new function
dsm_create_extended(). I eventually decided against it. The
dsm_create() API is relatively new at this point, so there probably
aren't too many people who will be inconvenienced by an API break now.
If we go ahead and create dsm_create_extended() now, and then need
to make another API change down the line, I doubt there will be much
support for dsm_create_extended2() or whatever. So my gut is that
it's better to just change this outright, and keep
dsm_create_extended() as an idea for the future. But I could go the
other way on that if people feel strongly about it.+1 for a clear API break.
I'm slightly confused. Does that mean "just change it" or does that
mean "add dsm_create_extended instead"?
The former.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I'm slightly confused. Does that mean "just change it" or does that
mean "add dsm_create_extended instead"?FWIW, I vote for "just change it". We change C-level APIs all the time,
and this function has surely not got either the longevity nor the wide
usage that might argue for keeping its API sancrosanct.
Agreed.
Thanks,
Stephen
On Thu, Mar 19, 2015 at 12:21 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I'm slightly confused. Does that mean "just change it" or does that
mean "add dsm_create_extended instead"?FWIW, I vote for "just change it". We change C-level APIs all the time,
and this function has surely not got either the longevity nor the wide
usage that might argue for keeping its API sancrosanct.Agreed.
OK, committed that way after, uh, actually testing it and fixing the bugs.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers