Make StringInfo available to frontend code.
Hi,
This patch, in a slightly rougher form, was submitted as part of [1]/messages/by-id/20190920051857.2fhnvhvx4qdddviz@alap3.anarazel.de,
but it seems worth bringing up separately, rather than just committing
hearing no objections.
From the commit message:
Make StringInfo available to frontend code.
There's plenty places in frontend code that could benefit from a
string buffer implementation. Some because it yields simpler and
faster code, and some others because of the desire to share code
between backend and frontend.
While there is a string buffer implementation available to frontend
code, libpq's PQExpBuffer, it is clunkier than stringinfo, it
introduces a libpq dependency, doesn't allow for sharing between
frontend and backend code, and has a higher API/ABI stability
requirement due to being exposed via libpq.
Therefore it seems best to just making StringInfo being usable by
frontend code. There's not much to do for that, except for rewriting
two subsequent elog/ereport calls into others types of error
reporting, and deciding on a maximum string length.
For the maximum string size I decided to privately define MaxAllocSize
to the same value as used in the backend. It seems likely that we'll
want to reconsider this for both backend and frontend code in the not
too far away future.
For now I've left stringinfo.h in lib/, rather than common/, to reduce
the likelihood of unnecessary breakage. We could alternatively decide
to provide a redirecting stringinfo.h in lib/, or just not provide
compatibility.
I'm still using stringinfo in the aforementioned thread, and I also want
to use it in a few more places. On the more ambitious side I really
would like to have a minimal version of elog.h available in the backend,
and that would really be a lot easier with stringinfo available.
I also would like to submit a few patches expanding stringinfo's
capabilities and performance, and it seems to me it'd be better to do so
after moving (lest they introduce new FE vs BE compat issues).
This allows us to remove compat.c hackery providing some stringinfo
functionality for pg_waldump (which now actually needs to pass in a
StringInfo...). I briefly played with converting more code in
pg_waldump.c than just that one call to StringInfo, but it seems that'd
be best done separately.
Comments?
Greetings,
Andres Freund
[1]: /messages/by-id/20190920051857.2fhnvhvx4qdddviz@alap3.anarazel.de
Attachments:
v2-0001-Make-StringInfo-available-to-frontend-code.patchtext/x-diff; charset=us-asciiDownload
From c37388addec295958787314d5d00b9d360da57c6 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 29 Oct 2019 17:09:41 -0700
Subject: [PATCH v2] Make StringInfo available to frontend code.
There's plenty places in frontend code that could benefit from a
string buffer implementation. Some because it yields simpler and
faster code, and some others because of the desire to share code
between backend and frontend.
While there is a string buffer implementation available to frontend
code, libpq's PQExpBuffer, it is clunkier than stringinfo, it
introduces a libpq dependency, doesn't allow for sharing between
frontend and backend code, and has a higher API/ABI stability
requirement due to being exposed via libpq.
Therefore it seems best to just making StringInfo being usable by
frontend code. There's not much to do for that, except for rewriting
two subsequent elog/ereport calls into others types of error
reporting, and deciding on a maximum string length.
For the maximum string size I decided to privately define MaxAllocSize
to the same value as used in the backend. It seems likely that we'll
want to reconsider this for both backend and frontend code in the not
too far away future.
For now I've left stringinfo.h in lib/, rather than common/, to reduce
the likelihood of unnecessary breakage. We could alternatively decide
to provide a redirecting stringinfo.h in lib/, or just not provide
compatibility.
Author: Andres Freund
Reviewed-By:
Discussion: https://postgr.es/m/20190920051857.2fhnvhvx4qdddviz@alap3.anarazel.de
---
src/backend/lib/Makefile | 2 +-
src/bin/pg_waldump/compat.c | 27 --------------
src/bin/pg_waldump/pg_waldump.c | 7 +++-
src/common/Makefile | 4 +-
src/{backend/lib => common}/stringinfo.c | 47 +++++++++++++++++++-----
src/tools/msvc/Mkvcbuild.pm | 2 +-
6 files changed, 47 insertions(+), 42 deletions(-)
rename src/{backend/lib => common}/stringinfo.c (87%)
diff --git a/src/backend/lib/Makefile b/src/backend/lib/Makefile
index 3c1ee1df83a..9a0c508e908 100644
--- a/src/backend/lib/Makefile
+++ b/src/backend/lib/Makefile
@@ -13,6 +13,6 @@ top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
OBJS = binaryheap.o bipartite_match.o bloomfilter.o dshash.o hyperloglog.o \
- ilist.o integerset.o knapsack.o pairingheap.o rbtree.o stringinfo.o
+ ilist.o integerset.o knapsack.o pairingheap.o rbtree.o
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/bin/pg_waldump/compat.c b/src/bin/pg_waldump/compat.c
index 7b389a20c96..5db83880fb7 100644
--- a/src/bin/pg_waldump/compat.c
+++ b/src/bin/pg_waldump/compat.c
@@ -20,7 +20,6 @@
#include <time.h>
-#include "lib/stringinfo.h"
#include "utils/datetime.h"
/* copied from timestamp.c */
@@ -63,29 +62,3 @@ timestamptz_to_str(TimestampTz dt)
return buf;
}
-
-/*
- * Provide a hacked up compat layer for StringInfos so xlog desc functions can
- * be linked/called.
- */
-void
-appendStringInfo(StringInfo str, const char *fmt,...)
-{
- va_list args;
-
- va_start(args, fmt);
- vprintf(fmt, args);
- va_end(args);
-}
-
-void
-appendStringInfoString(StringInfo str, const char *string)
-{
- appendStringInfo(str, "%s", string);
-}
-
-void
-appendStringInfoChar(StringInfo str, char ch)
-{
- appendStringInfo(str, "%c", ch);
-}
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 8e54d7f4e00..b783166e2d5 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -514,6 +514,7 @@ XLogDumpDisplayRecord(XLogDumpConfig *config, XLogReaderState *record)
int block_id;
uint8 info = XLogRecGetInfo(record);
XLogRecPtr xl_prev = XLogRecGetPrev(record);
+ StringInfoData s;
XLogDumpRecordLen(record, &rec_len, &fpi_len);
@@ -529,8 +530,10 @@ XLogDumpDisplayRecord(XLogDumpConfig *config, XLogReaderState *record)
(uint32) (xl_prev >> 32), (uint32) xl_prev);
printf("desc: %s ", id);
- /* the desc routine will printf the description directly to stdout */
- desc->rm_desc(NULL, record);
+ initStringInfo(&s);
+ desc->rm_desc(&s, record);
+ printf("%s", s.data);
+ pfree(s.data);
if (!config->bkp_details)
{
diff --git a/src/common/Makefile b/src/common/Makefile
index 2f22b9b101d..914a2184b8a 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -49,8 +49,8 @@ LIBS += $(PTHREAD_LIBS)
OBJS_COMMON = base64.o config_info.o controldata_utils.o d2s.o exec.o f2s.o \
file_perm.o ip.o keywords.o kwlookup.o link-canary.o md5.o \
pg_lzcompress.o pgfnames.o psprintf.o relpath.o \
- rmtree.o saslprep.o scram-common.o string.o unicode_norm.o \
- username.o wait_error.o
+ rmtree.o saslprep.o scram-common.o string.o stringinfo.o \
+ unicode_norm.o username.o wait_error.o
ifeq ($(with_openssl),yes)
OBJS_COMMON += sha2_openssl.o
diff --git a/src/backend/lib/stringinfo.c b/src/common/stringinfo.c
similarity index 87%
rename from src/backend/lib/stringinfo.c
rename to src/common/stringinfo.c
index 99c83c1549c..73f6501f6e4 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/common/stringinfo.c
@@ -2,21 +2,34 @@
*
* stringinfo.c
*
- * StringInfo provides an indefinitely-extensible string data type.
- * It can be used to buffer either ordinary C strings (null-terminated text)
- * or arbitrary binary data. All storage is allocated with palloc().
+ * StringInfo provides an extensible string data type. It can be used to
+ * buffer either ordinary C strings (null-terminated text) or arbitrary binary
+ * data. All storage is allocated with palloc() (falling back to malloc in
+ * frontend code).
*
* Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * src/backend/lib/stringinfo.c
+ * src/common/stringinfo.c
*
*-------------------------------------------------------------------------
*/
+
+#ifndef FRONTEND
+
#include "postgres.h"
+#include "utils/memutils.h"
+
+#else
+
+#include "postgres_fe.h"
+
+/* It's possible we could use a different value for this in frontend code */
+#define MaxAllocSize ((Size) 0x3fffffff) /* 1 gigabyte - 1 */
+
+#endif
#include "lib/stringinfo.h"
-#include "utils/memutils.h"
/*
@@ -261,10 +274,10 @@ appendBinaryStringInfoNT(StringInfo str, const char *data, int datalen)
* can save some palloc overhead by enlarging the buffer before starting
* to store data in it.
*
- * NB: because we use repalloc() to enlarge the buffer, the string buffer
- * will remain allocated in the same memory context that was current when
- * initStringInfo was called, even if another context is now current.
- * This is the desired and indeed critical behavior!
+ * NB: In the backend, because we use repalloc() to enlarge the buffer, the
+ * string buffer will remain allocated in the same memory context that was
+ * current when initStringInfo was called, even if another context is now
+ * current. This is the desired and indeed critical behavior!
*/
void
enlargeStringInfo(StringInfo str, int needed)
@@ -276,13 +289,29 @@ enlargeStringInfo(StringInfo str, int needed)
* an overflow or infinite loop in the following.
*/
if (needed < 0) /* should not happen */
+ {
+#ifndef FRONTEND
elog(ERROR, "invalid string enlargement request size: %d", needed);
+#else
+ fprintf(stderr, "invalid string enlargement request size: %d\n", needed);
+ exit(EXIT_FAILURE);
+#endif
+ }
if (((Size) needed) >= (MaxAllocSize - (Size) str->len))
+ {
+#ifndef FRONTEND
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("out of memory"),
errdetail("Cannot enlarge string buffer containing %d bytes by %d more bytes.",
str->len, needed)));
+#else
+ fprintf(stderr,
+ _("out of memory\n\nCannot enlarge string buffer containing %d bytes by %d more bytes.\n"),
+ str->len, needed);
+ exit(EXIT_FAILURE);
+#endif
+ }
needed += str->len + 1; /* total space required now */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 7a103e61406..9a0963a050d 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -123,7 +123,7 @@ sub mkvcbuild
base64.c config_info.c controldata_utils.c d2s.c exec.c f2s.c file_perm.c ip.c
keywords.c kwlookup.c link-canary.c md5.c
pg_lzcompress.c pgfnames.c psprintf.c relpath.c rmtree.c
- saslprep.c scram-common.c string.c unicode_norm.c username.c
+ saslprep.c scram-common.c string.c stringinfo.c unicode_norm.c username.c
wait_error.c);
if ($solution->{options}->{openssl})
--
2.23.0.385.gbc12974a89
At Tue, 29 Oct 2019 17:10:01 -0700, Andres Freund <andres@anarazel.de> wrote in
Hi,
This patch, in a slightly rougher form, was submitted as part of [1],
but it seems worth bringing up separately, rather than just committing
hearing no objections.
..
I'm still using stringinfo in the aforementioned thread, and I also want
to use it in a few more places. On the more ambitious side I really
would like to have a minimal version of elog.h available in the backend,
and that would really be a lot easier with stringinfo available.I also would like to submit a few patches expanding stringinfo's
capabilities and performance, and it seems to me it'd be better to do so
after moving (lest they introduce new FE vs BE compat issues).This allows us to remove compat.c hackery providing some stringinfo
functionality for pg_waldump (which now actually needs to pass in a
StringInfo...). I briefly played with converting more code in
pg_waldump.c than just that one call to StringInfo, but it seems that'd
be best done separately.Comments?
It uses different form for the same message for FE and BE.
common/stringinfo.c:289-
BE: ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("out of memory"),
errdetail("Cannot enlarge string buffer containing %d
bytes by %d more bytes.",FE: + _("out of memory\n\nCannot enlarge string buffer containing %d
bytes by %d more bytes.\n"),
.po files will be smaller and more stable if we keep the same
translation unit for the same messages. That being said it doesn't
matter if it is tentative and the minimal elog.h for frontend comes
soon.
/* It's possible we could use a different value for this in frontend code */
#define MaxAllocSize ((Size) 0x3fffffff) /* 1 gigabyte - 1 */
The same symbol is defined for frontend in psprint.c. Isn't it better
merge them and place it in postgres_fe.h?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hi,
On 2019-10-30 10:58:59 +0900, Kyotaro Horiguchi wrote:
At Tue, 29 Oct 2019 17:10:01 -0700, Andres Freund <andres@anarazel.de> wrote in
Hi,
This patch, in a slightly rougher form, was submitted as part of [1],
but it seems worth bringing up separately, rather than just committing
hearing no objections...
I'm still using stringinfo in the aforementioned thread, and I also want
to use it in a few more places. On the more ambitious side I really
would like to have a minimal version of elog.h available in the backend,
and that would really be a lot easier with stringinfo available.I also would like to submit a few patches expanding stringinfo's
capabilities and performance, and it seems to me it'd be better to do so
after moving (lest they introduce new FE vs BE compat issues).This allows us to remove compat.c hackery providing some stringinfo
functionality for pg_waldump (which now actually needs to pass in a
StringInfo...). I briefly played with converting more code in
pg_waldump.c than just that one call to StringInfo, but it seems that'd
be best done separately.Comments?
It uses different form for the same message for FE and BE.
common/stringinfo.c:289-
BE: ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("out of memory"),
errdetail("Cannot enlarge string buffer containing %d
bytes by %d more bytes.",FE: + _("out of memory\n\nCannot enlarge string buffer containing %d
bytes by %d more bytes.\n"),.po files will be smaller and more stable if we keep the same
translation unit for the same messages. That being said it doesn't
matter if it is tentative and the minimal elog.h for frontend comes
soon.
I'm inclined to think that the contortions necessary to allow reusing
the translation strings here would be more work than worthwhile. Also,
do we even try to share the translations between backend and frontend?
/* It's possible we could use a different value for this in frontend code */
#define MaxAllocSize ((Size) 0x3fffffff) /* 1 gigabyte - 1 */The same symbol is defined for frontend in psprint.c. Isn't it better
merge them and place it in postgres_fe.h?
No, I don't think so. I'd rather have less than more code depend on it.
Greetings,
Andres Freund
Hello.
At Tue, 29 Oct 2019 19:06:38 -0700, Andres Freund <andres@anarazel.de> wrote in
Hi,
On 2019-10-30 10:58:59 +0900, Kyotaro Horiguchi wrote:
At Tue, 29 Oct 2019 17:10:01 -0700, Andres Freund <andres@anarazel.de> wrote in
Hi,
This patch, in a slightly rougher form, was submitted as part of [1],
but it seems worth bringing up separately, rather than just committing
hearing no objections...
I'm still using stringinfo in the aforementioned thread, and I also want
to use it in a few more places. On the more ambitious side I really
would like to have a minimal version of elog.h available in the backend,
and that would really be a lot easier with stringinfo available.I also would like to submit a few patches expanding stringinfo's
capabilities and performance, and it seems to me it'd be better to do so
after moving (lest they introduce new FE vs BE compat issues).This allows us to remove compat.c hackery providing some stringinfo
functionality for pg_waldump (which now actually needs to pass in a
StringInfo...). I briefly played with converting more code in
pg_waldump.c than just that one call to StringInfo, but it seems that'd
be best done separately.Comments?
It uses different form for the same message for FE and BE.
common/stringinfo.c:289-
BE: ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("out of memory"),
errdetail("Cannot enlarge string buffer containing %d
bytes by %d more bytes.",FE: + _("out of memory\n\nCannot enlarge string buffer containing %d
bytes by %d more bytes.\n"),.po files will be smaller and more stable if we keep the same
translation unit for the same messages. That being said it doesn't
matter if it is tentative and the minimal elog.h for frontend comes
soon.I'm inclined to think that the contortions necessary to allow reusing
the translation strings here would be more work than worthwhile. Also,
Maybe so for doing that in this stage. So I expect that elog.h for FE
comes.
the translation strings here would be more work than worthwhile. Also,
do we even try to share the translations between backend and frontend?
No. I don't mean that. FE and BE have their own .po files, anyway.
/* It's possible we could use a different value for this in frontend code */
#define MaxAllocSize ((Size) 0x3fffffff) /* 1 gigabyte - 1 */The same symbol is defined for frontend in psprint.c. Isn't it better
merge them and place it in postgres_fe.h?No, I don't think so. I'd rather have less than more code depend on it.
Ok, understoold. Thanks for the reply.
regards.
On 30 Oct 2019, at 01:10, Andres Freund <andres@anarazel.de> wrote:
Make StringInfo available to frontend code.
I’ve certainly wanted just that on multiple occasions, so +1 on this.
Therefore it seems best to just making StringInfo being usable by
frontend code. There's not much to do for that, except for rewriting
two subsequent elog/ereport calls into others types of error
reporting, and deciding on a maximum string length.
Skimming (but not testing) the patch, it seems a reasonable approach.
+ * StringInfo provides an extensible string data type. It can be used to
It might be useful to point out the upper bound on the extensibility in the
rewrite of this sentence, and that it’s not guaranteed to be consistent between
frontend and backend.
I'm still using stringinfo in the aforementioned thread, and I also want
to use it in a few more places. On the more ambitious side I really
would like to have a minimal version of elog.h available in the backend,
and that would really be a lot easier with stringinfo available.
s/backend/frontend/?
cheers ./daniel
Hi,
On 2019-11-01 23:19:33 +0100, Daniel Gustafsson wrote:
On 30 Oct 2019, at 01:10, Andres Freund <andres@anarazel.de> wrote:
Make StringInfo available to frontend code.
I’ve certainly wanted just that on multiple occasions, so +1 on this.
Cool.
+ * StringInfo provides an extensible string data type. It can be used to
It might be useful to point out the upper bound on the extensibility in the
rewrite of this sentence, and that it’s not guaranteed to be consistent between
frontend and backend.
Hm. Something like 'Currently the maximum length of a StringInfo is
1GB.'? I don't really think it's worth pointing out that they may not be
consistent, when they currently are...
And I suspect we should just fix the length limit to be higher for both,
perhaps somehow allowing to limit the length for the backend cases where
we want to error out if a string gets too long (possibly adding a
separate initialization API that allows to specify the memory allocation
flags or such).
I'm still using stringinfo in the aforementioned thread, and I also want
to use it in a few more places. On the more ambitious side I really
would like to have a minimal version of elog.h available in the backend,
and that would really be a lot easier with stringinfo available.s/backend/frontend/?
Indeed.
Thanks for looking,
Andres Freund
On 2 Nov 2019, at 03:21, Andres Freund <andres@anarazel.de> wrote:
On 2019-11-01 23:19:33 +0100, Daniel Gustafsson wrote:
+ * StringInfo provides an extensible string data type. It can be used to
It might be useful to point out the upper bound on the extensibility in the
rewrite of this sentence, and that it’s not guaranteed to be consistent between
frontend and backend.Hm. Something like 'Currently the maximum length of a StringInfo is
1GB.’?
Something along those lines (or the define/mechanism with which the upper
boundary controlled, simply stating the limit seems more straightforward.)
I don't really think it's worth pointing out that they may not be
consistent, when they currently are…
Good point.
And I suspect we should just fix the length limit to be higher for both,
perhaps somehow allowing to limit the length for the backend cases where
we want to error out if a string gets too long (possibly adding a
separate initialization API that allows to specify the memory allocation
flags or such).
Sounds reasonable, maybe even where one can set errhint/errdetail on the “out
of memory” error to get better reporting on failures.
cheers ./daniel
Hi,
On 2019-11-02 23:57:06 +0100, Daniel Gustafsson wrote:
On 2 Nov 2019, at 03:21, Andres Freund <andres@anarazel.de> wrote:
On 2019-11-01 23:19:33 +0100, Daniel Gustafsson wrote:+ * StringInfo provides an extensible string data type. It can be used to
It might be useful to point out the upper bound on the extensibility in the
rewrite of this sentence, and that it’s not guaranteed to be consistent between
frontend and backend.Hm. Something like 'Currently the maximum length of a StringInfo is
1GB.’?Something along those lines (or the define/mechanism with which the upper
boundary controlled, simply stating the limit seems more straightforward.)
Pushed now, with a variation of my suggestion above.
As nobody commented on that, I've not adjusted the location of
stringinfo.h. If somebody has feelings about it being in the wrong
place, and whether to put a redirecting header in place, or whether to
just break extensions using stringinfo, ...
And I suspect we should just fix the length limit to be higher for both,
perhaps somehow allowing to limit the length for the backend cases where
we want to error out if a string gets too long (possibly adding a
separate initialization API that allows to specify the memory allocation
flags or such).Sounds reasonable, maybe even where one can set errhint/errdetail on the “out
of memory” error to get better reporting on failures.
That seems too much customization for the caller / too much added
complexity, for the little gain it'd provide. Normally if a caller wants
something like that, they can push something onto the error context
stack, and get context information out that way.
Greetings,
Andres Freund