Minor refactorings to eliminate some static buffers
Started by Heikki Linnakangasover 1 year ago8 messages
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)
Attachments:
v1-0001-Replace-static-bufs-with-StringInfo-in-cash_words.patchtext/x-patch; charset=UTF-8; name=v1-0001-Replace-static-bufs-with-StringInfo-in-cash_words.patchDownload
From f556fd49bf2df9f5e9636f60b1797b342fb8ad3f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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
v1-0002-Replace-static-buf-with-palloc-in-str_time.patchtext/x-patch; charset=UTF-8; name=v1-0002-Replace-static-buf-with-palloc-in-str_time.patchDownload
From 2fc38f7def122ac6cea63146ad93097cb0ec1071 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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
v1-0003-Replace-static-buf-with-a-stack-allocated-one-in-.patchtext/x-patch; charset=UTF-8; name=v1-0003-Replace-static-buf-with-a-stack-allocated-one-in-.patchDownload
From 06b642cc73a581e53d387af668057363110866d0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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
v1-0004-Replace-static-buf-with-a-stack-allocated-one-in-.patchtext/x-patch; charset=UTF-8; name=v1-0004-Replace-static-buf-with-a-stack-allocated-one-in-.patchDownload
From 7d4b15c0abe03f3fe8ffbdc302e7a56e15e5c4f2 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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
v1-0005-Refactor-getWeights-to-write-to-caller-supplied-b.patchtext/x-patch; charset=UTF-8; name=v1-0005-Refactor-getWeights-to-write-to-caller-supplied-b.patchDownload
From 7c8e78230dc313f68751e53e8221c7879d1505ab Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@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
Re: Minor refactorings to eliminate some static buffers
On Tue, Jul 30, 2024 at 7:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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.
I spent a few minutes looking through these patches and they seem like
good cleanups. I couldn't think of a plausible reason why someone
would object to any of these.
--
Robert Haas
EDB: http://www.enterprisedb.com
Re: Minor refactorings to eliminate some static buffers
On 30/07/2024 18:44, Robert Haas wrote:
On Tue, Jul 30, 2024 at 7:22 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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.I spent a few minutes looking through these patches and they seem like
good cleanups. I couldn't think of a plausible reason why someone
would object to any of these.
Committed, thanks for having a look.
--
Heikki Linnakangas
Neon (https://neon.tech)
Re: Minor refactorings to eliminate some static buffers
Here's another batch of little changes in the same vein. Mostly
converting static bufs that are never modified to consts.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
0001-Turn-a-few-validnsps-static-variables-into-locals.patchtext/x-patch; charset=UTF-8; name=0001-Turn-a-few-validnsps-static-variables-into-locals.patchDownload
From 6dd5a4a413212a61d9a4f5b9db73e812c8b5dcbd Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:58:29 +0300
Subject: [PATCH 1/5] Turn a few 'validnsps' static variables into locals
There was no need for these to be static buffers, a local variable
works just as well. I think they were marked as 'static' to imply that
they are read-only, but 'const' is more appropriate for that, so
change them to const.
To make it possible to mark the variables as 'const', also add 'const'
decorations to the transformRelOptions() signature.
Discussion: https://www.postgresql.org/message-id/7f86e06a-98c5-4ce3-8ec9-3885c8de0358@iki.fi
---
src/backend/access/common/reloptions.c | 2 +-
src/backend/commands/createas.c | 2 +-
src/backend/commands/tablecmds.c | 4 ++--
src/backend/tcop/utility.c | 2 +-
src/include/access/reloptions.h | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d6eb5d8559..49fd35bfc5 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1154,7 +1154,7 @@ add_local_string_reloption(local_relopts *relopts, const char *name,
*/
Datum
transformRelOptions(Datum oldOptions, List *defList, const char *namspace,
- char *validnsps[], bool acceptOidsOff, bool isReset)
+ const char *const validnsps[], bool acceptOidsOff, bool isReset)
{
Datum result;
ArrayBuildState *astate;
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index c71ff80188..0b629b1f79 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -83,7 +83,7 @@ create_ctas_internal(List *attrList, IntoClause *into)
bool is_matview;
char relkind;
Datum toast_options;
- static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
+ const char *const validnsps[] = HEAP_RELOPT_NAMESPACES;
ObjectAddress intoRelationAddr;
/* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0b2a52463f..1f94f4fdbb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -700,7 +700,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
ListCell *listptr;
AttrNumber attnum;
bool partitioned;
- static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
+ const char *const validnsps[] = HEAP_RELOPT_NAMESPACES;
Oid ofTypeId;
ObjectAddress address;
LOCKMODE parentLockmode;
@@ -14897,7 +14897,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
Datum repl_val[Natts_pg_class];
bool repl_null[Natts_pg_class];
bool repl_repl[Natts_pg_class];
- static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
+ const char *const validnsps[] = HEAP_RELOPT_NAMESPACES;
if (defList == NIL && operation != AT_ReplaceRelOptions)
return; /* nothing to do */
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 702a6c3a0b..2732d6bfc9 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1155,7 +1155,7 @@ ProcessUtilitySlow(ParseState *pstate,
{
CreateStmt *cstmt = (CreateStmt *) stmt;
Datum toast_options;
- static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
+ const char *validnsps[] = HEAP_RELOPT_NAMESPACES;
/* Remember transformed RangeVar for LIKE */
table_rv = cstmt->relation;
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 81829b8270..df6923c9d5 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -220,7 +220,7 @@ extern void add_local_string_reloption(local_relopts *relopts, const char *name,
fill_string_relopt filler, int offset);
extern Datum transformRelOptions(Datum oldOptions, List *defList,
- const char *namspace, char *validnsps[],
+ const char *namspace, const char *const validnsps[],
bool acceptOidsOff, bool isReset);
extern List *untransformRelOptions(Datum options);
extern bytea *extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
--
2.39.2
0002-Make-nullSemAction-const-add-const-decorators-to-rel.patchtext/x-patch; charset=UTF-8; name=0002-Make-nullSemAction-const-add-const-decorators-to-rel.patchDownload
From f108ae4c2ddfa6ca77e9736dc3fb20e6bda7b67c Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:59:33 +0300
Subject: [PATCH 2/5] Make nullSemAction const, add 'const' decorators to
related functions
To make it more clear that these should never be modified.
Discussion: https://www.postgresql.org/message-id/7f86e06a-98c5-4ce3-8ec9-3885c8de0358@iki.fi
---
src/backend/utils/adt/jsonfuncs.c | 2 +-
src/common/jsonapi.c | 26 +++++++++----------
src/include/common/jsonapi.h | 6 ++---
src/include/utils/jsonfuncs.h | 2 +-
.../test_json_parser_incremental.c | 2 +-
5 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 7076b344b7..5ecb9fffae 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -514,7 +514,7 @@ static JsonParseErrorType transform_string_values_scalar(void *state, char *toke
* returned when escontext is an ErrorSaveContext).
*/
bool
-pg_parse_json_or_errsave(JsonLexContext *lex, JsonSemAction *sem,
+pg_parse_json_or_errsave(JsonLexContext *lex, const JsonSemAction *sem,
Node *escontext)
{
JsonParseErrorType result;
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 2527dbe1da..2ffcaaa6fd 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -213,15 +213,15 @@ static char JSON_PROD_GOAL[] = {JSON_TOKEN_END, JSON_NT_JSON, 0};
static inline JsonParseErrorType json_lex_string(JsonLexContext *lex);
static inline JsonParseErrorType json_lex_number(JsonLexContext *lex, const char *s,
bool *num_err, size_t *total_len);
-static inline JsonParseErrorType parse_scalar(JsonLexContext *lex, JsonSemAction *sem);
-static JsonParseErrorType parse_object_field(JsonLexContext *lex, JsonSemAction *sem);
-static JsonParseErrorType parse_object(JsonLexContext *lex, JsonSemAction *sem);
-static JsonParseErrorType parse_array_element(JsonLexContext *lex, JsonSemAction *sem);
-static JsonParseErrorType parse_array(JsonLexContext *lex, JsonSemAction *sem);
+static inline JsonParseErrorType parse_scalar(JsonLexContext *lex, const JsonSemAction *sem);
+static JsonParseErrorType parse_object_field(JsonLexContext *lex, const JsonSemAction *sem);
+static JsonParseErrorType parse_object(JsonLexContext *lex, const JsonSemAction *sem);
+static JsonParseErrorType parse_array_element(JsonLexContext *lex, const JsonSemAction *sem);
+static JsonParseErrorType parse_array(JsonLexContext *lex, const JsonSemAction *sem);
static JsonParseErrorType report_parse_error(JsonParseContext ctx, JsonLexContext *lex);
/* the null action object used for pure validation */
-JsonSemAction nullSemAction =
+const JsonSemAction nullSemAction =
{
NULL, NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL
@@ -519,7 +519,7 @@ freeJsonLexContext(JsonLexContext *lex)
* other differences.
*/
JsonParseErrorType
-pg_parse_json(JsonLexContext *lex, JsonSemAction *sem)
+pg_parse_json(JsonLexContext *lex, const JsonSemAction *sem)
{
#ifdef FORCE_JSON_PSTACK
@@ -648,7 +648,7 @@ json_count_array_elements(JsonLexContext *lex, int *elements)
*/
JsonParseErrorType
pg_parse_json_incremental(JsonLexContext *lex,
- JsonSemAction *sem,
+ const JsonSemAction *sem,
const char *json,
size_t len,
bool is_last)
@@ -1005,7 +1005,7 @@ pg_parse_json_incremental(JsonLexContext *lex,
* - object field
*/
static inline JsonParseErrorType
-parse_scalar(JsonLexContext *lex, JsonSemAction *sem)
+parse_scalar(JsonLexContext *lex, const JsonSemAction *sem)
{
char *val = NULL;
json_scalar_action sfunc = sem->scalar;
@@ -1049,7 +1049,7 @@ parse_scalar(JsonLexContext *lex, JsonSemAction *sem)
}
static JsonParseErrorType
-parse_object_field(JsonLexContext *lex, JsonSemAction *sem)
+parse_object_field(JsonLexContext *lex, const JsonSemAction *sem)
{
/*
* An object field is "fieldname" : value where value can be a scalar,
@@ -1111,7 +1111,7 @@ parse_object_field(JsonLexContext *lex, JsonSemAction *sem)
}
static JsonParseErrorType
-parse_object(JsonLexContext *lex, JsonSemAction *sem)
+parse_object(JsonLexContext *lex, const JsonSemAction *sem)
{
/*
* an object is a possibly empty sequence of object fields, separated by
@@ -1185,7 +1185,7 @@ parse_object(JsonLexContext *lex, JsonSemAction *sem)
}
static JsonParseErrorType
-parse_array_element(JsonLexContext *lex, JsonSemAction *sem)
+parse_array_element(JsonLexContext *lex, const JsonSemAction *sem)
{
json_aelem_action astart = sem->array_element_start;
json_aelem_action aend = sem->array_element_end;
@@ -1229,7 +1229,7 @@ parse_array_element(JsonLexContext *lex, JsonSemAction *sem)
}
static JsonParseErrorType
-parse_array(JsonLexContext *lex, JsonSemAction *sem)
+parse_array(JsonLexContext *lex, const JsonSemAction *sem)
{
/*
* an array is a possibly empty sequence of array elements, separated by
diff --git a/src/include/common/jsonapi.h b/src/include/common/jsonapi.h
index 71a491d72d..a995fdbe08 100644
--- a/src/include/common/jsonapi.h
+++ b/src/include/common/jsonapi.h
@@ -153,16 +153,16 @@ typedef struct JsonSemAction
* does nothing and just continues.
*/
extern JsonParseErrorType pg_parse_json(JsonLexContext *lex,
- JsonSemAction *sem);
+ const JsonSemAction *sem);
extern JsonParseErrorType pg_parse_json_incremental(JsonLexContext *lex,
- JsonSemAction *sem,
+ const JsonSemAction *sem,
const char *json,
size_t len,
bool is_last);
/* the null action object used for pure validation */
-extern PGDLLIMPORT JsonSemAction nullSemAction;
+extern PGDLLIMPORT const JsonSemAction nullSemAction;
/*
* json_count_array_elements performs a fast secondary parse to determine the
diff --git a/src/include/utils/jsonfuncs.h b/src/include/utils/jsonfuncs.h
index 93384d900a..1f14ab7ddf 100644
--- a/src/include/utils/jsonfuncs.h
+++ b/src/include/utils/jsonfuncs.h
@@ -41,7 +41,7 @@ typedef text *(*JsonTransformStringValuesAction) (void *state, char *elem_value,
extern JsonLexContext *makeJsonLexContext(JsonLexContext *lex, text *json, bool need_escapes);
/* try to parse json, and errsave(escontext) on failure */
-extern bool pg_parse_json_or_errsave(JsonLexContext *lex, JsonSemAction *sem,
+extern bool pg_parse_json_or_errsave(JsonLexContext *lex, const JsonSemAction *sem,
struct Node *escontext);
#define pg_parse_json_or_ereport(lex, sem) \
diff --git a/src/test/modules/test_json_parser/test_json_parser_incremental.c b/src/test/modules/test_json_parser/test_json_parser_incremental.c
index 47040e1e42..294e5f74ea 100644
--- a/src/test/modules/test_json_parser/test_json_parser_incremental.c
+++ b/src/test/modules/test_json_parser/test_json_parser_incremental.c
@@ -84,7 +84,7 @@ main(int argc, char **argv)
size_t chunk_size = DEFAULT_CHUNK_SIZE;
struct stat statbuf;
off_t bytes_left;
- JsonSemAction *testsem = &nullSemAction;
+ const JsonSemAction *testsem = &nullSemAction;
char *testfile;
int c;
bool need_strings = false;
--
2.39.2
0003-Mark-misc-static-global-variables-as-const.patchtext/x-patch; charset=UTF-8; name=0003-Mark-misc-static-global-variables-as-const.patchDownload
From da6f101b0ecc2d4e4b33bbcae316dbaf72e67d14 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:59:45 +0300
Subject: [PATCH 3/5] Mark misc static global variables as const
Discussion: https://www.postgresql.org/message-id/7f86e06a-98c5-4ce3-8ec9-3885c8de0358@iki.fi
---
contrib/btree_gist/btree_interval.c | 2 +-
contrib/oid2name/oid2name.c | 2 +-
src/backend/access/transam/xlogprefetcher.c | 2 +-
src/common/sha1.c | 2 +-
src/pl/plpython/plpy_cursorobject.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/contrib/btree_gist/btree_interval.c b/contrib/btree_gist/btree_interval.c
index b0afdf02bb..156f2cebac 100644
--- a/contrib/btree_gist/btree_interval.c
+++ b/contrib/btree_gist/btree_interval.c
@@ -113,7 +113,7 @@ static const gbtree_ninfo tinfo =
Interval *
abs_interval(Interval *a)
{
- static Interval zero = {0, 0, 0};
+ static const Interval zero = {0, 0, 0};
if (DatumGetBool(DirectFunctionCall2(interval_lt,
IntervalPGetDatum(a),
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index e8c1e2c97b..c2785848f5 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -62,7 +62,7 @@ void sql_exec_dumpalltbspc(PGconn *conn, struct options *opts);
void
get_opts(int argc, char **argv, struct options *my_opts)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"dbname", required_argument, NULL, 'd'},
{"host", required_argument, NULL, 'h'},
{"host", required_argument, NULL, 'H'}, /* deprecated */
diff --git a/src/backend/access/transam/xlogprefetcher.c b/src/backend/access/transam/xlogprefetcher.c
index 84023d61ba..2dc2fb760a 100644
--- a/src/backend/access/transam/xlogprefetcher.c
+++ b/src/backend/access/transam/xlogprefetcher.c
@@ -362,7 +362,7 @@ XLogPrefetcher *
XLogPrefetcherAllocate(XLogReaderState *reader)
{
XLogPrefetcher *prefetcher;
- static HASHCTL hash_table_ctl = {
+ const HASHCTL hash_table_ctl = {
.keysize = sizeof(RelFileLocator),
.entrysize = sizeof(XLogPrefetcherFilter)
};
diff --git a/src/common/sha1.c b/src/common/sha1.c
index 0525c4ff31..abfbd1c087 100644
--- a/src/common/sha1.c
+++ b/src/common/sha1.c
@@ -61,7 +61,7 @@
#include "sha1_int.h"
/* constant table */
-static uint32 _K[] = {0x5a827999, 0x6ed9eba1, 0x8f1bbcdc, 0xca62c1d6};
+static const uint32 _K[] = {0x5a827999, 0x6ed9eba1, 0x8f1bbcdc, 0xca62c1d6};
#define K(t) _K[(t) / 20]
diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c
index 57e8f8ec21..24f2ac8c46 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -27,7 +27,7 @@ static PyObject *PLy_cursor_iternext(PyObject *self);
static PyObject *PLy_cursor_fetch(PyObject *self, PyObject *args);
static PyObject *PLy_cursor_close(PyObject *self, PyObject *unused);
-static char PLy_cursor_doc[] = "Wrapper around a PostgreSQL cursor";
+static const char PLy_cursor_doc[] = "Wrapper around a PostgreSQL cursor";
static PyMethodDef PLy_cursor_methods[] = {
{"fetch", PLy_cursor_fetch, METH_VARARGS, NULL},
--
2.39.2
0004-Constify-fields-and-parameters-in-spell.c.patchtext/x-patch; charset=UTF-8; name=0004-Constify-fields-and-parameters-in-spell.c.patchDownload
From 5d562f15aaba0bb082e714e844995705f0ca1368 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:59:52 +0300
Subject: [PATCH 4/5] Constify fields and parameters in spell.c
I started by marking VoidString as const, and fixing the fallout by
marking more fields and function arguments as const. It proliferated
quite a lot, but all within spell.c and spell.h.
A more narrow patch to get rid of the static VoidString buffer would
be to replace it with '#define VoidString ""', as C99 allows assigning
"" to a non-const pointer, even though you're not allowed to modify
it. But it seems like good hygiene to mark all these as const. In the
structs, the pointers can point to the constant VoidString, or a
buffer allocated with palloc(), or with compact_palloc(), so you
should not modify them.
Discussion: https://www.postgresql.org/message-id/7f86e06a-98c5-4ce3-8ec9-3885c8de0358@iki.fi
---
src/backend/tsearch/spell.c | 58 +++++++++++++++++--------------
src/include/tsearch/dicts/spell.h | 16 ++++-----
2 files changed, 39 insertions(+), 35 deletions(-)
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index 9b1441fa1a..1aef9ca1ef 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -191,7 +191,7 @@ lowerstr_ctx(IspellDict *Conf, const char *src)
#define GETWCHAR(W,L,N,T) ( ((const uint8*)(W))[ ((T)==FF_PREFIX) ? (N) : ( (L) - 1 - (N) ) ] )
#define GETCHAR(A,N,T) GETWCHAR( (A)->repl, (A)->replen, N, T )
-static char *VoidString = "";
+static const char *VoidString = "";
static int
cmpspell(const void *s1, const void *s2)
@@ -346,11 +346,11 @@ cmpaffix(const void *s1, const void *s2)
* sflag: returns an affix flag from sflagset.
*/
static void
-getNextFlagFromString(IspellDict *Conf, char **sflagset, char *sflag)
+getNextFlagFromString(IspellDict *Conf, const char **sflagset, char *sflag)
{
int32 s;
- char *next,
- *sbuf = *sflagset;
+ char *next;
+ const char *sbuf = *sflagset;
int maxstep;
bool stop = false;
bool met_comma = false;
@@ -453,7 +453,7 @@ getNextFlagFromString(IspellDict *Conf, char **sflagset, char *sflag)
static bool
IsAffixFlagInUse(IspellDict *Conf, int affix, const char *affixflag)
{
- char *flagcur;
+ const char *flagcur;
char flag[BUFSIZ];
if (*affixflag == 0)
@@ -1120,13 +1120,13 @@ addCompoundAffixFlagValue(IspellDict *Conf, char *s, uint32 val)
* flags s.
*/
static int
-getCompoundAffixFlagValue(IspellDict *Conf, char *s)
+getCompoundAffixFlagValue(IspellDict *Conf, const char *s)
{
uint32 flag = 0;
CompoundAffixFlag *found,
key;
char sflag[BUFSIZ];
- char *flagcur;
+ const char *flagcur;
if (Conf->nCompoundAffixFlag == 0)
return 0;
@@ -1155,7 +1155,7 @@ getCompoundAffixFlagValue(IspellDict *Conf, char *s)
* Conf->AffixData array and function returns its entry.
* Else function returns the s parameter.
*/
-static char *
+static const char *
getAffixFlagSet(IspellDict *Conf, char *s)
{
if (Conf->useFlagAliases && *s != '\0')
@@ -1323,7 +1323,7 @@ NIImportOOAffixes(IspellDict *Conf, const char *filename)
/* Also reserve place for empty flag set */
naffix++;
- Conf->AffixData = (char **) palloc0(naffix * sizeof(char *));
+ Conf->AffixData = (const char **) palloc0(naffix * sizeof(char *));
Conf->lenAffixData = Conf->nAffixData = naffix;
/* Add empty flag set into AffixData */
@@ -1571,7 +1571,7 @@ isnewformat:
static int
MergeAffix(IspellDict *Conf, int a1, int a2)
{
- char **ptr;
+ const char **ptr;
Assert(a1 < Conf->nAffixData && a2 < Conf->nAffixData);
@@ -1585,24 +1585,28 @@ MergeAffix(IspellDict *Conf, int a1, int a2)
if (Conf->nAffixData + 1 >= Conf->lenAffixData)
{
Conf->lenAffixData *= 2;
- Conf->AffixData = (char **) repalloc(Conf->AffixData,
- sizeof(char *) * Conf->lenAffixData);
+ Conf->AffixData = (const char **) repalloc(Conf->AffixData,
+ sizeof(char *) * Conf->lenAffixData);
}
ptr = Conf->AffixData + Conf->nAffixData;
if (Conf->flagMode == FM_NUM)
{
- *ptr = cpalloc(strlen(Conf->AffixData[a1]) +
- strlen(Conf->AffixData[a2]) +
- 1 /* comma */ + 1 /* \0 */ );
- sprintf(*ptr, "%s,%s", Conf->AffixData[a1], Conf->AffixData[a2]);
+ char *p = cpalloc(strlen(Conf->AffixData[a1]) +
+ strlen(Conf->AffixData[a2]) +
+ 1 /* comma */ + 1 /* \0 */ );
+
+ sprintf(p, "%s,%s", Conf->AffixData[a1], Conf->AffixData[a2]);
+ *ptr = p;
}
else
{
- *ptr = cpalloc(strlen(Conf->AffixData[a1]) +
- strlen(Conf->AffixData[a2]) +
- 1 /* \0 */ );
- sprintf(*ptr, "%s%s", Conf->AffixData[a1], Conf->AffixData[a2]);
+ char *p = cpalloc(strlen(Conf->AffixData[a1]) +
+ strlen(Conf->AffixData[a2]) +
+ 1 /* \0 */ );
+
+ sprintf(p, "%s%s", Conf->AffixData[a1], Conf->AffixData[a2]);
+ *ptr = p;
}
ptr++;
*ptr = NULL;
@@ -1785,7 +1789,7 @@ NISortDictionary(IspellDict *Conf)
* dictionary. Replace textual flag-field of Conf->Spell entries with
* indexes into Conf->AffixData array.
*/
- Conf->AffixData = (char **) palloc0(naffix * sizeof(char *));
+ Conf->AffixData = (const char **) palloc0(naffix * sizeof(const char *));
curaffix = -1;
for (i = 0; i < Conf->nspell; i++)
@@ -1954,7 +1958,7 @@ mkVoidAffix(IspellDict *Conf, bool issuffix, int startsuffix)
* returns false.
*/
static bool
-isAffixInUse(IspellDict *Conf, char *affixflag)
+isAffixInUse(IspellDict *Conf, const char *affixflag)
{
int i;
@@ -2169,7 +2173,7 @@ addToResult(char **forms, char **cur, char *word)
}
static char **
-NormalizeSubWord(IspellDict *Conf, char *word, int flag)
+NormalizeSubWord(IspellDict *Conf, const char *word, int flag)
{
AffixNodeData *suffix = NULL,
*prefix = NULL;
@@ -2255,7 +2259,7 @@ NormalizeSubWord(IspellDict *Conf, char *word, int flag)
if (CheckAffix(newword, swrdlen, prefix->aff[j], flag, pnewword, &baselen))
{
/* prefix success */
- char *ff = (prefix->aff[j]->flagflags & suffix->aff[i]->flagflags & FF_CROSSPRODUCT) ?
+ const char *ff = (prefix->aff[j]->flagflags & suffix->aff[i]->flagflags & FF_CROSSPRODUCT) ?
VoidString : prefix->aff[j]->flag;
if (FindWord(Conf, pnewword, ff, flag))
@@ -2287,7 +2291,7 @@ typedef struct SplitVar
} SplitVar;
static int
-CheckCompoundAffixes(CMPDAffix **ptr, char *word, int len, bool CheckInPlace)
+CheckCompoundAffixes(CMPDAffix **ptr, const char *word, int len, bool CheckInPlace)
{
bool issuffix;
@@ -2367,7 +2371,7 @@ AddStem(SplitVar *v, char *word)
}
static SplitVar *
-SplitToVariants(IspellDict *Conf, SPNode *snode, SplitVar *orig, char *word, int wordlen, int startpos, int minpos)
+SplitToVariants(IspellDict *Conf, SPNode *snode, SplitVar *orig, const char *word, int wordlen, int startpos, int minpos)
{
SplitVar *var = NULL;
SPNodeData *StopLow,
@@ -2533,7 +2537,7 @@ addNorm(TSLexeme **lres, TSLexeme **lcur, char *word, int flags, uint16 NVariant
}
TSLexeme *
-NINormalizeWord(IspellDict *Conf, char *word)
+NINormalizeWord(IspellDict *Conf, const char *word)
{
char **res;
TSLexeme *lcur = NULL,
diff --git a/src/include/tsearch/dicts/spell.h b/src/include/tsearch/dicts/spell.h
index 10c05769ac..1b881b8320 100644
--- a/src/include/tsearch/dicts/spell.h
+++ b/src/include/tsearch/dicts/spell.h
@@ -66,7 +66,7 @@ typedef struct spell_struct
* flag is filled in by NIImportDictionary(). After
* NISortDictionary(), d is used instead of flag.
*/
- char *flag;
+ const char *flag;
/* d is used in mkSPNode() */
struct
{
@@ -86,15 +86,15 @@ typedef struct spell_struct
*/
typedef struct aff_struct
{
- char *flag;
+ const char *flag;
/* FF_SUFFIX or FF_PREFIX */
uint32 type:1,
flagflags:7,
issimple:1,
isregis:1,
replen:14;
- char *find;
- char *repl;
+ const char *find;
+ const char *repl;
union
{
/*
@@ -146,7 +146,7 @@ typedef struct AffixNode
typedef struct
{
- char *affix;
+ const char *affix;
int len;
bool issuffix;
} CMPDAffix;
@@ -170,7 +170,7 @@ typedef struct CompoundAffixFlag
union
{
/* Flag name if flagMode is FM_CHAR or FM_LONG */
- char *s;
+ const char *s;
/* Flag name if flagMode is FM_NUM */
uint32 i;
} flag;
@@ -192,7 +192,7 @@ typedef struct
SPNode *Dictionary;
/* Array of sets of affixes */
- char **AffixData;
+ const char **AffixData;
int lenAffixData;
int nAffixData;
bool useFlagAliases;
@@ -229,7 +229,7 @@ typedef struct
size_t avail; /* free space remaining at firstfree */
} IspellDict;
-extern TSLexeme *NINormalizeWord(IspellDict *Conf, char *word);
+extern TSLexeme *NINormalizeWord(IspellDict *Conf, const char *word);
extern void NIStartBuild(IspellDict *Conf);
extern void NIImportAffixes(IspellDict *Conf, const char *filename);
--
2.39.2
0005-Use-psprintf-to-simplify-gtsvectorout.patchtext/x-patch; charset=UTF-8; name=0005-Use-psprintf-to-simplify-gtsvectorout.patchDownload
From bb66efccf4f97d0001b730a1376845c0a19c7f27 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 18:00:01 +0300
Subject: [PATCH 5/5] Use psprintf to simplify gtsvectorout()
The buffer allocation was correct, but looked archaic and scary:
- It was weird to calculate the buffer size before determining which
format string was used. With the same effort, we could've used the
right-sized buffer for each branch.
- Commit aa0d3504560 added one more possible return string ("all true
bits"), but didn't adjust the code at the top of the function to
calculate the returned string's max size. It was not a live bug,
because the new string was smaller than the existing ones, but
seemed wrong in principle.
- Use of sprintf() is generally eyebrow-raising these days
Switch to psprintf(). psprintf() allocates a larger buffer than what
was allocated before, 128 bytes vs 80 bytes, which is acceptable as
this code is not performance or space critical.
Discussion: https://www.postgresql.org/message-id/7f86e06a-98c5-4ce3-8ec9-3885c8de0358@iki.fi
---
src/backend/utils/adt/tsgistidx.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/src/backend/utils/adt/tsgistidx.c b/src/backend/utils/adt/tsgistidx.c
index 5698ee5502..1438ff3520 100644
--- a/src/backend/utils/adt/tsgistidx.c
+++ b/src/backend/utils/adt/tsgistidx.c
@@ -96,34 +96,25 @@ gtsvectorin(PG_FUNCTION_ARGS)
PG_RETURN_VOID(); /* keep compiler quiet */
}
-#define SINGOUTSTR "%d true bits, %d false bits"
-#define ARROUTSTR "%d unique words"
-#define EXTRALEN ( 2*13 )
-
-static int outbuf_maxlen = 0;
-
Datum
gtsvectorout(PG_FUNCTION_ARGS)
{
SignTSVector *key = (SignTSVector *) PG_DETOAST_DATUM(PG_GETARG_DATUM(0));
char *outbuf;
- if (outbuf_maxlen == 0)
- outbuf_maxlen = 2 * EXTRALEN + Max(strlen(SINGOUTSTR), strlen(ARROUTSTR)) + 1;
- outbuf = palloc(outbuf_maxlen);
-
if (ISARRKEY(key))
- sprintf(outbuf, ARROUTSTR, (int) ARRNELEM(key));
+ outbuf = psprintf("%d unique words", (int) ARRNELEM(key));
else
{
if (ISALLTRUE(key))
- sprintf(outbuf, "all true bits");
+ outbuf = pstrdup("all true bits");
else
{
int siglen = GETSIGLEN(key);
int cnttrue = sizebitvec(GETSIGN(key), siglen);
- sprintf(outbuf, SINGOUTSTR, cnttrue, (int) SIGLENBIT(siglen) - cnttrue);
+ outbuf = psprintf("%d true bits, %d false bits",
+ cnttrue, (int) SIGLENBIT(siglen) - cnttrue);
}
}
--
2.39.2
Re: Minor refactorings to eliminate some static buffers
Hi,
On 2024-08-06 18:13:56 +0300, Heikki Linnakangas wrote:
From 6dd5a4a413212a61d9a4f5b9db73e812c8b5dcbd Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:58:29 +0300
Subject: [PATCH 1/5] Turn a few 'validnsps' static variables into localsThere was no need for these to be static buffers, a local variable
works just as well. I think they were marked as 'static' to imply that
they are read-only, but 'const' is more appropriate for that, so
change them to const.
I looked at these at some point in the past. Unfortunately compilers don't
always generate better code with const than static :( (the static
initialization can be done once in a global var, the const one not
necessarily). Arguably what you'd want here is both.
I doubt that matters here though.
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 702a6c3a0b..2732d6bfc9 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1155,7 +1155,7 @@ ProcessUtilitySlow(ParseState *pstate, { CreateStmt *cstmt = (CreateStmt *) stmt; Datum toast_options; - static char *validnsps[] = HEAP_RELOPT_NAMESPACES; + const char *validnsps[] = HEAP_RELOPT_NAMESPACES;/* Remember transformed RangeVar for LIKE */
table_rv = cstmt->relation;
In the other places you used "const char * const", here just "const char *" - it
doesn't look like that's a required difference?
From f108ae4c2ddfa6ca77e9736dc3fb20e6bda7b67c Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:59:33 +0300
Subject: [PATCH 2/5] Make nullSemAction const, add 'const' decorators to
related functions
To make it more clear that these should never be modified.
Yep - and it reduces the size of writable mappings to boot.
LGTM.
From da6f101b0ecc2d4e4b33bbcae316dbaf72e67d14 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:59:45 +0300
Subject: [PATCH 3/5] Mark misc static global variables as const
LGTM
From 5d562f15aaba0bb082e714e844995705f0ca1368 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 17:59:52 +0300
Subject: [PATCH 4/5] Constify fields and parameters in spell.cI started by marking VoidString as const, and fixing the fallout by
marking more fields and function arguments as const. It proliferated
quite a lot, but all within spell.c and spell.h.A more narrow patch to get rid of the static VoidString buffer would
be to replace it with '#define VoidString ""', as C99 allows assigning
"" to a non-const pointer, even though you're not allowed to modify
it. But it seems like good hygiene to mark all these as const. In the
structs, the pointers can point to the constant VoidString, or a
buffer allocated with palloc(), or with compact_palloc(), so you
should not modify them.
Looks reasonable to me.
From bb66efccf4f97d0001b730a1376845c0a19c7f27 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 6 Aug 2024 18:00:01 +0300
Subject: [PATCH 5/5] Use psprintf to simplify gtsvectorout()The buffer allocation was correct, but looked archaic and scary:
- It was weird to calculate the buffer size before determining which
format string was used. With the same effort, we could've used the
right-sized buffer for each branch.- Commit aa0d3504560 added one more possible return string ("all true
bits"), but didn't adjust the code at the top of the function to
calculate the returned string's max size. It was not a live bug,
because the new string was smaller than the existing ones, but
seemed wrong in principle.- Use of sprintf() is generally eyebrow-raising these days
Switch to psprintf(). psprintf() allocates a larger buffer than what
was allocated before, 128 bytes vs 80 bytes, which is acceptable as
this code is not performance or space critical.
I find the undocumented EXTRALEN the most confusing :)
LGTM.
Greetings,
Andres Freund
Re: Minor refactorings to eliminate some static buffers
On 06/08/2024 18:52, Andres Freund wrote:
On 2024-08-06 18:13:56 +0300, Heikki Linnakangas wrote:
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 702a6c3a0b..2732d6bfc9 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1155,7 +1155,7 @@ ProcessUtilitySlow(ParseState *pstate, { CreateStmt *cstmt = (CreateStmt *) stmt; Datum toast_options; - static char *validnsps[] = HEAP_RELOPT_NAMESPACES; + const char *validnsps[] = HEAP_RELOPT_NAMESPACES;/* Remember transformed RangeVar for LIKE */
table_rv = cstmt->relation;In the other places you used "const char * const", here just "const char *" - it
doesn't look like that's a required difference?
Just an oversight.
I went back and forth on whether to use plain "char *validnps[]", "const
char *validnsps[]" or "const char *const validnsps[]". The first form
doesn't convey the fact it's read-only, like the "static" used to. The
second form hints that, but only for the strings, not for the pointers
in the array. The last form is what we want, but it's a bit verbose and
ugly. I chose the last form in the end, but missed this one.
Fixed that and pushed. Thanks!
--
Heikki Linnakangas
Neon (https://neon.tech)
Re: Minor refactorings to eliminate some static buffers
On 06.08.24 17:13, Heikki Linnakangas wrote:
--- a/src/backend/access/transam/xlogprefetcher.c +++ b/src/backend/access/transam/xlogprefetcher.c @@ -362,7 +362,7 @@ XLogPrefetcher * XLogPrefetcherAllocate(XLogReaderState *reader) { XLogPrefetcher *prefetcher; - static HASHCTL hash_table_ctl = { + const HASHCTL hash_table_ctl = {
Is there a reason this is not changed to
static const HASHCTL ...
? Most other places where changed in that way.
Re: Minor refactorings to eliminate some static buffers
On 06/08/2024 23:41, Peter Eisentraut wrote:
On 06.08.24 17:13, Heikki Linnakangas wrote:
--- a/src/backend/access/transam/xlogprefetcher.c +++ b/src/backend/access/transam/xlogprefetcher.c @@ -362,7 +362,7 @@ XLogPrefetcher * XLogPrefetcherAllocate(XLogReaderState *reader) { XLogPrefetcher *prefetcher; - static HASHCTL hash_table_ctl = { + const HASHCTL hash_table_ctl = {Is there a reason this is not changed to
static const HASHCTL ...
? Most other places where changed in that way.
No particular reason. Grepping for HASHCTL's, this is actually different
from all other uses of HASHCTL and hash_create. All others use a plain
local variable, and fill the fields like this:
HASHCTL hash_ctl;
hash_ctl.keysize = sizeof(missing_cache_key);
hash_ctl.entrysize = sizeof(missing_cache_key);
hash_ctl.hcxt = TopMemoryContext;
hash_ctl.hash = missing_hash;
hash_ctl.match = missing_match;
I think that's just because we haven't allowed C99 designated
initializers for very long.
--
Heikki Linnakangas
Neon (https://neon.tech)