Re: [HACKERS] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2017-01-06 Thread Robert Haas 
On Fri, Jan 6, 2017 at 11:06 AM, Andres Freund  wrote:
> On 2017-01-06 11:01:32 -0500, Robert Haas wrote:
>> On Fri, Jan 6, 2017 at 10:43 AM, Andres Freund  wrote:
>> > On 2016-12-16 09:34:31 -0800, Andres Freund wrote:
>> >> > To fix his issue, we need something like your 0001.  Are you going to
>> >> > polish that up soon here?
>> >>
>> >> Yes.
>> >
>> > I've two versions of a fix for this. One of them basically increases the
>> > "spread" of buckets when the density goes up too much. It does so by
>> > basically shifting the bucket number to the left (e.g. only every second
>> > bucket can be the "primary" bucket for a hash value).  The other
>> > basically just replaces the magic constants in my previous POC patch
>> > with slightly better documented constants.  For me the latter works just
>> > as well as the former, even though aesthetically/theoretically the
>> > former sounds better.  I'm inclined to commit the latter, at least for
>> > now.
>>
>> Did you intend to attach the patches?
>
> No, I hadn't. You're interested in the "spreading" version?

I honestly have no opinion.  If you're confident that you know what
you want to do, it's fine with me if you just do it.  If you want my
opinion I probably need to see both patches and compare.

-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2017-01-06 Thread Andres Freund 
On 2017-01-06 11:01:32 -0500, Robert Haas wrote:
> On Fri, Jan 6, 2017 at 10:43 AM, Andres Freund  wrote:
> > On 2016-12-16 09:34:31 -0800, Andres Freund wrote:
> >> > To fix his issue, we need something like your 0001.  Are you going to
> >> > polish that up soon here?
> >>
> >> Yes.
> >
> > I've two versions of a fix for this. One of them basically increases the
> > "spread" of buckets when the density goes up too much. It does so by
> > basically shifting the bucket number to the left (e.g. only every second
> > bucket can be the "primary" bucket for a hash value).  The other
> > basically just replaces the magic constants in my previous POC patch
> > with slightly better documented constants.  For me the latter works just
> > as well as the former, even though aesthetically/theoretically the
> > former sounds better.  I'm inclined to commit the latter, at least for
> > now.
> 
> Did you intend to attach the patches?

No, I hadn't. You're interested in the "spreading" version?

Andres


-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2017-01-06 Thread Robert Haas 
On Fri, Jan 6, 2017 at 10:43 AM, Andres Freund  wrote:
> On 2016-12-16 09:34:31 -0800, Andres Freund wrote:
>> > To fix his issue, we need something like your 0001.  Are you going to
>> > polish that up soon here?
>>
>> Yes.
>
> I've two versions of a fix for this. One of them basically increases the
> "spread" of buckets when the density goes up too much. It does so by
> basically shifting the bucket number to the left (e.g. only every second
> bucket can be the "primary" bucket for a hash value).  The other
> basically just replaces the magic constants in my previous POC patch
> with slightly better documented constants.  For me the latter works just
> as well as the former, even though aesthetically/theoretically the
> former sounds better.  I'm inclined to commit the latter, at least for
> now.

Did you intend to attach the patches?

-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2017-01-06 Thread Andres Freund 
On 2016-12-16 09:34:31 -0800, Andres Freund wrote:
> > To fix his issue, we need something like your 0001.  Are you going to
> > polish that up soon here?
>
> Yes.

I've two versions of a fix for this. One of them basically increases the
"spread" of buckets when the density goes up too much. It does so by
basically shifting the bucket number to the left (e.g. only every second
bucket can be the "primary" bucket for a hash value).  The other
basically just replaces the magic constants in my previous POC patch
with slightly better documented constants.  For me the latter works just
as well as the former, even though aesthetically/theoretically the
former sounds better.  I'm inclined to commit the latter, at least for
now.

Andres


-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-12-16 Thread Andres Freund 
On 2016-12-16 10:12:42 -0500, Robert Haas wrote:
> On Wed, Dec 14, 2016 at 11:20 PM, Robert Haas  wrote:
> > I've got no problem with that at all, but I want to unbreak things
> > more or less immediately and then you/we can further improve it later.
> 
> Committed

Thanks. Although "Fix drastic slowdown when ..." or so might have been a
less confusing commit message ;)

> , although I realize now that doesn't fix Dilip's problem,
> only my (somewhat different) problem.

Indeed. And the fix for Dilip's thing also fixes your issue, albeit what
you committed still helps noticeably (even with the old hashtable).

> To fix his issue, we need something like your 0001.  Are you going to
> polish that up soon here?

Yes.


Andres


-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-12-16 Thread Robert Haas 
On Wed, Dec 14, 2016 at 11:20 PM, Robert Haas  wrote:
> I've got no problem with that at all, but I want to unbreak things
> more or less immediately and then you/we can further improve it later.

Committed, although I realize now that doesn't fix Dilip's problem,
only my (somewhat different) problem.  To fix his issue, we need
something like your 0001.  Are you going to polish that up soon here?
I kinda want to get the regressions we've introduced fixed here.

-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-12-14 Thread Robert Haas 
On Wed, Dec 14, 2016 at 10:08 PM, Andres Freund  wrote:
> On 2016-12-14 22:00:45 -0500, Robert Haas wrote:
>> I took a look at Andres's patches today and saw that they can't really
>> be applied as-is, because they "temporarily" change the master's
>> ParallelWorkerNumber but have no provision for restoring it after an
>> ERROR.
>
> Yea, that's not quite right. Although I'm not sure it really matters
> that much, given that we're not continuing execution after an error. We
> could just reset it at appropriate points - but that seems ugly.

Yes.

>> I think that approach isn't right anyway; it seems to me that
>> what we want to do is set hash_iv based on we're in a Partial HashAgg,
>> not whether we're in a parallel worker.  For instance, if we're
>> somehow in a nodeSubplan.c there's no need for this just because we
>> happen to be in a worker -- I think.  That led me to develop the
>> attached patch.
>
> Hm, I'd rather have this be a more general solution - it doesn't seem
> unlikely that other parts of the system want to know whether they're
> currently executing a parallel portion of the plan. E.g. it really
> should be forbidden to do modifications even in the parallel portion of
> the plan the master executes.  Right now that'd escape notice, which I
> don't think is good.

Actually, that wouldn't escape notice.  You can test with
IsInParallelMode().  That's entered before beginning execution of the
plan at the outermost level - see ExecutePlan().

>> This may not be perfect, but it causes TPC-H Q18 to complete instead
>> of hanging forever, so I'm going to commit it RSN unless there are
>> loud objections combined with promising steps toward some alternative
>> fix.  It's been over a month since these problems were reported, and
>> it is not good that the tree has been in a broken state for that
>> entire time.
>
> Yea, sorry for that :(. Unfortunately the JIT stuff currently occupies a
> large portion of my brain.
>
> I really hope to come up with a new version of the patch that does the
> "smarter" expansion soon.

I've got no problem with that at all, but I want to unbreak things
more or less immediately and then you/we can further improve it later.

-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-12-14 Thread Andres Freund 
On 2016-12-14 22:00:45 -0500, Robert Haas wrote:
> I took a look at Andres's patches today and saw that they can't really
> be applied as-is, because they "temporarily" change the master's
> ParallelWorkerNumber but have no provision for restoring it after an
> ERROR.

Yea, that's not quite right. Although I'm not sure it really matters
that much, given that we're not continuing execution after an error. We
could just reset it at appropriate points - but that seems ugly.


> I think that approach isn't right anyway; it seems to me that
> what we want to do is set hash_iv based on we're in a Partial HashAgg,
> not whether we're in a parallel worker.  For instance, if we're
> somehow in a nodeSubplan.c there's no need for this just because we
> happen to be in a worker -- I think.  That led me to develop the
> attached patch.

Hm, I'd rather have this be a more general solution - it doesn't seem
unlikely that other parts of the system want to know whether they're
currently executing a parallel portion of the plan. E.g. it really
should be forbidden to do modifications even in the parallel portion of
the plan the master executes.  Right now that'd escape notice, which I
don't think is good.


> This may not be perfect, but it causes TPC-H Q18 to complete instead
> of hanging forever, so I'm going to commit it RSN unless there are
> loud objections combined with promising steps toward some alternative
> fix.  It's been over a month since these problems were reported, and
> it is not good that the tree has been in a broken state for that
> entire time.

Yea, sorry for that :(. Unfortunately the JIT stuff currently occupies a
large portion of my brain.

I really hope to come up with a new version of the patch that does the
"smarter" expansion soon.

Greetings,

Andres Freund


-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-12-14 Thread Robert Haas 
On Thu, Dec 8, 2016 at 5:23 PM, Robert Haas  wrote:
> On Wed, Nov 23, 2016 at 3:33 AM, Andres Freund  wrote:
>> On 2016-11-18 08:00:40 -0500, Robert Haas wrote:
>>> On Tue, Nov 15, 2016 at 2:28 PM, Andres Freund  wrote:
>>> > I've a working fix for this, and for a similar issue Robert found. I'm
>>> > still playing around with it, but basically the fix is to make the
>>> > growth policy a bit more adaptive.
>>>
>>> Any chance you can post a patch soon?
>>
>> Here's my WIP series addressing this and related problems. With this
>> we're again noticeably faster than the dynahash implementation, in both
>> the case here, and the query you brought up over IM.
>>
>> This definitely needs some more TLC, but the general approach seems
>> good. I particularly like that it apparently allows us to increase the
>> default fillfactor without much downside according to my measurements.
>
> Are you going to commit something here?  At least enough to make
> Finalize HashAgg -> Gather -> Partial HashAgg terminate in finite
> time?  Because the fact that it doesn't really sucks.

I took a look at Andres's patches today and saw that they can't really
be applied as-is, because they "temporarily" change the master's
ParallelWorkerNumber but have no provision for restoring it after an
ERROR.  I think that approach isn't right anyway; it seems to me that
what we want to do is set hash_iv based on we're in a Partial HashAgg,
not whether we're in a parallel worker.  For instance, if we're
somehow in a nodeSubplan.c there's no need for this just because we
happen to be in a worker -- I think.  That led me to develop the
attached patch.

This may not be perfect, but it causes TPC-H Q18 to complete instead
of hanging forever, so I'm going to commit it RSN unless there are
loud objections combined with promising steps toward some alternative
fix.  It's been over a month since these problems were reported, and
it is not good that the tree has been in a broken state for that
entire time.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/executor/execGrouping.c b/src/backend/executor/execGrouping.c
index 94cc59d..3149fbe 100644
--- a/src/backend/executor/execGrouping.c
+++ b/src/backend/executor/execGrouping.c
@@ -18,6 +18,8 @@
  */
 #include "postgres.h"
 
+#include "access/hash.h"
+#include "access/parallel.h"
 #include "executor/executor.h"
 #include "miscadmin.h"
 #include "utils/lsyscache.h"
@@ -289,7 +291,8 @@ BuildTupleHashTable(int numCols, AttrNumber *keyColIdx,
 	FmgrInfo *eqfunctions,
 	FmgrInfo *hashfunctions,
 	long nbuckets, Size additionalsize,
-	MemoryContext tablecxt, MemoryContext tempcxt)
+	MemoryContext tablecxt, MemoryContext tempcxt,
+	bool use_variable_hash_iv)
 {
 	TupleHashTable hashtable;
 	Size		entrysize = sizeof(TupleHashEntryData) + additionalsize;
@@ -314,6 +317,19 @@ BuildTupleHashTable(int numCols, AttrNumber *keyColIdx,
 	hashtable->in_hash_funcs = NULL;
 	hashtable->cur_eq_funcs = NULL;
 
+	/*
+	 * If parallelism is in use, even if the master backend is performing the
+	 * scan itself, we don't want to create the hashtable exactly the same way
+	 * in all workers. As hashtables are iterated over in keyspace-order,
+	 * doing so in all processes in the same way is likely to lead to
+	 * "unbalanced" hashtables when the table size initially is
+	 * underestimated.
+	 */
+	if (use_variable_hash_iv)
+		hashtable->hash_iv = hash_uint32(ParallelWorkerNumber);
+	else
+		hashtable->hash_iv = 0;
+
 	hashtable->hashtab = tuplehash_create(tablecxt, nbuckets);
 	hashtable->hashtab->private_data = hashtable;
 
@@ -450,7 +466,7 @@ TupleHashTableHash(struct tuplehash_hash *tb, const MinimalTuple tuple)
 	TupleHashTable hashtable = (TupleHashTable) tb->private_data;
 	int			numCols = hashtable->numCols;
 	AttrNumber *keyColIdx = hashtable->keyColIdx;
-	uint32		hashkey = 0;
+	uint32		hashkey = hashtable->hash_iv;
 	TupleTableSlot *slot;
 	FmgrInfo   *hashfunctions;
 	int			i;
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index eefb3d6..a093862 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1723,7 +1723,8 @@ build_hash_table(AggState *aggstate)
 			  node->numGroups,
 			  additionalsize,
 			 aggstate->aggcontexts[0]->ecxt_per_tuple_memory,
-			  tmpmem);
+			  tmpmem,
+  !DO_AGGSPLIT_SKIPFINAL(aggstate->aggsplit));
 }
 
 /*
diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c
index acded07..5b734c0 100644
--- a/src/backend/executor/nodeRecursiveunion.c
+++ b/src/backend/executor/nodeRecursiveunion.c
@@ -43,7 +43,8 @@ build_hash_table(RecursiveUnionState *rustate)
 			 node->numGroups,
 			 0,
 			 rustate->tableContext,
-			 rustate->tempContext);
+

Re: [HACKERS] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-12-08 Thread Robert Haas 
On Wed, Nov 23, 2016 at 3:33 AM, Andres Freund  wrote:
> On 2016-11-18 08:00:40 -0500, Robert Haas wrote:
>> On Tue, Nov 15, 2016 at 2:28 PM, Andres Freund  wrote:
>> > I've a working fix for this, and for a similar issue Robert found. I'm
>> > still playing around with it, but basically the fix is to make the
>> > growth policy a bit more adaptive.
>>
>> Any chance you can post a patch soon?
>
> Here's my WIP series addressing this and related problems. With this
> we're again noticeably faster than the dynahash implementation, in both
> the case here, and the query you brought up over IM.
>
> This definitely needs some more TLC, but the general approach seems
> good. I particularly like that it apparently allows us to increase the
> default fillfactor without much downside according to my measurements.

Are you going to commit something here?  At least enough to make
Finalize HashAgg -> Gather -> Partial HashAgg terminate in finite
time?  Because the fact that it doesn't really sucks.

-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-11-27 Thread Dilip Kumar 
On Wed, Nov 23, 2016 at 2:03 PM, Andres Freund  wrote:
> Here's my WIP series addressing this and related problems. With this
> we're again noticeably faster than the dynahash implementation, in both
> the case here, and the query you brought up over IM.
>
> This definitely needs some more TLC, but the general approach seems
> good. I particularly like that it apparently allows us to increase the
> default fillfactor without much downside according to my measurements.

This patch fixes the performance problem which I have reported
upthread. Time taken in Bitmap Index Scan is back to normal which was
drastically improved by 75ae538bc3168bf44475240d4e0487ee2f3bb376
commit.

--
-
 Limit  (cost=1583130.59..1583130.60 rows=1 width=32) (actual
time=14041.922..14041.923 rows=1 loops=1)
   ->  Aggregate  (cost=1583130.59..1583130.60 rows=1 width=32)
(actual time=14041.921..14041.921 rows=1 loops=1)
 ->  Bitmap Heap Scan on lineitem  (cost=296012.30..1577117.95
rows=1202528 width=12) (actual time=1711.899..13332.892 rows=1190658
loops=1)
   Recheck Cond: ((l_shipdate >= '1995-01-01'::date) AND
(l_shipdate < '1996-01-01 00:00:00'::timestamp without time zone) AND
(l_discount >= 0.07
) AND (l_discount <= 0.09) AND (l_quantity < '25'::numeric))
   Rows Removed by Index Recheck: 27585320
   Heap Blocks: exact=101349 lossy=580141
   ->  Bitmap Index Scan on idx_lineitem_shipdate
(cost=0.00..295711.67 rows=1202528 width=0) (actual
time=1689.478..1689.478 rows=1190658 loops=
1)
 Index Cond: ((l_shipdate >= '1995-01-01'::date)
AND (l_shipdate < '1996-01-01 00:00:00'::timestamp without time zone)
AND (l_discount >=
0.07) AND (l_discount <= 0.09) AND (l_quantity < '25'::numeric))
 Planning time: 0.292 ms
 Execution time: 14041.968 ms
(10 rows)

 revenue
-
 1784930119.2454
(1 row)


-- 
Regards,
Dilip Kumar
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-11-23 Thread Andres Freund 
On 2016-11-18 08:00:40 -0500, Robert Haas wrote:
> On Tue, Nov 15, 2016 at 2:28 PM, Andres Freund  wrote:
> > I've a working fix for this, and for a similar issue Robert found. I'm
> > still playing around with it, but basically the fix is to make the
> > growth policy a bit more adaptive.
> 
> Any chance you can post a patch soon?

Here's my WIP series addressing this and related problems. With this
we're again noticeably faster than the dynahash implementation, in both
the case here, and the query you brought up over IM.

This definitely needs some more TLC, but the general approach seems
good. I particularly like that it apparently allows us to increase the
default fillfactor without much downside according to my measurements.

Greetings,

Andres Freund
>From efc603dd67a1c95f2d24844b86436ed6b7a28c80 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 23 Nov 2016 00:23:42 -0800
Subject: [PATCH 1/3] Resize simplehash.h table in case of long runs.

This also allows to increase the default hashtable.
---
 src/include/lib/simplehash.h | 36 +---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h
index 12aedbc..9522fa5 100644
--- a/src/include/lib/simplehash.h
+++ b/src/include/lib/simplehash.h
@@ -152,10 +152,12 @@ SH_SCOPE void SH_STAT(SH_TYPE *tb);
 
 #include "utils/memutils.h"
 
-/* conservative fillfactor for a robin hood table, might want to adjust */
-#define SH_FILLFACTOR (0.8)
+/* normal fillfactor, unless already close to maximum */
+#define SH_FILLFACTOR (0.9)
 /* increase fillfactor if we otherwise would error out */
-#define SH_MAX_FILLFACTOR (0.95)
+#define SH_MAX_FILLFACTOR (0.98)
+/* collision chain length at which we resize */
+#define SH_MAX_FILLFACTOR (0.98)
 /* max data array size,we allow up to PG_UINT32_MAX buckets, including 0 */
 #define SH_MAX_SIZE (((uint64) PG_UINT32_MAX) + 1)
 
@@ -550,6 +552,34 @@ SH_INSERT(SH_TYPE *tb, SH_KEY_TYPE key, bool *found)
 #endif
 			entry->status = SH_STATUS_IN_USE;
 			*found = false;
+
+			/*
+			 * To avoid negative consequences from overly imbalanced
+			 * hashtables, grow the hashtable if collisions lead to large
+			 * runs. The most likely cause of such imbalance is filling a
+			 * (currently) small table, from a currently big one, in
+			 * hash-table order.
+			 *
+			 * FIXME: compute boundaries in a more principled manner.
+			 */
+			if (unlikely(insertdist > 20 ||
+		 SH_DISTANCE_FROM_OPTIMAL(tb, curoptimal, emptyelem) > 1000))
+			{
+/* don't grow if the table would grow overly much */
+if (tb->members / ((double) tb->size) > 0.1)
+{
+	/*
+	 * elog(WARNING, "clumping b, growing this thing");
+	 * SH_STAT(tb);
+	 */
+	/*
+	 * Trigger a growth cycle next round, easier than resizing
+	 * now.
+	 */
+	tb->grow_threshold = 0;
+}
+			}
+
 			return entry;
 		}
 
-- 
2.10.2

>From 150bc7fb1e9eacaee5d46852fa464e83e7930aca Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 22 Nov 2016 20:15:37 -0800
Subject: [PATCH 2/3] Signal when a master backend is doing parallel work.

---
 src/backend/access/transam/parallel.c | 14 --
 src/backend/executor/nodeGather.c |  6 ++
 src/include/access/parallel.h | 20 +++-
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 59dc394..c038ea1 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -88,12 +88,14 @@ typedef struct FixedParallelState
 } FixedParallelState;
 
 /*
- * Our parallel worker number.  We initialize this to -1, meaning that we are
- * not a parallel worker.  In parallel workers, it will be set to a value >= 0
- * and < the number of workers before any user code is invoked; each parallel
- * worker will get a different parallel worker number.
+ * Our parallel worker number.  We initialize this to PARALLEL_WORKER_MASTER,
+ * meaning that we are not a parallel worker.  In parallel workers, it will be
+ * set to a value >= 0 and < the number of workers before any user code is
+ * invoked; each parallel worker will get a different parallel worker number.
+ * If the master backend performs parallel work, this will temporarily be set
+ * to PARALLEL_WORKER_MASTER_PARALLEL while doing so.
  */
-int			ParallelWorkerNumber = -1;
+int			ParallelWorkerNumber = PARALLEL_WORKER_MASTER;
 
 /* Is there a parallel message pending which we need to receive? */
 volatile bool ParallelMessagePending = false;
@@ -955,7 +957,7 @@ ParallelWorkerMain(Datum main_arg)
 	BackgroundWorkerUnblockSignals();
 
 	/* Determine and set our parallel worker number. */
-	Assert(ParallelWorkerNumber == -1);
+	Assert(ParallelWorkerNumber == PARALLEL_WORKER_MASTER);
 	memcpy(, MyBgworkerEntry->bgw_extra, sizeof(int));
 
 	/*

Re: [HACKERS] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-11-18 Thread Robert Haas 
On Tue, Nov 15, 2016 at 2:28 PM, Andres Freund  wrote:
> I've a working fix for this, and for a similar issue Robert found. I'm
> still playing around with it, but basically the fix is to make the
> growth policy a bit more adaptive.

Any chance you can post a patch soon?

-- 
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-11-16 Thread Dilip Kumar 
On Wed, Nov 16, 2016 at 12:58 AM, Andres Freund  wrote:
> It's not really related to lossy pages, it's just that due to deletions
> / insertions a lot more "shapes" of the hashtable are hit.
Okay..
>
> I suspect that this is with parallelism disabled? Without that the query
> ends up using a parallel sequential scan for me.
>

It's with max_parallel_worker_per_gather=2, I always noticed that Q6
takes parallel seq scan only for
max_parallel_worker_per_gather=4 or more..
>
> I've a working fix for this, and for a similar issue Robert found. I'm
> still playing around with it, but basically the fix is to make the
> growth policy a bit more adaptive.

Okay.. Thanks.


-- 
Regards,
Dilip Kumar
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] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-11-15 Thread Andres Freund 
Hi Dilip,

thanks for noticing that one.

On 2016-11-09 21:17:22 +0530, Dilip Kumar wrote:
> While testing bitmap performance, I have observed that some of the
> TPCH queries taking huge time in BitmapIndexScan node, when there are
> lossy pages.

It's not really related to lossy pages, it's just that due to deletions
/ insertions a lot more "shapes" of the hashtable are hit.

I suspect that this is with parallelism disabled? Without that the query
ends up using a parallel sequential scan for me.


I've a working fix for this, and for a similar issue Robert found. I'm
still playing around with it, but basically the fix is to make the
growth policy a bit more adaptive.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Performance degradation in Bitmapscan (commit 75ae538bc3168bf44475240d4e0487ee2f3bb376)

2016-11-09 Thread Dilip Kumar 
While testing bitmap performance, I have observed that some of the
TPCH queries taking huge time in BitmapIndexScan node, when there are
lossy pages.

I suspected 75ae538bc3168bf44475240d4e0487ee2f3bb376 commit, because
prior to that it used to take very less time. So I tested by reverting
suspected commit and problem is solved.

Here is explained analyze result for TPCH query 6 (scale factor 10)

work_mem=10M
shared_buffers=20GB
machine under test: POWER, 4 socket machine

->On Head:

postgres=# \i 6.explain.sql

   QUERY PLAN

--
-
 Limit  (cost=1585507.74..1585507.75 rows=1 width=32) (actual
time=21626.467..21626.467 rows=1 loops=1)
   ->  Aggregate  (cost=1585507.74..1585507.75 rows=1 width=32)
(actual time=21626.466..21626.466 rows=1 loops=1)
 ->  Bitmap Heap Scan on lineitem  (cost=299632.60..1579529.48
rows=1195652 width=12) (actual time=9204.770..20910.089 rows=1190658
loops=1)
   Recheck Cond: ((l_shipdate >= '1995-01-01'::date) AND
(l_shipdate < '1996-01-01 00:00:00'::timestamp without time zone) AND
(l_discount >= 0.07
) AND (l_discount <= 0.09) AND (l_quantity < '25'::numeric))
   Rows Removed by Index Recheck: 27584798
   Heap Blocks: exact=101349 lossy=580141
   ->  Bitmap Index Scan on idx_lineitem_shipdate
(cost=0.00..299333.68 rows=1195652 width=0) (actual
time=9185.490..9185.490 rows=1190658 loops=
1)
 Index Cond: ((l_shipdate >= '1995-01-01'::date)
AND (l_shipdate < '1996-01-01 00:00:00'::timestamp without time zone)
AND (l_discount >=
0.07) AND (l_discount <= 0.09) AND (l_quantity < '25'::numeric))
 Planning time: 0.675 ms
 Execution time: 21626.838 ms
(10 rows)


->After reverting Commit: 75ae538bc3168bf44475240d4e0487ee2f3bb376
postgres=# \i 6.explain.sql

   QUERY PLAN

--
-
 Limit  (cost=1585507.74..1585507.75 rows=1 width=32) (actual
time=12807.293..12807.294 rows=1 loops=1)
   ->  Aggregate  (cost=1585507.74..1585507.75 rows=1 width=32)
(actual time=12807.291..12807.291 rows=1 loops=1)
 ->  Bitmap Heap Scan on lineitem  (cost=299632.60..1579529.48
rows=1195652 width=12) (actual time=1632.351..12131.552 rows=1190658
loops=1)
   Recheck Cond: ((l_shipdate >= '1995-01-01'::date) AND
(l_shipdate < '1996-01-01 00:00:00'::timestamp without time zone) AND
(l_discount >= 0.07
) AND (l_discount <= 0.09) AND (l_quantity < '25'::numeric))
   Rows Removed by Index Recheck: 28401743
   Heap Blocks: exact=84860 lossy=596630
   ->  Bitmap Index Scan on idx_lineitem_shipdate
(cost=0.00..299333.68 rows=1195652 width=0) (actual
time=1613.166..1613.166 rows=1190658 loops=
1)
 Index Cond: ((l_shipdate >= '1995-01-01'::date)
AND (l_shipdate < '1996-01-01 00:00:00'::timestamp without time zone)
AND (l_discount >=
0.07) AND (l_discount <= 0.09) AND (l_quantity < '25'::numeric))
 Planning time: 0.173 ms
 Execution time: 12807.380 ms
(10 rows)


>From above explain analyze result we can see that with commit
75ae538bc3168bf44475240d4e0487ee2f3bb376, Bitmap Index Scan node is
way slower than without this commit.

Perf result:
Head
+   13.12% 0.01%  postgres  postgres[.] tbm_lossify
+   13.10%13.10%  postgres  postgres[.]
tbm_mark_page_lossy
+   12.84%12.82%  postgres  postgres[.]
slot_deform_tuple
+6.94% 0.00%  postgres  postgres[.] _bt_next
+6.94% 0.02%  postgres  postgres[.] _bt_steppage
+6.55% 0.05%  postgres  postgres[.] _bt_readpage
+6.41% 1.00%  postgres  postgres[.] _bt_checkkeys

After Reverting 75ae538bc3168bf44475240d4e0487ee2f3bb376:
+0.71% 0.71%  postgres  postgres[.] cmp_var_common
+0.62% 0.02%  postgres  postgres[.] tbm_lossify
+0.62% 0.62%  postgres  postgres[.] AllocSetReset
+0.60% 0.11%  postgres  [kernel.kallsyms]   [k] sys_read
+0.59% 0.10%  postgres  postgres[.]
advance_transition_function

I think in new hash implementation, delete from pagetable have severe
performance issue.

Note: If I set work_mem=100MB (no lossy page) then performance is fine.


-- 
Regards,
Dilip Kumar
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