Re: plpgsql function startup-time improvements

Moin,

On Thu, December 28, 2017 5:43 pm, Tom Lane wrote:
> "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 ;-).

Hmn, yes, I can see my confusion. And a test shows, that sizeof(bool) is 1
here. and *char etc are 8.

> 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.

Sounds logical, thanx for the detailed explanation. In my test the first 4
padding bytes are probably not there, because I probably miss something
that aligns ptrs on 8-byte boundaries. At least that would explain the
sizes seen here.

So, if you moved "isnull" and "freeval" right behind "isconst" and
"notnull", you could save another 2 byte, land at 64, and thus no extra
padding would keep it at 64?

>> 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.)

Already had a hunch the idea wouldn't be popular, and this are all pretty
solid arguments against it. Nevermind, then :)

Best wishes,

Tels