plpgsql function startup-time improvements

On 12/27/17 15:38, Tom Lane wrote:

It seems possible that the "promise" technique could be useful for
other plpgsql special variables in future. I thought briefly about
applying it to triggers' NEW and OLD arguments, but desisted because
(a) it's only a win if triggers will commonly not touch the variable,
which seems unlikely to be true for NEW/OLD; and (b) it would have
required infrastructure for attaching a promise to a DTYPE_REC
variable, which was more pain than I wanted. But I wonder if it'd
be useful for, say, the special variables that exception blocks create.

This might be useful for instantiating virtual generated column values
in AFTER triggers on demand. Although this would require promises on
record *fields*. Anyway, it's useful infrastructure, and could have
more uses.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Tom,

On Wed, December 27, 2017 3:38 pm, Tom Lane wrote:

Attached are patches for two performance-improvement ideas that came
to me while working on

[snip]

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. There are two motivations for this.
It saves a whole 8 bytes per PLpgSQL_var, at least on 64-bit machines,
because the fields now fit into what had been wasted padding space;
reducing the size of what we have to copy during copy_plpgsql_datums
has to be worth something. Second, those fields are now adjacent to
the common PLpgSQL_variable fields, which will simplify migrating them
into PLpgSQL_variable, as I anticipate we'll want to do at some point
when we allow composite variables to be marked CONSTANT and maybe NOT
NULL.

More performance, especially in plpgsql is always a good thing :)

After a few experiments I've got a question or two, though (and please
excuse if this are stupid questions :)

My C is a bit rusty, so I embarked on a mission to learn more.

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.

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

/messages/by-id/attachment/54267/0006-Add-bool8-typedef-for-system-catalog-structs.patch

I used 64-bit Kubuntu, the gcc that came with the system, and installed
Postgres 10 to see what MAXIMUM_ALIGNOF should be (it says 8 here).

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

However, I probably did not have the real compiler settings, which might
influence the size of bool or the enum definition?

Maybe someone could shed some light on these questions:

* Does bool vs. int really save space or is this maybe system/compiler
dependend?

* Or should the structs use "bool8" to ensure this?

* I dimly remember that access to 1-byte fields might be slower than to 4
byte fields on X86-64. Would the size savings still worth it?

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

Best regards,

Tels

"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]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.
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]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.
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.
on compiler. But the alignment requirement for dno would mean dtype plus
any padding after it would occupy 4 bytes no matter what.

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

"Tels" <nospam-pg-abuse@bloodgate.com> writes:

On Thu, December 28, 2017 5:43 pm, Tom Lane wrote:

Which field order were you checking? Are you accounting for alignment
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.

Hm, yeah, you would get different results on a compiler that considered
pointers to only require 4-byte alignment. Although that would be an
ABI break, so I'm surprised to hear there are any x86_64 compilers that
do that by default.

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?

I thought about that idea while reading your earlier message,
but I don't favor it for two reasons:

* It'd be an illogical ordering of the fields: isnull and freeval
belong with the value, not somewhere else. This is not simply an
academic point, because of the field-sharing conventions represented
by PLpgSQL_variable. I expect that sometime soon we will get
around to implementing CONSTANT, NOT NULL, and initializer properties
for composite variables, and the most natural way to do that will be
to include the isconst, notnull, and default_val fields into
PLpgSQL_variable so they can be shared by PLpgSQL_var and PLpgSQL_rec.
Shoving isnull and freeval into the middle of that would be really ugly.

* The second patch I posted in this thread adds another enum field
at the end of PLpgSQL_var. Right now, with either field ordering
under discussion, that's basically free space-wise because it's
just going into what would otherwise be trailing padding space.
But it destroys any space savings from moving isnull and freeval
somewhere else.

In the end, it's not wise to put too much emphasis on struct size
and padding considerations, as that just ends up being platform
dependent anyway. None of what we've said in this back-and-forth
is quite right for 32-bit-pointer machines, and once Peter's stdbool
patch lands, the assumption that bool is 1 byte will be shaky as
well. I think actually the point about maintaining a logical field
order is the most significant consideration here. There's no great
harm in trying to avoid space wastage on today's most popular
machines, but we shouldn't let that consideration contort the code
very far.

regards, tom lane

Hi

2017-12-27 21:38 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

Attached are patches for two performance-improvement ideas that came
to me while working on
/messages/by-id/8962.1514399547@sss.pgh.pa.us
The three patches are logically independent and could be committed in
any order. But they touch some overlapping code, so as presented,
you need to apply that other patch first and then these two in
sequence.

please, can you rebase all three patches necessary for patching?

The motivation for the first patch is that I noticed that for simple
plpgsql functions, especially triggers, the per-datum palloc()s performed
by copy_plpgsql_datum() during function entry amounted to a significant
fraction of the total runtime. To fix that, the patch simply does one
palloc for the space needed by all datums, relying on a space calculation
performed at the end of function compilation by plpgsql_finish_datums().
This does nothing much for trivial functions with only a few datums, but
for ones with more, it's a worthwhile savings.

BTW, I experimented with a more drastic solution involving separating
the "read only" and "writable" parts of PLpgSQL_datum structs and then
instantiating only the "writable" parts, thus considerably reducing the
amount of data to be copied during per-call initialization. But that
was a lot more invasive to the code, and it seemed to be slower than
what I present here, because performance-critical accesses to variables
had to compute the addresses of both structs associated with the variable.
I've not totally given up hope for that idea, but it'll require more
tuning than I had time for.

In addition to the core idea of the patch, I noticed that there is no
good reason for PLpgSQL_expr to be treated as a kind of PLpgSQL_datum;
those structs are never members of the estate->datums[] array, nor is
there any code expecting them to be structural supersets of PLpgSQL_datum.
So the patch also removes PLPGSQL_DTYPE_EXPR and the useless fields of
PLpgSQL_expr.

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. There are two motivations for this.
It saves a whole 8 bytes per PLpgSQL_var, at least on 64-bit machines,
because the fields now fit into what had been wasted padding space;
reducing the size of what we have to copy during copy_plpgsql_datums
has to be worth something. Second, those fields are now adjacent to
the common PLpgSQL_variable fields, which will simplify migrating them
into PLpgSQL_variable, as I anticipate we'll want to do at some point
when we allow composite variables to be marked CONSTANT and maybe NOT
NULL.

The idea of the second patch came from noticing that in simple trigger
functions, quite a large fraction of cycles went into setting up the
"built in" variables such as tg_name, even though many trigger functions
probably never read most of those variables. We could improve that by
not computing the values until/unless they're read. There are various
names for this technique, but the one that seemed most evocative to me
was to say that these variables have "promises" attached to them. So
that's what the patch calls them. We mark the variables with an enum
indicating which promise needs to be fulfilled for each one, and then
when about to read a datum, we fulfill the promise if needed.

The method I settled on for that was to invent a separate DTYPE_PROMISE,
which otherwise is treated exactly like DTYPE_VAR, and to code places
like exec_eval_datum() like this:

switch (datum->dtype)
{
+       case PLPGSQL_DTYPE_PROMISE:
+           /* fulfill promise if needed, then handle like regular var */
+           plpgsql_fulfill_promise(estate, (PLpgSQL_var *) datum);
+
+           /* FALL THRU */
+
case PLPGSQL_DTYPE_VAR:
{
PLpgSQL_var *var = (PLpgSQL_var *) datum;

The extra DTYPE is a little bit grotty, but it's not awful. One
alternative I experimented with was to just treat these variables
as plain DTYPE_VAR, requiring coding like

case PLPGSQL_DTYPE_VAR:
{
PLpgSQL_var *var = (PLpgSQL_var *) datum;

+               if (unlikely(var->promise != PLPGSQL_PROMISE_NONE))
+                   plpgsql_fulfill_promise(estate, var);
+
*typeid = var->datatype->typoid;
*typetypmod = var->datatype->atttypmod;
*value = var->value;

However, this way is injecting an additional test-and-branch into
hot code paths, and it was demonstrably slower.

With these patches, I see performance improvements of 10% to 20%
on simple but not totally unrealistic triggers, for example

create or replace function mytrig() returns trigger language plpgsql as
$$
begin
if (new.f1 != new.f2) or (new.f3 != new.f4) then
new.f3 = 42;
end if;
return new;
end$$ stable;

(BTW, those are percentages of total INSERT runtime, not just of
the trigger proper; though I cheated to the extent of using a
temp not regular table.)

It seems possible that the "promise" technique could be useful for
other plpgsql special variables in future. I thought briefly about
applying it to triggers' NEW and OLD arguments, but desisted because
(a) it's only a win if triggers will commonly not touch the variable,
which seems unlikely to be true for NEW/OLD; and (b) it would have
required infrastructure for attaching a promise to a DTYPE_REC
variable, which was more pain than I wanted. But I wonder if it'd
be useful for, say, the special variables that exception blocks create.

I'll add this to the January commitfest.

All mentioned ideas has sense.

Regards

Pavel

Pavel Stehule <pavel.stehule@gmail.com> writes:

please, can you rebase all three patches necessary for patching?

Done. These now need to be applied over
/messages/by-id/833.1516834367@sss.pgh.pa.us

regards, tom lane

Hi

2018-01-25 0:16 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

Pavel Stehule <pavel.stehule@gmail.com> writes:

please, can you rebase all three patches necessary for patching?

Done. These now need to be applied over
/messages/by-id/833.1516834367@sss.pgh.pa.us

Thank you

I checked it

1. there are no problem with patching
2. all tests passed
3. I can confirm so some best case speedup is about 15-20%. Worst case is
hard to test - but these changes should not be slower than current master.
4. faster-plpgsql-datum-setup-2.patch is simple patch with few lines of new
code
5. plpgsql-promises-2.patch is little bit more complex, but still it is
simple
6. the code is well formatted and well commented
7. there are not new test and code, what is not problem - these patches
doesn't add any new feature

I'll mark these patches as ready for commiter

Regards

Pavel