Thread: Minor refactorings to eliminate some static buffers
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)
Attachment
- v1-0001-Replace-static-bufs-with-StringInfo-in-cash_words.patch
- v1-0002-Replace-static-buf-with-palloc-in-str_time.patch
- v1-0003-Replace-static-buf-with-a-stack-allocated-one-in-.patch
- v1-0004-Replace-static-buf-with-a-stack-allocated-one-in-.patch
- v1-0005-Refactor-getWeights-to-write-to-caller-supplied-b.patch
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)
Attachment
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)