Re: plpgsql function startup-time improvements

Moin,

On Thu, December 28, 2017 5:43 pm, Tom Lane wrote:
> "Tels" <nospam-pg-abuse(at)bloodgate(dot)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