Re: Minor refactorings to eliminate some static buffers - Mailing list pgsql-hackers

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