pgsql: Add new function dsa_allocate0.

Started by Robert Haasalmost 9 years ago11 messages
#1Robert Haas
rhaas@postgresql.org

Add new function dsa_allocate0.

This does the same thing as dsa_allocate, except that the memory
is guaranteed to be zero-filled on return.

Dilip Kumar, adjusted by me.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54

Modified Files
--------------
src/backend/utils/mmgr/dsa.c | 16 ++++++++++++++++
src/include/utils/dsa.h | 1 +
2 files changed, 17 insertions(+)

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#2Thomas Munro
thomas.munro@enterprisedb.com
In reply to: Robert Haas (#1)
Re: pgsql: Add new function dsa_allocate0.

On Fri, Feb 17, 2017 at 7:02 AM, Robert Haas <rhaas@postgresql.org> wrote:

Add new function dsa_allocate0.

This does the same thing as dsa_allocate, except that the memory
is guaranteed to be zero-filled on return.

Dilip Kumar, adjusted by me.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54

Modified Files
--------------
src/backend/utils/mmgr/dsa.c | 16 ++++++++++++++++
src/include/utils/dsa.h | 1 +
2 files changed, 17 insertions(+)

Hmm. This will segfault if you're out of memory.

--
Thomas Munro
http://www.enterprisedb.com

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#3Thomas Munro
thomas.munro@enterprisedb.com
In reply to: Thomas Munro (#2)
1 attachment(s)
Re: pgsql: Add new function dsa_allocate0.

On Fri, Feb 17, 2017 at 11:34 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Fri, Feb 17, 2017 at 7:02 AM, Robert Haas <rhaas@postgresql.org> wrote:

http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54

Hmm. This will segfault if you're out of memory.

Or to provide a more useful response... maybe this should be like the
attached? Or maybe people think that dsa_allocate should throw on
failure to allocate, like palloc?

--
Thomas Munro
http://www.enterprisedb.com

Attachments:

fix-dsa-allocate0.patchapplication/octet-stream; name=fix-dsa-allocate0.patchDownload
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 3eb3d4d..5026bc9 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -762,11 +762,10 @@ dsa_pointer
 dsa_allocate0(dsa_area *area, Size size)
 {
 	dsa_pointer dp;
-	char	   *object;
 
 	dp = dsa_allocate(area, size);
-	object = dsa_get_address(area, dp);
-	memset(object, 0, size);
+	if (DsaPointerIsValid(dp))
+		memset(dsa_get_address(area, dp), 0, size);
 
 	return dp;
 }
#4Michael Paquier
michael.paquier@gmail.com
In reply to: Thomas Munro (#3)
Re: pgsql: Add new function dsa_allocate0.

On Fri, Feb 17, 2017 at 12:03 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Fri, Feb 17, 2017 at 11:34 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Fri, Feb 17, 2017 at 7:02 AM, Robert Haas <rhaas@postgresql.org> wrote:

http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54

Hmm. This will segfault if you're out of memory.

Or to provide a more useful response... maybe this should be like the
attached? Or maybe people think that dsa_allocate should throw on
failure to allocate, like palloc?

     dp = dsa_allocate(area, size);
-    object = dsa_get_address(area, dp);
-    memset(object, 0, size);
+    if (DsaPointerIsValid(dp))
+        memset(dsa_get_address(area, dp), 0, size);
What you are proposing here looks like the right answer to me. Like
dsa_allocate, dsa_allocate0 should allow users to fallback to other
methods if what is returned is InvalidDsaPointer for consistency.
-- 
Michael

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

#5Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#4)
Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

On Thu, Feb 16, 2017 at 10:09 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Feb 17, 2017 at 12:03 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Fri, Feb 17, 2017 at 11:34 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Fri, Feb 17, 2017 at 7:02 AM, Robert Haas <rhaas@postgresql.org> wrote:

http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54

Hmm. This will segfault if you're out of memory.

Or to provide a more useful response... maybe this should be like the
attached? Or maybe people think that dsa_allocate should throw on
failure to allocate, like palloc?

dp = dsa_allocate(area, size);
-    object = dsa_get_address(area, dp);
-    memset(object, 0, size);
+    if (DsaPointerIsValid(dp))
+        memset(dsa_get_address(area, dp), 0, size);
What you are proposing here looks like the right answer to me. Like
dsa_allocate, dsa_allocate0 should allow users to fallback to other
methods if what is returned is InvalidDsaPointer for consistency.

I'm thinking we should change this to look more like the
MemoryContextAlloc interface. Let's have DSA_ALLOC_HUGE,
DSA_ALLOC_NO_OOM, and DSA_ALLOC_ZERO, just like the corresponding
MCXT_* flags, and a function dsa_allocate_extended() that takes a
flags argument. Then, dsa_allocate(x,y) can be a macro for
dsa_allocate_extended(x,y,0) and dsa_allocate0(x,y) can be a macro for
dsa_allocate_extended(x,y,DSA_ALLOC_ZERO). What this goof on my (and
Dilip's) part illustrates to me is that having this interface behave
significantly differently from the MemoryContextAlloc interface is
going to cause mistakes.

--
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

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#5)
Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

Robert Haas <robertmhaas@gmail.com> writes:

I'm thinking we should change this to look more like the
MemoryContextAlloc interface. Let's have DSA_ALLOC_HUGE,
DSA_ALLOC_NO_OOM, and DSA_ALLOC_ZERO, just like the corresponding
MCXT_* flags, and a function dsa_allocate_extended() that takes a
flags argument. Then, dsa_allocate(x,y) can be a macro for
dsa_allocate_extended(x,y,0) and dsa_allocate0(x,y) can be a macro for
dsa_allocate_extended(x,y,DSA_ALLOC_ZERO). What this goof on my (and
Dilip's) part illustrates to me is that having this interface behave
significantly differently from the MemoryContextAlloc interface is
going to cause mistakes.

+1

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

#7Thomas Munro
thomas.munro@enterprisedb.com
In reply to: Tom Lane (#6)
1 attachment(s)
Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I'm thinking we should change this to look more like the
MemoryContextAlloc interface. Let's have DSA_ALLOC_HUGE,
DSA_ALLOC_NO_OOM, and DSA_ALLOC_ZERO, just like the corresponding
MCXT_* flags, and a function dsa_allocate_extended() that takes a
flags argument. Then, dsa_allocate(x,y) can be a macro for
dsa_allocate_extended(x,y,0) and dsa_allocate0(x,y) can be a macro for
dsa_allocate_extended(x,y,DSA_ALLOC_ZERO). What this goof on my (and
Dilip's) part illustrates to me is that having this interface behave
significantly differently from the MemoryContextAlloc interface is
going to cause mistakes.

+1

Maybe something like the attached? I didn't add DSA_ALLOC_HUGE
because there is currently no limit on allocation size (other than the
limit on total size which you can set with dsa_set_size_limit, but
that causes allocation failure, not a separate kind of error). Should
there be a per-allocation size sanity check of 1GB like palloc?

--
Thomas Munro
http://www.enterprisedb.com

Attachments:

dsa-extended.patchapplication/octet-stream; name=dsa-extended.patchDownload
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 3eb3d4d..8ffef37 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -644,13 +644,17 @@ dsa_pin_mapping(dsa_area *area)
  * that can be passed to other processes, and converted to a local pointer
  * with dsa_get_address.  If no memory is available, returns
  * InvalidDsaPointer.
+ *
+ * See also the macros dsa_allocate and dsa_allocate0 which expand to a call
+ * to this function with commonly used flags.
  */
 dsa_pointer
-dsa_allocate(dsa_area *area, Size size)
+dsa_allocate_extended(dsa_area *area, Size size, int flags)
 {
 	uint16		size_class;
 	dsa_pointer start_pointer;
 	dsa_segment_map *segment_map;
+	dsa_pointer result;
 
 	Assert(size > 0);
 
@@ -684,6 +688,14 @@ dsa_allocate(dsa_area *area, Size size)
 			/* Can't make any more segments: game over. */
 			LWLockRelease(DSA_AREA_LOCK(area));
 			dsa_free(area, span_pointer);
+
+			/* Raise error unless asked not to. */
+			if ((flags & MCXT_ALLOC_NO_OOM) == 0)
+				ereport(ERROR,
+						(errcode(ERRCODE_OUT_OF_MEMORY),
+						 errmsg("out of memory"),
+						 errdetail("Failed on DSA request of size %zu.",
+								   size)));
 			return InvalidDsaPointer;
 		}
 
@@ -710,6 +722,10 @@ dsa_allocate(dsa_area *area, Size size)
 		segment_map->pagemap[first_page] = span_pointer;
 		LWLockRelease(DSA_SCLASS_LOCK(area, DSA_SCLASS_SPAN_LARGE));
 
+		/* Zero-initialize the memory if requested. */
+		if ((flags & DSA_ALLOC_ZERO) != 0)
+			memset(dsa_get_address(area, start_pointer), 0, size);
+
 		return start_pointer;
 	}
 
@@ -748,27 +764,28 @@ dsa_allocate(dsa_area *area, Size size)
 	Assert(size <= dsa_size_classes[size_class]);
 	Assert(size_class == 0 || size > dsa_size_classes[size_class - 1]);
 
-	/*
-	 * Attempt to allocate an object from the appropriate pool.  This might
-	 * return InvalidDsaPointer if there's no space available.
-	 */
-	return alloc_object(area, size_class);
-}
+	/* Attempt to allocate an object from the appropriate pool. */
+	result = alloc_object(area, size_class);
 
-/*
- * As dsa_allocate, but zeroes the allocated memory.
- */
-dsa_pointer
-dsa_allocate0(dsa_area *area, Size size)
-{
-	dsa_pointer dp;
-	char	   *object;
+	/* Check for failure to allocate. */
+	if (!DsaPointerIsValid(result))
+	{
+		/* Raise error unless asked not to. */
+		if ((flags & DSA_ALLOC_NO_OOM) == 0)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_OUT_OF_MEMORY),
+					 errmsg("out of memory"),
+					 errdetail("Failed on DSA request of size %zu.", size)));
+		}
+		return InvalidDsaPointer;
+	}
 
-	dp = dsa_allocate(area, size);
-	object = dsa_get_address(area, dp);
-	memset(object, 0, size);
+	/* Zero-initialize the memory if requested. */
+	if ((flags & DSA_ALLOC_ZERO) != 0)
+		memset(dsa_get_address(area, result), 0, size);
 
-	return dp;
+	return result;
 }
 
 /*
diff --git a/src/include/utils/dsa.h b/src/include/utils/dsa.h
index 3fd9bbd..cccd4d0 100644
--- a/src/include/utils/dsa.h
+++ b/src/include/utils/dsa.h
@@ -71,12 +71,24 @@ typedef pg_atomic_uint64 dsa_pointer_atomic;
 #define DSA_POINTER_FORMAT "%016" INT64_MODIFIER "x"
 #endif
 
+/* Flags for dsa_allocate_extended. */
+#define DSA_ALLOC_NO_OOM	0x02	/* no failure if out-of-memory */
+#define DSA_ALLOC_ZERO		0x04	/* zero allocated memory */
+
 /* A sentinel value for dsa_pointer used to indicate failure to allocate. */
 #define InvalidDsaPointer ((dsa_pointer) 0)
 
 /* Check if a dsa_pointer value is valid. */
 #define DsaPointerIsValid(x) ((x) != InvalidDsaPointer)
 
+/* Allocate uninitialized memory with error on out-of-memory. */
+#define dsa_allocate(area, size) \
+	dsa_allocate_extended(area, size, 0)
+
+/* Allocate zero-initialized memory with error on out-of-memory. */
+#define dsa_allocate0(area, size) \
+	dsa_allocate_extended(area, size, DSA_ALLOC_ZERO)
+
 /*
  * The type used for dsa_area handles.  dsa_handle values can be shared with
  * other processes, so that they can attach to them.  This provides a way to
@@ -105,8 +117,7 @@ extern void dsa_unpin(dsa_area *area);
 extern void dsa_set_size_limit(dsa_area *area, Size limit);
 extern Size dsa_minimum_size(void);
 extern dsa_handle dsa_get_handle(dsa_area *area);
-extern dsa_pointer dsa_allocate(dsa_area *area, Size size);
-extern dsa_pointer dsa_allocate0(dsa_area *area, Size size);
+extern dsa_pointer dsa_allocate_extended(dsa_area *area, Size size, int flags);
 extern void dsa_free(dsa_area *area, dsa_pointer dp);
 extern void *dsa_get_address(dsa_area *area, dsa_pointer dp);
 extern void dsa_trim(dsa_area *area);
#8Tom Lane
tgl@sss.pgh.pa.us
In reply to: Thomas Munro (#7)
Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

Thomas Munro <thomas.munro@enterprisedb.com> writes:

On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I'm thinking we should change this to look more like the
MemoryContextAlloc interface.

+1

Maybe something like the attached? I didn't add DSA_ALLOC_HUGE
because there is currently no limit on allocation size (other than the
limit on total size which you can set with dsa_set_size_limit, but
that causes allocation failure, not a separate kind of error). Should
there be a per-allocation size sanity check of 1GB like palloc?

I think it's not a bad idea. It could help catch faulty allocation
requests (since I'd bet very few call sites actually intend to allocate
gigabytes in one go), and as Robert says, there is substantial value in
the semantics being as much like palloc() as possible. People are
likely to assume that even if it isn't true.

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

#9Thomas Munro
thomas.munro@enterprisedb.com
In reply to: Tom Lane (#8)
1 attachment(s)
Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@enterprisedb.com> writes:

On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I'm thinking we should change this to look more like the
MemoryContextAlloc interface.

+1

Maybe something like the attached? I didn't add DSA_ALLOC_HUGE
because there is currently no limit on allocation size (other than the
limit on total size which you can set with dsa_set_size_limit, but
that causes allocation failure, not a separate kind of error). Should
there be a per-allocation size sanity check of 1GB like palloc?

I think it's not a bad idea. It could help catch faulty allocation
requests (since I'd bet very few call sites actually intend to allocate
gigabytes in one go), and as Robert says, there is substantial value in
the semantics being as much like palloc() as possible. People are
likely to assume that even if it isn't true.

Agreed. Here's a patch like that.

--
Thomas Munro
http://www.enterprisedb.com

Attachments:

dsa-extended-v2.patchapplication/octet-stream; name=dsa-extended-v2.patchDownload
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 3eb3d4d..f91f286 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -644,16 +644,32 @@ dsa_pin_mapping(dsa_area *area)
  * that can be passed to other processes, and converted to a local pointer
  * with dsa_get_address.  If no memory is available, returns
  * InvalidDsaPointer.
+ *
+ * 'flags' is a bitmap which should be constructed from the following values:
+ * DSA_ALLOC_HUGE to allow very large allocations, DSA_ALLOC_NO_OOM to return
+ * InvalidDsaPointer instead of raising an error on failrue to allocate due to
+ * lack of memory (including failure due to exhausting a size limit imposed by
+ * calling dsa_set_size_limit) and DSA_ALLOC_ZERO to request that the memory
+ * be set to all zeroes.  These flags correspond to similarly named flags used
+ * by MemoryContextAllocExtended().  See also the macros dsa_allocate and
+ * dsa_allocate0 which expand to a call to this function with commonly used
+ * flags.
  */
 dsa_pointer
-dsa_allocate(dsa_area *area, Size size)
+dsa_allocate_extended(dsa_area *area, Size size, int flags)
 {
 	uint16		size_class;
 	dsa_pointer start_pointer;
 	dsa_segment_map *segment_map;
+	dsa_pointer result;
 
 	Assert(size > 0);
 
+	/* Sanity check on huge individual allocation size. */
+	if (((flags & DSA_ALLOC_HUGE) != 0 && !AllocHugeSizeIsValid(size)) ||
+		((flags & DSA_ALLOC_HUGE) == 0 && !AllocSizeIsValid(size)))
+		elog(ERROR, "invalid DSA memory alloc request size %zu", size);
+
 	/*
 	 * If bigger than the largest size class, just grab a run of pages from
 	 * the free page manager, instead of allocating an object from a pool.
@@ -684,6 +700,14 @@ dsa_allocate(dsa_area *area, Size size)
 			/* Can't make any more segments: game over. */
 			LWLockRelease(DSA_AREA_LOCK(area));
 			dsa_free(area, span_pointer);
+
+			/* Raise error unless asked not to. */
+			if ((flags & MCXT_ALLOC_NO_OOM) == 0)
+				ereport(ERROR,
+						(errcode(ERRCODE_OUT_OF_MEMORY),
+						 errmsg("out of memory"),
+						 errdetail("Failed on DSA request of size %zu.",
+								   size)));
 			return InvalidDsaPointer;
 		}
 
@@ -710,6 +734,10 @@ dsa_allocate(dsa_area *area, Size size)
 		segment_map->pagemap[first_page] = span_pointer;
 		LWLockRelease(DSA_SCLASS_LOCK(area, DSA_SCLASS_SPAN_LARGE));
 
+		/* Zero-initialize the memory if requested. */
+		if ((flags & DSA_ALLOC_ZERO) != 0)
+			memset(dsa_get_address(area, start_pointer), 0, size);
+
 		return start_pointer;
 	}
 
@@ -748,27 +776,28 @@ dsa_allocate(dsa_area *area, Size size)
 	Assert(size <= dsa_size_classes[size_class]);
 	Assert(size_class == 0 || size > dsa_size_classes[size_class - 1]);
 
-	/*
-	 * Attempt to allocate an object from the appropriate pool.  This might
-	 * return InvalidDsaPointer if there's no space available.
-	 */
-	return alloc_object(area, size_class);
-}
+	/* Attempt to allocate an object from the appropriate pool. */
+	result = alloc_object(area, size_class);
 
-/*
- * As dsa_allocate, but zeroes the allocated memory.
- */
-dsa_pointer
-dsa_allocate0(dsa_area *area, Size size)
-{
-	dsa_pointer dp;
-	char	   *object;
+	/* Check for failure to allocate. */
+	if (!DsaPointerIsValid(result))
+	{
+		/* Raise error unless asked not to. */
+		if ((flags & DSA_ALLOC_NO_OOM) == 0)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_OUT_OF_MEMORY),
+					 errmsg("out of memory"),
+					 errdetail("Failed on DSA request of size %zu.", size)));
+		}
+		return InvalidDsaPointer;
+	}
 
-	dp = dsa_allocate(area, size);
-	object = dsa_get_address(area, dp);
-	memset(object, 0, size);
+	/* Zero-initialize the memory if requested. */
+	if ((flags & DSA_ALLOC_ZERO) != 0)
+		memset(dsa_get_address(area, result), 0, size);
 
-	return dp;
+	return result;
 }
 
 /*
diff --git a/src/include/utils/dsa.h b/src/include/utils/dsa.h
index 3fd9bbd..c4b122e 100644
--- a/src/include/utils/dsa.h
+++ b/src/include/utils/dsa.h
@@ -71,12 +71,25 @@ typedef pg_atomic_uint64 dsa_pointer_atomic;
 #define DSA_POINTER_FORMAT "%016" INT64_MODIFIER "x"
 #endif
 
+/* Flags for dsa_allocate_extended. */
+#define DSA_ALLOC_HUGE		0x01	/* allow huge allocation (> 1 GB) */
+#define DSA_ALLOC_NO_OOM	0x02	/* no failure if out-of-memory */
+#define DSA_ALLOC_ZERO		0x04	/* zero allocated memory */
+
 /* A sentinel value for dsa_pointer used to indicate failure to allocate. */
 #define InvalidDsaPointer ((dsa_pointer) 0)
 
 /* Check if a dsa_pointer value is valid. */
 #define DsaPointerIsValid(x) ((x) != InvalidDsaPointer)
 
+/* Allocate uninitialized memory with error on out-of-memory. */
+#define dsa_allocate(area, size) \
+	dsa_allocate_extended(area, size, 0)
+
+/* Allocate zero-initialized memory with error on out-of-memory. */
+#define dsa_allocate0(area, size) \
+	dsa_allocate_extended(area, size, DSA_ALLOC_ZERO)
+
 /*
  * The type used for dsa_area handles.  dsa_handle values can be shared with
  * other processes, so that they can attach to them.  This provides a way to
@@ -105,8 +118,7 @@ extern void dsa_unpin(dsa_area *area);
 extern void dsa_set_size_limit(dsa_area *area, Size limit);
 extern Size dsa_minimum_size(void);
 extern dsa_handle dsa_get_handle(dsa_area *area);
-extern dsa_pointer dsa_allocate(dsa_area *area, Size size);
-extern dsa_pointer dsa_allocate0(dsa_area *area, Size size);
+extern dsa_pointer dsa_allocate_extended(dsa_area *area, Size size, int flags);
 extern void dsa_free(dsa_area *area, dsa_pointer dp);
 extern void *dsa_get_address(dsa_area *area, dsa_pointer dp);
 extern void dsa_trim(dsa_area *area);
#10Thomas Munro
thomas.munro@enterprisedb.com
In reply to: Thomas Munro (#9)
1 attachment(s)
Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

On Sun, Feb 19, 2017 at 12:27 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@enterprisedb.com> writes:

On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

I'm thinking we should change this to look more like the
MemoryContextAlloc interface.

+1

Maybe something like the attached? I didn't add DSA_ALLOC_HUGE
because there is currently no limit on allocation size (other than the
limit on total size which you can set with dsa_set_size_limit, but
that causes allocation failure, not a separate kind of error). Should
there be a per-allocation size sanity check of 1GB like palloc?

I think it's not a bad idea. It could help catch faulty allocation
requests (since I'd bet very few call sites actually intend to allocate
gigabytes in one go), and as Robert says, there is substantial value in
the semantics being as much like palloc() as possible. People are
likely to assume that even if it isn't true.

Agreed. Here's a patch like that.

Oops, that had a typo in a comment. Here's a better one.

--
Thomas Munro
http://www.enterprisedb.com

Attachments:

dsa-extended-v3.patchapplication/octet-stream; name=dsa-extended-v3.patchDownload
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 3eb3d4d..e42d6ea 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -644,16 +644,32 @@ dsa_pin_mapping(dsa_area *area)
  * that can be passed to other processes, and converted to a local pointer
  * with dsa_get_address.  If no memory is available, returns
  * InvalidDsaPointer.
+ *
+ * 'flags' is a bitmap which should be constructed from the following values:
+ * DSA_ALLOC_HUGE to allow very large allocations, DSA_ALLOC_NO_OOM to return
+ * InvalidDsaPointer instead of raising an error on failure to allocate due to
+ * lack of memory (including failure due to exhausting a size limit imposed by
+ * calling dsa_set_size_limit) and DSA_ALLOC_ZERO to request that the memory
+ * be set to all zeroes.  These flags correspond to similarly named flags used
+ * by MemoryContextAllocExtended().  See also the macros dsa_allocate and
+ * dsa_allocate0 which expand to a call to this function with commonly used
+ * flags.
  */
 dsa_pointer
-dsa_allocate(dsa_area *area, Size size)
+dsa_allocate_extended(dsa_area *area, Size size, int flags)
 {
 	uint16		size_class;
 	dsa_pointer start_pointer;
 	dsa_segment_map *segment_map;
+	dsa_pointer result;
 
 	Assert(size > 0);
 
+	/* Sanity check on huge individual allocation size. */
+	if (((flags & DSA_ALLOC_HUGE) != 0 && !AllocHugeSizeIsValid(size)) ||
+		((flags & DSA_ALLOC_HUGE) == 0 && !AllocSizeIsValid(size)))
+		elog(ERROR, "invalid DSA memory alloc request size %zu", size);
+
 	/*
 	 * If bigger than the largest size class, just grab a run of pages from
 	 * the free page manager, instead of allocating an object from a pool.
@@ -684,6 +700,14 @@ dsa_allocate(dsa_area *area, Size size)
 			/* Can't make any more segments: game over. */
 			LWLockRelease(DSA_AREA_LOCK(area));
 			dsa_free(area, span_pointer);
+
+			/* Raise error unless asked not to. */
+			if ((flags & MCXT_ALLOC_NO_OOM) == 0)
+				ereport(ERROR,
+						(errcode(ERRCODE_OUT_OF_MEMORY),
+						 errmsg("out of memory"),
+						 errdetail("Failed on DSA request of size %zu.",
+								   size)));
 			return InvalidDsaPointer;
 		}
 
@@ -710,6 +734,10 @@ dsa_allocate(dsa_area *area, Size size)
 		segment_map->pagemap[first_page] = span_pointer;
 		LWLockRelease(DSA_SCLASS_LOCK(area, DSA_SCLASS_SPAN_LARGE));
 
+		/* Zero-initialize the memory if requested. */
+		if ((flags & DSA_ALLOC_ZERO) != 0)
+			memset(dsa_get_address(area, start_pointer), 0, size);
+
 		return start_pointer;
 	}
 
@@ -748,27 +776,28 @@ dsa_allocate(dsa_area *area, Size size)
 	Assert(size <= dsa_size_classes[size_class]);
 	Assert(size_class == 0 || size > dsa_size_classes[size_class - 1]);
 
-	/*
-	 * Attempt to allocate an object from the appropriate pool.  This might
-	 * return InvalidDsaPointer if there's no space available.
-	 */
-	return alloc_object(area, size_class);
-}
+	/* Attempt to allocate an object from the appropriate pool. */
+	result = alloc_object(area, size_class);
 
-/*
- * As dsa_allocate, but zeroes the allocated memory.
- */
-dsa_pointer
-dsa_allocate0(dsa_area *area, Size size)
-{
-	dsa_pointer dp;
-	char	   *object;
+	/* Check for failure to allocate. */
+	if (!DsaPointerIsValid(result))
+	{
+		/* Raise error unless asked not to. */
+		if ((flags & DSA_ALLOC_NO_OOM) == 0)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_OUT_OF_MEMORY),
+					 errmsg("out of memory"),
+					 errdetail("Failed on DSA request of size %zu.", size)));
+		}
+		return InvalidDsaPointer;
+	}
 
-	dp = dsa_allocate(area, size);
-	object = dsa_get_address(area, dp);
-	memset(object, 0, size);
+	/* Zero-initialize the memory if requested. */
+	if ((flags & DSA_ALLOC_ZERO) != 0)
+		memset(dsa_get_address(area, result), 0, size);
 
-	return dp;
+	return result;
 }
 
 /*
diff --git a/src/include/utils/dsa.h b/src/include/utils/dsa.h
index 3fd9bbd..c4b122e 100644
--- a/src/include/utils/dsa.h
+++ b/src/include/utils/dsa.h
@@ -71,12 +71,25 @@ typedef pg_atomic_uint64 dsa_pointer_atomic;
 #define DSA_POINTER_FORMAT "%016" INT64_MODIFIER "x"
 #endif
 
+/* Flags for dsa_allocate_extended. */
+#define DSA_ALLOC_HUGE		0x01	/* allow huge allocation (> 1 GB) */
+#define DSA_ALLOC_NO_OOM	0x02	/* no failure if out-of-memory */
+#define DSA_ALLOC_ZERO		0x04	/* zero allocated memory */
+
 /* A sentinel value for dsa_pointer used to indicate failure to allocate. */
 #define InvalidDsaPointer ((dsa_pointer) 0)
 
 /* Check if a dsa_pointer value is valid. */
 #define DsaPointerIsValid(x) ((x) != InvalidDsaPointer)
 
+/* Allocate uninitialized memory with error on out-of-memory. */
+#define dsa_allocate(area, size) \
+	dsa_allocate_extended(area, size, 0)
+
+/* Allocate zero-initialized memory with error on out-of-memory. */
+#define dsa_allocate0(area, size) \
+	dsa_allocate_extended(area, size, DSA_ALLOC_ZERO)
+
 /*
  * The type used for dsa_area handles.  dsa_handle values can be shared with
  * other processes, so that they can attach to them.  This provides a way to
@@ -105,8 +118,7 @@ extern void dsa_unpin(dsa_area *area);
 extern void dsa_set_size_limit(dsa_area *area, Size limit);
 extern Size dsa_minimum_size(void);
 extern dsa_handle dsa_get_handle(dsa_area *area);
-extern dsa_pointer dsa_allocate(dsa_area *area, Size size);
-extern dsa_pointer dsa_allocate0(dsa_area *area, Size size);
+extern dsa_pointer dsa_allocate_extended(dsa_area *area, Size size, int flags);
 extern void dsa_free(dsa_area *area, dsa_pointer dp);
 extern void *dsa_get_address(dsa_area *area, dsa_pointer dp);
 extern void dsa_trim(dsa_area *area);
#11Robert Haas
robertmhaas@gmail.com
In reply to: Thomas Munro (#10)
Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

On Sat, Feb 18, 2017 at 6:31 PM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

Agreed. Here's a patch like that.

Oops, that had a typo in a comment. Here's a better one.

This looked fine, except that the new comment in dsa_allocate
contained an extremely long sentence which happened to contradict the
last sentence of the existing comment. Committed after frobbing the
comment to fix those two issues.

--
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