On Wed, Jun 17, 2015 at 9:56 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Jun 17, 2015 at 9:32 AM, Thomas Munro
> <thomas.mu...@enterprisedb.com> wrote:
>> We saw a rather extreme performance problem in a cluster upgraded from
>> 9.1 to 9.3. It uses a largish number of child tables (partitions) and
>> many columns. Planning a simple UPDATE via the base table started
>> using a very large amount of memory and CPU time.
>>
>> My colleague Rushabh Lathia tracked the performance change down to
>> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=c03ad5602f529787968fa3201b35c119bbc6d782
>> .
>>
>> The call to copyObject in the loop introduced here seems to be
>> problematic (copying append_rel_list for every element in
>> append_rel_list unconditionally), though we're still trying to figure
>> it out. Attached is a simple repro script, with variables to tweak.
>>
>> Quite a few others have posted about this sort of thing and been
>> politely reminded of the 100 table caveat [1][2] which is fair enough,
>> but the situations seems to have got dramatically worse for some users
>> after an upgrade.
>
> Yes. The copyObject() call introduced by this commit seems to have
> complexity O(T^2*C) where T is the number of child tables and C is the
> number of columns per child. That's because the List that is being
> copied is a list of AppendRelInfo nodes, each of which has a
> translated_vars member that is a list of every Var in one table, and
> we copy that list once per child.
>
> It appears that in a lot of cases this work is unnecessary. The
> second (long) for loop in inheritance_planner copies root->rowMarks
> and root->append_rel_list so as to be able to apply ChangeVarNodes()
> to the result, but we only actually do that if the rte is of type
> RTE_SUBQUERY or if it has security quals. In the common case where we
> reach inheritance_planner not because of UNION ALL but just because
> the table has a bunch of inheritance children (none of which have RLS
> policies applied), we copy everything and then modify none of it,
> using up startling large amounts of memory in ways that pre-9.2
> versions did not.
The attached patch helps. It does two things:
1. It arranges for inheritance_planner to throw away the memory
consumed by the subroot's rowMarks and append_rel_list after the call
to grouping_planner for that subroot returns. This prevents the
explosive growth of memory usage in all cases I've tested so far, but
planning is still really slow.
2. It arranges not to deep-copy append_rel_list when the root's
append_rel_list doesn't need to be modified for the subroot. This
makes planning much much faster in simple cases, like a simple update
on a table with many partitions. But if you do attach a FROM clause
containing a subquery to such an update, then this optimization
doesn't kick in any more and things are still very slow (though still
memory-bounded, due to part 1).
I feel I might be missing a trick here. It seems unlikely to me that
we actually need the entire append_rel_list for every subquery; and we
almost certainly don't need to modify every element of the
append_rel_list for every subquery. Even the ones that no
ChangeVarNodes() call mutates still get deep-copied.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 8afde2b..84c75f1 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -43,6 +43,7 @@
#include "parser/parsetree.h"
#include "parser/parse_agg.h"
#include "rewrite/rewriteManip.h"
+#include "utils/memutils.h"
#include "utils/rel.h"
#include "utils/selfuncs.h"
@@ -845,9 +846,17 @@ inheritance_planner(PlannerInfo *root)
List *returningLists = NIL;
List *rowMarks;
ListCell *lc;
+ MemoryContext myctx;
+ MemoryContext oldctx;
Assert(parse->commandType != CMD_INSERT);
+ myctx = AllocSetContextCreate(CurrentMemoryContext,
+ "inheritance_planner",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+
/*
* We generate a modified instance of the original Query for each target
* relation, plan that, and put all the plans into a list that will be
@@ -908,18 +917,16 @@ inheritance_planner(PlannerInfo *root)
/*
* The rowMarks list might contain references to subquery RTEs, so
- * make a copy that we can apply ChangeVarNodes to. (Fortunately, the
- * executor doesn't need to see the modified copies --- we can just
- * pass it the original rowMarks list.)
+ * make a copy that we can apply ChangeVarNodes to. If any security
+ * barrier quals are present, the rowMarks list may be further modified
+ * by grouping_planner. (Fortunately, the executor doesn't need to see
+ * the modified copies --- we can just pass it the original rowMarks
+ * list. For the same reason, we can arrange to throw away the copy
+ * we make here relatively quickly.)
*/
+ oldctx = MemoryContextSwitchTo(myctx);
subroot.rowMarks = (List *) copyObject(root->rowMarks);
-
- /*
- * The append_rel_list likewise might contain references to subquery
- * RTEs (if any subqueries were flattenable UNION ALLs). So prepare
- * to apply ChangeVarNodes to that, too.
- */
- subroot.append_rel_list = (List *) copyObject(root->append_rel_list);
+ MemoryContextSwitchTo(oldctx);
/*
* Add placeholders to the child Query's rangetable list to fill the
@@ -942,6 +949,7 @@ inheritance_planner(PlannerInfo *root)
if (final_rtable != NIL)
{
ListCell *lr;
+ bool did_copy = false;
rti = 1;
foreach(lr, parse->rtable)
@@ -960,6 +968,21 @@ inheritance_planner(PlannerInfo *root)
Index newrti;
/*
+ * The append_rel_list likewise might contain references
+ * to subquery RTEs (if any subqueries were flattenable
+ * UNION ALLs). So we need to mutate that too, and must
+ * therefore copy it.
+ */
+ if (!did_copy)
+ {
+ oldctx = MemoryContextSwitchTo(myctx);
+ subroot.append_rel_list =
+ (List *) copyObject(root->append_rel_list);
+ MemoryContextSwitchTo(oldctx);
+ did_copy = true;
+ }
+
+ /*
* The RTE can't contain any references to its own RT
* index, except in the security barrier quals, so we can
* save a few cycles by applying ChangeVarNodes before we
@@ -989,6 +1012,9 @@ inheritance_planner(PlannerInfo *root)
/* Generate plan */
subplan = grouping_planner(&subroot, 0.0 /* retrieve all tuples */ );
+ /* Reclaim some memory */
+ MemoryContextReset(myctx);
+
/*
* Planning may have modified the query result relation (if there were
* security barrier quals on the result RTE).
@@ -1097,6 +1123,8 @@ inheritance_planner(PlannerInfo *root)
Assert(!parse->onConflict);
}
+ MemoryContextDelete(myctx);
+
/* Mark result as unordered (probably unnecessary) */
root->query_pathkeys = NIL;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
