Thread
Commits
-
Speed up plpgsql trigger startup by introducing "promises".
- fd333bc763ea 11.0 landed
-
Speed up plpgsql function startup by doing fewer pallocs.
- 40301c1c8bcb 11.0 landed
-
plpgsql function startup-time improvements
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-27T20:38:34Z
Attached are patches for two performance-improvement ideas that came to me while working on https://www.postgresql.org/message-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. 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. regards, tom lane -
Re: plpgsql function startup-time improvements
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2017-12-28T15:12:59Z
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
-
Re: plpgsql function startup-time improvements
Tels <nospam-pg-abuse@bloodgate.com> — 2017-12-28T18:18:19Z
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. https://www.postgresql.org/message-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
-
Re: plpgsql function startup-time improvements
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-28T22:43:58Z
"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. -
Re: plpgsql function startup-time improvements
Tels <nospam-pg-abuse@bloodgate.com> — 2017-12-29T12:47:28Z
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
-
Re: plpgsql function startup-time improvements
Tom Lane <tgl@sss.pgh.pa.us> — 2017-12-29T16:43:07Z
"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
-
Re: plpgsql function startup-time improvements
Pavel Stehule <pavel.stehule@gmail.com> — 2018-01-24T10:53:04Z
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 > https://www.postgresql.org/message-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 > regards, tom lane > > -
Re: plpgsql function startup-time improvements
Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-24T23:16:24Z
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 https://www.postgresql.org/message-id/833.1516834367@sss.pgh.pa.us regards, tom lane
-
Re: plpgsql function startup-time improvements
Pavel Stehule <pavel.stehule@gmail.com> — 2018-01-25T18:58:03Z
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 > https://www.postgresql.org/message-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 > > > regards, tom lane > >