On Tue, Aug 9, 2022 at 9:04 PM David Rowley <dgrowle...@gmail.com> wrote:
> On Wed, 10 Aug 2022 at 03:16, Zhihong Yu <z...@yugabyte.com> wrote: > > On Tue, Aug 9, 2022 at 8:01 AM Robert Haas <robertmh...@gmail.com> > wrote: > >> > >> One problem with this patch is that, if I apply it, PostgreSQL does not > compile: > >> > >> nodeSort.c:197:6: error: use of undeclared identifier 'tupDesc' > >> if (tupDesc->natts == 1) > >> ^ > >> 1 error generated. > >> > >> Leaving that aside, I don't really see any advantage in this sort of > change. > > I'm with Robert on this one. If you look at the discussion for that > commit, you'll find quite a bit of benchmarking was done around this > [1]. The "if" test here is in quite a hot code path, so we want to > ensure whatever is being referenced here causes the least amount of > CPU cache misses. Volcano executors which process a single row at a > time are not exactly an ideal workload for a modern processor due to > the bad L1i and L1d hit ratios. This becomes worse with more plan > nodes as these caches are more likely to get flushed of useful cache > lines when mode notes are visited. Various other fields in 'node' have > just been accessed in the code leading up to the "if > (node->datumSort)" check, so we're probably not going to encounter any > CPU pipeline stalls waiting for cache lines to be loaded into L1d. It > seems you're proposing to change this and have offered no evidence of > no performance regressions from doing so. Going by the compilation > error above, it seems unlikely that you've given performance any > consideration at all. > > I mean this in the best way possible; for the future, I really > recommend arriving with ideas that are well researched and tested. > When you can, arrive with evidence to prove your change is good. For > this case, evidence would be benchmark results. The problem is if you > were to arrive with patches such as this too often then you'll start > to struggle to get attention from anyone, let alone a committer. You > don't want to build a reputation for bad quality work as it's likely > most committers will steer clear of your work. If you want a good > recent example of a good proposal, have a look at Yuya Watari's > write-up at [2] and [3]. There was a clear problem statement there and > a patch that was clearly a proof of concept only, so the author was > under no illusion that what he had was ideal. Now, some other ideas > were suggested on that thread to hopefully simplify the task and > provide even better performance. Yuya went off and did that and > arrived back armed with further benchmark results. I was quite > impressed with this considering he's not posted to -hackers very > often. Now, if you were a committer, would you be looking at the > patches from the person who sends in half-thought-through ideas, or > patches from someone that has clearly put a great deal of effort into > first clearly stating the problem and then proving that the problem is > solved by the given patch? > > David > > [1] > https://www.postgresql.org/message-id/CAApHDvrWV%3Dv0qKsC9_BHqhCn9TusrNvCaZDz77StCO--fmgbKA%40mail.gmail.com > [2] > https://www.postgresql.org/message-id/caj2pmkzncgoukse+_5lthd+kbxkvq6h2hqn8esxpxd+cxmg...@mail.gmail.com > [3] > https://www.postgresql.org/message-id/caj2pmkzkfvmphovyyuebpwb_nyyvk2+gadqgzxzvnjkvxgt...@mail.gmail.com Hi, David: Thanks for your detailed response.
