On Mon, Feb 29, 2016 at 11:10 PM, Robert Haas <robertmh...@gmail.com> wrote: > > On Fri, Feb 26, 2016 at 11:37 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila <amit.kapil...@gmail.com> > > wrote: > >> > >> Here, we can see that there is a gain of ~15% to ~38% at higher client > >> count. > >> > >> The attached document (perf_write_clogcontrollock_data_v6.ods) contains > >> data, mainly focussing on single client performance. The data is for > >> multiple runs on different machines, so I thought it is better to present in > >> form of document rather than dumping everything in e-mail. Do let me know > >> if there is any confusion in understanding/interpreting the data. > > > > Forgot to mention that all these tests have been done by reverting > > commit-ac1d794. > > OK, that seems better. But I have a question: if we don't really need > to make this optimization apply only when everything is on the same > page, then why even try? If we didn't try, we wouldn't need the > all_trans_same_page flag, which would reduce the amount of code > change.
I am not sure if I understood your question, do you want to know why at the first place transactions spanning more than one-page call the function TransactionIdSetPageStatus()? If we want to avoid trying the transaction status update when they are on different page, then I think we need some major changes in TransactionIdSetTreeStatus(). > Would that hurt anything? Taking it even further, we could > remove the check from TransactionGroupUpdateXidStatus too. I'd be > curious to know whether that set of changes would improve performance > or regress it. Or maybe it does nothing, in which case perhaps > simpler is better. > > All things being equal, it's probably better if the cases where > transactions from different pages get into the list together is > something that is more or less expected rather than a > once-in-a-blue-moon scenario - that way, if any bugs exist, we'll find > them. The downside of that is that we could increase latency for the > leader that way - doing other work on the same page shouldn't hurt > much but different pages is a bigger hit. But that hit might be > trivial enough not to be worth worrying about. > In my tests, the check related to same page in TransactionGroupUpdateXidStatus() doesn't impact performance in any way and I think the reason is that, it happens rarely that group contain multiple pages and even it occurs, there is hardly much impact. So, I will remove that check and I think thats what you also want for now. > + /* > + * Now that we've released the lock, go back and wake everybody up. We > + * don't do this under the lock so as to keep lock hold times to a > + * minimum. The system calls we need to perform to wake other processes > + * up are probably much slower than the simple memory writes > we did while > + * holding the lock. > + */ > > This comment was true in the place that you cut-and-pasted it from, > but it's not true here, since we potentially need to read from disk. > Okay, will change. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
