Re: Large writable variables

Andres Freund <andres@anarazel.de>

From: Andres Freund <andres@anarazel.de>
To: pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@enterprisedb.com>
Date: 2018-10-16T04:50:51Z
Lists: pgsql-hackers
Hi,

After the last few changes (including a proposed one), we now have in
the .data segment (i.e. mutable initialized variables):

$ objdump -j .data -t ~/build/postgres/dev-optimize/vpath/src/backend/postgres|awk '{print $4, $5, $6}'|sort -r|lessGreetings,
.data 0000000000004380 ConfigureNamesInt
.data 0000000000003570 ConfigureNamesBool
.data 0000000000002140 ConfigureNamesString
.data 00000000000010e0 ConfigureNamesEnum
.data 0000000000000d20 ConfigureNamesReal

These are by far the largest chunk of the .data segment (like 47152
bytes out of 51168 bytes).  It's not entirely trivial to fix. While we
can slap consts onto large parts of the functions taking a struct
config_generic*, there's a few places where *do* modify them.

ISTM, to actually fix, we'd have to split
struct config_generic
{
	/* constant fields, must be set correctly in initial value: */
	const char *name;			/* name of variable - MUST BE FIRST */
	GucContext	context;		/* context required to set the variable */
	enum config_group group;	/* to help organize variables by function */
	const char *short_desc;		/* short desc. of this variable's purpose */
	const char *long_desc;		/* long desc. of this variable's purpose */
	int			flags;			/* flag bits, see guc.h */
	/* variable fields, initialized at runtime: */
	enum config_type vartype;	/* type of variable (set only at startup) */
	int			status;			/* status bits, see below */
	GucSource	source;			/* source of the current actual value */
	GucSource	reset_source;	/* source of the reset_value */
	GucContext	scontext;		/* context that set the current value */
	GucContext	reset_scontext; /* context that set the reset value */
	GucStack   *stack;			/* stacked prior values */
	void	   *extra;			/* "extra" pointer for current actual value */
	char	   *sourcefile;		/* file current setting is from (NULL if not
								 * set in config file) */
	int			sourceline;		/* line in source file */
};

into two different arrays. Namely the already denoted 'constant fields'
and 'variable fields.  But it's a bit more complicated than that: The
size doesn't come just from the base config_struct, but also from the
config_{bool,int,real,string,enum} arrays.  Where we again have separate
'constant fields' and 'variable fields'.

So we'd have to refactor more heavily than just splitting the above
array into two.

I kind of wonder, if we shouldn't remove the separate ConfigureNamesInt*
int arrays and change things so we have one

struct config_def
{
	const char *name;                       /* name of variable - MUST BE FIRST */
	GucContext      context;                /* context required to set the variable */
	enum config_group group;        /* to help organize variables by function */
	const char *short_desc;         /* short desc. of this variable's purpose */
	const char *long_desc;          /* long desc. of this variable's purpose */
	int                     flags;                  /* flag bits, see guc.h */
	enum config_type vartype;       /* type of variable (set only at startup) */

	union
	{
		struct
		{
			bool       *variable;
			bool            boot_val;
			GucBoolCheckHook check_hook;
			GucBoolAssignHook assign_hook;
			GucShowHook show_hook;
		} config_bool;

		struct
		{
			int		   *variable;
			int			boot_val;
			int			min;
			int			max;
			GucIntCheckHook check_hook;
			GucIntAssignHook assign_hook;
			GucShowHook show_hook;
		} config_int

		...
	} pertype;
}

and then a corresponding struct config_val with a similar union.

Besides reducing modified memory, it'd also have the benefit of making
the grouping within the various arrays much more sensible, because
related fields could be grouped together.


.data 0000000000000420 intRelOpts
.data 00000000000001c0 realRelOpts
.data 0000000000000118 boolRelOpts
.data 00000000000000a8 stringRelOpts

these should be readonly, but the code doesn't make that easy. There's
pending refactorings, I don't know how they effect this.


.data 0000000000000068 SnapshotSelfData
.data 0000000000000068 SnapshotAnyData
.data 0000000000000068 SecondarySnapshotData
.data 0000000000000068 CurrentSnapshotData
.data 0000000000000068 CatalogSnapshotData

The obviously actually are modifyable.


.data 0000000000000028 spi_printtupDR
.data 0000000000000028 printsimpleDR
.data 0000000000000028 donothingDR
.data 0000000000000028 debugtupDR

These we could actually make constant, but CreateDestReceiver() as an
API makes that inconvenient. They also are pretty darn small...  There's
a security benefit in making them constant and casting the constness
away - I think that might not be insane.


- Andres


Commits

  1. Apply unconstify() in more places

  2. Improve unconstify() documentation

  3. Drop const cast from dlsym() calls

  4. Const-ify a few more large static tables.

  5. Improve tzparse's handling of TZDEFRULES ("posixrules") zone data.

  6. Avoid statically allocating statement cache in ecpglib/prepare.c.

  7. Reorder FmgrBuiltin members, saving 25% in size.

  8. Add macro to cast away const without allowing changes to underlying type.

  9. Mark constantly allocated dest receiver as const.

  10. Avoid statically allocating formatting.c's format string caches.

  11. Correct constness of system attributes in heap.c & prerequisites.

  12. Avoid statically allocating gmtsub()'s timezone workspace.

  13. Correct constness of a few variables.

  14. Move the replication lag tracker into heap memory.