[PATCH] Make various variables read-only (const)

Started by Oskari Saarenmaaabout 12 years ago9 messages

os@ohmu.fi

about 12 years ago

1 attachment(s)

This allows the variables to be moved from .data to .rodata section which
means that more data can be shared by processes and makes sure that nothing
can accidentally overwrite the read-only definitions. On a x86-64 Linux
system this moves roughly 9 kilobytes of previously writable data to the
read-only data segment in the backend and 4 kilobytes in libpq.

https://github.com/saaros/postgres/compare/constify

24 files changed, 108 insertions(+), 137 deletions(-)

/ Oskari

Attachments:

0001-Make-various-variables-read-only-const.patchtext/plain; charset=us-asciiDownload

>From 89f0348747b356eaccf5edc2f85bf47d0a35c4f4 Mon Sep 17 00:00:00 2001
From: Oskari Saarenmaa <os@ohmu.fi>
Date: Fri, 20 Dec 2013 18:38:10 +0200
Subject: [PATCH] Make various variables read-only (const)

This allows the variables to be moved from .data to .rodata section which
means that more data can be shared by processes and makes sure that nothing
can accidentally overwrite the read-only definitions.  On a x86-64 Linux
system this moves roughly 9 kilobytes of previously writable data to the
read-only data segment in the backend and 4 kilobytes in libpq.
---
 src/backend/access/common/reloptions.c | 31 ++++------------
 src/backend/catalog/objectaddress.c    |  4 +-
 src/backend/commands/conversioncmds.c  |  2 +-
 src/backend/commands/createas.c        |  3 +-
 src/backend/commands/tablecmds.c       |  8 ++--
 src/backend/optimizer/path/costsize.c  |  2 +-
 src/backend/regex/regc_lex.c           |  4 +-
 src/backend/regex/regcomp.c            |  2 +-
 src/backend/regex/regerror.c           |  6 +--
 src/backend/tcop/utility.c             |  3 +-
 src/backend/tsearch/wparser_def.c      |  4 +-
 src/backend/utils/adt/datetime.c       |  6 +--
 src/backend/utils/adt/formatting.c     | 67 +++++++++++++++-------------------
 src/backend/utils/adt/tsrank.c         | 16 ++++----
 src/backend/utils/mb/encnames.c        | 31 ++++++++--------
 src/backend/utils/mb/mbutils.c         |  8 ++--
 src/backend/utils/mb/wchar.c           |  2 +-
 src/common/relpath.c                   |  8 ++--
 src/include/access/reloptions.h        |  5 +--
 src/include/common/relpath.h           |  3 +-
 src/include/mb/pg_wchar.h              | 18 ++++-----
 src/include/optimizer/cost.h           |  2 +-
 src/include/utils/datetime.h           |  2 +
 src/port/chklocale.c                   |  8 ++--
 24 files changed, 108 insertions(+), 137 deletions(-)

diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 31941e9..5ec617b 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -564,18 +564,17 @@ add_string_reloption(bits32 kinds, char *name, char *desc, char *default_val,
  * If ignoreOids is true, then we should ignore any occurrence of "oids"
  * in the list (it will be or has been handled by interpretOidsOption()).
  *
- * Note that this is not responsible for determining whether the options
- * are valid, but it does check that namespaces for all the options given are
- * listed in validnsps.  The NULL namespace is always valid and need not be
- * explicitly listed.  Passing a NULL pointer means that only the NULL
- * namespace is valid.
+ * Note that this is not responsible for determining whether the options are
+ * valid, but it does check that namespaces for all the options given
+ * matches validnsp.  The NULL namespace is always valid.  Passing a NULL
+ * valinsp means that only the NULL namespace is valid.
  *
  * Both oldOptions and the result are text arrays (or NULL for "default"),
  * but we declare them as Datums to avoid including array.h in reloptions.h.
  */
 Datum
-transformRelOptions(Datum oldOptions, List *defList, char *namspace,
-					char *validnsps[], bool ignoreOids, bool isReset)
+transformRelOptions(Datum oldOptions, List *defList, const char *namspace,
+					const char *validnsp, bool ignoreOids, bool isReset)
 {
 	Datum		result;
 	ArrayBuildState *astate;
@@ -667,23 +666,7 @@ transformRelOptions(Datum oldOptions, List *defList, char *namspace,
 			 */
 			if (def->defnamespace != NULL)
 			{
-				bool		valid = false;
-				int			i;
-
-				if (validnsps)
-				{
-					for (i = 0; validnsps[i]; i++)
-					{
-						if (pg_strcasecmp(def->defnamespace,
-										  validnsps[i]) == 0)
-						{
-							valid = true;
-							break;
-						}
-					}
-				}
-
-				if (!valid)
+				if (!validnsp || pg_strcasecmp(def->defnamespace, validnsp))
 					ereport(ERROR,
 							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 							 errmsg("unrecognized parameter namespace \"%s\"",
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 9011190..dce69c7 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -81,7 +81,7 @@
  * This array provides a common part of system object structure; to help
  * consolidate routines to handle various kind of object classes.
  */
-typedef struct
+typedef const struct
 {
 	Oid			class_oid;		/* oid of catalog */
 	Oid			oid_index_oid;	/* oid of index on system oid column */
@@ -101,7 +101,7 @@ typedef struct
 										 * class? */
 } ObjectPropertyType;
 
-static ObjectPropertyType ObjectProperty[] =
+static const ObjectPropertyType ObjectProperty[] =
 {
 	{
 		CastRelationId,
diff --git a/src/backend/commands/conversioncmds.c b/src/backend/commands/conversioncmds.c
index 84ab434..3938106 100644
--- a/src/backend/commands/conversioncmds.c
+++ b/src/backend/commands/conversioncmds.c
@@ -46,7 +46,7 @@ CreateConversionCommand(CreateConversionStmt *stmt)
 	const char *from_encoding_name = stmt->for_encoding_name;
 	const char *to_encoding_name = stmt->to_encoding_name;
 	List	   *func_name = stmt->func_name;
-	static Oid	funcargs[] = {INT4OID, INT4OID, CSTRINGOID, INTERNALOID, INT4OID};
+	static const Oid	funcargs[] = {INT4OID, INT4OID, CSTRINGOID, INTERNALOID, INT4OID};
 	char		result[1];
 
 	/* Convert list of names to a name and namespace */
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 9c1fd2c..7cbacf6 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -264,7 +264,6 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 	Datum		toast_options;
 	ListCell   *lc;
 	int			attnum;
-	static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
 
 	Assert(into != NULL);		/* else somebody forgot to set it */
 
@@ -368,7 +367,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
 	toast_options = transformRelOptions((Datum) 0,
 										create->options,
 										"toast",
-										validnsps,
+										"toast",
 										true, false);
 
 	(void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b9cd88d..e1aa759 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -448,7 +448,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId)
 	Datum		reloptions;
 	ListCell   *listptr;
 	AttrNumber	attnum;
-	static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
 	Oid			ofTypeId;
 
 	/*
@@ -529,7 +528,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId)
 	/*
 	 * Parse and validate reloptions, if any.
 	 */
-	reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
+	reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, "toast",
 									 true, false);
 
 	(void) heap_reloptions(relkind, reloptions, true);
@@ -8745,7 +8744,6 @@ 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;
 
 	if (defList == NIL && operation != AT_ReplaceRelOptions)
 		return;					/* nothing to do */
@@ -8776,7 +8774,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 
 	/* Generate new proposed reloptions (text array) */
 	newOptions = transformRelOptions(isnull ? (Datum) 0 : datum,
-									 defList, NULL, validnsps, false,
+									 defList, NULL, "toast", false,
 									 operation == AT_ResetRelOptions);
 
 	/* Validate */
@@ -8894,7 +8892,7 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
 		}
 
 		newOptions = transformRelOptions(isnull ? (Datum) 0 : datum,
-										 defList, "toast", validnsps, false,
+										 defList, "toast", "toast", false,
 										 operation == AT_ResetRelOptions);
 
 		(void) heap_reloptions(RELKIND_TOASTVALUE, newOptions, true);
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 50f0852..95fc427 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -104,7 +104,7 @@ double		cpu_operator_cost = DEFAULT_CPU_OPERATOR_COST;
 
 int			effective_cache_size = -1;
 
-Cost		disable_cost = 1.0e10;
+const Cost		disable_cost = 1.0e10;
 
 bool		enable_seqscan = true;
 bool		enable_indexscan = true;
diff --git a/src/backend/regex/regc_lex.c b/src/backend/regex/regc_lex.c
index 3360cfb..7f03df1 100644
--- a/src/backend/regex/regc_lex.c
+++ b/src/backend/regex/regc_lex.c
@@ -716,10 +716,10 @@ static int						/* not actually used, but convenient for RETV */
 lexescape(struct vars * v)
 {
 	chr			c;
-	static chr	alert[] = {
+	static const chr	alert[] = {
 		CHR('a'), CHR('l'), CHR('e'), CHR('r'), CHR('t')
 	};
-	static chr	esc[] = {
+	static const chr	esc[] = {
 		CHR('E'), CHR('S'), CHR('C')
 	};
 	const chr  *save;
diff --git a/src/backend/regex/regcomp.c b/src/backend/regex/regcomp.c
index b5988a2..ca1ccc5 100644
--- a/src/backend/regex/regcomp.c
+++ b/src/backend/regex/regcomp.c
@@ -274,7 +274,7 @@ struct vars
 
 
 /* static function list */
-static struct fns functions = {
+static const struct fns functions = {
 	rfree,						/* regfree insides */
 };
 
diff --git a/src/backend/regex/regerror.c b/src/backend/regex/regerror.c
index f6a3f26..4b2573e 100644
--- a/src/backend/regex/regerror.c
+++ b/src/backend/regex/regerror.c
@@ -34,10 +34,10 @@
 #include "regex/regguts.h"
 
 /* unknown-error explanation */
-static char unk[] = "*** unknown regex error code 0x%x ***";
+static const char unk[] = "*** unknown regex error code 0x%x ***";
 
 /* struct to map among codes, code names, and explanations */
-static struct rerr
+static const struct rerr
 {
 	int			code;
 	const char *name;
@@ -62,7 +62,7 @@ pg_regerror(int errcode,		/* error code, or REG_ATOI or REG_ITOA */
 			char *errbuf,		/* result buffer (unless errbuf_size==0) */
 			size_t errbuf_size) /* available space in errbuf, can be 0 */
 {
-	struct rerr *r;
+	const struct rerr *r;
 	const char *msg;
 	char		convbuf[sizeof(unk) + 50];		/* 50 = plenty for int */
 	size_t		len;
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index dca4503..10ba5a2 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -937,7 +937,6 @@ ProcessUtilitySlow(Node *parsetree,
 						if (IsA(stmt, CreateStmt))
 						{
 							Datum		toast_options;
-							static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
 
 							/* Create the table itself */
 							relOid = DefineRelation((CreateStmt *) stmt,
@@ -957,7 +956,7 @@ ProcessUtilitySlow(Node *parsetree,
 							toast_options = transformRelOptions((Datum) 0,
 											  ((CreateStmt *) stmt)->options,
 																"toast",
-																validnsps,
+																"toast",
 																true,
 																false);
 							(void) heap_reloptions(RELKIND_TOASTVALUE,
diff --git a/src/backend/tsearch/wparser_def.c b/src/backend/tsearch/wparser_def.c
index cb2f8eb..f822abe 100644
--- a/src/backend/tsearch/wparser_def.c
+++ b/src/backend/tsearch/wparser_def.c
@@ -789,7 +789,7 @@ p_isspecial(TParser *prs)
 	 */
 	if (GetDatabaseEncoding() == PG_UTF8 && prs->usewide)
 	{
-		static pg_wchar strange_letter[] = {
+		static const pg_wchar strange_letter[] = {
 			/*
 			 * use binary search, so elements should be ordered
 			 */
@@ -1023,7 +1023,7 @@ p_isspecial(TParser *prs)
 			0xAA34,				/* CHAM CONSONANT SIGN RA */
 			0xAA4D				/* CHAM CONSONANT SIGN FINAL H */
 		};
-		pg_wchar   *StopLow = strange_letter,
+		const pg_wchar   *StopLow = strange_letter,
 				   *StopHigh = strange_letter + lengthof(strange_letter),
 				   *StopMiddle;
 		pg_wchar	c;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 1c8291c..c43d419 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -59,10 +59,10 @@ const int	day_tab[2][13] =
 	{31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31, 0}
 };
 
-char	   *months[] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun",
+const char	   *const months[] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun",
 "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", NULL};
 
-char	   *days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
+const char	   *const days[] = {"Sunday", "Monday", "Tuesday", "Wednesday",
 "Thursday", "Friday", "Saturday", NULL};
 
 
@@ -186,7 +186,7 @@ static const datetkn datetktbl[] = {
 
 static int	szdatetktbl = sizeof datetktbl / sizeof datetktbl[0];
 
-static datetkn deltatktbl[] = {
+static const datetkn deltatktbl[] = {
 	/* text, token, lexval */
 	{"@", IGNORE_DTF, 0},		/* postgres relative prefix */
 	{DAGO, AGO, 0},				/* "ago" indicates negative time offset */
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 946f3e2..5e58571 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -122,13 +122,6 @@
 
 
 /* ----------
- * External (defined in PgSQL datetime.c (timestamp utils))
- * ----------
- */
-extern char *months[],			/* month abbreviation	*/
-		   *days[];				/* full days		*/
-
-/* ----------
  * Format parser structs
  * ----------
  */
@@ -188,12 +181,12 @@ struct FormatNode
  * Full months
  * ----------
  */
-static char *months_full[] = {
+static const char *const months_full[] = {
 	"January", "February", "March", "April", "May", "June", "July",
 	"August", "September", "October", "November", "December", NULL
 };
 
-static char *days_short[] = {
+static const char *const days_short[] = {
 	"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", NULL
 };
 
@@ -226,8 +219,8 @@ static char *days_short[] = {
  * matches for BC have an odd index.  So the boolean value for BC is given by
  * taking the array index of the match, modulo 2.
  */
-static char *adbc_strings[] = {ad_STR, bc_STR, AD_STR, BC_STR, NULL};
-static char *adbc_strings_long[] = {a_d_STR, b_c_STR, A_D_STR, B_C_STR, NULL};
+static const char *const adbc_strings[] = {ad_STR, bc_STR, AD_STR, BC_STR, NULL};
+static const char *const adbc_strings_long[] = {a_d_STR, b_c_STR, A_D_STR, B_C_STR, NULL};
 
 /* ----------
  * AM / PM
@@ -253,8 +246,8 @@ static char *adbc_strings_long[] = {a_d_STR, b_c_STR, A_D_STR, B_C_STR, NULL};
  * matches for PM have an odd index.  So the boolean value for PM is given by
  * taking the array index of the match, modulo 2.
  */
-static char *ampm_strings[] = {am_STR, pm_STR, AM_STR, PM_STR, NULL};
-static char *ampm_strings_long[] = {a_m_STR, p_m_STR, A_M_STR, P_M_STR, NULL};
+static const char *const ampm_strings[] = {am_STR, pm_STR, AM_STR, PM_STR, NULL};
+static const char *const ampm_strings_long[] = {a_m_STR, p_m_STR, A_M_STR, P_M_STR, NULL};
 
 /* ----------
  * Months in roman-numeral
@@ -262,26 +255,26 @@ static char *ampm_strings_long[] = {a_m_STR, p_m_STR, A_M_STR, P_M_STR, NULL};
  *	'VIII' must have higher precedence than 'V')
  * ----------
  */
-static char *rm_months_upper[] =
+static const char *const rm_months_upper[] =
 {"XII", "XI", "X", "IX", "VIII", "VII", "VI", "V", "IV", "III", "II", "I", NULL};
 
-static char *rm_months_lower[] =
+static const char *const rm_months_lower[] =
 {"xii", "xi", "x", "ix", "viii", "vii", "vi", "v", "iv", "iii", "ii", "i", NULL};
 
 /* ----------
  * Roman numbers
  * ----------
  */
-static char *rm1[] = {"I", "II", "III", "IV", "V", "VI", "VII", "VIII", "IX", NULL};
-static char *rm10[] = {"X", "XX", "XXX", "XL", "L", "LX", "LXX", "LXXX", "XC", NULL};
-static char *rm100[] = {"C", "CC", "CCC", "CD", "D", "DC", "DCC", "DCCC", "CM", NULL};
+static const char *const rm1[] = {"I", "II", "III", "IV", "V", "VI", "VII", "VIII", "IX", NULL};
+static const char *const rm10[] = {"X", "XX", "XXX", "XL", "L", "LX", "LXX", "LXXX", "XC", NULL};
+static const char *const rm100[] = {"C", "CC", "CCC", "CD", "D", "DC", "DCC", "DCCC", "CM", NULL};
 
 /* ----------
  * Ordinal postfixes
  * ----------
  */
-static char *numTH[] = {"ST", "ND", "RD", "TH", NULL};
-static char *numth[] = {"st", "nd", "rd", "th", NULL};
+static const char *const numTH[] = {"ST", "ND", "RD", "TH", NULL};
+static const char *const numth[] = {"st", "nd", "rd", "th", NULL};
 
 /* ----------
  * Flags & Options:
@@ -525,7 +518,7 @@ do { \
  * Suffixes definition for DATE-TIME TO/FROM CHAR
  * ----------
  */
-static KeySuffix DCH_suff[] = {
+static const KeySuffix DCH_suff[] = {
 	{"FM", 2, DCH_S_FM, SUFFTYPE_PREFIX},
 	{"fm", 2, DCH_S_FM, SUFFTYPE_PREFIX},
 	{"TM", 2, DCH_S_TM, SUFFTYPE_PREFIX},
@@ -950,10 +943,10 @@ typedef struct NUMProc
  */
 static const KeyWord *index_seq_search(char *str, const KeyWord *kw,
 				 const int *index);
-static KeySuffix *suff_search(char *str, KeySuffix *suf, int type);
+static const KeySuffix *suff_search(char *str, const KeySuffix *suf, int type);
 static void NUMDesc_prepare(NUMDesc *num, FormatNode *n);
 static void parse_format(FormatNode *node, char *str, const KeyWord *kw,
-			 KeySuffix *suf, const int *index, int ver, NUMDesc *Num);
+			 const KeySuffix *suf, const int *index, int ver, NUMDesc *Num);
 
 static void DCH_to_char(FormatNode *node, bool is_interval,
 			TmToChar *in, char *out, Oid collid);
@@ -964,7 +957,7 @@ static void dump_index(const KeyWord *k, const int *index);
 static void dump_node(FormatNode *node, int max);
 #endif
 
-static char *get_th(char *num, int type);
+static const char *get_th(char *num, int type);
 static char *str_numth(char *dest, char *num, int type);
 static int	adjust_partial_year_to_2020(int year);
 static int	strspace_len(char *str);
@@ -973,8 +966,8 @@ static void from_char_set_mode(TmFromChar *tmfc, const FromCharDateMode mode);
 static void from_char_set_int(int *dest, const int value, const FormatNode *node);
 static int	from_char_parse_int_len(int *dest, char **src, const int len, FormatNode *node);
 static int	from_char_parse_int(int *dest, char **src, FormatNode *node);
-static int	seq_search(char *name, char **array, int type, int max, int *len);
-static int	from_char_seq_search(int *dest, char **src, char **array, int type, int max, FormatNode *node);
+static int	seq_search(char *name, const char *const *array, int type, int max, int *len);
+static int	from_char_seq_search(int *dest, char **src, const char *const *array, int type, int max, FormatNode *node);
 static void do_to_timestamp(text *date_txt, text *fmt,
 				struct pg_tm * tm, fsec_t *fsec);
 static char *fill_str(char *str, int c, int max);
@@ -1024,10 +1017,10 @@ index_seq_search(char *str, const KeyWord *kw, const int *index)
 	return NULL;
 }
 
-static KeySuffix *
-suff_search(char *str, KeySuffix *suf, int type)
+static const KeySuffix *
+suff_search(char *str, const KeySuffix *suf, int type)
 {
-	KeySuffix  *s;
+	const KeySuffix  *s;
 
 	for (s = suf; s->name != NULL; s++)
 	{
@@ -1237,9 +1230,9 @@ NUMDesc_prepare(NUMDesc *num, FormatNode *n)
  */
 static void
 parse_format(FormatNode *node, char *str, const KeyWord *kw,
-			 KeySuffix *suf, const int *index, int ver, NUMDesc *Num)
+			 const KeySuffix *suf, const int *index, int ver, NUMDesc *Num)
 {
-	KeySuffix  *s;
+	const KeySuffix  *s;
 	FormatNode *n;
 	int			node_set = 0,
 				suffix,
@@ -1401,7 +1394,7 @@ dump_node(FormatNode *node, int max)
  * type --> 0 upper, 1 lower
  * ----------
  */
-static char *
+static const char *
 get_th(char *num, int type)
 {
 	int			len = strlen(num),
@@ -2268,11 +2261,11 @@ from_char_parse_int(int *dest, char **src, FormatNode *node)
  * ----------
  */
 static int
-seq_search(char *name, char **array, int type, int max, int *len)
+seq_search(char *name, const char *const *array, int type, int max, int *len)
 {
-	char	   *p,
-			   *n,
-			  **a;
+	const char		*p,
+				*const *a;
+	char			*n;
 	int			last,
 				i;
 
@@ -2346,7 +2339,7 @@ seq_search(char *name, char **array, int type, int max, int *len)
  * If the string doesn't match, throw an error.
  */
 static int
-from_char_seq_search(int *dest, char **src, char **array, int type, int max,
+from_char_seq_search(int *dest, char **src, const char *const *array, int type, int max,
 					 FormatNode *node)
 {
 	int			len;
diff --git a/src/backend/utils/adt/tsrank.c b/src/backend/utils/adt/tsrank.c
index 1dbe47e..cb6112a 100644
--- a/src/backend/utils/adt/tsrank.c
+++ b/src/backend/utils/adt/tsrank.c
@@ -20,7 +20,7 @@
 #include "miscadmin.h"
 
 
-static float weights[] = {0.1f, 0.2f, 0.4f, 1.0f};
+static const float weights[] = {0.1f, 0.2f, 0.4f, 1.0f};
 
 #define wpos(wep)	( w[ WEP_GETWEIGHT(wep) ] )
 
@@ -33,8 +33,8 @@ static float weights[] = {0.1f, 0.2f, 0.4f, 1.0f};
 #define RANK_NORM_RDIVRPLUS1	0x20
 #define DEF_NORM_METHOD			RANK_NO_NORM
 
-static float calc_rank_or(float *w, TSVector t, TSQuery q);
-static float calc_rank_and(float *w, TSVector t, TSQuery q);
+static float calc_rank_or(const float *w, TSVector t, TSQuery q);
+static float calc_rank_and(const float *w, TSVector t, TSQuery q);
 
 /*
  * Returns a weight of a word collocation
@@ -202,7 +202,7 @@ static WordEntryPosVector POSNULL = {
 };
 
 static float
-calc_rank_and(float *w, TSVector t, TSQuery q)
+calc_rank_and(const float *w, TSVector t, TSQuery q)
 {
 	WordEntryPosVector **pos;
 	int			i,
@@ -278,7 +278,7 @@ calc_rank_and(float *w, TSVector t, TSQuery q)
 }
 
 static float
-calc_rank_or(float *w, TSVector t, TSQuery q)
+calc_rank_or(const float *w, TSVector t, TSQuery q)
 {
 	WordEntry  *entry,
 			   *firstentry;
@@ -347,7 +347,7 @@ calc_rank_or(float *w, TSVector t, TSQuery q)
 }
 
 static float
-calc_rank(float *w, TSVector t, TSQuery q, int32 method)
+calc_rank(const float *w, TSVector t, TSQuery q, int32 method)
 {
 	QueryItem  *item = GETQUERY(q);
 	float		res = 0.0;
@@ -387,7 +387,7 @@ calc_rank(float *w, TSVector t, TSQuery q, int32 method)
 	return res;
 }
 
-static float *
+static const float *
 getWeights(ArrayType *win)
 {
 	static float ws[lengthof(weights)];
@@ -723,7 +723,7 @@ get_docrep(TSVector txt, QueryRepresentation *qr, int *doclen)
 }
 
 static float4
-calc_rank_cd(float4 *arrdata, TSVector txt, TSQuery query, int method)
+calc_rank_cd(const float4 *arrdata, TSVector txt, TSQuery query, int method)
 {
 	DocRepresentation *doc;
 	int			len,
diff --git a/src/backend/utils/mb/encnames.c b/src/backend/utils/mb/encnames.c
index 772d4a5..23b2215 100644
--- a/src/backend/utils/mb/encnames.c
+++ b/src/backend/utils/mb/encnames.c
@@ -29,7 +29,7 @@
  * Karel Zak, Aug 2001
  * ----------
  */
-pg_encname	pg_encname_tbl[] =
+const pg_encname	pg_encname_tbl[] =
 {
 	{
 		"abc", PG_WIN1258
@@ -285,14 +285,11 @@ pg_encname	pg_encname_tbl[] =
 	},							/* alias for UHC */
 	{
 		"windows950", PG_BIG5
-	},							/* alias for BIG5 */
-	{
-		NULL, 0
-	}							/* last */
+	}							/* alias for BIG5 */
 };
 
-unsigned int pg_encname_tbl_sz = \
-sizeof(pg_encname_tbl) / sizeof(pg_encname_tbl[0]) - 1;
+const unsigned int pg_encname_tbl_sz = \
+sizeof(pg_encname_tbl) / sizeof(pg_encname_tbl[0]);
 
 /* ----------
  * These are "official" encoding names.
@@ -300,11 +297,15 @@ sizeof(pg_encname_tbl) / sizeof(pg_encname_tbl[0]) - 1;
  * ----------
  */
 #ifndef WIN32
-#define DEF_ENC2NAME(name, codepage) { #name, PG_##name }
+/* The extra NUL-terminator will make sure a warning is raised if the
+ * storage space for name is too small, otherwise when strlen(name) ==
+ * sizeof(pg_enc2name.name) the NUL-terminator would be silently dropped.
+ */
+#define DEF_ENC2NAME(name, codepage) { #name "\0", PG_##name }
 #else
 #define DEF_ENC2NAME(name, codepage) { #name, PG_##name, codepage }
 #endif
-pg_enc2name pg_enc2name_tbl[] =
+const pg_enc2name pg_enc2name_tbl[] =
 {
 	DEF_ENC2NAME(SQL_ASCII, 0),
 	DEF_ENC2NAME(EUC_JP, 20932),
@@ -356,7 +357,7 @@ pg_enc2name pg_enc2name_tbl[] =
  * This covers all encodings except MULE_INTERNAL, which is alien to gettext.
  * ----------
  */
-pg_enc2gettext pg_enc2gettext_tbl[] =
+const pg_enc2gettext pg_enc2gettext_tbl[] =
 {
 	{PG_SQL_ASCII, "US-ASCII"},
 	{PG_UTF8, "UTF-8"},
@@ -399,7 +400,7 @@ pg_enc2gettext pg_enc2gettext_tbl[] =
 	{PG_GB18030, "GB18030"},
 	{PG_JOHAB, "JOHAB"},
 	{PG_SHIFT_JIS_2004, "SHIFT_JISX0213"},
-	{0, NULL}
+	{0, ""}
 };
 
 
@@ -469,11 +470,11 @@ clean_encoding_name(const char *key, char *newkey)
  * Search encoding by encoding name
  * ----------
  */
-pg_encname *
+const pg_encname *
 pg_char_to_encname_struct(const char *name)
 {
 	unsigned int nel = pg_encname_tbl_sz;
-	pg_encname *base = pg_encname_tbl,
+	const pg_encname *base = pg_encname_tbl,
 			   *last = base + nel - 1,
 			   *position;
 	int			result;
@@ -521,7 +522,7 @@ pg_char_to_encname_struct(const char *name)
 int
 pg_char_to_encoding(const char *name)
 {
-	pg_encname *p;
+	const pg_encname *p;
 
 	if (!name)
 		return -1;
@@ -545,7 +546,7 @@ pg_encoding_to_char(int encoding)
 {
 	if (PG_VALID_ENCODING(encoding))
 	{
-		pg_enc2name *p = &pg_enc2name_tbl[encoding];
+		const pg_enc2name *p = &pg_enc2name_tbl[encoding];
 
 		Assert(encoding == p->encoding);
 		return p->name;
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index 6d1cd8e..bb5d63b 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -55,9 +55,9 @@ static FmgrInfo *ToClientConvProc = NULL;
 /*
  * These variables track the currently-selected encodings.
  */
-static pg_enc2name *ClientEncoding = &pg_enc2name_tbl[PG_SQL_ASCII];
-static pg_enc2name *DatabaseEncoding = &pg_enc2name_tbl[PG_SQL_ASCII];
-static pg_enc2name *MessageEncoding = &pg_enc2name_tbl[PG_SQL_ASCII];
+static const pg_enc2name *ClientEncoding = &pg_enc2name_tbl[PG_SQL_ASCII];
+static const pg_enc2name *DatabaseEncoding = &pg_enc2name_tbl[PG_SQL_ASCII];
+static const pg_enc2name *MessageEncoding = &pg_enc2name_tbl[PG_SQL_ASCII];
 
 /*
  * During backend startup we can't set client encoding because we (a)
@@ -903,7 +903,7 @@ raw_pg_bind_textdomain_codeset(const char *domainname, int encoding)
 	bool		elog_ok = (CurrentMemoryContext != NULL);
 	int			i;
 
-	for (i = 0; pg_enc2gettext_tbl[i].name != NULL; i++)
+	for (i = 0; *pg_enc2gettext_tbl[i].name; i++)
 	{
 		if (pg_enc2gettext_tbl[i].encoding == encoding)
 		{
diff --git a/src/backend/utils/mb/wchar.c b/src/backend/utils/mb/wchar.c
index 45bc3c1..6d03a10 100644
--- a/src/backend/utils/mb/wchar.c
+++ b/src/backend/utils/mb/wchar.c
@@ -1720,7 +1720,7 @@ pg_eucjp_increment(unsigned char *charptr, int length)
  * XXX must be sorted by the same order as enum pg_enc (in mb/pg_wchar.h)
  *-------------------------------------------------------------------
  */
-pg_wchar_tbl pg_wchar_table[] = {
+const pg_wchar_tbl pg_wchar_table[] = {
 	{pg_ascii2wchar_with_len, pg_wchar2single_with_len, pg_ascii_mblen, pg_ascii_dsplen, pg_ascii_verifier, 1}, /* PG_SQL_ASCII */
 	{pg_eucjp2wchar_with_len, pg_wchar2euc_with_len, pg_eucjp_mblen, pg_eucjp_dsplen, pg_eucjp_verifier, 3},	/* PG_EUC_JP */
 	{pg_euccn2wchar_with_len, pg_wchar2euc_with_len, pg_euccn_mblen, pg_euccn_dsplen, pg_euccn_verifier, 2},	/* PG_EUC_CN */
diff --git a/src/common/relpath.c b/src/common/relpath.c
index 737003a..177eaf5 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -20,16 +20,14 @@
 #include "common/relpath.h"
 #include "storage/backendid.h"
 
-#define FORKNAMECHARS	4		/* max chars for a fork name */
-
 /*
  * Lookup table of fork name by fork number.
  *
  * If you add a new entry, remember to update the errhint below, and the
- * documentation for pg_relation_size(). Also keep FORKNAMECHARS above
- * up-to-date.
+ * documentation for pg_relation_size(). Also keep FORKNAMECHARS in
+ * src/include/common/relpath.h up-to-date.
  */
-const char *forkNames[] = {
+const char forkNames[][FORKNAMECHARS+1] = {
 	"main",						/* MAIN_FORKNUM */
 	"fsm",						/* FSM_FORKNUM */
 	"vm",						/* VISIBILITYMAP_FORKNUM */
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 5a4664b..e02e15a 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -51,9 +51,6 @@ typedef enum relopt_kind
 	RELOPT_KIND_MAX = (1 << 30)
 } relopt_kind;
 
-/* reloption namespaces allowed for heaps -- currently only TOAST */
-#define HEAP_RELOPT_NAMESPACES { "toast", NULL }
-
 /* generic struct to hold shared data */
 typedef struct relopt_gen
 {
@@ -251,7 +248,7 @@ extern void add_string_reloption(bits32 kinds, char *name, char *desc,
 					 char *default_val, validate_string_relopt validator);
 
 extern Datum transformRelOptions(Datum oldOptions, List *defList,
-					char *namspace, char *validnsps[],
+					const char *namspace, const char *validnsp,
 					bool ignoreOids, bool isReset);
 extern List *untransformRelOptions(Datum options);
 extern bytea *extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
diff --git a/src/include/common/relpath.h b/src/include/common/relpath.h
index fec7e06..d1001f0 100644
--- a/src/include/common/relpath.h
+++ b/src/include/common/relpath.h
@@ -21,11 +21,12 @@
 #include "storage/relfilenode.h"
 
 
+#define FORKNAMECHARS		4		/* max chars for a fork name */
 #define OIDCHARS		10		/* max chars printed by %u */
 #define TABLESPACE_VERSION_DIRECTORY	"PG_" PG_MAJORVERSION "_" \
 									CppAsString2(CATALOG_VERSION_NO)
 
-extern const char *forkNames[];
+extern const char forkNames[][FORKNAMECHARS+1];
 extern int	forkname_chars(const char *str, ForkNumber *fork);
 extern char *relpathbackend(RelFileNode rnode, BackendId backend,
 			   ForkNumber forknum);
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index d255c64..5dc472c 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -307,12 +307,12 @@ typedef enum pg_enc
  */
 typedef struct pg_encname
 {
-	char	   *name;
+	char		name[16];
 	pg_enc		encoding;
 } pg_encname;
 
-extern pg_encname pg_encname_tbl[];
-extern unsigned int pg_encname_tbl_sz;
+extern const pg_encname pg_encname_tbl[];
+extern const unsigned int pg_encname_tbl_sz;
 
 /*
  * Careful:
@@ -322,14 +322,14 @@ extern unsigned int pg_encname_tbl_sz;
  */
 typedef struct pg_enc2name
 {
-	char	   *name;
+	char		name[16];
 	pg_enc		encoding;
 #ifdef WIN32
 	unsigned	codepage;		/* codepage for WIN32 */
 #endif
 } pg_enc2name;
 
-extern pg_enc2name pg_enc2name_tbl[];
+extern const pg_enc2name pg_enc2name_tbl[];
 
 /*
  * Encoding names for gettext
@@ -337,10 +337,10 @@ extern pg_enc2name pg_enc2name_tbl[];
 typedef struct pg_enc2gettext
 {
 	pg_enc		encoding;
-	const char *name;
+	char		name[16];
 } pg_enc2gettext;
 
-extern pg_enc2gettext pg_enc2gettext_tbl[];
+extern const pg_enc2gettext pg_enc2gettext_tbl[];
 
 /*
  * pg_wchar stuff
@@ -373,7 +373,7 @@ typedef struct
 	int			maxmblen;		/* max bytes for a char in this encoding */
 } pg_wchar_tbl;
 
-extern pg_wchar_tbl pg_wchar_table[];
+extern const pg_wchar_tbl pg_wchar_table[];
 
 /*
  * UTF-8 to local code conversion map
@@ -441,7 +441,7 @@ extern int	pg_valid_server_encoding_id(int encoding);
  * Remaining functions are not considered part of libpq's API, though many
  * of them do exist inside libpq.
  */
-extern pg_encname *pg_char_to_encname_struct(const char *name);
+extern const pg_encname *pg_char_to_encname_struct(const char *name);
 
 extern int	pg_mb2wchar(const char *from, pg_wchar *to);
 extern int	pg_mb2wchar_with_len(const char *from, pg_wchar *to, int len);
diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h
index 444ab740..756c04c 100644
--- a/src/include/optimizer/cost.h
+++ b/src/include/optimizer/cost.h
@@ -49,7 +49,7 @@ extern PGDLLIMPORT double cpu_tuple_cost;
 extern PGDLLIMPORT double cpu_index_tuple_cost;
 extern PGDLLIMPORT double cpu_operator_cost;
 extern PGDLLIMPORT int effective_cache_size;
-extern Cost disable_cost;
+extern const Cost disable_cost;
 extern bool enable_seqscan;
 extern bool enable_indexscan;
 extern bool enable_indexonlyscan;
diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index 4e59e44..45e3cec 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -247,6 +247,8 @@ do { \
  * Include check for leap year.
  */
 
+extern const char *const months[];		/* month abbreviation	*/
+extern const char *const days[];		/* full days		*/
 extern const int day_tab[2][13];
 
 #define isleap(y) (((y) % 4) == 0 && (((y) % 100) != 0 || ((y) % 400) == 0))
diff --git a/src/port/chklocale.c b/src/port/chklocale.c
index 3c9d7ab..962575e 100644
--- a/src/port/chklocale.c
+++ b/src/port/chklocale.c
@@ -40,7 +40,7 @@
 struct encoding_match
 {
 	enum pg_enc pg_enc_code;
-	const char *system_enc_name;
+	const char system_enc_name[16];
 };
 
 static const struct encoding_match encoding_match_list[] = {
@@ -185,7 +185,7 @@ static const struct encoding_match encoding_match_list[] = {
 
 	{PG_SQL_ASCII, "US-ASCII"},
 
-	{PG_SQL_ASCII, NULL}		/* end marker */
+	{PG_SQL_ASCII, ""}		/* end marker */
 };
 
 #ifdef WIN32
@@ -251,7 +251,7 @@ pg_codepage_to_encoding(UINT cp)
 	sprintf(sys, "CP%u", cp);
 
 	/* Check the table */
-	for (i = 0; encoding_match_list[i].system_enc_name; i++)
+	for (i = 0; *encoding_match_list[i].system_enc_name; i++)
 		if (pg_strcasecmp(sys, encoding_match_list[i].system_enc_name) == 0)
 			return encoding_match_list[i].pg_enc_code;
 
@@ -347,7 +347,7 @@ pg_get_encoding_from_locale(const char *ctype, bool write_message)
 		return -1;				/* out of memory; unlikely */
 
 	/* Check the table */
-	for (i = 0; encoding_match_list[i].system_enc_name; i++)
+	for (i = 0; *encoding_match_list[i].system_enc_name; i++)
 	{
 		if (pg_strcasecmp(sys, encoding_match_list[i].system_enc_name) == 0)
 		{
-- 
1.8.4.2

Robert Haas

robertmhaas@gmail.com

about 12 years ago

In reply to: Oskari Saarenmaa (#1)

Re: [PATCH] Make various variables read-only (const)

On Fri, Dec 20, 2013 at 12:01 PM, Oskari Saarenmaa <os@ohmu.fi> wrote:

This allows the variables to be moved from .data to .rodata section which
means that more data can be shared by processes and makes sure that nothing
can accidentally overwrite the read-only definitions. On a x86-64 Linux
system this moves roughly 9 kilobytes of previously writable data to the
read-only data segment in the backend and 4 kilobytes in libpq.

https://github.com/saaros/postgres/compare/constify

24 files changed, 108 insertions(+), 137 deletions(-)

This sounds like a broadly good thing, but I've had enough painful
experiences with const to be a little wary. And how much does this
really affect data sharing? Doesn't copy-on-write do the same thing
for writable data? Could we get most of the benefit by const-ifying
one or two large data structures and forget the rest?

Other comments:

- The first hunk of the patch mutilates the comment it modifies for no
apparent reason. Please revert.

- Why change the API of transformRelOptions()?

-#define DEF_ENC2NAME(name, codepage) { #name, PG_##name }
+/* The extra NUL-terminator will make sure a warning is raised if the
+ * storage space for name is too small, otherwise when strlen(name) ==
+ * sizeof(pg_enc2name.name) the NUL-terminator would be silently dropped.
+ */
+#define DEF_ENC2NAME(name, codepage) { #name "\0", PG_##name }

- The above hunk is not related to the primary purpose of this patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Oskari Saarenmaa

os@ohmu.fi

about 12 years ago

In reply to: Robert Haas (#2)

Re: [PATCH] Make various variables read-only (const)

On Sun, Dec 22, 2013 at 09:43:57PM -0500, Robert Haas wrote:

On Fri, Dec 20, 2013 at 12:01 PM, Oskari Saarenmaa <os@ohmu.fi> wrote:

This allows the variables to be moved from .data to .rodata section which
means that more data can be shared by processes and makes sure that nothing
can accidentally overwrite the read-only definitions. On a x86-64 Linux
system this moves roughly 9 kilobytes of previously writable data to the
read-only data segment in the backend and 4 kilobytes in libpq.

https://github.com/saaros/postgres/compare/constify

24 files changed, 108 insertions(+), 137 deletions(-)

This sounds like a broadly good thing, but I've had enough painful
experiences with const to be a little wary. And how much does this
really affect data sharing? Doesn't copy-on-write do the same thing
for writable data? Could we get most of the benefit by const-ifying
one or two large data structures and forget the rest?

Thanks for the review and sorry for the late reply, I was offline for a
while.

As Wim Lewis pointed out in his mail the const data is most likely
mixed with non-const data and copy-on-write won't help with all of it.
Also, some of the const data includes duplicates and thus .data actually
shrinks more than .rodata grows.

We'd probably get most of the space-saving benefits by just constifying the
biggest variables, but I think applying const to more things will also make
things more correct.

Other comments:

- The first hunk of the patch mutilates the comment it modifies for no
apparent reason. Please revert.

- Why change the API of transformRelOptions()?

The comment was changed to reflect the new API, I modified
transformRelOptions to only accept a single valid namespace to make things
simpler in the calling code. Nothing used more than one valid namespace
anyway, and it allows us to just use a constant "toast" without having to
create a 2 char* array with a NULL.

-#define DEF_ENC2NAME(name, codepage) { #name, PG_##name }
+/* The extra NUL-terminator will make sure a warning is raised if the
+ * storage space for name is too small, otherwise when strlen(name) ==
+ * sizeof(pg_enc2name.name) the NUL-terminator would be silently dropped.
+ */
+#define DEF_ENC2NAME(name, codepage) { #name "\0", PG_##name }

- The above hunk is not related to the primary purpose of this patch.

It sort-of is. Without fixed size char-arrays it's not possible to move
everything to .rodata, but fixed size char-arrays come with the drawback of
silently dropping the NUL-terminator when strlen(str) == sizeof(array), by
forcing a NUL-terminator in we always get a warning if it would've been
dropped and the size of the array can then be increased.

Thanks,
Oskari

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

almost 12 years ago

In reply to: Oskari Saarenmaa (#3)

Re: [PATCH] Make various variables read-only (const)

Oskari Saarenmaa <os@ohmu.fi> writes:

On Sun, Dec 22, 2013 at 09:43:57PM -0500, Robert Haas wrote:

- Why change the API of transformRelOptions()?

The comment was changed to reflect the new API, I modified
transformRelOptions to only accept a single valid namespace to make things
simpler in the calling code. Nothing used more than one valid namespace
anyway, and it allows us to just use a constant "toast" without having to
create a 2 char* array with a NULL.

That may be true today, but the code was obviously designed to allow for
more than one namespace in the future. I'm inclined to reject this part
of the patch, or at least rework it to const-ify the existing data
structure rather than editorialize on what's needed.

However, I believe this code was Alvaro's to begin with, so I'd be
interested in his opinion on how important it is for transformRelOptions
to allow more than one namespace per call.

Actually, I'm kind of wondering why the code has a concept of namespaces
at all, given the fact that it fails to store them in the resulting array.
It seems impossible to verify after the fact that an option was given with
the right namespace, so isn't the feature pretty much pointless?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

almost 12 years ago

In reply to: Tom Lane (#4)

Re: [PATCH] Make various variables read-only (const)

I wrote:

However, I believe this code was Alvaro's to begin with, so I'd be
interested in his opinion on how important it is for transformRelOptions
to allow more than one namespace per call.

Actually, I'm kind of wondering why the code has a concept of namespaces
at all, given the fact that it fails to store them in the resulting array.
It seems impossible to verify after the fact that an option was given with
the right namespace, so isn't the feature pretty much pointless?

After thinking about it some more, I realize that the intended usage
pattern is to call transformRelOptions once for each allowed namespace.
Since each call would ignore options outside its namespace, that would
result in any options with invalid namespace names being silently ignored;
so the fix was to add a parameter saying which namespaces are valid,
and then each call checks that, independently of extracting the options
it cares about.

This design seems a bit odd to me; it's certainly wasting effort, since
each namespace check after the first one is redundant. I'm inclined to
propose that we factor out the namespace validity checking into a separate
function, such that you do something like

void checkOptionNamespaces(List *defList, const char * const validnsps[])

just once, and then call transformRelOptions for each of the desired
namespaces; transformRelOptions's validnsps argument goes away. In at
least some places this looks like it would net out cleaner; for instance,
there is no good reason why callers that are trying to extract "toast"
options should have to know which other namespaces are allowed.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

almost 12 years ago

In reply to: Tom Lane (#5)

Re: [PATCH] Make various variables read-only (const)

I wrote:

[ speculation about refactoring the API of transformRelOptions ]

This morning I remembered that there's another patch in the queue with
an interest in the API and behavior of transformRelOptions:
https://commitfest.postgresql.org/action/patch_view?id=1318

While I think that one has no chance of getting committed in exactly
the submitted form, some descendant patch may well make it in; if we do
anything to transformRelOptions right now it'll create merge issues for
any work in that area.

Moreover, the amount of .data space that'd be saved by twiddling
transformRelOptions' API is negligible, so it's not really that
interesting for the purposes of this patch. So I think the right
thing to do for now is just drop the relevant parts of this patch.
We can revisit the issue, if still needed, after the extension-options
dust settles.

I'll keep looking at the rest of this patch.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

almost 12 years ago

In reply to: Oskari Saarenmaa (#3)

Re: [PATCH] Make various variables read-only (const)

Oskari Saarenmaa <os@ohmu.fi> writes:

On Sun, Dec 22, 2013 at 09:43:57PM -0500, Robert Haas wrote:

-#define DEF_ENC2NAME(name, codepage) { #name, PG_##name }
+/* The extra NUL-terminator will make sure a warning is raised if the
+ * storage space for name is too small, otherwise when strlen(name) ==
+ * sizeof(pg_enc2name.name) the NUL-terminator would be silently dropped.
+ */
+#define DEF_ENC2NAME(name, codepage) { #name "\0", PG_##name }

- The above hunk is not related to the primary purpose of this patch.

It sort-of is. Without fixed size char-arrays it's not possible to move
everything to .rodata, but fixed size char-arrays come with the drawback of
silently dropping the NUL-terminator when strlen(str) == sizeof(array), by
forcing a NUL-terminator in we always get a warning if it would've been
dropped and the size of the array can then be increased.

AFAICT, this change and some similar ones are based on a false assumption.
It is *not* necessary to replace pointers by fixed-length arrays in order
to get things into .rodata. For example, in chklocale.c where we already
had this coding:

struct encoding_match
{
enum pg_enc pg_enc_code;
const char *system_enc_name;
};

static const struct encoding_match encoding_match_list[] = {
{PG_EUC_JP, "EUC-JP"},
{PG_EUC_JP, "eucJP"},
...

what I see in gcc -S (on x86_64 Linux) is:

.section .rodata
.align 32
.type encoding_match_list, @object
.size encoding_match_list, 1776
encoding_match_list:
.long 1
.zero 4
.quad .LC5
.long 1
.zero 4
.quad .LC6
...
.section .rodata.str1.1
.LC5:
.string "EUC-JP"
.LC6:
.string "eucJP"
...

So it's all read-only data anyway, as can be confirmed with "size"
which shows that the .o file contains *no* data outside the text
segment. There might be some platforms where this isn't the case,
but I don't think they're mainstream.

Converting the pointer arrangement to a fixed-length struct member
as per the patch has the following implications:

* We save the pointer, and possibly some alignment padding, at the cost
that now every string has to be padded to the maximal length. This
might produce a net space savings if all the strings in the table
are of similar length, but it's hardly a guaranteed win. Note also
that we no longer get to share storage with any other uses of the
same strings as plain literals.

* Possibly there's some savings in executable startup time as a
consequence of eliminating pointer-relocation work. This would be
platform dependent, and in any case I've never heard any complaints
suggesting that this is a metric we need to care about at all.

* We now have a risk of possible silent breakage if any string reaches
the maximum length. This is exacerbated by the temptation to not leave
a lot of daylight in the chosen maximum length, so as to minimize the
amount of bloat created.

The hunk Robert is complaining about is attempting to deal with that last
drawback; but it seems like a mighty ugly technique to me, and there are
other places where you've replaced pointers by fixed-length arrays without
adding any such protection (because it's notationally impractical).

TBH I don't find this to be an improvement, especially in cases where
it forces code changes beyond just const-ifying the data declarations.
I don't have a lot of confidence for instance that you found all the
places that were checking for a NULL pointer as an array terminator.
Having to change existing code greatly weakens the argument that this
is a correctness-improving patch, because now you have to factor in a
risk of actively breaking something.

So I think the fixed-length-array changes in this patch are a bad
idea; it's sufficient to make sure we've const-ified both the pointers
and the containing structs, as in the existing chklocale.c coding.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

almost 12 years ago

In reply to: Tom Lane (#7)

Re: [PATCH] Make various variables read-only (const)

I wrote:

AFAICT, this change and some similar ones are based on a false assumption.
It is *not* necessary to replace pointers by fixed-length arrays in order
to get things into .rodata.

After further experimentation I find that this claim is true when
compiling "normally", but apparently not when using -fpic, at least
not on RHEL6 x86_64. Even when const-ified, the tables in encnames.o
and wchar.o end up in the data segment (though the underlying strings
are not). So if we want a meaningful shrinkage in the size of the data
segment in libpq.so, we'd have to do something similar to what Oskari
proposes.

However, I'm still against doing so; the other points I made before
still apply, and I think on balance fixed-length arrays are still a
bad idea. It seems like a particularly bad idea to expose a limit
on the length of an encoding name as part of the ABI defined by
pg_wchar.h, as the submitted patch did. I'm on board with making
changes like this where they can be argued to improve correctness and
maintainability, but surely moving to fixed-length arrays is the
opposite of that.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

almost 12 years ago

In reply to: Oskari Saarenmaa (#1)

Re: [PATCH] Make various variables read-only (const)

Oskari Saarenmaa <os@ohmu.fi> writes:

This allows the variables to be moved from .data to .rodata section which
means that more data can be shared by processes and makes sure that nothing
can accidentally overwrite the read-only definitions. On a x86-64 Linux
system this moves roughly 9 kilobytes of previously writable data to the
read-only data segment in the backend and 4 kilobytes in libpq.

Committed, with the changes mentioned upthread and some other minor
editorialization. I also adopted Wim Lewis' suggestion to not export
pg_encname_tbl[] at all anymore, since it's hard to see what the point
is of doing anything besides pg_char_to_encoding() with it.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers