As part of the multithreading work, it'd be nice to get rid of as many
global or static variables as possible. Remaining ones can be converted
to thread locals as appropriate, but where possible, it's better to just
get rid of them.
Here are patches to get rid of a few static variables, by e.g.
converting them to regular local variables or palloc'd return values, as
appropriate.
This doesn't move the needle much, but every little helps, and these
seem like nice little changes in any case.
--
Heikki Linnakangas
Neon (https://neon.tech)
From f556fd49bf2df9f5e9636f60b1797b342fb8ad3f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 30 Jul 2024 13:22:14 +0300
Subject: [PATCH v1 1/5] Replace static bufs with StringInfo in cash_words()
For clarity. The code was correct, and the buffer was large enough,
but string manipulation with no bounds checking is scary.
This incurs an extra palloc+pfree to every call, but in quick
performance testing, it doesn't seem to be significant.
---
src/backend/utils/adt/cash.c | 85 +++++++++++++++++++-----------------
1 file changed, 44 insertions(+), 41 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index b20c358486d..ec3c08acfc2 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -35,10 +35,9 @@
* Private routines
************************************************************************/
-static const char *
-num_word(Cash value)
+static void
+append_num_word(StringInfo buf, Cash value)
{
- static char buf[128];
static const char *const small[] = {
"zero", "one", "two", "three", "four", "five", "six", "seven",
"eight", "nine", "ten", "eleven", "twelve", "thirteen", "fourteen",
@@ -50,13 +49,16 @@ num_word(Cash value)
/* deal with the simple cases first */
if (value <= 20)
- return small[value];
+ {
+ appendStringInfoString(buf, small[value]);
+ return;
+ }
/* is it an even multiple of 100? */
if (!tu)
{
- sprintf(buf, "%s hundred", small[value / 100]);
- return buf;
+ appendStringInfo(buf, "%s hundred", small[value / 100]);
+ return;
}
/* more than 99? */
@@ -64,27 +66,25 @@ num_word(Cash value)
{
/* is it an even multiple of 10 other than 10? */
if (value % 10 == 0 && tu > 10)
- sprintf(buf, "%s hundred %s",
- small[value / 100], big[tu / 10]);
+ appendStringInfo(buf, "%s hundred %s",
+ small[value / 100], big[tu / 10]);
else if (tu < 20)
- sprintf(buf, "%s hundred and %s",
- small[value / 100], small[tu]);
+ appendStringInfo(buf, "%s hundred and %s",
+ small[value / 100], small[tu]);
else
- sprintf(buf, "%s hundred %s %s",
- small[value / 100], big[tu / 10], small[tu % 10]);
+ appendStringInfo(buf, "%s hundred %s %s",
+ small[value / 100], big[tu / 10], small[tu % 10]);
}
else
{
/* is it an even multiple of 10 other than 10? */
if (value % 10 == 0 && tu > 10)
- sprintf(buf, "%s", big[tu / 10]);
+ appendStringInfoString(buf, big[tu / 10]);
else if (tu < 20)
- sprintf(buf, "%s", small[tu]);
+ appendStringInfoString(buf, small[tu]);
else
- sprintf(buf, "%s %s", big[tu / 10], small[tu % 10]);
+ appendStringInfo(buf, "%s %s", big[tu / 10], small[tu % 10]);
}
-
- return buf;
} /* num_word() */
static inline Cash
@@ -960,8 +960,9 @@ cash_words(PG_FUNCTION_ARGS)
{
Cash value = PG_GETARG_CASH(0);
uint64 val;
- char buf[256];
- char *p = buf;
+ StringInfoData buf;
+ text *res;
+ Cash dollars;
Cash m0;
Cash m1;
Cash m2;
@@ -970,19 +971,19 @@ cash_words(PG_FUNCTION_ARGS)
Cash m5;
Cash m6;
+ initStringInfo(&buf);
+
/* work with positive numbers */
if (value < 0)
{
value = -value;
- strcpy(buf, "minus ");
- p += 6;
+ appendStringInfoString(&buf, "minus ");
}
- else
- buf[0] = '\0';
/* Now treat as unsigned, to avoid trouble at INT_MIN */
val = (uint64) value;
+ dollars = val / INT64CONST(100);
m0 = val % INT64CONST(100); /* cents */
m1 = (val / INT64CONST(100)) % 1000; /* hundreds */
m2 = (val / INT64CONST(100000)) % 1000; /* thousands */
@@ -993,49 +994,51 @@ cash_words(PG_FUNCTION_ARGS)
if (m6)
{
- strcat(buf, num_word(m6));
- strcat(buf, " quadrillion ");
+ append_num_word(&buf, m6);
+ appendStringInfoString(&buf, " quadrillion ");
}
if (m5)
{
- strcat(buf, num_word(m5));
- strcat(buf, " trillion ");
+ append_num_word(&buf, m5);
+ appendStringInfoString(&buf, " trillion ");
}
if (m4)
{
- strcat(buf, num_word(m4));
- strcat(buf, " billion ");
+ append_num_word(&buf, m4);
+ appendStringInfoString(&buf, " billion ");
}
if (m3)
{
- strcat(buf, num_word(m3));
- strcat(buf, " million ");
+ append_num_word(&buf, m3);
+ appendStringInfoString(&buf, " million ");
}
if (m2)
{
- strcat(buf, num_word(m2));
- strcat(buf, " thousand ");
+ append_num_word(&buf, m2);
+ appendStringInfoString(&buf, " thousand ");
}
if (m1)
- strcat(buf, num_word(m1));
+ append_num_word(&buf, m1);
- if (!*p)
- strcat(buf, "zero");
+ if (dollars == 0)
+ appendStringInfoString(&buf, "zero");
- strcat(buf, (val / 100) == 1 ? " dollar and " : " dollars and ");
- strcat(buf, num_word(m0));
- strcat(buf, m0 == 1 ? " cent" : " cents");
+ appendStringInfoString(&buf, dollars == 1 ? " dollar and " : " dollars and ");
+ append_num_word(&buf, m0);
+ appendStringInfoString(&buf, m0 == 1 ? " cent" : " cents");
/* capitalize output */
- buf[0] = pg_toupper((unsigned char) buf[0]);
+ buf.data[0] = pg_toupper((unsigned char) buf.data[0]);
/* return as text datum */
- PG_RETURN_TEXT_P(cstring_to_text(buf));
+ res = cstring_to_text_with_len(buf.data, buf.len);
+ pfree(buf.data);
+ PG_RETURN_TEXT_P(res);
}
--
2.39.2
From 2fc38f7def122ac6cea63146ad93097cb0ec1071 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 30 Jul 2024 13:22:23 +0300
Subject: [PATCH v1 2/5] Replace static buf with palloc in str_time()
The function is only used once in startup process, so the leak into
current memory context is harmless.
This is tiny step in making the server thread-safe.
---
src/backend/access/transam/xlog.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f86f4b5c4b7..75463e153ec 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5168,9 +5168,9 @@ BootStrapXLOG(uint32 data_checksum_version)
static char *
str_time(pg_time_t tnow)
{
- static char buf[128];
+ char *buf = palloc(128);
- pg_strftime(buf, sizeof(buf),
+ pg_strftime(buf, 128,
"%Y-%m-%d %H:%M:%S %Z",
pg_localtime(&tnow, log_timezone));
--
2.39.2
From 06b642cc73a581e53d387af668057363110866d0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 30 Jul 2024 13:24:22 +0300
Subject: [PATCH v1 3/5] Replace static buf with a stack-allocated one in
ReadControlFile
It's only used very locally within the function.
---
src/backend/access/transam/xlog.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 75463e153ec..4a8a2f6098f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4314,7 +4314,7 @@ ReadControlFile(void)
{
pg_crc32c crc;
int fd;
- static char wal_segsz_str[20];
+ char wal_segsz_str[20];
int r;
/*
--
2.39.2
From 7d4b15c0abe03f3fe8ffbdc302e7a56e15e5c4f2 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 30 Jul 2024 13:25:44 +0300
Subject: [PATCH v1 4/5] Replace static buf with a stack-allocated one in 'seg'
extension
The buffer is used only very locally within the function. Also, the
initialization to '0' characters was unnecessary, the buffer was
always overwritten to with sprintf(). I don't understand why it was
done that way, but it's been like that since forever.
In the passing, change from sprintf() to snprintf(). The buffer was
long enough so sprintf() was fine, but this makes it more obvious that
there's no risk of buffer overflow.x
---
contrib/seg/segparse.y | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/contrib/seg/segparse.y b/contrib/seg/segparse.y
index 729d4b6390b..9635c3af6e6 100644
--- a/contrib/seg/segparse.y
+++ b/contrib/seg/segparse.y
@@ -29,14 +29,6 @@ static bool seg_atof(char *value, float *result, struct Node *escontext);
static int sig_digits(const char *value);
-static char strbuf[25] = {
- '0', '0', '0', '0', '0',
- '0', '0', '0', '0', '0',
- '0', '0', '0', '0', '0',
- '0', '0', '0', '0', '0',
- '0', '0', '0', '0', '\0'
-};
-
%}
/* BISON Declarations */
@@ -69,11 +61,13 @@ static char strbuf[25] = {
range: boundary PLUMIN deviation
{
+ char strbuf[25];
+
result->lower = $1.val - $3.val;
result->upper = $1.val + $3.val;
- sprintf(strbuf, "%g", result->lower);
+ snprintf(strbuf, sizeof(strbuf), "%g", result->lower);
result->l_sigd = Max(sig_digits(strbuf), Max($1.sigd, $3.sigd));
- sprintf(strbuf, "%g", result->upper);
+ snprintf(strbuf, sizeof(strbuf), "%g", result->upper);
result->u_sigd = Max(sig_digits(strbuf), Max($1.sigd, $3.sigd));
result->l_ext = '\0';
result->u_ext = '\0';
--
2.39.2
From 7c8e78230dc313f68751e53e8221c7879d1505ab Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Tue, 30 Jul 2024 14:00:22 +0300
Subject: [PATCH v1 5/5] Refactor getWeights to write to caller-supplied buffer
This gets rid of the static result buffer.
---
src/backend/utils/adt/tsrank.c | 53 ++++++++++++++++++++--------------
1 file changed, 31 insertions(+), 22 deletions(-)
diff --git a/src/backend/utils/adt/tsrank.c b/src/backend/utils/adt/tsrank.c
index c2285cf27e9..d456a039510 100644
--- a/src/backend/utils/adt/tsrank.c
+++ b/src/backend/utils/adt/tsrank.c
@@ -21,7 +21,8 @@
#include "utils/array.h"
#include "utils/fmgrprotos.h"
-static const float weights[] = {0.1f, 0.2f, 0.4f, 1.0f};
+#define NUM_WEIGHTS 4
+static const float default_weights[NUM_WEIGHTS] = {0.1f, 0.2f, 0.4f, 1.0f};
#define wpos(wep) ( w[ WEP_GETWEIGHT(wep) ] )
@@ -396,22 +397,24 @@ calc_rank(const float *w, TSVector t, TSQuery q, int32 method)
return res;
}
-static const float *
-getWeights(ArrayType *win)
+/*
+ * Extract weights from an array. The weights are stored in *ws, which must
+ * have space for NUM_WEIGHTS elements.
+ */
+static void
+getWeights(ArrayType *win, float *ws)
{
- static float ws[lengthof(weights)];
int i;
float4 *arrdata;
- if (win == NULL)
- return weights;
+ Assert(win != NULL);
if (ARR_NDIM(win) != 1)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
errmsg("array of weight must be one-dimensional")));
- if (ArrayGetNItems(ARR_NDIM(win), ARR_DIMS(win)) < lengthof(weights))
+ if (ArrayGetNItems(ARR_NDIM(win), ARR_DIMS(win)) < NUM_WEIGHTS)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
errmsg("array of weight is too short")));
@@ -422,16 +425,14 @@ getWeights(ArrayType *win)
errmsg("array of weight must not contain nulls")));
arrdata = (float4 *) ARR_DATA_PTR(win);
- for (i = 0; i < lengthof(weights); i++)
+ for (i = 0; i < NUM_WEIGHTS; i++)
{
- ws[i] = (arrdata[i] >= 0) ? arrdata[i] : weights[i];
+ ws[i] = (arrdata[i] >= 0) ? arrdata[i] : default_weights[i];
if (ws[i] > 1.0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("weight out of range")));
}
-
- return ws;
}
Datum
@@ -441,9 +442,11 @@ ts_rank_wttf(PG_FUNCTION_ARGS)
TSVector txt = PG_GETARG_TSVECTOR(1);
TSQuery query = PG_GETARG_TSQUERY(2);
int method = PG_GETARG_INT32(3);
+ float weights[NUM_WEIGHTS];
float res;
- res = calc_rank(getWeights(win), txt, query, method);
+ getWeights(win, weights);
+ res = calc_rank(weights, txt, query, method);
PG_FREE_IF_COPY(win, 0);
PG_FREE_IF_COPY(txt, 1);
@@ -457,9 +460,11 @@ ts_rank_wtt(PG_FUNCTION_ARGS)
ArrayType *win = (ArrayType *) PG_DETOAST_DATUM(PG_GETARG_DATUM(0));
TSVector txt = PG_GETARG_TSVECTOR(1);
TSQuery query = PG_GETARG_TSQUERY(2);
+ float weights[NUM_WEIGHTS];
float res;
- res = calc_rank(getWeights(win), txt, query, DEF_NORM_METHOD);
+ getWeights(win, weights);
+ res = calc_rank(weights, txt, query, DEF_NORM_METHOD);
PG_FREE_IF_COPY(win, 0);
PG_FREE_IF_COPY(txt, 1);
@@ -475,7 +480,7 @@ ts_rank_ttf(PG_FUNCTION_ARGS)
int method = PG_GETARG_INT32(2);
float res;
- res = calc_rank(getWeights(NULL), txt, query, method);
+ res = calc_rank(default_weights, txt, query, method);
PG_FREE_IF_COPY(txt, 0);
PG_FREE_IF_COPY(query, 1);
@@ -489,7 +494,7 @@ ts_rank_tt(PG_FUNCTION_ARGS)
TSQuery query = PG_GETARG_TSQUERY(1);
float res;
- res = calc_rank(getWeights(NULL), txt, query, DEF_NORM_METHOD);
+ res = calc_rank(default_weights, txt, query, DEF_NORM_METHOD);
PG_FREE_IF_COPY(txt, 0);
PG_FREE_IF_COPY(query, 1);
@@ -855,16 +860,16 @@ calc_rank_cd(const float4 *arrdata, TSVector txt, TSQuery query, int method)
doclen = 0;
CoverExt ext;
double Wdoc = 0.0;
- double invws[lengthof(weights)];
+ double invws[NUM_WEIGHTS];
double SumDist = 0.0,
PrevExtPos = 0.0;
int NExtent = 0;
QueryRepresentation qr;
- for (i = 0; i < lengthof(weights); i++)
+ for (i = 0; i < NUM_WEIGHTS; i++)
{
- invws[i] = ((double) ((arrdata[i] >= 0) ? arrdata[i] : weights[i]));
+ invws[i] = ((double) ((arrdata[i] >= 0) ? arrdata[i] : default_weights[i]));
if (invws[i] > 1.0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -956,9 +961,11 @@ ts_rankcd_wttf(PG_FUNCTION_ARGS)
TSVector txt = PG_GETARG_TSVECTOR(1);
TSQuery query = PG_GETARG_TSQUERY(2);
int method = PG_GETARG_INT32(3);
+ float weights[NUM_WEIGHTS];
float res;
- res = calc_rank_cd(getWeights(win), txt, query, method);
+ getWeights(win, weights);
+ res = calc_rank_cd(weights, txt, query, method);
PG_FREE_IF_COPY(win, 0);
PG_FREE_IF_COPY(txt, 1);
@@ -972,9 +979,11 @@ ts_rankcd_wtt(PG_FUNCTION_ARGS)
ArrayType *win = (ArrayType *) PG_DETOAST_DATUM(PG_GETARG_DATUM(0));
TSVector txt = PG_GETARG_TSVECTOR(1);
TSQuery query = PG_GETARG_TSQUERY(2);
+ float weights[NUM_WEIGHTS];
float res;
- res = calc_rank_cd(getWeights(win), txt, query, DEF_NORM_METHOD);
+ getWeights(win, weights);
+ res = calc_rank_cd(weights, txt, query, DEF_NORM_METHOD);
PG_FREE_IF_COPY(win, 0);
PG_FREE_IF_COPY(txt, 1);
@@ -990,7 +999,7 @@ ts_rankcd_ttf(PG_FUNCTION_ARGS)
int method = PG_GETARG_INT32(2);
float res;
- res = calc_rank_cd(getWeights(NULL), txt, query, method);
+ res = calc_rank_cd(default_weights, txt, query, method);
PG_FREE_IF_COPY(txt, 0);
PG_FREE_IF_COPY(query, 1);
@@ -1004,7 +1013,7 @@ ts_rankcd_tt(PG_FUNCTION_ARGS)
TSQuery query = PG_GETARG_TSQUERY(1);
float res;
- res = calc_rank_cd(getWeights(NULL), txt, query, DEF_NORM_METHOD);
+ res = calc_rank_cd(default_weights, txt, query, DEF_NORM_METHOD);
PG_FREE_IF_COPY(txt, 0);
PG_FREE_IF_COPY(query, 1);
--
2.39.2
