Re: Large writable variables - Mailing list 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