Re: plpgsql function startup-time improvements

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

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.