Tomas Vondra wrote:
> A few minor comments regarding the patch:
>
> 1) CopyStartSend seems pretty pointless - It only has one function call
> in it, and is called on exactly one place (and all other places simply
> call allowLongStringInfo directly). I'd get rid of this function and
> replace the call in CopyOneRowTo(() with allowLongStringInfo().
>
> 2) allowlong seems awkward, allowLong or allow_long would be better
>
> 3) Why does resetStringInfo reset the allowLong flag? Are there any
> cases when we want/need to forget the flag value? I don't think so, so
> let's simply not reset it and get rid of the allowLongStringInfo()
> calls. Maybe it'd be better to invent a new makeLongStringInfo() method
> instead, which would make it clear that the flag value is permanent.
>
> 4) HandleParallelMessage needs a tweak, as it uses msg->len in a log
> message, but with '%d' and not '%ld' (as needed after changing the type
> to Size).
>
> 5) The comment at allowLongStringInfo talks about allocLongStringInfo
> (i.e. wrong function name).
Here's an updated patch. Compared to the previous version:
- removed CopyStartSend (per comment #1 in review)
- renamed flag to allow_long (comment #2)
- resetStringInfo no longer resets the flag (comment #3).
- allowLongStringInfo() is removed (comment #3 and #5),
makeLongStringInfo() and initLongStringInfo() are provided
instead, as alternatives to makeStringInfo() and initStringInfo().
- StringInfoData.len is back to int type, 2GB max.
(addresses comment #4 incidentally).
This is safer because many routines peeking
into StringInfoData use int for offsets into the buffer,
for instance most of the stuff in backend/libpq/pqformat.c
Altough these routines are not supposed to be called on long
buffers, this assertion was not enforced in the code, and
with a 64-bit length effectively over 2GB, they would misbehave
pretty badly.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
diff --git a/src/backend/access/common/heaptuple.c
b/src/backend/access/common/heaptuple.c
index 6d0f3f3..ed9cabd 100644
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -741,7 +741,9 @@ heap_form_tuple(TupleDesc tupleDescriptor,
* Allocate and zero the space needed. Note that the tuple body and
* HeapTupleData management structure are allocated in one chunk.
*/
- tuple = (HeapTuple) palloc0(HEAPTUPLESIZE + len);
+ tuple = MemoryContextAllocExtended(CurrentMemoryContext,
+
HEAPTUPLESIZE + len,
+
MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO);
tuple->t_data = td = (HeapTupleHeader) ((char *) tuple + HEAPTUPLESIZE);
/*
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 432b0ca..7419362 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -397,7 +397,7 @@ ReceiveCopyBegin(CopyState cstate)
pq_sendint(&buf, format, 2); /* per-column
formats */
pq_endmessage(&buf);
cstate->copy_dest = COPY_NEW_FE;
- cstate->fe_msgbuf = makeStringInfo();
+ cstate->fe_msgbuf = makeLongStringInfo();
}
else if (PG_PROTOCOL_MAJOR(FrontendProtocol) >= 2)
{
@@ -1892,7 +1892,7 @@ CopyTo(CopyState cstate)
cstate->null_print_client = cstate->null_print; /* default */
/* We use fe_msgbuf as a per-row buffer regardless of copy_dest */
- cstate->fe_msgbuf = makeStringInfo();
+ cstate->fe_msgbuf = makeLongStringInfo();
/* Get info about the columns we need to process. */
cstate->out_functions = (FmgrInfo *) palloc(num_phys_attrs *
sizeof(FmgrInfo));
@@ -2707,8 +2707,8 @@ BeginCopyFrom(ParseState *pstate,
cstate->cur_attval = NULL;
/* Set up variables to avoid per-attribute overhead. */
- initStringInfo(&cstate->attribute_buf);
- initStringInfo(&cstate->line_buf);
+ initLongStringInfo(&cstate->attribute_buf);
+ initLongStringInfo(&cstate->line_buf);
cstate->line_buf_converted = false;
cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
cstate->raw_buf_index = cstate->raw_buf_len = 0;
diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index 7382e08..0125047 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -37,6 +37,24 @@ makeStringInfo(void)
}
/*
+ * makeLongStringInfo
+ *
+ * Same as makeStringInfo for larger strings.
+ */
+StringInfo
+makeLongStringInfo(void)
+{
+ StringInfo res;
+
+ res = (StringInfo) palloc(sizeof(StringInfoData));
+
+ initLongStringInfo(res);
+
+ return res;
+}
+
+
+/*
* initStringInfo
*
* Initialize a StringInfoData struct (with previously undefined contents)
@@ -47,12 +65,25 @@ initStringInfo(StringInfo str)
{
int size = 1024; /* initial default buffer size
*/
- str->data = (char *) palloc(size);
+ str->data = (char *) palloc(size); /* no need for "huge" at this
point */
str->maxlen = size;
+ str->allow_long = false;
resetStringInfo(str);
}
/*
+ * initLongStringInfo
+ * Same as initLongStringInfo for larger strings.
+ */
+void
+initLongStringInfo(StringInfo str)
+{
+ initStringInfo(str);
+ str->allow_long = true;
+}
+
+
+/*
* resetStringInfo
*
* Reset the StringInfo: the data buffer remains valid, but its
@@ -245,6 +276,14 @@ void
enlargeStringInfo(StringInfo str, int needed)
{
int newlen;
+ int limit;
+
+ /*
+ * Maximum size for strings with allow_long=true.
+ * It must not exceed INT_MAX, as the StringInfo routines
+ * expect offsets into the buffer to fit into an int.
+ */
+ const int max_alloc_long = 0x7fffffff;
/*
* Guard against out-of-range "needed" values. Without this, we can get
@@ -252,7 +291,10 @@ enlargeStringInfo(StringInfo str, int needed)
*/
if (needed < 0) /* should not happen */
elog(ERROR, "invalid string enlargement request size: %d",
needed);
- if (((Size) needed) >= (MaxAllocSize - (Size) str->len))
+
+ /* choose the proper limit and verify this allocation wouldn't exceed
it */
+ limit = str->allow_long ? max_alloc_long : MaxAllocSize;
+ if (((Size) needed) >= (limit - (Size) str->len))
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("out of memory"),
@@ -261,7 +303,7 @@ enlargeStringInfo(StringInfo str, int needed)
needed += str->len + 1; /* total space required now */
- /* Because of the above test, we now have needed <= MaxAllocSize */
+ /* Because of the above test, we now have needed <= limit */
if (needed <= str->maxlen)
return; /* got enough space
already */
@@ -271,19 +313,18 @@ enlargeStringInfo(StringInfo str, int needed)
* for efficiency, double the buffer size each time it overflows.
* Actually, we might need to more than double it if 'needed' is big...
*/
- newlen = 2 * str->maxlen;
- while (needed > newlen)
- newlen = 2 * newlen;
- /*
- * Clamp to MaxAllocSize in case we went past it. Note we are assuming
- * here that MaxAllocSize <= INT_MAX/2, else the above loop could
- * overflow. We will still have newlen >= needed.
- */
- if (newlen > (int) MaxAllocSize)
- newlen = (int) MaxAllocSize;
+ newlen = str->maxlen;
+
+ do
+ {
+ if (newlen < limit / 2) /* prevent integer overflow */
+ newlen = 2 * newlen;
+ else
+ newlen = limit;
+ } while (newlen < needed); /* can not loop forever,
because needed <= limit */
- str->data = (char *) repalloc(str->data, newlen);
+ str->data = (char *) repalloc_huge(str->data, (Size)newlen);
str->maxlen = newlen;
}
diff --git a/src/include/lib/stringinfo.h b/src/include/lib/stringinfo.h
index f644067..3d8621f 100644
--- a/src/include/lib/stringinfo.h
+++ b/src/include/lib/stringinfo.h
@@ -30,6 +30,8 @@
* cursor is initialized to zero by makeStringInfo or
initStringInfo,
* but is not otherwise touched by the
stringinfo.c routines.
* Some routines use it to scan through a
StringInfo.
+ * allow_long boolean flag indicating whether this StringInfo can
allocate
+ * more than MaxAllocSize bytes.
*-------------------------
*/
typedef struct StringInfoData
@@ -38,6 +40,7 @@ typedef struct StringInfoData
int len;
int maxlen;
int cursor;
+ bool allow_long;
} StringInfoData;
typedef StringInfoData *StringInfo;
@@ -70,6 +73,7 @@ typedef StringInfoData *StringInfo;
* Create an empty 'StringInfoData' & return a pointer to it.
*/
extern StringInfo makeStringInfo(void);
+extern StringInfo makeLongStringInfo(void);
/*------------------------
* initStringInfo
@@ -79,6 +83,12 @@ extern StringInfo makeStringInfo(void);
extern void initStringInfo(StringInfo str);
/*------------------------
+ * initLongStringInfo
+ * Same as initStringInfo but for larger strings (up to 2GB)
+ */
+extern void initLongStringInfo(StringInfo str);
+
+/*------------------------
* resetStringInfo
* Clears the current content of the StringInfo, if any. The
* StringInfo remains valid.
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
