plenty code is confused about function level static
Hi,
We have a fair amount of code that uses non-constant function level static
variables for read-only data. Which makes little sense - it prevents the
compiler from understanding
a) that the data is read only and can thus be put into a segment that's shared
between all invocations of the program
b) the data will be the same on every invocation, and thus from optimizing
based on that.
The most common example of this is that all our binaries use
static struct option long_options[] = { ... };
which prevents long_options from being put into read-only memory.
Is there some reason we went for this pattern in a fair number of places? I
assume it's mostly copy-pasta, but...
In practice it often is useful to use 'static const' instead of just
'const'. At least gcc otherwise soemtimes fills the data on the stack, instead
of having a read-only data member that's already initialized. I'm not sure
why, tbh.
Attached are fixes for struct option and a few more occurrences I've found
with a bit of grepping.
There are lots of places that could benefit from adding 'static
const'.
E.g. most, if not all, HASHCTL's should be that, but that's more verbose to
change, so I didn't do that.
Greetings,
Andres Freund
Attachments:
v1-0001-static-const-Convert-struct-option-arrays.patchtext/x-diff; charset=us-asciiDownload
From d43a10b5ba46b010c8e075b1062f9f30eb013498 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 17 Apr 2024 14:27:03 -0700
Subject: [PATCH v1 1/3] static const: Convert struct option arrays
---
src/bin/initdb/initdb.c | 2 +-
src/bin/pg_amcheck/pg_amcheck.c | 2 +-
src/bin/pg_archivecleanup/pg_archivecleanup.c | 2 +-
src/bin/pg_basebackup/pg_basebackup.c | 2 +-
src/bin/pg_basebackup/pg_createsubscriber.c | 2 +-
src/bin/pg_basebackup/pg_receivewal.c | 2 +-
src/bin/pg_basebackup/pg_recvlogical.c | 2 +-
src/bin/pg_checksums/pg_checksums.c | 2 +-
src/bin/pg_combinebackup/pg_combinebackup.c | 2 +-
src/bin/pg_controldata/pg_controldata.c | 2 +-
src/bin/pg_ctl/pg_ctl.c | 2 +-
src/bin/pg_dump/pg_dump.c | 2 +-
src/bin/pg_dump/pg_dumpall.c | 2 +-
src/bin/pg_resetwal/pg_resetwal.c | 2 +-
src/bin/pg_rewind/pg_rewind.c | 2 +-
src/bin/pg_test_fsync/pg_test_fsync.c | 2 +-
src/bin/pg_test_timing/pg_test_timing.c | 2 +-
src/bin/pg_upgrade/option.c | 2 +-
src/bin/pg_verifybackup/pg_verifybackup.c | 2 +-
src/bin/pg_waldump/pg_waldump.c | 2 +-
src/bin/pg_walsummary/pg_walsummary.c | 2 +-
src/bin/pgbench/pgbench.c | 2 +-
src/bin/psql/startup.c | 2 +-
src/bin/scripts/clusterdb.c | 2 +-
src/bin/scripts/createdb.c | 2 +-
src/bin/scripts/createuser.c | 2 +-
src/bin/scripts/dropdb.c | 2 +-
src/bin/scripts/dropuser.c | 2 +-
src/bin/scripts/pg_isready.c | 2 +-
src/bin/scripts/reindexdb.c | 2 +-
src/bin/scripts/vacuumdb.c | 2 +-
contrib/btree_gist/btree_interval.c | 2 +-
contrib/oid2name/oid2name.c | 2 +-
contrib/vacuumlo/vacuumlo.c | 2 +-
src/interfaces/ecpg/preproc/ecpg.c | 2 +-
src/test/regress/pg_regress.c | 2 +-
36 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 30e17bd1d1e..5d4044d5a90 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -3093,7 +3093,7 @@ initialize_data_directory(void)
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
{"encoding", required_argument, NULL, 'E'},
{"locale", required_argument, NULL, 1},
diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 7e3101704d4..bb100022a0d 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -233,7 +233,7 @@ main(int argc, char *argv[])
uint64 relprogress = 0;
int pattern_id;
- static struct option long_options[] = {
+ static const struct option long_options[] = {
/* Connection options */
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 07bf356b70c..6cdf471597e 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -285,7 +285,7 @@ usage(void)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"clean-backup-history", no_argument, NULL, 'b'},
{"debug", no_argument, NULL, 'd'},
{"dry-run", no_argument, NULL, 'n'},
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 8f3dd04fd22..0220614993f 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2354,7 +2354,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
{"pgdata", required_argument, NULL, 'D'},
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 90cc580811d..cbb4b92be87 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -1754,7 +1754,7 @@ enable_subscription(PGconn *conn, const struct LogicalRepInfo *dbinfo)
int
main(int argc, char **argv)
{
- static struct option long_options[] =
+ static const struct option long_options[] =
{
{"database", required_argument, NULL, 'd'},
{"pgdata", required_argument, NULL, 'D'},
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 555f0175f0e..857460e7977 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -623,7 +623,7 @@ sigexit_handler(SIGNAL_ARGS)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
{"directory", required_argument, NULL, 'D'},
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index 3db520ed38b..fb15a09f72d 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -691,7 +691,7 @@ sighup_handler(SIGNAL_ARGS)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
/* general options */
{"file", required_argument, NULL, 'f'},
{"fsync-interval", required_argument, NULL, 'F'},
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 9e6fd435f60..4f49b3a74a6 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -432,7 +432,7 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"check", no_argument, NULL, 'c'},
{"pgdata", required_argument, NULL, 'D'},
{"disable", no_argument, NULL, 'd'},
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 2788c78fdd4..080c9244e8a 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -121,7 +121,7 @@ static void slurp_file(int fd, char *filename, StringInfo buf, int maxlen);
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"debug", no_argument, NULL, 'd'},
{"dry-run", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 93e0837947c..b4117e5d990 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -88,7 +88,7 @@ wal_level_str(WalLevel wal_level)
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
{NULL, 0, NULL, 0}
};
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 405e223c190..5d18781eaf4 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -2189,7 +2189,7 @@ get_control_dbstate(void)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},
{"log", required_argument, NULL, 'l'},
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index c52e961b309..63f941ec344 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -374,7 +374,7 @@ main(int argc, char **argv)
static DumpOptions dopt;
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"data-only", no_argument, NULL, 'a'},
{"blobs", no_argument, NULL, 'b'},
{"large-objects", no_argument, NULL, 'b'},
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 73337f33923..0bcc4079547 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -126,7 +126,7 @@ static SimpleStringList database_exclude_names = {NULL, NULL};
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"data-only", no_argument, NULL, 'a'},
{"clean", no_argument, NULL, 'c'},
{"encoding", required_argument, NULL, 'E'},
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index e9dcb5a6d89..184cab2a842 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -93,7 +93,7 @@ static void usage(void);
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"commit-timestamp-ids", required_argument, NULL, 'c'},
{"pgdata", required_argument, NULL, 'D'},
{"epoch", required_argument, NULL, 'e'},
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 8449ae78ef7..675cb5e739c 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -120,7 +120,7 @@ usage(const char *progname)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"target-pgdata", required_argument, NULL, 'D'},
{"write-recovery-conf", no_argument, NULL, 'R'},
diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 5c0da425fbb..071a3bc2599 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -147,7 +147,7 @@ main(int argc, char *argv[])
static void
handle_args(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"filename", required_argument, NULL, 'f'},
{"secs-per-test", required_argument, NULL, 's'},
{NULL, 0, NULL, 0}
diff --git a/src/bin/pg_test_timing/pg_test_timing.c b/src/bin/pg_test_timing/pg_test_timing.c
index c29d6f87629..3996889d2fd 100644
--- a/src/bin/pg_test_timing/pg_test_timing.c
+++ b/src/bin/pg_test_timing/pg_test_timing.c
@@ -42,7 +42,7 @@ main(int argc, char *argv[])
static void
handle_args(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"duration", required_argument, NULL, 'd'},
{NULL, 0, NULL, 0}
};
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 548ea4e6236..c841ddea776 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -38,7 +38,7 @@ UserOpts user_opts;
void
parseCommandLine(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"old-datadir", required_argument, NULL, 'd'},
{"new-datadir", required_argument, NULL, 'D'},
{"old-bindir", required_argument, NULL, 'b'},
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index fd610c20a65..b34c0bd6bde 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -176,7 +176,7 @@ static uint64 done_size = 0;
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"exit-on-error", no_argument, NULL, 'e'},
{"ignore", required_argument, NULL, 'i'},
{"manifest-path", required_argument, NULL, 'm'},
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 1f9403fc5cf..321299c0e00 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -802,7 +802,7 @@ main(int argc, char **argv)
char *waldir = NULL;
char *errormsg;
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"bkp-details", no_argument, NULL, 'b'},
{"block", required_argument, NULL, 'B'},
{"end", required_argument, NULL, 'e'},
diff --git a/src/bin/pg_walsummary/pg_walsummary.c b/src/bin/pg_walsummary/pg_walsummary.c
index daf6cd14ce7..9a5785bbefc 100644
--- a/src/bin/pg_walsummary/pg_walsummary.c
+++ b/src/bin/pg_walsummary/pg_walsummary.c
@@ -52,7 +52,7 @@ static void walsummary_error_callback(void *callback_arg, char *fmt,...) pg_attr
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"individual", no_argument, NULL, 'i'},
{"quiet", no_argument, NULL, 'q'},
{NULL, 0, NULL, 0}
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index af776b31d8f..1e99f6c31e1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -6616,7 +6616,7 @@ set_random_seed(const char *seed)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
/* systematic long/short named options */
{"builtin", required_argument, NULL, 'b'},
{"client", required_argument, NULL, 'c'},
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 036caaec2ff..b22a1cf8c52 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -482,7 +482,7 @@ error:
static void
parse_psql_options(int argc, char *argv[], struct adhoc_opts *options)
{
- static struct option long_options[] =
+ static const struct option long_options[] =
{
{"echo-all", no_argument, NULL, 'a'},
{"no-align", no_argument, NULL, 'A'},
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index 3503a3bb584..90810946c62 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -30,7 +30,7 @@ static void help(const char *progname);
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
{"username", required_argument, NULL, 'U'},
diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c
index 007061e756f..55f1be58a9e 100644
--- a/src/bin/scripts/createdb.c
+++ b/src/bin/scripts/createdb.c
@@ -23,7 +23,7 @@ static void help(const char *progname);
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
{"username", required_argument, NULL, 'U'},
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index f5f03c1145f..4b880815fba 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -27,7 +27,7 @@ static void help(const char *progname);
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"with-admin", required_argument, NULL, 'a'},
{"connection-limit", required_argument, NULL, 'c'},
{"createdb", no_argument, NULL, 'd'},
diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
index 57656ba8988..bf896a2919b 100644
--- a/src/bin/scripts/dropdb.c
+++ b/src/bin/scripts/dropdb.c
@@ -25,7 +25,7 @@ main(int argc, char *argv[])
{
static int if_exists = 0;
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
{"username", required_argument, NULL, 'U'},
diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c
index 233ccd288b7..723358b5470 100644
--- a/src/bin/scripts/dropuser.c
+++ b/src/bin/scripts/dropuser.c
@@ -26,7 +26,7 @@ main(int argc, char *argv[])
{
static int if_exists = 0;
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
{"username", required_argument, NULL, 'U'},
diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c
index 100589da147..f56532678be 100644
--- a/src/bin/scripts/pg_isready.c
+++ b/src/bin/scripts/pg_isready.c
@@ -55,7 +55,7 @@ main(int argc, char **argv)
* connecting with invalid params
*/
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"dbname", required_argument, NULL, 'd'},
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 904c196fbba..85ba625b73d 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -60,7 +60,7 @@ static void help(const char *progname);
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
{"username", required_argument, NULL, 'U'},
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 7138c6e97e4..169a5960ae0 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -96,7 +96,7 @@ static char *escape_quotes(const char *src);
int
main(int argc, char *argv[])
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
{"username", required_argument, NULL, 'U'},
diff --git a/contrib/btree_gist/btree_interval.c b/contrib/btree_gist/btree_interval.c
index b0afdf02bb5..156f2cebac5 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 e8c1e2c97bd..c2785848f55 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/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 0d99428dec6..b5ba5acd67d 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -436,7 +436,7 @@ usage(const char *progname)
int
main(int argc, char **argv)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"host", required_argument, NULL, 'h'},
{"limit", required_argument, NULL, 'l'},
{"dry-run", no_argument, NULL, 'n'},
diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c
index 73c37631acc..3f744e7995d 100644
--- a/src/interfaces/ecpg/preproc/ecpg.c
+++ b/src/interfaces/ecpg/preproc/ecpg.c
@@ -128,7 +128,7 @@ add_preprocessor_define(char *define)
int
main(int argc, char *const argv[])
{
- static struct option ecpg_options[] = {
+ static const struct option ecpg_options[] = {
{"regression", no_argument, NULL, ECPG_GETOPT_LONG_REGRESSION},
{NULL, 0, NULL, 0}
};
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 06f6775fc65..f0693463ee0 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -2066,7 +2066,7 @@ regression_main(int argc, char *argv[],
test_start_function startfunc,
postprocess_result_function postfunc)
{
- static struct option long_options[] = {
+ static const struct option long_options[] = {
{"help", no_argument, NULL, 'h'},
{"version", no_argument, NULL, 'V'},
{"dbname", required_argument, NULL, 1},
--
2.44.0.279.g3bd955d269
v1-0002-static-const-Convert-transformRelOptions-callers.patchtext/x-diff; charset=us-asciiDownload
From da0e1c728d263caf038fb117f280397389ba4c63 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 17 Apr 2024 14:28:05 -0700
Subject: [PATCH v1 2/3] static const: Convert transformRelOptions callers
Note this is an API change.
---
src/include/access/reloptions.h | 2 +-
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 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 81829b8270a..0e6b8ec142d 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 *validnsps[],
bool acceptOidsOff, bool isReset);
extern List *untransformRelOptions(Datum options);
extern bytea *extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d6eb5d85599..ddb18f71d5b 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 *validnsps[], bool acceptOidsOff, bool isReset)
{
Datum result;
ArrayBuildState *astate;
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 62050f4dc59..b4f2b7c162f 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;
+ static const char *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 027d68e5d2a..19091e48aa0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -715,7 +715,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
ListCell *listptr;
AttrNumber attnum;
bool partitioned;
- static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
+ static const char *validnsps[] = HEAP_RELOPT_NAMESPACES;
Oid ofTypeId;
ObjectAddress address;
LOCKMODE parentLockmode;
@@ -15531,7 +15531,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;
+ static const char *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 fa66b8017ed..67b8ba0262a 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;
+ static const char *validnsps[] = HEAP_RELOPT_NAMESPACES;
/* Remember transformed RangeVar for LIKE */
table_rv = cstmt->relation;
--
2.44.0.279.g3bd955d269
v1-0003-static-const-Convert-some-more.patchtext/x-diff; charset=us-asciiDownload
From 529b4438213cdd1805436b47b168a34d7a3a610d Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 17 Apr 2024 14:28:47 -0700
Subject: [PATCH v1 3/3] static const: Convert some more
---
src/backend/access/transam/xlogprefetcher.c | 2 +-
src/backend/catalog/pg_attrdef.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/xlogprefetcher.c b/src/backend/access/transam/xlogprefetcher.c
index fc80c37e554..6c93107608e 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 = {
+ static const HASHCTL hash_table_ctl = {
.keysize = sizeof(RelFileLocator),
.entrysize = sizeof(XLogPrefetcherFilter)
};
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index 003ae70b4d2..95887113539 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -50,7 +50,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
Relation adrel;
HeapTuple tuple;
Datum values[4];
- static bool nulls[4] = {false, false, false, false};
+ static const bool nulls[4] = {false, false, false, false};
Relation attrrel;
HeapTuple atttup;
Form_pg_attribute attStruct;
--
2.44.0.279.g3bd955d269
On 18/04/2024 00:39, Andres Freund wrote:
Hi,
We have a fair amount of code that uses non-constant function level static
variables for read-only data. Which makes little sense - it prevents the
compiler from understandinga) that the data is read only and can thus be put into a segment that's shared
between all invocations of the program
b) the data will be the same on every invocation, and thus from optimizing
based on that.The most common example of this is that all our binaries use
static struct option long_options[] = { ... };
which prevents long_options from being put into read-only memory.Is there some reason we went for this pattern in a fair number of places? I
assume it's mostly copy-pasta, but...In practice it often is useful to use 'static const' instead of just
'const'. At least gcc otherwise soemtimes fills the data on the stack, instead
of having a read-only data member that's already initialized. I'm not sure
why, tbh.
Weird. I guess it can be faster if you assume the data in the read-only
section might not be in cache, but the few instructions needed to fill
the data locally in stack are.
Attached are fixes for struct option and a few more occurrences I've found
with a bit of grepping.
+1
--
Heikki Linnakangas
Neon (https://neon.tech)
On 17.04.24 23:39, Andres Freund wrote:
Is there some reason we went for this pattern in a fair number of places? I
assume it's mostly copy-pasta, but...
Right. I don't think it is commonly understood that adding const
qualifiers can help compiler optimization, and it's difficult to
systematically check for omissions or verify the optimization effects.
So I think we just have to keep trying to do our best manually for now.
Attached are fixes for struct option and a few more occurrences I've found
with a bit of grepping.
These look good to me.
On 18 Apr 2024, at 02:39, Andres Freund <andres@anarazel.de> wrote:
There are lots of places that could benefit from adding 'static
const'.
+1 for helping compiler.
GCC has a -Wsuggest-attribute=const, we can count these warnings and threat increase as an error :)
Best regards, Andrey Borodin.
On 18.04.24 10:43, Andrey M. Borodin wrote:
On 18 Apr 2024, at 02:39, Andres Freund <andres@anarazel.de> wrote:
There are lots of places that could benefit from adding 'static
const'.+1 for helping compiler.
GCC has a -Wsuggest-attribute=const, we can count these warnings and threat increase as an error :)
This is different. It's an attribute, not a qualifier, and it's for
functions, not variables. But it could undoubtedly also have a
performance benefit.
On 18/04/2024 00:39, Andres Freund wrote:
We have a fair amount of code that uses non-constant function level static
variables for read-only data. Which makes little sense - it prevents the
compiler from understanding
a) that the data is read only and can thus be put into a segment that's
shared
between all invocations of the program
b) the data will be the same on every invocation, and thus from optimizing
based on that.
The most common example of this is that all our binaries use
static struct option long_options[] = { ... };
which prevents long_options from being put into read-only memory.
+1 static const allows the compiler to make additional optimizations.
There are lots of places that could benefit from adding 'static
const'.
I found a few more places.
Patch 004
The opposite would also help, adding static.
In these places, I believe it is safe to add static,
allowing the compiler to transform into read-only, definitively.
Patch 005
best regards,
Ranier Vilela
Attachments:
0004-static-const-convert-plpython.patchapplication/octet-stream; name=0004-static-const-convert-plpython.patchDownload
diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c
index 57e8f8ec21..77c5c6e070 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -29,7 +29,7 @@ static PyObject *PLy_cursor_close(PyObject *self, PyObject *unused);
static char PLy_cursor_doc[] = "Wrapper around a PostgreSQL cursor";
-static PyMethodDef PLy_cursor_methods[] = {
+static const PyMethodDef PLy_cursor_methods[] = {
{"fetch", PLy_cursor_fetch, METH_VARARGS, NULL},
{"close", PLy_cursor_close, METH_NOARGS, NULL},
{NULL, NULL, 0, NULL}
diff --git a/src/pl/plpython/plpy_planobject.c b/src/pl/plpython/plpy_planobject.c
index ec2439c6a1..c3e7ed19bd 100644
--- a/src/pl/plpython/plpy_planobject.c
+++ b/src/pl/plpython/plpy_planobject.c
@@ -20,7 +20,7 @@ static PyObject *PLy_plan_status(PyObject *self, PyObject *args);
static char PLy_plan_doc[] = "Store a PostgreSQL plan";
-static PyMethodDef PLy_plan_methods[] = {
+static const PyMethodDef PLy_plan_methods[] = {
{"cursor", PLy_plan_cursor, METH_VARARGS, NULL},
{"execute", PLy_plan_execute, METH_VARARGS, NULL},
{"status", PLy_plan_status, METH_VARARGS, NULL},
diff --git a/src/pl/plpython/plpy_subxactobject.c b/src/pl/plpython/plpy_subxactobject.c
index 5c92a0e089..44cab89909 100644
--- a/src/pl/plpython/plpy_subxactobject.c
+++ b/src/pl/plpython/plpy_subxactobject.c
@@ -22,7 +22,7 @@ static PyObject *PLy_subtransaction_exit(PyObject *self, PyObject *args);
static char PLy_subtransaction_doc[] =
"PostgreSQL subtransaction context manager";
-static PyMethodDef PLy_subtransaction_methods[] = {
+static const PyMethodDef PLy_subtransaction_methods[] = {
{"__enter__", PLy_subtransaction_enter, METH_VARARGS, NULL},
{"__exit__", PLy_subtransaction_exit, METH_VARARGS, NULL},
/* user-friendly names for Python <2.6 */
0005-const-convert-static-const.patchapplication/octet-stream; name=0005-const-convert-static-const.patchDownload
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 37628a85c7..94422fd1ad 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -207,7 +207,7 @@ foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
}
# Create the fmgr_builtins table, collect data for fmgr_builtin_oid_index
-print $tfh "\nconst FmgrBuiltin fmgr_builtins[] = {\n";
+print $tfh "\nstatic const FmgrBuiltin fmgr_builtins[] = {\n";
my %bmap;
$bmap{'t'} = 'true';
$bmap{'f'} = 'false';
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 1b86fff2fb..1c826bae15 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -72,16 +72,16 @@ static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
DateTimeErrorExtra *extra);
-const int day_tab[2][13] =
+static const int day_tab[2][13] =
{
{31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31, 0},
{31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31, 0}
};
-const char *const months[] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun",
+static const char *const months[] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun",
"Jul", "Aug", "Sep", "Oct", "Nov", "Dec", NULL};
-const char *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
+static const char *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
"Thursday", "Friday", "Saturday", NULL};
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index 13009cc3d0..bbbcad8d44 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -26,7 +26,7 @@
* in the docs for the pg_locks view and update the WaitEventLOCK section in
* src/backend/utils/activity/wait_event_names.txt.
*/
-const char *const LockTagTypeNames[] = {
+static const char *const LockTagTypeNames[] = {
"relation",
"extend",
"frozenid",
diff --git a/src/common/wchar.c b/src/common/wchar.c
index 76b7dfdfcb..a1fe7c6dc7 100644
--- a/src/common/wchar.c
+++ b/src/common/wchar.c
@@ -2012,7 +2012,7 @@ pg_utf8_islegal(const unsigned char *source, int length)
* encoding info table
*-------------------------------------------------------------------
*/
-const pg_wchar_tbl pg_wchar_table[] = {
+static const pg_wchar_tbl pg_wchar_table[] = {
[PG_SQL_ASCII] = {pg_ascii2wchar_with_len, pg_wchar2single_with_len, pg_ascii_mblen, pg_ascii_dsplen, pg_ascii_verifychar, pg_ascii_verifystr, 1},
[PG_EUC_JP] = {pg_eucjp2wchar_with_len, pg_wchar2euc_with_len, pg_eucjp_mblen, pg_eucjp_dsplen, pg_eucjp_verifychar, pg_eucjp_verifystr, 3},
[PG_EUC_CN] = {pg_euccn2wchar_with_len, pg_wchar2euc_with_len, pg_euccn_mblen, pg_euccn_dsplen, pg_euccn_verifychar, pg_euccn_verifystr, 2},Import Notes
Resolved by subject fallback
Hi,
On 2024-04-18 10:33:30 +0200, Peter Eisentraut wrote:
Attached are fixes for struct option and a few more occurrences I've found
with a bit of grepping.These look good to me.
Thoughts about when to apply these? Arguably they're fixing mildly broken
code, making it appropriate to fix in 17, but it's also something that we
could end up fixing for a while...
There are some variations of this that are a bit harder to fix, btw. We have
objdump -j .data -t src/backend/postgres|sort -k5
...
0000000001474d00 g O .data 00000000000015f0 ConfigureNamesReal
0000000001479a80 g O .data 0000000000001fb0 ConfigureNamesEnum
0000000001476300 g O .data 0000000000003778 ConfigureNamesString
...
00000000014682e0 g O .data 0000000000005848 ConfigureNamesBool
000000000146db40 g O .data 00000000000071c0 ConfigureNamesInt
Not that thta's all *that* much these days, but it's still pretty silly to use
~80kB of memory in every postgres instance just because we didn't set
conf->gen.vartype = PGC_BOOL;
etc at compile time.
Large modifiable arrays with callbacks are also quite useful for exploitation,
as one doesn't need to figure out precise addresses.
Greetings,
Andres Freund
Hi,
On 2024-04-18 09:07:43 -0300, Ranier Vilela wrote:
On 18/04/2024 00:39, Andres Freund wrote:
There are lots of places that could benefit from adding 'static
const'.I found a few more places.
Good catches.
Patch 004
The opposite would also help, adding static.
In these places, I believe it is safe to add static,
allowing the compiler to transform into read-only, definitively.
I don't think this would even compile? E.g. LockTagTypeNames, pg_wchar_table
are declared in a header and used across translation units.
Greetings,
Andres Freund
Em qui., 18 de abr. de 2024 às 14:16, Andres Freund <andres@anarazel.de>
escreveu:
Hi,
On 2024-04-18 09:07:43 -0300, Ranier Vilela wrote:
On 18/04/2024 00:39, Andres Freund wrote:
There are lots of places that could benefit from adding 'static
const'.I found a few more places.
Good catches.
Patch 004
The opposite would also help, adding static.
In these places, I believe it is safe to add static,
allowing the compiler to transform into read-only, definitively.I don't think this would even compile?
Compile, at least with msvc 2022.
Pass ninja test.
E.g. LockTagTypeNames, pg_wchar_table
are declared in a header and used across translation units.
Sad.
There should be a way to export a read-only (static const) variable.
Better remove these.
v1-0005 attached.
best regards,
Ranier Vilela
Attachments:
v1-0005-const-convert-static-const.patchapplication/octet-stream; name=v1-0005-const-convert-static-const.patchDownload
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 37628a85c7..94422fd1ad 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -207,7 +207,7 @@ foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
}
# Create the fmgr_builtins table, collect data for fmgr_builtin_oid_index
-print $tfh "\nconst FmgrBuiltin fmgr_builtins[] = {\n";
+print $tfh "\nstatic const FmgrBuiltin fmgr_builtins[] = {\n";
my %bmap;
$bmap{'t'} = 'true';
$bmap{'f'} = 'false';
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 1b86fff2fb..1c826bae15 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -72,16 +72,16 @@ static pg_tz *FetchDynamicTimeZone(TimeZoneAbbrevTable *tbl, const datetkn *tp,
DateTimeErrorExtra *extra);
-const int day_tab[2][13] =
+static const int day_tab[2][13] =
{
{31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31, 0},
{31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31, 0}
};
-const char *const months[] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun",
+static const char *const months[] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun",
"Jul", "Aug", "Sep", "Oct", "Nov", "Dec", NULL};
-const char *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
+static const char *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
"Thursday", "Friday", "Saturday", NULL};On 18.04.24 19:11, Andres Freund wrote:
Thoughts about when to apply these? Arguably they're fixing mildly broken
code, making it appropriate to fix in 17, but it's also something that we
could end up fixing for a while...
Yeah, let's keep these for later. They are not regressions, and there
is no end in sight yet. I have some other related stuff queued up, so
if we're going to start adjusting these kinds of things now, it would
open a can of worms.
On Thu, Apr 18, 2024 at 07:56:35PM +0200, Peter Eisentraut wrote:
On 18.04.24 19:11, Andres Freund wrote:
Thoughts about when to apply these? Arguably they're fixing mildly broken
code, making it appropriate to fix in 17, but it's also something that we
could end up fixing for a while...Yeah, let's keep these for later. They are not regressions, and there is no
end in sight yet. I have some other related stuff queued up, so if we're
going to start adjusting these kinds of things now, it would open a can of
worms.
This is a set of optimizations for stuff that has accumulated across
the years in various code paths, so I'd vote on the side of caution
and wait until v18 opens for business.
--
Michael
Em qui., 18 de abr. de 2024 às 14:43, Ranier Vilela <ranier.vf@gmail.com>
escreveu:
Em qui., 18 de abr. de 2024 às 14:16, Andres Freund <andres@anarazel.de>
escreveu:Hi,
On 2024-04-18 09:07:43 -0300, Ranier Vilela wrote:
On 18/04/2024 00:39, Andres Freund wrote:
There are lots of places that could benefit from adding 'static
const'.I found a few more places.
Good catches.
Patch 004
The opposite would also help, adding static.
In these places, I believe it is safe to add static,
allowing the compiler to transform into read-only, definitively.I don't think this would even compile?
Compile, at least with msvc 2022.
Pass ninja test.E.g. LockTagTypeNames, pg_wchar_table
are declared in a header and used across translation units.
Sad.
There should be a way to export a read-only (static const) variable.
Better remove these.v1-0005 attached.
Now with v18 open, any plans to forward this?
best regards,
Ranier Vilela