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

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)

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)

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

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

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

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)

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.

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)