Re: [HACKERS] Deferring some AtStart* allocations?
On Tue, Oct 28, 2014 at 10:16 AM, Andres Freund wrote: > On 2014-10-24 11:25:23 -0400, Robert Haas wrote: >> On Fri, Oct 24, 2014 at 10:10 AM, Andres Freund >> wrote: >> > What I was thinking was that you'd append the messages to the layer one >> > level deeper than the parent. Then we'd missed the invalidations when >> > rolling back the intermediate xact. But since I was quite mistaken >> > above, this isn't a problem :) >> >> So, you happy with the patch now? > > Yes. Great. Committed. Thanks for the review. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-24 11:25:23 -0400, Robert Haas wrote: > On Fri, Oct 24, 2014 at 10:10 AM, Andres Freund > wrote: > > What I was thinking was that you'd append the messages to the layer one > > level deeper than the parent. Then we'd missed the invalidations when > > rolling back the intermediate xact. But since I was quite mistaken > > above, this isn't a problem :) > > So, you happy with the patch now? Yes. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On Fri, Oct 24, 2014 at 10:10 AM, Andres Freund wrote: > What I was thinking was that you'd append the messages to the layer one > level deeper than the parent. Then we'd missed the invalidations when > rolling back the intermediate xact. But since I was quite mistaken > above, this isn't a problem :) So, you happy with the patch now? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-24 09:45:59 -0400, Robert Haas wrote: > On Fri, Oct 24, 2014 at 9:17 AM, Andres Freund wrote: > >> If > >> that subtransaction abouts, AtEOSubXact_Inval() gets called again, > >> sees that it has messages (that it inherited from the innermost > >> subtransaction), and takes the exact same code-path that it would have > >> pre-patch. > > > > Consider what happens if the innermost transaction commits and the > > second innermost one rolls back. AtEOSubXact_Inval() won't do anything > > because it doesn't have any entries itself. > > This is the part I don't understand. After the innermost transaction > commits, it *does* have entries itself. Sure, otherwise there'd be no problem. > This whole block is basically just an optimization: > + if (myInfo->parent == NULL || myInfo->parent->my_level > < my_level - 1) > + { > + myInfo->my_level--; > + return; > + } > > If we removed that code, then we'd just do this instead: > > /* Pass up my inval messages to parent */ > AppendInvalidationMessages(&myInfo->parent->PriorCmdInvalidMsgs, >&myInfo->PriorCmdInvalidMsgs); > > /* Pending relcache inval becomes parent's problem too */ > if (myInfo->RelcacheInitFileInval) > myInfo->parent->RelcacheInitFileInval = true; Ick. I somehow misimagined that you'd just append them one layer further up. I obviously shouldn't review code during a conference. > > Even though there's still > > valid cache inval entries containing the innermost xact's version of the > > catalog, now not valid anymore. > > This part doesn't make sense to me either. The invalidation entries > don't put data into the caches; they take data out. When we change > stuff, we generate invalidation messages. What I was thinking was that you'd append the messages to the layer one level deeper than the parent. Then we'd missed the invalidations when rolling back the intermediate xact. But since I was quite mistaken above, this isn't a problem :) Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On Fri, Oct 24, 2014 at 9:17 AM, Andres Freund wrote: >> If >> that subtransaction abouts, AtEOSubXact_Inval() gets called again, >> sees that it has messages (that it inherited from the innermost >> subtransaction), and takes the exact same code-path that it would have >> pre-patch. > > Consider what happens if the innermost transaction commits and the > second innermost one rolls back. AtEOSubXact_Inval() won't do anything > because it doesn't have any entries itself. This is the part I don't understand. After the innermost transaction commits, it *does* have entries itself. This whole block is basically just an optimization: + if (myInfo->parent == NULL || myInfo->parent->my_level < my_level - 1) + { + myInfo->my_level--; + return; + } If we removed that code, then we'd just do this instead: /* Pass up my inval messages to parent */ AppendInvalidationMessages(&myInfo->parent->PriorCmdInvalidMsgs, &myInfo->PriorCmdInvalidMsgs); /* Pending relcache inval becomes parent's problem too */ if (myInfo->RelcacheInitFileInval) myInfo->parent->RelcacheInitFileInval = true; That should be basically equivalent. I mean, we need a bit of surgery to ensure that the parent entry actually exists before we attach stuff to it, but that's mechanical. The whole point here though is that, pre-patch, the parent has an empty entry and we have one with stuff in it. What I think happens in the current code is that we take all of the stuff in our non-empty entry and move it into the parent's empty entry, so that the parent ends up with an entry identical to ours but with a level one less than we had. We could do that here too, manufacturing the empty entry and then moving our stuff into it and then deleting our original entry, but that seems silly; just change the level and call it good. IOW, I don't see how I'm *changing* the logic here at all. Why doesn't whatever problem you're concerned about exist in the current coding, too? > Even though there's still > valid cache inval entries containing the innermost xact's version of the > catalog, now not valid anymore. This part doesn't make sense to me either. The invalidation entries don't put data into the caches; they take data out. When we change stuff, we generate invalidation messages. At end of statement, once we've done CommandCounterIncrement(), we execute those invalidation messages locally, so that the next cache reload the updated state. At end of transaction, once we've committed, we send those invalidation messages to the shared queue to be executed by everyone else, so that they also see the updated state. If we abort a transaction or sub-transaction, we re-execute those same invalidation messages so that we flush the new state, forcing the next set of cache reloads to again pull in the old state. The patch isn't changing - or not intentionally anyway - anything about any of that logic. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-23 12:04:40 -0400, Robert Haas wrote: > On Tue, Oct 21, 2014 at 12:00 PM, Andres Freund > wrote: > > On 2014-10-09 15:01:19 -0400, Robert Haas wrote: > >> /* > >> @@ -960,18 +966,38 @@ AtEOXact_Inval(bool isCommit) > > ... > >> + /* > >> + * We create invalidation stack entries lazily, so the > >> parent might > >> + * not have one. Instead of creating one, moving all the > >> data over, > >> + * and then freeing our own, we can just adjust the level of > >> our own > >> + * entry. > >> + */ > >> + if (myInfo->parent == NULL || myInfo->parent->my_level < > >> my_level - 1) > >> + { > >> + myInfo->my_level--; > >> + return; > >> + } > >> + > > > > I think this bit might not be correct. What if the subxact one level up > > aborts? Then it'll miss dealing with these invalidation entries. Or am I > > misunderstanding something? > > One of us is. I think you're asking about a situation where we have a > transaction, and a subtransaction, and within that another > subtransaction. Only the innermost subtransaction has invalidation > messages. At the innermost level, we commit; the above code makes > those messages the responsibility of the outer subtransaction. Exactly. > If > that subtransaction abouts, AtEOSubXact_Inval() gets called again, > sees that it has messages (that it inherited from the innermost > subtransaction), and takes the exact same code-path that it would have > pre-patch. Consider what happens if the innermost transaction commits and the second innermost one rolls back. AtEOSubXact_Inval() won't do anything because it doesn't have any entries itself. Even though there's still valid cache inval entries containing the innermost xact's version of the catalog, now not valid anymore. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On Tue, Oct 21, 2014 at 12:00 PM, Andres Freund wrote: > On 2014-10-09 15:01:19 -0400, Robert Haas wrote: >> /* >> @@ -960,18 +966,38 @@ AtEOXact_Inval(bool isCommit) > ... >> + /* >> + * We create invalidation stack entries lazily, so the parent >> might >> + * not have one. Instead of creating one, moving all the data >> over, >> + * and then freeing our own, we can just adjust the level of >> our own >> + * entry. >> + */ >> + if (myInfo->parent == NULL || myInfo->parent->my_level < >> my_level - 1) >> + { >> + myInfo->my_level--; >> + return; >> + } >> + > > I think this bit might not be correct. What if the subxact one level up > aborts? Then it'll miss dealing with these invalidation entries. Or am I > misunderstanding something? One of us is. I think you're asking about a situation where we have a transaction, and a subtransaction, and within that another subtransaction. Only the innermost subtransaction has invalidation messages. At the innermost level, we commit; the above code makes those messages the responsibility of the outer subtransaction. If that subtransaction abouts, AtEOSubXact_Inval() gets called again, sees that it has messages (that it inherited from the innermost subtransaction), and takes the exact same code-path that it would have pre-patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
Hi, On 2014-10-09 15:01:19 -0400, Robert Haas wrote: > /* > @@ -960,18 +966,38 @@ AtEOXact_Inval(bool isCommit) ... > + /* > + * We create invalidation stack entries lazily, so the parent > might > + * not have one. Instead of creating one, moving all the data > over, > + * and then freeing our own, we can just adjust the level of > our own > + * entry. > + */ > + if (myInfo->parent == NULL || myInfo->parent->my_level < > my_level - 1) > + { > + myInfo->my_level--; > + return; > + } > + I think this bit might not be correct. What if the subxact one level up aborts? Then it'll miss dealing with these invalidation entries. Or am I misunderstanding something? I like the patch, except the above potential issue. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-08 13:52:14 -0400, Robert Haas wrote: > On Sun, Jun 29, 2014 at 9:12 PM, Tom Lane wrote: > > Meh. Even "SELECT 1" is going to be doing *far* more pallocs than that to > > get through raw parsing, parse analysis, planning, and execution startup. > > If you can find a few hundred pallocs we can avoid in trivial queries, > > it would get interesting; but I'll be astonished if saving 4 is measurable. > > I got nerd-sniped by this problem today, probably after staring at the > profiling data very similar to what led Andres to ask the question in > the first place. I've loolked through this patch and I'm happy with it. One could argue that the current afterTriggers == NULL checks should be replaced with a Assert ensuring we're inside the xact. But I think you're right in removing them, and I don't think we actually need the asserts. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 9 October 2014 20:01, Robert Haas wrote: > OK, here's an attempt at a real patch for that. I haven't perf-tested this. Patch looks good to me. Surprised we didn't do that before. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-09 17:02:02 -0400, Robert Haas wrote: > On Thu, Oct 9, 2014 at 3:53 PM, Andres Freund wrote: > >> OK, here's an attempt at a real patch for that. I haven't perf-tested > >> this. > > > > Neato. With a really trivial SELECT: > > > > before: > > tps = 28150.794776 (excluding connections establishing) > > after: > > tps = 29978.767703 (excluding connections establishing) > > > > slightly more meaningful: > > > > before: > > tps = 21272.400039 (including connections establishing) > > after: > > tps = 22290.703482 (excluding connections establishing) > > > > So that's a noticeable win. Obviously it's going to be less for more > > complicated stuff, but still... > > > > I've not really looked at the patches though. > > Yeah, not bad at all for the amount of work involved. Want to > disclose the actual queries? SELECT 1; SELECT * FROM smalltable WHERE id = xxx; The latter is something quite frequent in the real world, so it's not all academic... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On Thu, Oct 9, 2014 at 3:53 PM, Andres Freund wrote: >> OK, here's an attempt at a real patch for that. I haven't perf-tested this. > > Neato. With a really trivial SELECT: > > before: > tps = 28150.794776 (excluding connections establishing) > after: > tps = 29978.767703 (excluding connections establishing) > > slightly more meaningful: > > before: > tps = 21272.400039 (including connections establishing) > after: > tps = 22290.703482 (excluding connections establishing) > > So that's a noticeable win. Obviously it's going to be less for more > complicated stuff, but still... > > I've not really looked at the patches though. Yeah, not bad at all for the amount of work involved. Want to disclose the actual queries? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-09 15:01:19 -0400, Robert Haas wrote: > On Thu, Oct 9, 2014 at 8:20 AM, Andres Freund wrote: > > On 2014-10-09 08:18:18 -0400, Robert Haas wrote: > >> On Thu, Oct 9, 2014 at 5:34 AM, Andres Freund > >> wrote: > >> > Interesting - in my local profile AtStart_Inval() is more pronounced > >> > than AfterTriggerBeginQuery(). I've quickly and in a ugly fashion hacked > >> > AtStart_Inval() out of readonly queries ontop of your patch. Together > >> > that yields a ~3.5% performance improvement in my trivial 'SELECT * FROM > >> > tbl WHER pkey = xxx' testcase. > >> > >> Whoa. Now that's clearly significant. > > > > Well, my guess it'll be far less noticeable in less trivial > > workloads. But it does seem worthwile. > > > >> You didn't attach the patch; was that inadvertent, or was it too ugly > >> for that? > > > > Far, far too ugly ;). I just removed the AtStart() call from xact.c and > > sprinkled it around relevant places instead ;) > > OK, here's an attempt at a real patch for that. I haven't perf-tested this. Neato. With a really trivial SELECT: before: tps = 28150.794776 (excluding connections establishing) after: tps = 29978.767703 (excluding connections establishing) slightly more meaningful: before: tps = 21272.400039 (including connections establishing) after: tps = 22290.703482 (excluding connections establishing) So that's a noticeable win. Obviously it's going to be less for more complicated stuff, but still... I've not really looked at the patches though. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On Thu, Oct 9, 2014 at 8:20 AM, Andres Freund wrote: > On 2014-10-09 08:18:18 -0400, Robert Haas wrote: >> On Thu, Oct 9, 2014 at 5:34 AM, Andres Freund wrote: >> > Interesting - in my local profile AtStart_Inval() is more pronounced >> > than AfterTriggerBeginQuery(). I've quickly and in a ugly fashion hacked >> > AtStart_Inval() out of readonly queries ontop of your patch. Together >> > that yields a ~3.5% performance improvement in my trivial 'SELECT * FROM >> > tbl WHER pkey = xxx' testcase. >> >> Whoa. Now that's clearly significant. > > Well, my guess it'll be far less noticeable in less trivial > workloads. But it does seem worthwile. > >> You didn't attach the patch; was that inadvertent, or was it too ugly >> for that? > > Far, far too ugly ;). I just removed the AtStart() call from xact.c and > sprinkled it around relevant places instead ;) OK, here's an attempt at a real patch for that. I haven't perf-tested this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 5b5d31b..651a5c4 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1838,7 +1838,6 @@ StartTransaction(void) * initialize other subsystems for new transaction */ AtStart_GUC(); - AtStart_Inval(); AtStart_Cache(); AfterTriggerBeginXact(); @@ -4151,7 +4150,6 @@ StartSubTransaction(void) */ AtSubStart_Memory(); AtSubStart_ResourceOwner(); - AtSubStart_Inval(); AtSubStart_Notify(); AfterTriggerBeginSubXact(); diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c index a7a768e..6b6c88e 100644 --- a/src/backend/utils/cache/inval.c +++ b/src/backend/utils/cache/inval.c @@ -693,19 +693,32 @@ AcceptInvalidationMessages(void) } /* - * AtStart_Inval - * Initialize inval lists at start of a main transaction. + * PrepareInvalidationState + * Initialize inval lists for the current (sub)transaction. */ -void -AtStart_Inval(void) +static void +PrepareInvalidationState(void) { - Assert(transInvalInfo == NULL); - transInvalInfo = (TransInvalidationInfo *) + TransInvalidationInfo *myInfo; + + if (transInvalInfo != NULL && + transInvalInfo->my_level == GetCurrentTransactionNestLevel()) + return; + + myInfo = (TransInvalidationInfo *) MemoryContextAllocZero(TopTransactionContext, sizeof(TransInvalidationInfo)); - transInvalInfo->my_level = GetCurrentTransactionNestLevel(); - SharedInvalidMessagesArray = NULL; - numSharedInvalidMessagesArray = 0; + myInfo->parent = transInvalInfo; + myInfo->my_level = GetCurrentTransactionNestLevel(); + + /* + * If there's any previous entry, this one should be for a deeper + * nesting level. + */ + Assert(transInvalInfo == NULL || + myInfo->my_level > transInvalInfo->my_level); + + transInvalInfo = myInfo; } /* @@ -727,24 +740,6 @@ PostPrepare_Inval(void) } /* - * AtSubStart_Inval - * Initialize inval lists at start of a subtransaction. - */ -void -AtSubStart_Inval(void) -{ - TransInvalidationInfo *myInfo; - - Assert(transInvalInfo != NULL); - myInfo = (TransInvalidationInfo *) - MemoryContextAllocZero(TopTransactionContext, - sizeof(TransInvalidationInfo)); - myInfo->parent = transInvalInfo; - myInfo->my_level = GetCurrentTransactionNestLevel(); - transInvalInfo = myInfo; -} - -/* * Collect invalidation messages into SharedInvalidMessagesArray array. */ static void @@ -803,8 +798,16 @@ xactGetCommittedInvalidationMessages(SharedInvalidationMessage **msgs, { MemoryContext oldcontext; + /* Quick exit if we haven't done anything with invalidation messages. */ + if (transInvalInfo == NULL) + { + *RelcacheInitFileInval = false; + *msgs = NULL; + return 0; + } + /* Must be at top of stack */ - Assert(transInvalInfo != NULL && transInvalInfo->parent == NULL); + Assert(transInvalInfo->my_level == 1 && transInvalInfo->parent == NULL); /* * Relcache init file invalidation requires processing both before and @@ -904,11 +907,15 @@ ProcessCommittedInvalidationMessages(SharedInvalidationMessage *msgs, void AtEOXact_Inval(bool isCommit) { + /* Quick exit if no messages */ + if (transInvalInfo == NULL) + return; + + /* Must be at top of stack */ + Assert(transInvalInfo->my_level == 1 && transInvalInfo->parent == NULL); + if (isCommit) { - /* Must be at top of stack */ - Assert(transInvalInfo != NULL && transInvalInfo->parent == NULL); - /* * Relcache init file invalidation requires processing both before and * after we send the SI messages. However, we need not do anything @@ -926,17 +933,16 @@ AtEOXact_Inval(bool isCommit) if (transInvalInfo->RelcacheInitFileInval) RelationCacheInitFilePostInvalidate(); } - else if (transInvalInfo != NULL) + else { - /* Must be at top of stack */ - Assert(transInvalInfo->parent == NULL); - ProcessInvalidationMessages(&transInvalInfo->PriorCmdInvalidMsgs, LocalExec
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-09 08:18:18 -0400, Robert Haas wrote: > On Thu, Oct 9, 2014 at 5:34 AM, Andres Freund wrote: > > Interesting - in my local profile AtStart_Inval() is more pronounced > > than AfterTriggerBeginQuery(). I've quickly and in a ugly fashion hacked > > AtStart_Inval() out of readonly queries ontop of your patch. Together > > that yields a ~3.5% performance improvement in my trivial 'SELECT * FROM > > tbl WHER pkey = xxx' testcase. > > Whoa. Now that's clearly significant. Well, my guess it'll be far less noticeable in less trivial workloads. But it does seem worthwile. > You didn't attach the patch; was that inadvertent, or was it too ugly > for that? Far, far too ugly ;). I just removed the AtStart() call from xact.c and sprinkled it around relevant places instead ;) Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On Thu, Oct 9, 2014 at 5:34 AM, Andres Freund wrote: > Interesting - in my local profile AtStart_Inval() is more pronounced > than AfterTriggerBeginQuery(). I've quickly and in a ugly fashion hacked > AtStart_Inval() out of readonly queries ontop of your patch. Together > that yields a ~3.5% performance improvement in my trivial 'SELECT * FROM > tbl WHER pkey = xxx' testcase. Whoa. Now that's clearly significant. You didn't attach the patch; was that inadvertent, or was it too ugly for that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-10-08 13:52:14 -0400, Robert Haas wrote: > On Sun, Jun 29, 2014 at 9:12 PM, Tom Lane wrote: > > Meh. Even "SELECT 1" is going to be doing *far* more pallocs than that to > > get through raw parsing, parse analysis, planning, and execution startup. > > If you can find a few hundred pallocs we can avoid in trivial queries, > > it would get interesting; but I'll be astonished if saving 4 is measurable. > > I got nerd-sniped by this problem today, probably after staring at the > profiling data very similar to what led Andres to ask the question in > the first place. The gains are indeed not measurable on a > macrobenchmark, but I had to write the patch to figure that out, so > here it is. > > Although the gain isn't a measurable percentage of total runtime, it > does appear to be a significant percentage of the palloc traffic on > trivial queries. I did 30-second SELECT-only pgbench runs at scale > factor 10, using prepared queries. According to perf, on unpatched > master, StartTransactionCommand accounts for 11.46% of the calls to > AllocSetAlloc. (I imagine this is by runtime, not by call count, but > it probably doesn't matter much either way.) But with this patch, > StartTransactionCommand's share drops to 4.43%. Most of that is > coming from AtStart_Inval(), which wouldn't be hard to fix either. Interesting - in my local profile AtStart_Inval() is more pronounced than AfterTriggerBeginQuery(). I've quickly and in a ugly fashion hacked AtStart_Inval() out of readonly queries ontop of your patch. Together that yields a ~3.5% performance improvement in my trivial 'SELECT * FROM tbl WHER pkey = xxx' testcase. > I'm inclined to think that tamping down palloc traffic is worthwhile > even if we can't directly measure the savings on a macrobenchmark. > AllocSetAlloc is often at or near the top of profiling results, but > there's rarely a single caller responsible for enough of those calls > for to make it worth optimizing. But that means that if we refuse to > take patches that save just a few pallocs, we're basically giving up > on ever improving anything in this area, and that doesn't seem like a > good idea either; it's only going to get slowly worse over time as we > add more features. I think it depends a bit on the callsites. If its somewhere that nobody will ever care, because it's a slowpath, then we shouldn't care either. But that's not the case here, so I do think that makes sense. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On Wed, Oct 8, 2014 at 11:22 PM, Robert Haas wrote: > > On Sun, Jun 29, 2014 at 9:12 PM, Tom Lane wrote: > > Meh. Even "SELECT 1" is going to be doing *far* more pallocs than that to > > get through raw parsing, parse analysis, planning, and execution startup. > > If you can find a few hundred pallocs we can avoid in trivial queries, > > it would get interesting; but I'll be astonished if saving 4 is measurable. > > I got nerd-sniped by this problem today, probably after staring at the > profiling data very similar to what led Andres to ask the question in > the first place. The gains are indeed not measurable on a > macrobenchmark, but I had to write the patch to figure that out, so > here it is. > > Although the gain isn't a measurable percentage of total runtime, it > does appear to be a significant percentage of the palloc traffic on > trivial queries. I did 30-second SELECT-only pgbench runs at scale > factor 10, using prepared queries. According to perf, on unpatched > master, StartTransactionCommand accounts for 11.46% of the calls to > AllocSetAlloc. (I imagine this is by runtime, not by call count, but > it probably doesn't matter much either way.) But with this patch, > StartTransactionCommand's share drops to 4.43%. Most of that is > coming from AtStart_Inval(), which wouldn't be hard to fix either. > > I'm inclined to think that tamping down palloc traffic is worthwhile > even if we can't directly measure the savings on a macrobenchmark. > AllocSetAlloc is often at or near the top of profiling results, but > there's rarely a single caller responsible for enough of those calls > for to make it worth optimizing. But that means that if we refuse to > take patches that save just a few pallocs, we're basically giving up > on ever improving anything in this area, and that doesn't seem like a > good idea either; it's only going to get slowly worse over time as we > add more features. Yeah, this makes sense, even otherwise also if somebody comes up with a much larger patch which reduce few dozen of pallocs and shows noticeable gain, then it will be difficult to quantify the patch based on which particular pallocs gives improvements, as reducing few pallocs might not give any noticeable gain. > BTW, if anyone's curious what the biggest source of AllocSetAlloc > calls is on this workload, the answer is ExecInitIndexScan(), which > looks to be indirectly responsible for almost half the total (or just > over half, with the patch applied). That apparently calls a lot of > different things that allocate small amounts of memory, and it > accounts for nearly all of the AllocSetAlloc traffic that originates > from PortalStart(). One way to dig into this problem is that we look at each individual pallocs in PortalStart() path and try to see which one's can be optimized, another way is that we introduce a concept of Prepared Portals some thing similar to Prepared Statements, but for Portal. This will not only avoid the pallocs in executor startup phase, but can improve the performance by avoiding many other calls related to executor startup. I understand that this will be a much bigger project, however the gains can also be substantial for prepared statements when all/most of the data fits in memory. It seems other databases also have similar concepts for execution (example Oracle uses Cursor_sharing/Shared cursors to achieve the same) With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Deferring some AtStart* allocations?
On Sun, Jun 29, 2014 at 9:12 PM, Tom Lane wrote: > Meh. Even "SELECT 1" is going to be doing *far* more pallocs than that to > get through raw parsing, parse analysis, planning, and execution startup. > If you can find a few hundred pallocs we can avoid in trivial queries, > it would get interesting; but I'll be astonished if saving 4 is measurable. I got nerd-sniped by this problem today, probably after staring at the profiling data very similar to what led Andres to ask the question in the first place. The gains are indeed not measurable on a macrobenchmark, but I had to write the patch to figure that out, so here it is. Although the gain isn't a measurable percentage of total runtime, it does appear to be a significant percentage of the palloc traffic on trivial queries. I did 30-second SELECT-only pgbench runs at scale factor 10, using prepared queries. According to perf, on unpatched master, StartTransactionCommand accounts for 11.46% of the calls to AllocSetAlloc. (I imagine this is by runtime, not by call count, but it probably doesn't matter much either way.) But with this patch, StartTransactionCommand's share drops to 4.43%. Most of that is coming from AtStart_Inval(), which wouldn't be hard to fix either. I'm inclined to think that tamping down palloc traffic is worthwhile even if we can't directly measure the savings on a macrobenchmark. AllocSetAlloc is often at or near the top of profiling results, but there's rarely a single caller responsible for enough of those calls for to make it worth optimizing. But that means that if we refuse to take patches that save just a few pallocs, we're basically giving up on ever improving anything in this area, and that doesn't seem like a good idea either; it's only going to get slowly worse over time as we add more features. BTW, if anyone's curious what the biggest source of AllocSetAlloc calls is on this workload, the answer is ExecInitIndexScan(), which looks to be indirectly responsible for almost half the total (or just over half, with the patch applied). That apparently calls a lot of different things that allocate small amounts of memory, and it accounts for nearly all of the AllocSetAlloc traffic that originates from PortalStart(). I haven't poked around in there enough to know whether there's anything worth optimizing, but given the degree to which this patch shifts the profile, I bet the number that it takes to make a material savings is more like a few dozen than a few hundred. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index f4c0ffa..1db0666 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -90,6 +90,7 @@ static void AfterTriggerSaveEvent(EState *estate, ResultRelInfo *relinfo, int event, bool row_trigger, HeapTuple oldtup, HeapTuple newtup, List *recheckIndexes, Bitmapset *modifiedCols); +static void AfterTriggerEnlargeQueryState(void); /* @@ -3203,9 +3204,7 @@ typedef struct AfterTriggersData int maxtransdepth; /* allocated len of above arrays */ } AfterTriggersData; -typedef AfterTriggersData *AfterTriggers; - -static AfterTriggers afterTriggers; +static AfterTriggersData afterTriggers; static void AfterTriggerExecute(AfterTriggerEvent event, Relation rel, TriggerDesc *trigdesc, @@ -3228,7 +3227,7 @@ GetCurrentFDWTuplestore() { Tuplestorestate *ret; - ret = afterTriggers->fdw_tuplestores[afterTriggers->query_depth]; + ret = afterTriggers.fdw_tuplestores[afterTriggers.query_depth]; if (ret == NULL) { MemoryContext oldcxt; @@ -3255,7 +3254,7 @@ GetCurrentFDWTuplestore() CurrentResourceOwner = saveResourceOwner; MemoryContextSwitchTo(oldcxt); - afterTriggers->fdw_tuplestores[afterTriggers->query_depth] = ret; + afterTriggers.fdw_tuplestores[afterTriggers.query_depth] = ret; } return ret; @@ -3271,7 +3270,7 @@ static bool afterTriggerCheckState(AfterTriggerShared evtshared) { Oid tgoid = evtshared->ats_tgoid; - SetConstraintState state = afterTriggers->state; + SetConstraintState state = afterTriggers.state; int i; /* @@ -3282,19 +3281,22 @@ afterTriggerCheckState(AfterTriggerShared evtshared) return false; /* - * Check if SET CONSTRAINTS has been executed for this specific trigger. + * If constraint state exists, SET CONSTRAINTS might have been executed + * either for this trigger or for all triggers. */ - for (i = 0; i < state->numstates; i++) + if (state != NULL) { - if (state->trigstates[i].sct_tgoid == tgoid) - return state->trigstates[i].sct_tgisdeferred; - } + /* Check for SET CONSTRAINTS for this specific trigger. */ + for (i = 0; i < state->numstates; i++) + { + if (state->trigstates[i].sct_tgoid == tgoid) +return state->trigstates[i].sct_tgisdeferred; + } - /* - * Check if SET CONSTRAINTS ALL has been executed; if so use that. - */ - if (state->all
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-06-29 21:12:49 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2014-06-29 19:52:23 -0400, Tom Lane wrote: > >> Andres Freund writes: > >>> Why aren't we delaying allocations in e.g. AtStart_Inval(), > >>> AfterTriggerBeginXact() to when the data structures are acutally used? > >> > >> Aren't we? Neither of those would be doing much work certainly. > > > They are perhaps not doing much in absolute terms, but it's a fair share > > of the processing overhead for simple statements. AfterTriggerBeginXact() > > is called unconditionally from StartTransaction() and does three > > MemoryContextAlloc()s. AtStart_Inval() one. > > I think they should just be initialized whenever the memory is used? > > Doesn't look too complicated to me. > > Meh. Even "SELECT 1" is going to be doing *far* more pallocs than that to > get through raw parsing, parse analysis, planning, and execution > startup. The quick test I ran used prepared statements - there the number of memory allocations is *much* lower... > If you can find a few hundred pallocs we can avoid in trivial queries, > it would get interesting; but I'll be astonished if saving 4 is measurable. I only noticed it because it shows up in profiles. I doubt it'll even remotely be noticeable without using prepared statements, but a lot of people do use those. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
Andres Freund writes: > On 2014-06-29 19:52:23 -0400, Tom Lane wrote: >> Andres Freund writes: >>> Why aren't we delaying allocations in e.g. AtStart_Inval(), >>> AfterTriggerBeginXact() to when the data structures are acutally used? >> >> Aren't we? Neither of those would be doing much work certainly. > They are perhaps not doing much in absolute terms, but it's a fair share > of the processing overhead for simple statements. AfterTriggerBeginXact() > is called unconditionally from StartTransaction() and does three > MemoryContextAlloc()s. AtStart_Inval() one. > I think they should just be initialized whenever the memory is used? > Doesn't look too complicated to me. Meh. Even "SELECT 1" is going to be doing *far* more pallocs than that to get through raw parsing, parse analysis, planning, and execution startup. If you can find a few hundred pallocs we can avoid in trivial queries, it would get interesting; but I'll be astonished if saving 4 is measurable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
On 2014-06-29 19:52:23 -0400, Tom Lane wrote: > Andres Freund writes: > > Why aren't we delaying allocations in e.g. AtStart_Inval(), > > AfterTriggerBeginXact() to when the data structures are acutally used? > > Aren't we? Neither of those would be doing much work certainly. They are perhaps not doing much in absolute terms, but it's a fair share of the processing overhead for simple statements. AfterTriggerBeginXact() is called unconditionally from StartTransaction() and does three MemoryContextAlloc()s. AtStart_Inval() one. I think they should just be initialized whenever the memory is used? Doesn't look too complicated to me. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Deferring some AtStart* allocations?
Andres Freund writes: > Why aren't we delaying allocations in e.g. AtStart_Inval(), > AfterTriggerBeginXact() to when the data structures are acutally used? Aren't we? Neither of those would be doing much work certainly. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Deferring some AtStart* allocations?
Hi, In workloads with mostly simple statements, memory allocations are the primary bottleneck. Some of the allocations are unlikely to be avoidable without major work, but others seem superflous in many scenarios. Why aren't we delaying allocations in e.g. AtStart_Inval(), AfterTriggerBeginXact() to when the data structures are acutally used? In most transactions neither will be? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers