On Fri, Dec 4, 2015 at 7:15 AM, Aleksander Alekseev <a.aleks...@postgrespro.ru> wrote: > Current implementation of ResourceOwner uses arrays to store resources > like TupleDescs, Snapshots, etc. When we want to release one of these > resources ResourceOwner finds it with linear scan. Granted, resource > array are usually small so its not a problem most of the time. But it > appears to be a bottleneck when we are working with tables which have a > lot of partitions. > > To reproduce this issue: > 1. run `./gen.pl 10000 | psql my_database postgres` > 2. run `pgbench -j 8 -c 8 -f q.sql -T 100 my_database` > 3. in second terminal run `sudo perf top -u postgres` > > Both gen.pl and q.sql are attached to this message. > > You will see that postgres spends a lot of time in ResourceOwnerForget* > procedures: > > 32.80% postgres [.] list_nth > 20.29% postgres [.] ResourceOwnerForgetRelationRef > 12.87% postgres [.] find_all_inheritors > 7.90% postgres [.] get_tabstat_entry > 6.68% postgres [.] ResourceOwnerForgetTupleDesc > 1.17% postgres [.] hash_search_with_hash_value > ... < 1% ... > > I would like to suggest a patch (see attachment) witch fixes this > bottleneck. Also I discovered that there is a lot of code duplication in > ResourceOwner. Patch fixes this too. The idea is quite simple. I just > replaced arrays with something that could be considered hash tables, > but with some specific optimizations. > > After applying this patch we can see that bottleneck is gone: > > 42.89% postgres [.] list_nth > 18.30% postgres [.] find_all_inheritors > 10.97% postgres [.] get_tabstat_entry > 1.82% postgres [.] hash_search_with_hash_value > 1.21% postgres [.] SearchCatCache > ... < 1% ... > > For tables with thousands partitions we gain in average 32.5% more TPS. > > As far as I can see in the same time patch doesn't make anything worse. > `make check` passes with asserts enabled and disabled. There is no > performance degradation according to both standard pgbench benchmark > and benchmark described above for tables with 10 and 100 partitions.
I do think it's interesting to try to speed up the ResourceOwner mechanism when there are many resources owned. We've had some forays in that direction in the past, and I think it's useful to continue that work. But I also think that this patch, while interesting, is not something we can seriously consider committing in its current form, because: 1. It lacks adequate comments and submission notes to explain the general theory of operation of the patch. 2. It seems to use uint8 * rather freely as a substitute for char * or void *, which doesn't seem like a great idea. 3. It doesn't follow PostgreSQL formatting conventions (uncuddled curly braces, space after if and before open paren, etc.). 4. It mixes together multiple ideas in a single patch, not only introducing a hashing concept but also striping a brand-new layer of abstraction across the resource-owner mechanism. I am not sure that layer of abstraction is a very good idea, but if it needs to be done, I think it should be a separate 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
