Re: [HACKERS] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Thu, Aug 07, 2014 at 09:39:57AM -0400, Robert Haas wrote: On Wed, Aug 6, 2014 at 9:35 PM, Bruce Momjian br...@momjian.us wrote: On Sun, Jan 12, 2014 at 12:53:40PM -0500, Noah Misch wrote: Further study revealed a defect in the patch's memory management, and I have not gotten around to correcting that. I talked to Noah and he can't continue on this item. Can someone else work on it? Well, it would be helpful if he could describe the defect he found, so that the next person doesn't have to guess. Quite right. Primarily, a leak: FreeTupleDesc() did a mere pfree() on a node tree, leaking all but the root node. Perhaps the best fix there is to give each TupleDesc a memory context and then simplify FreeTupleDesc() to a context deletion. That will tend to waste more memory, but I haven't thought of something clearly better. The patch passes a relcache-owned node tree to eval_const_expressions() via ExecPrepareExpr(). If that stands, I should add a comment to the effect that eval_const_expressions() must not modify in place the original tree or reference it from the result tree. The comment at expression_planner() implies that's already true. I know of no exceptions, but I have not audited. How reasonable is reliance on continued conformance to that rule? An alternative is to have the relcache always return a node tree copy, like it does in RelationGetIndexExpressions(). That sacrifices about 1/4 of the performance gain. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Wed, Aug 6, 2014 at 9:35 PM, Bruce Momjian br...@momjian.us wrote: On Sun, Jan 12, 2014 at 12:53:40PM -0500, Noah Misch wrote: On Sat, Jan 11, 2014 at 02:10:01PM -0500, Bruce Momjian wrote: On Mon, Jun 3, 2013 at 03:07:27PM -0400, Noah Misch wrote: A colleague, Korry Douglas, observed a table partitioning scenario where deserializing pg_constraint.ccbin is a hot spot. The following test case, a simplification of a typical partitioning setup, spends 28% of its time in stringToNode() and callees thereof: Noah, what is the status on this? Further study revealed a defect in the patch's memory management, and I have not gotten around to correcting that. I talked to Noah and he can't continue on this item. Can someone else work on it? Well, it would be helpful if he could describe the defect he found, so that the next person doesn't have to guess. -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Sun, Jan 12, 2014 at 12:53:40PM -0500, Noah Misch wrote: On Sat, Jan 11, 2014 at 02:10:01PM -0500, Bruce Momjian wrote: On Mon, Jun 3, 2013 at 03:07:27PM -0400, Noah Misch wrote: A colleague, Korry Douglas, observed a table partitioning scenario where deserializing pg_constraint.ccbin is a hot spot. The following test case, a simplification of a typical partitioning setup, spends 28% of its time in stringToNode() and callees thereof: Noah, what is the status on this? Further study revealed a defect in the patch's memory management, and I have not gotten around to correcting that. I talked to Noah and he can't continue on this item. Can someone else work on it? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Sat, Jan 11, 2014 at 02:10:01PM -0500, Bruce Momjian wrote: On Mon, Jun 3, 2013 at 03:07:27PM -0400, Noah Misch wrote: A colleague, Korry Douglas, observed a table partitioning scenario where deserializing pg_constraint.ccbin is a hot spot. The following test case, a simplification of a typical partitioning setup, spends 28% of its time in stringToNode() and callees thereof: Noah, what is the status on this? Further study revealed a defect in the patch's memory management, and I have not gotten around to correcting that. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Mon, Jun 3, 2013 at 03:07:27PM -0400, Noah Misch wrote: A colleague, Korry Douglas, observed a table partitioning scenario where deserializing pg_constraint.ccbin is a hot spot. The following test case, a simplification of a typical partitioning setup, spends 28% of its time in stringToNode() and callees thereof: Noah, what is the status on this? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Friday, June 07, 2013 2:10 AM Noah Misch wrote: On Thu, Jun 06, 2013 at 07:02:27PM +0530, Amit Kapila wrote: On Tuesday, June 04, 2013 12:37 AM Noah Misch wrote: This patch can give good performance gain in the scenario described by you. Infact I had taken the readings with patch, it shows similar gain. Thanks for testing. This patch would increase the relcache size, as for each constraint on table it would increase 4 bytes irrespective of whether that can give performance benefit or not. Yep, sizeof(Node *) bytes. So the memory increase number's would like: Example for 64-bit m/c In database, there are 100 tables, each having 2 constraints and 30 live connections Size increase = no. of tables * size of (Node*) * number of constraints * no. of live sessions = 100 * 8 * 2 * 30 = 46.8K So if such a memory increase seems reasonable, then I think it would be really beneficial for the load of data in inherited tables. If no one has objections on this part, then I think this is really useful patch for PostgreSQL. With Regards, Amit Kapila. -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Fri, Jun 07, 2013 at 11:58:14AM +0530, Amit Kapila wrote: So the memory increase number's would like: Example for 64-bit m/c In database, there are 100 tables, each having 2 constraints and 30 live connections Size increase = no. of tables * size of (Node*) * number of constraints * no. of live sessions = 100 * 8 * 2 * 30 = 46.8K That's the increase when nothing consults the constraints. Upon first use of a particular constraint, one cache can easily grow by a few hundred bytes. So if such a memory increase seems reasonable, then I think it would be really beneficial for the load of data in inherited tables. I agree that the memory usage is the main downside. We already do similar caching for index expressions and for rules, incidentally. On that note, I see that the patch does not make RelationDestroyRelation() correctly free the newly-cached data. Will fix. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Tuesday, June 04, 2013 12:37 AM Noah Misch wrote: A colleague, Korry Douglas, observed a table partitioning scenario where deserializing pg_constraint.ccbin is a hot spot. The following test case, a simplification of a typical partitioning setup, spends 28% of its time in stringToNode() and callees thereof: The executor caches each CHECK constraint in ResultRelInfo as a planned expression. That cache is highly effectively for long-running statements, but the trivial INSERTs effectively work without a cache. Korry devised this patch to cache the stringToNode() form of the constraint in the relcache. It improves the benchmark's partitioned scenario by 33%: -- Timings (seconds) -- master, INSERT parent: 14.2, 14.4, 14.4 patched, INSERT parent: 9.6, 9.7, 9.7 master, INSERT*10 child: 9.9, 9.9, 10.2 patched, INSERT*10 child:10.0, 10.2, 10.2 There's still not much to like about that tenfold overhead from use of the partition routing trigger, but this patch makes a nice cut into that overhead without doing anything aggressive. This patch can give good performance gain in the scenario described by you. Infact I had taken the readings with patch, it shows similar gain. -- Timings (seconds) -- master, INSERT parent: 14.9, 15.4, 15.4 patched, INSERT parent: 9.9, 9.6, 9.5 master, INSERT*10 child: 13.8, 14.5, 15.6 patched, INSERT*10 child:13.0, 14.3, 14.6 This patch would increase the relcache size, as for each constraint on table it would increase 4 bytes irrespective of whether that can give performance benefit or not. Why in function CheckConstraintFetch(), the node is not formed from string? Some call sites need to modify the node tree, so the patch has them do copyObject(). I ran a microbenchmark of copyObject() on the cached node tree vs. redoing stringToNode(), and copyObject() still won by a factor of four. I have not tried any performance run to measure if extra copyObject() has added any benefit. What kind of benchmark you use to validate it? With Regards, Amit Kapila. -- 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] Partitioning performance: cache stringToNode() of pg_constraint.ccbin
On Thu, Jun 06, 2013 at 07:02:27PM +0530, Amit Kapila wrote: On Tuesday, June 04, 2013 12:37 AM Noah Misch wrote: This patch can give good performance gain in the scenario described by you. Infact I had taken the readings with patch, it shows similar gain. Thanks for testing. This patch would increase the relcache size, as for each constraint on table it would increase 4 bytes irrespective of whether that can give performance benefit or not. Yep, sizeof(Node *) bytes. Why in function CheckConstraintFetch(), the node is not formed from string? That's to avoid the cost of stringToNode() if the field is not needed during the life of the cache entry. Some call sites need to modify the node tree, so the patch has them do copyObject(). I ran a microbenchmark of copyObject() on the cached node tree vs. redoing stringToNode(), and copyObject() still won by a factor of four. I have not tried any performance run to measure if extra copyObject() has added any benefit. Callers must not modify the cache entry; those that would otherwise do so must use copyObject() first. I benchmarked to ensure they wouldn't be better off ignoring the cached node tree and calling stringToNode() themselves. What kind of benchmark you use to validate it? It boiled down to comparing runtime of loops like these: while (n-- 0) copyObject(RelationGetConstraint(r, 0)); while (n-- 0) stringToNode(r-rd_att-constr-check[0].ccbin); -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
