pg.ddx.io pgsql-performance@postgresql.org mailing list archive
help / color / mirror / Atom feedRe: Planning performance problem (67626.278ms)
12+ messages / 4 participants
[nested] [flat]
* Re: Planning performance problem (67626.278ms)
@ 2021-06-14 17:25 Manuel Weitzman <manuelweitzman@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Manuel Weitzman @ 2021-06-14 17:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
> However, I'm skeptical that any problem actually remains in
> real-world use cases.
Hello Tom,
We also had some issues with planning and get_actual_variable_range(). We
actually found some interesting behaviour that probably requires an eye with
better expertise in how the planner works.
For the example being discussed you can add some joins into the equation and
planning times deteriorate quite a bit.
I'll just skip posting the first executions as it is already established that
a subsequent one will be faster.
create table b (b int primary key, a int references a(a))
with (autovacuum_enabled=off);
insert into a select x from generate_series(1,10000000) x;
insert into b select x, x from generate_series(1,10000000) x;
create index b_a_idx on b(a);
analyze a, b;
For our case a rollback of a bulk insert causes bloat on the index.
begin;
insert into a select x from generate_series(10000001,20000000) x;
rollback;
explain (analyze, buffers)
select * from a
join b on (b.a = a.a)
where b.a in (1,100,10000,1000000,1000001);
Planning:
Buffers: shared hit=9 read=27329
Planning Time: 134.560 ms
Execution Time: 0.100 ms
I see a lot of buffers being read for some reason (wasn't this fixed?). And
times are slow too. But it get's worse with each join added to the select.
explain (analyze, buffers)
select * from a
join b b1 on (b1.a = a.a)
join b b2 on (b2.a = a.a)
where b1.a in (1,100,10000,1000000,1000001);
Planning:
Buffers: shared hit=38 read=81992
Planning Time: 312.826 ms
Execution Time: 0.131 ms
Just add a few more joins and it is a recipe for disaster.
Apparently, the planner isn't reusing the data boundaries across alternative
plans. It would be nicer if the planner remembered each column boundaries
for later reuse (within the same planner execution).
Another thing that worries me is that even the second run has faster planning
it is still way slower than the case without lots of bloat in the index. And
I don't think this is just an edge case. Rollbacks on bulk inserts can be
quite common, and joins are expected in a SQL database.
We had downtime due to how the planner works on this case. Unfortunately
setting more aggressive vacuum settings won't fix our problems. Most of the
read queries are being issued to a replica. When the issues with the planner
start happening, CPU usage on that node goes to 100% which interferes with the
replication process.
This means the replica cannot get to a new checkpoint with a new live
max value in the index nor can it delete the bloat that vacuum has already
cleaned on the leader server.
Oh, by the way, we're running version 13.2
Regards,
Manuel
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-20 00:09 Manuel Weitzman <manuelweitzman@gmail.com>
parent: Manuel Weitzman <manuelweitzman@gmail.com>
0 siblings, 2 replies; 12+ messages in thread
From: Manuel Weitzman @ 2021-06-20 00:09 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
Hello everyone,
> Apparently, the planner isn't reusing the data boundaries across alternative
> plans. It would be nicer if the planner remembered each column boundaries
> for later reuse (within the same planner execution).
I've written a very naive (and crappy) patch to show how adding
memorization to get_actual_variable_range() could help the planner on
scenarios with a big number of joins.
For the previous example,
> explain (analyze, buffers)
> select * from a
> join b b1 on (b1.a = a.a)
> join b b2 on (b2.a = a.a)
> where b1.a in (1,100,10000,1000000,1000001);
each time you add a join clause the planner has to read an extra ~5[K]
buffers and gets about 200[ms] slower.
1 join
Planning:
Buffers: shared hit=9 read=27329
Planning Time: 101.745 ms
Execution Time: 0.082 ms
2 joins
Planning:
Buffers: shared hit=42 read=81988
Planning Time: 303.237 ms
Execution Time: 0.102 ms
3 joins
Planning:
Buffers: shared hit=94 read=136660
Planning Time: 508.947 ms
Execution Time: 0.155 ms
4 joins
Planning:
Buffers: shared hit=188 read=191322
Planning Time: 710.981 ms
Execution Time: 0.168 ms
After adding memorization the cost in buffers remains constant and the
latency deteriorates only marginally (as expected) with each join.
1 join
Planning:
Buffers: shared hit=10 read=27328
Planning Time: 97.889 ms
Execution Time: 0.066 ms
2 joins
Planning:
Buffers: shared hit=7 read=27331
Planning Time: 100.589 ms
Execution Time: 0.111 ms
3 joins
Planning:
Buffers: shared hit=9 read=27329
Planning Time: 105.669 ms
Execution Time: 0.134 ms
4 joins
Planning:
Buffers: shared hit=132 read=27370
Planning Time: 155.716 ms
Execution Time: 0.219 ms
I'd be happy to improve this patch into something better. Though I'd
like suggestions on how to do it:
I have this idea of creating a local "memorization" struct instance within
standard_planner(). That would require passing on a pointer down until
it reaches get_actual_variable_range(), which I think would be quite
ugly, if done just to improve the planner for this scenario.
Is there any better mechanism I could reuse from other modules? (utils
or cache, for example).
Regards,
Manuel
Attachments:
[application/octet-stream] actual_variable_range_memorization.diff (5.7K, ../../F06D0C00-DE75-4688-98B0-85A46E8F312C@gmail.com/2-actual_variable_range_memorization.diff)
download | inline diff:
diff --git i/src/backend/optimizer/plan/planner.c w/src/backend/optimizer/plan/planner.c
index 5d515842dc..65d74be557 100644
--- i/src/backend/optimizer/plan/planner.c
+++ w/src/backend/optimizer/plan/planner.c
@@ -316,6 +316,10 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
glob->transientPlan = false;
glob->dependsOnRole = false;
+
+ /* clear crappy variable range cache */
+ clear_variable_range_cache();
+
/*
* Assess whether it's feasible to use parallel mode for this query. We
* can't do this in a standalone backend, or if the command will try to
diff --git i/src/backend/utils/adt/selfuncs.c w/src/backend/utils/adt/selfuncs.c
index e96bfce4dc..0e0cc68307 100644
--- i/src/backend/utils/adt/selfuncs.c
+++ w/src/backend/utils/adt/selfuncs.c
@@ -5567,6 +5567,99 @@ get_stats_slot_range(AttStatsSlot *sslot, Oid opfuncoid, FmgrInfo *opproc,
}
+/*
+ * a lot of crappy code to emulate variable range caching
+ * just intended for testing purposes
+ */
+typedef struct ActualVariableRangeCache ActualVariableRangeCache;
+struct ActualVariableRangeCache {
+ Oid indexoid;
+ int64 min_value;
+ int64 max_value;
+ bool has_min;
+ bool has_max;
+};
+
+static unsigned long range_caches_size = 0;
+static ActualVariableRangeCache range_caches[1000] = {{0, 0, 0, false, false}};
+
+void
+clear_variable_range_cache(void)
+{
+ for (unsigned long i = 0; i < range_caches_size; i++) {
+ range_caches[i].indexoid = 0;
+ range_caches[i].has_min = false;
+ range_caches[i].has_max = false;
+ }
+ range_caches_size = 0;
+}
+
+static bool
+get_cached_actual_variable_min(Oid indexoid, Datum *min)
+{
+ for (unsigned long i = 0; i < range_caches_size; i++) {
+ if (range_caches[i].indexoid == indexoid) {
+ if (!range_caches[i].has_min)
+ return false;
+
+ *min = range_caches[i].min_value;
+ return true;
+ }
+ }
+ return false;
+}
+
+
+static bool
+get_cached_actual_variable_max(Oid indexoid, Datum *max)
+{
+ for (unsigned long i = 0; i < range_caches_size; i++) {
+ if (range_caches[i].indexoid == indexoid) {
+ if (!range_caches[i].has_max)
+ return false;
+
+ *max = range_caches[i].max_value;
+ return true;
+ }
+ }
+ return false;
+}
+
+
+static void
+add_actual_variable_min_to_cache(Oid indexoid, Datum *min)
+{
+ for (unsigned long i = 0; i < range_caches_size; i++) {
+ if (range_caches[i].indexoid == indexoid) {
+ range_caches[i].min_value = *min;
+ range_caches[i].has_min = true;
+ return;
+ }
+ }
+ range_caches[range_caches_size].indexoid = indexoid;
+ range_caches[range_caches_size].min_value = *min;
+ range_caches[range_caches_size].has_min = true;
+ range_caches_size++;
+}
+
+
+static void
+add_actual_variable_max_to_cache(Oid indexoid, Datum *max)
+{
+ for (unsigned long i = 0; i < range_caches_size; i++) {
+ if (range_caches[i].indexoid == indexoid) {
+ range_caches[i].max_value = *max;
+ range_caches[i].has_max = true;
+ return;
+ }
+ }
+ range_caches[range_caches_size].indexoid = indexoid;
+ range_caches[range_caches_size].max_value = *max;
+ range_caches[range_caches_size].has_max = true;
+ range_caches_size++;
+}
+
+
/*
* get_actual_variable_range
* Attempt to identify the current *actual* minimum and/or maximum
@@ -5691,15 +5784,23 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
/* If min is requested ... */
if (min)
{
- have_data = get_actual_variable_endpoint(heapRel,
- indexRel,
- indexscandir,
- scankeys,
- typLen,
- typByVal,
- slot,
- oldcontext,
- min);
+ /* try to reuse min from previous planner nodes */
+ have_data = get_cached_actual_variable_min(index->indexoid, min);
+ /* or fetch from index if first time here */
+ if (!have_data)
+ have_data = get_actual_variable_endpoint(heapRel,
+ indexRel,
+ indexscandir,
+ scankeys,
+ typLen,
+ typByVal,
+ slot,
+ oldcontext,
+ min);
+ /* update the min memorization */
+ if (have_data)
+ add_actual_variable_min_to_cache(index->indexoid,
+ min);
}
else
{
@@ -5710,16 +5811,25 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
/* If max is requested, and we didn't find the index is empty */
if (max && have_data)
{
+ /* try to reuse max from previous planner nodes */
+ have_data = get_cached_actual_variable_max(index->indexoid, max);
+
+ /* or fetch from index if first time here */
/* scan in the opposite direction; all else is the same */
- have_data = get_actual_variable_endpoint(heapRel,
- indexRel,
- -indexscandir,
- scankeys,
- typLen,
- typByVal,
- slot,
- oldcontext,
- max);
+ if (!have_data)
+ have_data = get_actual_variable_endpoint(heapRel,
+ indexRel,
+ -indexscandir,
+ scankeys,
+ typLen,
+ typByVal,
+ slot,
+ oldcontext,
+ max);
+ /* update the max memorization */
+ if (have_data)
+ add_actual_variable_max_to_cache(index->indexoid,
+ max);
}
/* Clean everything up */
diff --git i/src/include/utils/selfuncs.h w/src/include/utils/selfuncs.h
index 7ac4a06391..a46c272ac7 100644
--- i/src/include/utils/selfuncs.h
+++ w/src/include/utils/selfuncs.h
@@ -220,4 +220,7 @@ extern Selectivity scalararraysel_containment(PlannerInfo *root,
Oid elemtype, bool isEquality, bool useOr,
int varRelid);
+extern void
+clear_variable_range_cache(void);
+
#endif /* SELFUNCS_H */
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-20 21:06 Tom Lane <tgl@sss.pgh.pa.us>
parent: Manuel Weitzman <manuelweitzman@gmail.com>
1 sibling, 0 replies; 12+ messages in thread
From: Tom Lane @ 2021-06-20 21:06 UTC (permalink / raw)
To: Manuel Weitzman <manuelweitzman@gmail.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
Manuel Weitzman <manuelweitzman@gmail.com> writes:
> I've written a very naive (and crappy) patch to show how adding
> memorization to get_actual_variable_range() could help the planner on
> scenarios with a big number of joins.
So ... the reason why there's not caching of get_actual_variable_range
results already is that I'd supposed it wouldn't be necessary given
the caching of selectivity estimates that happens at the RestrictInfo
level. I don't have any objection in principle to adding another
caching layer if that one's not working well enough, but I think it'd
be wise to first understand why it's needed.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-20 23:23 Ranier Vilela <ranier.vf@gmail.com>
parent: Manuel Weitzman <manuelweitzman@gmail.com>
1 sibling, 1 reply; 12+ messages in thread
From: Ranier Vilela @ 2021-06-20 23:23 UTC (permalink / raw)
To: Manuel Weitzman <manuelweitzman@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
Em dom., 20 de jun. de 2021 às 14:50, Manuel Weitzman <
manuelweitzman@gmail.com> escreveu:
> Hello everyone,
>
> > Apparently, the planner isn't reusing the data boundaries across
> alternative
> > plans. It would be nicer if the planner remembered each column boundaries
> > for later reuse (within the same planner execution).
>
> I've written a very naive (and crappy) patch to show how adding
> memorization to get_actual_variable_range() could help the planner on
> scenarios with a big number of joins.
>
> For the previous example,
>
> > explain (analyze, buffers)
> > select * from a
> > join b b1 on (b1.a = a.a)
> > join b b2 on (b2.a = a.a)
> > where b1.a in (1,100,10000,1000000,1000001);
>
> each time you add a join clause the planner has to read an extra ~5[K]
> buffers and gets about 200[ms] slower.
>
> 1 join
> Planning:
> Buffers: shared hit=9 read=27329
> Planning Time: 101.745 ms
> Execution Time: 0.082 ms
>
> 2 joins
> Planning:
> Buffers: shared hit=42 read=81988
> Planning Time: 303.237 ms
> Execution Time: 0.102 ms
>
> 3 joins
> Planning:
> Buffers: shared hit=94 read=136660
> Planning Time: 508.947 ms
> Execution Time: 0.155 ms
>
> 4 joins
> Planning:
> Buffers: shared hit=188 read=191322
> Planning Time: 710.981 ms
> Execution Time: 0.168 ms
>
>
> After adding memorization the cost in buffers remains constant and the
> latency deteriorates only marginally (as expected) with each join.
>
> 1 join
> Planning:
> Buffers: shared hit=10 read=27328
> Planning Time: 97.889 ms
> Execution Time: 0.066 ms
>
> 2 joins
> Planning:
> Buffers: shared hit=7 read=27331
> Planning Time: 100.589 ms
> Execution Time: 0.111 ms
>
> 3 joins
> Planning:
> Buffers: shared hit=9 read=27329
> Planning Time: 105.669 ms
> Execution Time: 0.134 ms
>
> 4 joins
> Planning:
> Buffers: shared hit=132 read=27370
> Planning Time: 155.716 ms
> Execution Time: 0.219 ms
>
>
> I'd be happy to improve this patch into something better. Though I'd
> like suggestions on how to do it:
> I have this idea of creating a local "memorization" struct instance within
> standard_planner(). That would require passing on a pointer down until
> it reaches get_actual_variable_range(), which I think would be quite
> ugly, if done just to improve the planner for this scenario.
> Is there any better mechanism I could reuse from other modules? (utils
> or cache, for example).
>
Without going into the merits of whether this cache will be adopted or not,
I have some comments about the code.
1. Prefer to use .patch instead of .diff, it makes it easier for browsers
such as firefox to read and show the content automatically.
2. New struct?
Oid is unsigned int, lower than int64.
Better struct is:
+struct ActualVariableRangeCache {
+ int64 min_value; /* 8 bytes */
+ int64 max_value; /* 8 bytes */
+ Oid indexoid; /* 4 bytes */
+ bool has_min; /* 1 byte */
+ bool has_max; /*1 byte */
+};
Takes up less space.
3. Avoid use of type *long*, it is very problematic with 64 bits.
Windows 64 bits, long is 4 (four) bytes.
Linux 64 bits, long is 8 (eight) bytes.
4. Avoid C99 style declarations
for(unsigned long i = 0;)
Prefer:
size_t i;
for(i = 0;)
Helps backpatching to C89 versions.
regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-21 00:17 Tom Lane <tgl@sss.pgh.pa.us>
parent: Ranier Vilela <ranier.vf@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2021-06-21 00:17 UTC (permalink / raw)
To: Ranier Vilela <ranier.vf@gmail.com>; +Cc: Manuel Weitzman <manuelweitzman@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
Ranier Vilela <ranier.vf@gmail.com> writes:
> 3. Avoid use of type *long*, it is very problematic with 64 bits.
> Windows 64 bits, long is 4 (four) bytes.
> Linux 64 bits, long is 8 (eight) bytes.
Agreed.
> 4. Avoid C99 style declarations
> for(unsigned long i = 0;)
> Prefer:
> size_t i;
> for(i = 0;)
> Helps backpatching to C89 versions.
It seems unlikely that we'd consider back-patching this into pre-C99
branches, so I see no reason not to use C99 loop style. (But do
keep in mind that we avoid most other C99-isms, such as intermixed
decls and code.)
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-29 19:26 Manuel Weitzman <manuelweitzman@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Manuel Weitzman @ 2021-06-29 19:26 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Ranier Vilela <ranier.vf@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
> On 20-06-2021, at 17:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> So ... the reason why there's not caching of get_actual_variable_range
> results already is that I'd supposed it wouldn't be necessary given
> the caching of selectivity estimates that happens at the RestrictInfo
> level. I don't have any objection in principle to adding another
> caching layer if that one's not working well enough, but I think it'd
> be wise to first understand why it's needed.
For what I could make out from the code, the caching done at the
RestrictInfo level is already saving a lot of work, but there's a
different RestrictInfo instance for each alternative path created by
make_one_rel().
I wouldn't know how to reuse instances of RestrictInfo or if that
would even be the correct approach. My guess is that doing so would
be incorrect.
I'll improve the patch with the suggestions from Ranier and you in
the meantime.
Best regards,
Manuel
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-29 19:43 Tom Lane <tgl@sss.pgh.pa.us>
parent: Manuel Weitzman <manuelweitzman@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2021-06-29 19:43 UTC (permalink / raw)
To: Manuel Weitzman <manuelweitzman@gmail.com>; +Cc: Ranier Vilela <ranier.vf@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
Manuel Weitzman <manuelweitzman@gmail.com> writes:
>> On 20-06-2021, at 17:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So ... the reason why there's not caching of get_actual_variable_range
>> results already is that I'd supposed it wouldn't be necessary given
>> the caching of selectivity estimates that happens at the RestrictInfo
>> level. I don't have any objection in principle to adding another
>> caching layer if that one's not working well enough, but I think it'd
>> be wise to first understand why it's needed.
> For what I could make out from the code, the caching done at the
> RestrictInfo level is already saving a lot of work, but there's a
> different RestrictInfo instance for each alternative path created by
> make_one_rel().
That seems a bit broken; a given WHERE clause should produce only one
RestrictInfo. Can you provide a more concrete example?
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-29 21:35 Manuel Weitzman <manuelweitzman@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Manuel Weitzman @ 2021-06-29 21:35 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Ranier Vilela <ranier.vf@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
> On 29-06-2021, at 15:43, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Manuel Weitzman <manuelweitzman@gmail.com> writes:
>>> On 20-06-2021, at 17:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> So ... the reason why there's not caching of get_actual_variable_range
>>> results already is that I'd supposed it wouldn't be necessary given
>>> the caching of selectivity estimates that happens at the RestrictInfo
>>> level. I don't have any objection in principle to adding another
>>> caching layer if that one's not working well enough, but I think it'd
>>> be wise to first understand why it's needed.
>
>> For what I could make out from the code, the caching done at the
>> RestrictInfo level is already saving a lot of work, but there's a
>> different RestrictInfo instance for each alternative path created by
>> make_one_rel().
>
> That seems a bit broken; a given WHERE clause should produce only one
> RestrictInfo. Can you provide a more concrete example?
>
I added some logging to see hits and misses on cached_scansel() for
this query
> explain (analyze, buffers)
> select * from a
> join b b1 on (b1.a = a.a)
> join b b2 on (b2.a = a.a)
> where b1.a in (1,100,10000,1000000,1000001);
Apparently there's a RestrictInfo for each possible way of doing merge
join (are those created dynamically for planning?), for example:
- a join (b1 join b2)
- b1 join (a join b2)
- b2 join (a join b1)
When the cost of a possible mergejoin path hasn't been computed yet,
then mergejoinscansel() would have to check the bloated index again.
I attached a patch so you can see the hits and misses on cached_scansel().
Each time there's a miss logged, there's also a different RestrictInfo
pointer involved.
Best regards,
Manuel
Attachments:
[application/octet-stream] cached_scansel_hitmiss.patch (1.3K, ../../F9C9FBE0-C535-49DA-B913-F11AC30F8F4E@gmail.com/2-cached_scansel_hitmiss.patch)
download | inline diff:
diff --git i/src/backend/optimizer/path/costsize.c w/src/backend/optimizer/path/costsize.c
index ffd2bf8783..180cdc5f10 100644
--- i/src/backend/optimizer/path/costsize.c
+++ w/src/backend/optimizer/path/costsize.c
@@ -3431,10 +3431,21 @@ cached_scansel(PlannerInfo *root, RestrictInfo *rinfo, PathKey *pathkey)
if (cache->opfamily == pathkey->pk_opfamily &&
cache->collation == pathkey->pk_eclass->ec_collation &&
cache->strategy == pathkey->pk_strategy &&
- cache->nulls_first == pathkey->pk_nulls_first)
+ cache->nulls_first == pathkey->pk_nulls_first) {
+ elog(INFO, " [HIT][%p]\topfamily: %u, collation: %u, strategy: %d, nulls_first: %s, left_relids:%p, right_relids:%p",
+ rinfo,
+ pathkey->pk_opfamily, pathkey->pk_eclass->ec_collation,
+ pathkey->pk_strategy, pathkey->pk_nulls_first ? "t" : "f",
+ rinfo->left_relids, rinfo->right_relids);
return cache;
+ }
}
+ elog(INFO, "[MISS][%p]\topfamily:%u, collation: %u, strategy: %d, nulls_first: %s, left_relids:%p, right_relids:%p",
+ rinfo,
+ pathkey->pk_opfamily, pathkey->pk_eclass->ec_collation,
+ pathkey->pk_strategy, pathkey->pk_nulls_first ? "t" : "f",
+ rinfo->left_relids, rinfo->right_relids);
/* Nope, do the computation */
mergejoinscansel(root,
(Node *) rinfo->clause,
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-29 22:31 Tom Lane <tgl@sss.pgh.pa.us>
parent: Manuel Weitzman <manuelweitzman@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2021-06-29 22:31 UTC (permalink / raw)
To: Manuel Weitzman <manuelweitzman@gmail.com>; +Cc: Ranier Vilela <ranier.vf@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
Manuel Weitzman <manuelweitzman@gmail.com> writes:
> On 29-06-2021, at 15:43, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That seems a bit broken; a given WHERE clause should produce only one
>> RestrictInfo. Can you provide a more concrete example?
>> explain (analyze, buffers)
>> select * from a
>> join b b1 on (b1.a = a.a)
>> join b b2 on (b2.a = a.a)
>> where b1.a in (1,100,10000,1000000,1000001);
Hm. By my count, this example generates 3 RestrictInfos during
deconstruct_jointree, representing the three original clauses
from the query, and then 4 more in generate_join_implied_equalities,
representing the EC-derived clauses
a.a = b1.a
a.a = b2.a
b1.a = b2.a
b1.a = a.a
The third of these seems legit enough; it's a new fact that we
want to apply while considering joining b1 directly to b2.
The other ones get made despite create_join_clause's efforts
to avoid making duplicate clauses, because of two things:
1. create_join_clause doesn't trouble to look for commuted
equivalents, which perhaps is penny-wise and pound-foolish.
The cost of re-deriving selectivity estimates could be way
more than the cost of checking this.
2. Although these look like they ought to be equivalent to the
original clauses (modulo commutation, for some of them), they don't
look that way to create_join_clause, because it's also checking
for parent_ec equality. Per the comment,
* parent_ec is either equal to ec (if the clause is a potentially-redundant
* join clause) or NULL (if not). We have to treat this as part of the
* match requirements --- it's possible that a clause comparing the same two
* EMs is a join clause in one join path and a restriction clause in another.
It might be worth digging into the git history to see why that
became a thing and then considering whether there's a way around it.
(I'm pretty sure that comment is mine, but I don't recall the details
anymore.)
Anyway, it's certainly not the case that we're making new
RestrictInfos for every pair of rels. It looks that way in this
example because the join vars all belong to the same EC, but
that typically wouldn't be the case in more complex queries.
So we could look into whether this code can be improved to share
RestrictInfos across more cases. Another thought is that even
if we need to keep original and derived clauses separate, maybe it'd
be all right to copy previously-determined cached selectivity values
from an original clause to an otherwise-identical derived clause
(cf. commute_restrictinfo()). I'm not sure though whether it's
reliably the case that we'd have filled in selectivities for the
original clauses before this code wants to clone them.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-06-30 20:56 Manuel Weitzman <manuelweitzman@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 12+ messages in thread
From: Manuel Weitzman @ 2021-06-30 20:56 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Ranier Vilela <ranier.vf@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
> 1. create_join_clause doesn't trouble to look for commuted
> equivalents, which perhaps is penny-wise and pound-foolish.
> The cost of re-deriving selectivity estimates could be way
> more than the cost of checking this.
Agreed.
> 2. Although these look like they ought to be equivalent to the
> original clauses (modulo commutation, for some of them), they don't
> look that way to create_join_clause, because it's also checking
> for parent_ec equality. Per the comment,
>
> * parent_ec is either equal to ec (if the clause is a potentially-redundant
> * join clause) or NULL (if not). We have to treat this as part of the
> * match requirements --- it's possible that a clause comparing the same two
> * EMs is a join clause in one join path and a restriction clause in another.
>
> It might be worth digging into the git history to see why that
> became a thing and then considering whether there's a way around it.
> (I'm pretty sure that comment is mine, but I don't recall the details
> anymore.)
To me that sounds OK, I cannot prove that they're equivalent to the
original clauses so I think it is fine to assume they're not (not an
expert here, quite the opposite).
> Anyway, it's certainly not the case that we're making new
> RestrictInfos for every pair of rels. It looks that way in this
> example because the join vars all belong to the same EC, but
> that typically wouldn't be the case in more complex queries.
Good to know, this wasn't clear to me.
> So we could look into whether this code can be improved to share
> RestrictInfos across more cases. Another thought is that even
> if we need to keep original and derived clauses separate, maybe it'd
> be all right to copy previously-determined cached selectivity values
> from an original clause to an otherwise-identical derived clause
> (cf. commute_restrictinfo()). I'm not sure though whether it's
> reliably the case that we'd have filled in selectivities for the
> original clauses before this code wants to clone them.
To be honest, even if that sounds like a good idea to dig on, I think
it wouldn't completely solve the problem with repeated calls to
get_actual_variable_range().
The example query I gave is doing a lot of simple auto-joins which
makes the thought process simpler, but I worry more about the more
"common" case in which there is more than 2 distinct tables involved
in the query
For example, instead of having "b1, b2, ..., bn" as aliases of "b" in
this query
>> explain (analyze, buffers)
>> select * from a
>> join b b1 on (b1.a = a.a)
>> join b b2 on (b2.a = a.a)
>> where b1.a in (1,100,10000,1000000,1000001);
it is also possible to reproduce the increasing cost in planning
buffers for each new join on a distinct table being added:
explain (analyze, buffers)
select * from a
join b on (b.a = a.a)
join c on (c.a = a.a)
-- ... (etc)
where c.a in (1,100,10000,1000000,1000001);
I can imagine that deconstruct_jointree() and
generate_join_implied_equalities() would generate multiple
RestrictInfos, in which many of them a constraint on a.a would be
involved (each involving a different table).
b.a = a.a
c.a = a.a
c.a = b.a
a.a = b.a
a.a = c.a
... (etc)
(if we wanted, we could also add a different WHERE clause on each of
the tables involved to make really sure all RestrictInfos are
different).
For each of these RestrictInfos there *could* be one cache miss on
cached_scansel() that *could* force the planner to compute
get_actual_variable_range() for the same variable (a.a) over and over,
as mergejoinscansel() always computes the selectivity for the
intervals that require actual extremal values. In practice this
re-computing of the variable range seems to happen a lot.
One way in which I see possible to share this kind of information (of
extremal values) across RestrictInfos is to store the known variable
ranges in PlannerInfo (or within a member of such struct), which seems
to be around everywhere it would be needed.
Best regards,
Manuel
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-07-01 20:49 Manuel Weitzman <manuelweitzman@gmail.com>
parent: Manuel Weitzman <manuelweitzman@gmail.com>
1 sibling, 0 replies; 12+ messages in thread
From: Manuel Weitzman @ 2021-07-01 20:49 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Ranier Vilela <ranier.vf@gmail.com>; David Rowley <dgrowleyml@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
> On 30-06-2021, at 16:56, Manuel Weitzman <manuelweitzman@gmail.com> wrote:
>
> One way in which I see possible to share this kind of information (of
> extremal values) across RestrictInfos is to store the known variable
> ranges in PlannerInfo (or within a member of such struct), which seems
> to be around everywhere it would be needed.
I have written a new patch that's (hopefully) better than the first
one I sent, to store the extremal index values within PlannerInfo.
> it is also possible to reproduce the increasing cost in planning
> buffers for each new join on a distinct table being added:
>
> [...]
>
> I can imagine that deconstruct_jointree() and
> generate_join_implied_equalities() would generate multiple
> RestrictInfos, in which many of them a constraint on a.a would be
> involved (each involving a different table).
I also attached an example in which there are RestrictInfos generated
for multiple tables instead of just a single aliased one. The buffers
read for planning also increase with each join added to the query.
Best regards,
Manuel
Attachments:
[application/octet-stream] actual_variable_range_cache.patch (5.6K, ../../B7F75B2A-A241-46BA-B6FF-E95874C39D65@gmail.com/2-actual_variable_range_cache.patch)
download | inline diff:
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 5d515842dc..cb397c6012 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -636,6 +636,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
root->wt_param_id = -1;
root->non_recursive_path = NULL;
root->partColsUpdated = false;
+ root->actual_var_ranges = NIL;
/*
* If there is a WITH list, process each WITH query and either convert it
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 9ff2b595a8..9672934534 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -936,6 +936,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
subroot->hasRecursion = false;
subroot->wt_param_id = -1;
subroot->non_recursive_path = NULL;
+ subroot->actual_var_ranges = NIL;
/* No CTEs to worry about */
Assert(subquery->cteList == NIL);
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index e96bfce4dc..a6e73d0b8f 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -5585,9 +5585,12 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
Datum *min, Datum *max)
{
bool have_data = false;
+ bool have_cache = false;
+ ActualVarRangeInfo *cache = NULL;
RelOptInfo *rel = vardata->rel;
RangeTblEntry *rte;
ListCell *lc;
+ PlannerInfo *proot;
/* No hope if no relation or it doesn't have indexes */
if (rel == NULL || rel->indexlist == NIL)
@@ -5648,7 +5651,37 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
}
/*
- * Found a suitable index to extract data from. Set up some data that
+ * Found a suitable index to extract data from.
+ * Do we have the extremal values on PlannerInfo cache?
+ *
+ * If this is a subquery, traverse to the top-level PlannerInfo
+ * to read/store ActualVarRangeInfos.
+ */
+ proot = root;
+ while (proot->parent_root != NULL)
+ proot = proot->parent_root;
+
+ foreach(lc, proot->actual_var_ranges)
+ {
+ cache = (ActualVarRangeInfo *) lfirst(lc);
+
+ if (cache->indexoid != index->indexoid)
+ continue;
+ have_cache = true;
+
+ if (min && cache->min)
+ *min = datumCopy(cache->min, cache->typByVal, cache->typLen);
+ if (max && cache->max)
+ *max = datumCopy(cache->max, cache->typByVal, cache->typLen);
+
+ if ((min && !cache->min) || (max && !cache->max))
+ break;
+
+ return have_cache;
+ }
+
+ /*
+ * Nope, read actual range from index. Set up some data that
* can be used by both invocations of get_actual_variable_endpoint.
*/
{
@@ -5689,7 +5722,7 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
(Datum) 0); /* constant */
/* If min is requested ... */
- if (min)
+ if (min && (!have_cache || !cache->min))
{
have_data = get_actual_variable_endpoint(heapRel,
indexRel,
@@ -5708,7 +5741,7 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
}
/* If max is requested, and we didn't find the index is empty */
- if (max && have_data)
+ if (max && have_data && (!have_cache || !cache->max))
{
/* scan in the opposite direction; all else is the same */
have_data = get_actual_variable_endpoint(heapRel,
@@ -5731,6 +5764,36 @@ get_actual_variable_range(PlannerInfo *root, VariableStatData *vardata,
MemoryContextSwitchTo(oldcontext);
MemoryContextDelete(tmpcontext);
+ /*
+ * Store variable range in PlannerInfo cache for subsequent calls.
+ *
+ * If there's already a ActualVarRangeInfo for this index, just
+ * fill the missing information.
+ */
+ if (have_data && !have_cache)
+ {
+ oldcontext = MemoryContextSwitchTo(proot->planner_cxt);
+ cache = (ActualVarRangeInfo *) palloc(sizeof(ActualVarRangeInfo));
+
+ cache->indexoid = index->indexoid;
+ cache->min = PointerGetDatum(NULL);
+ cache->max = PointerGetDatum(NULL);
+ cache->typByVal = typByVal;
+ cache->typLen = typLen;
+
+ proot->actual_var_ranges = lappend(proot->actual_var_ranges,
+ cache);
+ MemoryContextSwitchTo(oldcontext);
+ }
+
+ if (have_data)
+ {
+ if (min && !cache->min)
+ cache->min = datumCopy(*min, typByVal, typLen);
+ if (max && !cache->max)
+ cache->max = datumCopy(*max, typByVal, typLen);
+ }
+
/* And we're done */
break;
}
diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h
index 69150e46eb..d92f169b76 100644
--- a/src/include/nodes/pathnodes.h
+++ b/src/include/nodes/pathnodes.h
@@ -362,6 +362,9 @@ struct PlannerInfo
/* Does this query modify any partition key columns? */
bool partColsUpdated;
+
+ /* List of ActualVarRangeInfos, a cache for get_actual_variable_range */
+ List *actual_var_ranges;
};
@@ -2333,6 +2336,24 @@ typedef struct MinMaxAggInfo
Param *param; /* param for subplan's output */
} MinMaxAggInfo;
+
+/*
+ * Store both extremal values of an index.
+ *
+ * The planner stores ActualVarRangeInfos in PlannerInfo to avoid repeated
+ * executions of get_actual_variable_range that can be very expensive,
+ * particularly if the index is bloated on any of the extremes.
+ */
+typedef struct ActualVarRangeInfo
+{
+ Datum min; /* index minimal value */
+ Datum max; /* index maximal value */
+ Oid indexoid;
+ int16 typLen;
+ bool typByVal;
+} ActualVarRangeInfo;
+
+
/*
* At runtime, PARAM_EXEC slots are used to pass values around from one plan
* node to another. They can be used to pass values down into subqueries (for
[application/octet-stream] planning_buffers.sql (1.5K, ../../B7F75B2A-A241-46BA-B6FF-E95874C39D65@gmail.com/3-planning_buffers.sql)
download
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: Planning performance problem (67626.278ms)
@ 2021-07-02 09:55 David Rowley <dgrowleyml@gmail.com>
parent: Manuel Weitzman <manuelweitzman@gmail.com>
1 sibling, 0 replies; 12+ messages in thread
From: David Rowley @ 2021-07-02 09:55 UTC (permalink / raw)
To: Manuel Weitzman <manuelweitzman@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Ranier Vilela <ranier.vf@gmail.com>; Jeremy Schneider <schnjere@amazon.com>; pgsql-performance@lists.postgresql.org <pgsql-performance@lists.postgresql.org>; Andres Freund <andres@anarazel.de>
On Thu, 1 Jul 2021 at 08:56, Manuel Weitzman <manuelweitzman@gmail.com> wrote:
> For each of these RestrictInfos there *could* be one cache miss on
> cached_scansel() that *could* force the planner to compute
> get_actual_variable_range() for the same variable (a.a) over and over,
> as mergejoinscansel() always computes the selectivity for the
> intervals that require actual extremal values. In practice this
> re-computing of the variable range seems to happen a lot.
Recently, for some other topic, I was thinking about if we were ever
to have the executor give up on a plan because something did not play
out the way the planner expected it to, that if the executor to ever
be given that ability to throw the plan back at the planner with some
hints about where it went wrong, then I wondered what exactly these
hints would look like.
I guessed these would be a list of some rough guidelines saying that x
JOIN y ON x.a=y.b produces at least Z rows. Then the planner would
find that when estimating the size of the join between x and y and
take the maximum of two values. That would need to be designed in
such a way that the planner could quickly consult the feedback, e.g
hashtable lookup on Oid.
Anyway, I don't really have any clearly thought-through plans for that
idea as it would be a large project that would need a lot of changes
before it could be even thought about seriously. However, it did cause
me to think about that again when reading this thread as it seems
similar. You've learned the actual variable range through actual
execution, so it does not seem too unreasonable that information might
get stored in a similar place, if that place were to exist.
I'm not saying that's what needs to happen here. It's more just food
for thought. The caching you need does seem more specific to Oid than
relid.
David
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2021-07-02 09:55 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-06-14 17:25 Re: Planning performance problem (67626.278ms) Manuel Weitzman <manuelweitzman@gmail.com>
2021-06-20 00:09 ` Manuel Weitzman <manuelweitzman@gmail.com>
2021-06-20 21:06 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-20 23:23 ` Ranier Vilela <ranier.vf@gmail.com>
2021-06-21 00:17 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-29 19:26 ` Manuel Weitzman <manuelweitzman@gmail.com>
2021-06-29 19:43 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-29 21:35 ` Manuel Weitzman <manuelweitzman@gmail.com>
2021-06-29 22:31 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-30 20:56 ` Manuel Weitzman <manuelweitzman@gmail.com>
2021-07-01 20:49 ` Manuel Weitzman <manuelweitzman@gmail.com>
2021-07-02 09:55 ` David Rowley <dgrowleyml@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox