Re: plpgsql function startup-time improvements

"Tels" <nospam-pg-abuse@bloodgate.com> writes:
> On Wed, December 27, 2017 3:38 pm, Tom Lane wrote:
>> Also, I changed PLpgSQL_var.isconst and PLpgSQL_var.notnull from "int"
>> to "bool", which is what they should have been all along, and relocated
>> them in the PLpgSQL_var struct.

> With a short test program printing out the size of PLpgSQL_var to check, I
> always saw 72 bytes, regardless of bool vs. int. So a bool would be 4
> bytes here. Hmm.

Seems fairly unlikely, especially given that we typedef bool as char ;-).

Which field order were you checking?  Are you accounting for alignment
padding?

By my count, with the existing field order on typical 64-bit hardware,
we ought to have

    PLpgSQL_datum_type dtype;       -- 4 bytes [1]
    int         dno;                -- 4 bytes
    char       *refname;            -- 8 bytes
    int         lineno;             -- 4 bytes
                                    -- 4 bytes wasted due to padding here
    PLpgSQL_type *datatype;         -- 8 bytes
    int         isconst;            -- 4 bytes
    int         notnull;            -- 4 bytes
    PLpgSQL_expr *default_val;      -- 8 bytes
    PLpgSQL_expr *cursor_explicit_expr;     -- 8 bytes
    int         cursor_explicit_argrow;     -- 4 bytes
    int         cursor_options;             -- 4 bytes

    Datum       value;              -- 8 bytes
    bool        isnull;             -- 1 byte
    bool        freeval;            -- 1 byte

so at this point we've consumed 74 bytes, but the whole struct has
to be aligned on 8-byte boundaries because of the pointers, so
sizeof(PLpgSQL_var) ought to be 80 --- and that is what I see here.

With the proposed redesign,

    PLpgSQL_datum_type dtype;       -- 4 bytes [1]
    int         dno;                -- 4 bytes
    char       *refname;            -- 8 bytes
    int         lineno;             -- 4 bytes

    bool        isconst;            -- 1 byte
    bool        notnull;            -- 1 byte
                                    -- 2 bytes wasted due to padding here
    PLpgSQL_type *datatype;         -- 8 bytes
    PLpgSQL_expr *default_val;      -- 8 bytes
    PLpgSQL_expr *cursor_explicit_expr;     -- 8 bytes
    int         cursor_explicit_argrow;     -- 4 bytes
    int         cursor_options;             -- 4 bytes

    Datum       value;              -- 8 bytes
    bool        isnull;             -- 1 byte
    bool        freeval;            -- 1 byte

so we've consumed 66 bytes, which rounds up to 72 with the addition
of trailing padding.

> Googling around, this patch comment from Peter Eisentraut says that "bool"
> can be more than one byte, and only "bool8" is really one byte.

Even if you're allowing stdbool.h to determine sizeof(bool), I'm pretty
sure all Intel-based platforms define that to be 1.

> Since I could get the test program to compile against all PG header files
> (which is probably me being just being dense...), I used a stripped down
> test.c (attached).

Hm.  I wonder if your <stdbool.h> behaves differently from mine.
You might try printing out sizeof(bool) directly to see.

> And maybe folding all four bool fields into an "int flags" field with bits
> would save space, and not much slower (depending on often how the
> different flags are accessed due to the ANDing and ORing ops)?

That'd be pretty messy/invasive in terms of the code changes needed,
and I don't think it'd save any space once you account for alignment
and the fact that my other patch proposes to add another enum at
the end of the struct.  Also, I'm not exactly convinced that
replacing byte sets and tests with bitflag operations would be
cheap time-wise.  (It would particularly be a mess for isnull,
since then there'd be an impedance mismatch with a whole lotta PG
APIs that expect null flags to be bools.)

            regards, tom lane

[1] Actually, in principle that enum could be 1, 2, or 4 bytes depending
on compiler.  But the alignment requirement for dno would mean dtype plus
any padding after it would occupy 4 bytes no matter what.