Re: [HACKERS] [PATCH] Call RelationDropStorage() for broader range of object drops.

[Hadi Moshayedi]

To provide more context, in cstore_fdw creating the storage is easy, we
only need to hook into CREATE FOREIGN TABLE using event triggers. Removing
the storage is not that easy, for DROP FOREIGN TABLE we can use event
triggers. But when we do DROP EXTENSION, the event triggers don't get fired
(because they have already been dropped), so to handle DROP EXTENSION, we
need to hook into the process utility hook. Now to implement this, (1) we
get a list of all cstore tables (2) call postgres's utility hook, (3) if #2
succeeds clean-up all cstore table storage's. But when #3 happens the
relation isn't there anymore, so we create a pseudo-relation [1] and call
RelationDropStorage().

Implementing all of this seemed messy, so we thought maybe postgres could
try to clean-up storage for every relation it assigns a relfilenode for. We
are open to ideas.

[1]
https://github.com/citusdata/cstore_fdw/blob/store_data_in_internal_storage/cstore_fdw.c#L907

Re: [HACKERS] [PATCH] Call RelationDropStorage() for broader range of object drops.

[Hadi Moshayedi]

On Wed, Sep 13, 2017 at 12:12 AM, Michael Paquier  wrote:
>
> Foreign tables do not have physical storage assigned to by default. At
> least heap_create() tells so, create_storage being set to false for a
> foreign table. So there is nothing to clean up normally. Or is
> cstore_fdw using directly heap_create with its own relfilenode set,
> creating a physical storage?
>

cstore_fdw (in store_data_in_internal_storage branch) calls
RelationCreateStorage() after CREATE FOREIGN TABLE completes [1]. Later it
also creates the FSM fork and uses it for storing some metadata.

[1]
https://github.com/citusdata/cstore_fdw/blob/store_data_in_internal_storage/cstore_fdw.c#L237

[HACKERS] [PATCH] Call RelationDropStorage() for broader range of object drops.

[Hadi Moshayedi]

Motivation for this patch is that some FDWs (notably, cstore_fdw) try
utilizing PostgreSQL internal storage. PostgreSQL assigns relfilenode's to
foreign tables, but doesn't clean up storage for foreign tables when
dropping tables. Therefore, in cstore_fdw we have to do some tricks to
handle dropping objects that lead to dropping of cstore table properly.

As far as I can see in the code, the requirement for
RelationDropStorage(rel) is having valid rel->rd_node.relNode, but it
doesn't actually require the storage files to actually exist. We don't emit
warning messages in mdunlinkfork() if the result of unlink() is ENOENT. So
we can RelationDropStorage() regardless of storage files existing or not,
given that the relation has valid relfilenode.

So I am suggesting to change the check at heap_drop_with_catalog() at
src/backend/catalog/heap.c:

-if (rel->rd_rel->relkind != RELKIND_VIEW &&
-rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE &&
-rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
-rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+if (OidIsValid(rel->rd_node.relNode))
 {
 RelationDropStorage(rel);
 }

Any feedback on this?

Thanks,
Hadi
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 45ee9ac8b9..6ec2a98a99 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1828,10 +1828,7 @@ heap_drop_with_catalog(Oid relid)
 	/*
 	 * Schedule unlinking of the relation's physical files at commit.
 	 */
-	if (rel->rd_rel->relkind != RELKIND_VIEW &&
-		rel->rd_rel->relkind != RELKIND_COMPOSITE_TYPE &&
-		rel->rd_rel->relkind != RELKIND_FOREIGN_TABLE &&
-		rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+	if (OidIsValid(rel->rd_node.relNode))
 	{
 		RelationDropStorage(rel);
 	}

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

[HACKERS] [PATCH] Make ExplainOpenGroup()/ExplainCloseGroup() available for extensions.

[Hadi Moshayedi]

Hello Hackers,

The attached patch moves declarations of
ExplainOpenGroup()/ExplainCloseGroup() from explain.c to explain.h.

This can be useful for extensions that need explain groups in their
custom-scan explain output.

For example, Citus uses groups in its custom explain outputs [1]. But it
achieves it by having a copy of ExplainOpenGroup()/ExplainCloseGroup() in
its source code, which is not the best way.

Please review.

[1] https://github.com/citusdata/citus/blob/master/src/backend/
distributed/planner/multi_explain.c

Thanks,
Hadi
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 7648201218..46467e1045 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -124,10 +124,6 @@ static void ExplainCustomChildren(CustomScanState *css,
 	  List *ancestors, ExplainState *es);
 static void ExplainProperty(const char *qlabel, const char *value,
 bool numeric, ExplainState *es);
-static void ExplainOpenGroup(const char *objtype, const char *labelname,
- bool labeled, ExplainState *es);
-static void ExplainCloseGroup(const char *objtype, const char *labelname,
-  bool labeled, ExplainState *es);
 static void ExplainDummyGroup(const char *objtype, const char *labelname,
   ExplainState *es);
 static void ExplainXMLTag(const char *tagname, int flags, ExplainState *es);
@@ -3213,7 +3209,7 @@ ExplainPropertyBool(const char *qlabel, bool value, ExplainState *es)
  * If labeled is true, the group members will be labeled properties,
  * while if it's false, they'll be unlabeled objects.
  */
-static void
+void
 ExplainOpenGroup(const char *objtype, const char *labelname,
  bool labeled, ExplainState *es)
 {
@@ -3276,7 +3272,7 @@ ExplainOpenGroup(const char *objtype, const char *labelname,
  * Close a group of related objects.
  * Parameters must match the corresponding ExplainOpenGroup call.
  */
-static void
+void
 ExplainCloseGroup(const char *objtype, const char *labelname,
   bool labeled, ExplainState *es)
 {
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 78822b766a..543b2bb0c6 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -101,4 +101,9 @@ extern void ExplainPropertyFloat(const char *qlabel, double value, int ndigits,
 extern void ExplainPropertyBool(const char *qlabel, bool value,
 	ExplainState *es);
 
+extern void ExplainOpenGroup(const char *objtype, const char *labelname,
+ bool labeled, ExplainState *es);
+extern void ExplainCloseGroup(const char *objtype, const char *labelname,
+  bool labeled, ExplainState *es);
+
 #endif			/* EXPLAIN_H */

-- 
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] [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

[Hadi Moshayedi]

Title should have been "Make ExplainOpenGroup()/ExplainCloseGroup()
public.".

Sorry for the misspell.

[HACKERS] [PATCH] Make ExplainBeginGroup()/ExplainEndGroup() public.

[Hadi Moshayedi]

Hello,

The attached patch moves declarations of
ExplainBeginGroup()/ExplainEndGroup() from explain.c to explain.h.

This can be useful for extensions that need explain groups in their
custom-scan explain output.

For example, Citus uses groups in its custom explain outputs [1]. But it
achieves it by having a copy of
ExplainBeginGroup()/ExplainEndGroup() in its source code, which is not the
best way.

Please review.

[1]
https://github.com/citusdata/citus/blob/master/src/backend/distributed/planner/multi_explain.c

Thanks,
Hadi
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 7648201218..46467e1045 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -124,10 +124,6 @@ static void ExplainCustomChildren(CustomScanState *css,
 	  List *ancestors, ExplainState *es);
 static void ExplainProperty(const char *qlabel, const char *value,
 bool numeric, ExplainState *es);
-static void ExplainOpenGroup(const char *objtype, const char *labelname,
- bool labeled, ExplainState *es);
-static void ExplainCloseGroup(const char *objtype, const char *labelname,
-  bool labeled, ExplainState *es);
 static void ExplainDummyGroup(const char *objtype, const char *labelname,
   ExplainState *es);
 static void ExplainXMLTag(const char *tagname, int flags, ExplainState *es);
@@ -3213,7 +3209,7 @@ ExplainPropertyBool(const char *qlabel, bool value, ExplainState *es)
  * If labeled is true, the group members will be labeled properties,
  * while if it's false, they'll be unlabeled objects.
  */
-static void
+void
 ExplainOpenGroup(const char *objtype, const char *labelname,
  bool labeled, ExplainState *es)
 {
@@ -3276,7 +3272,7 @@ ExplainOpenGroup(const char *objtype, const char *labelname,
  * Close a group of related objects.
  * Parameters must match the corresponding ExplainOpenGroup call.
  */
-static void
+void
 ExplainCloseGroup(const char *objtype, const char *labelname,
   bool labeled, ExplainState *es)
 {
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 78822b766a..543b2bb0c6 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -101,4 +101,9 @@ extern void ExplainPropertyFloat(const char *qlabel, double value, int ndigits,
 extern void ExplainPropertyBool(const char *qlabel, bool value,
 	ExplainState *es);
 
+extern void ExplainOpenGroup(const char *objtype, const char *labelname,
+ bool labeled, ExplainState *es);
+extern void ExplainCloseGroup(const char *objtype, const char *labelname,
+  bool labeled, ExplainState *es);
+
 #endif			/* EXPLAIN_H */

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

[HACKERS] Faster array_length()

[Hadi Moshayedi]

Hello,

The attached patch improves the performance of array_length() by detoasting
only the overhead part of the datum.

Here is a test case:

postgres=# create table array_length_test as select array_agg(a) a from
generate_series(1, 1) a, generate_series(1, 1) b group by b;

Without the patch:

postgres=#  select sum(array_length(a, 1)) from array_length_test;
sum
---
 1
(1 row)

Time: 199.002 ms

With the patch:

postgres=#  select sum(array_length(a, 1)) from array_length_test;
sum
---
 1
(1 row)

Time: 34.599 ms

The motivation for patch is that some of our customers use arrays to store
a sequence of tens of thousands of events in each row. They often need to
get the last 10 event for each row, for which we do A[array_length(A, 1) -
9: 100] (assuming 1M is an upper-bound. we could use array_length()
instead of this constant too, but that is unnecessary if we know the
upper-bound and only slows down the query). Without this optimization,
array gets detoasted twice. With this patch, array_length() becomes much
faster, and the whole query saves few seconds.

Of course this technique is applicable to some other functions too, but
they have never become a bottleneck for me, so I decided to keep the
changes only to this function.

Another alternative I could think of was introducing python style slicing,
in which negative indexes start from end of array, so -10 means 10th
element from end. I thought this would be a bigger change and is probably
unnecessary, so I decided to improve array_length() instead.

Feedback is welcome.

Thanks,
   -- Hadi
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 91df184..5e1d9c2 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -1719,11 +1719,15 @@ array_upper(PG_FUNCTION_ARGS)
 Datum
 array_length(PG_FUNCTION_ARGS)
 {
-	ArrayType  *v = PG_GETARG_ARRAYTYPE_P(0);
+	Datum		arrdatum = PG_GETARG_DATUM(0);
 	int			reqdim = PG_GETARG_INT32(1);
+	ArrayType  *v;
 	int		   *dimv;
 	int			result;
 
+	v = (ArrayType *) PG_DETOAST_DATUM_SLICE(arrdatum, 0,
+			 ARR_OVERHEAD_NONULLS(MAXDIM));
+
 	/* Sanity check: does it look like an array at all? */
 	if (ARR_NDIM(v) = 0 || ARR_NDIM(v)  MAXDIM)
 		PG_RETURN_NULL();

-- 
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] Faster array_length()

[Hadi Moshayedi]

Thanks for looking into this.


 With that in mind, I was surprised that your test case showed any
 improvement at all --- it looks like the arrays aren't getting compressed
 for some reason.


You are right, it seems that they were not getting compressed, probably
because the arrays were seq 1 which seems to not get compressed by
pglz. When I changed the test data to an array containing 1 ones, there
were no speed improvement anymore.

I'll look into how to improve the compressed case and other issues you
raised.

Thanks,
   -- Hadi

Re: [HACKERS] Proposal for Merge Join for Non '=' Operators

[Hadi Moshayedi]

Hello Dilip,

Query: select count(*) from t1,t2 where t1.bt2.b and t1.b  12000;

 Test Result:
 Nest Loop Join with Index Scan   : 1653.506 ms
 Sort Merge Join for (seq scan)   : 610.257ms


This looks like a great improvement. Repeating Nicolas's question, do you
have a real-world example of such joins?

In my experience, I see more queries like self-join table A and table B
where A.time BETWEEN B.time - '1 week' and B.time, similar to
what Nicolas and Tom mentioned. As an example, count users who placed an
order in the week following their registration.

Can you send a patch so we can also try it?

Thanks,
  -- Hadi

Re: [HACKERS] A question about code in DefineRelation()

[Hadi Moshayedi]

 On second thought I noticed that that makes CREATE FOREIGN TABLE include
 an OID column in newly-created foreign tables wrongly, when the
 default_with_oids parameter is set to on.  Please find attached a patch.


The fix makes sense to me, since in ALTER TABLE SET WITH OIDS we check that
the relation is a table and not a foreign table:

3160 case AT_AddOids: /* SET WITH OIDS */
3161 ATSimplePermissions(rel, ATT_TABLE);

So, I think we should be consistent between DefineRelation() and alter
table.

-- Hadi.

Re: [HACKERS] PostgreSQL Columnar Store for Analytic Workloads

[Hadi Moshayedi]

Hi Stefan,

On Tue, Apr 8, 2014 at 9:28 AM, Stefan Keller sfkel...@gmail.com wrote:

 Hi Hadi

 Do you think that cstore_fd*w* is also welll suited for storing and
 retrieving linked data (RDF)?




I am not very familiar with RDF. Note that cstore_fdw doesn't change the
query language of PostgreSQL, so if your queries are expressible in SQL,
they can be answered using cstore_fdw too. If your data is huge and doesn't
fit in memory, then using cstore_fdw can be beneficial for you.

Can you give some more information about your use case? For example, what
are some of your queries? do you have sample data? how much memory do you
have? how large is the data?

-- Hadi

[HACKERS] PostgreSQL Columnar Store for Analytic Workloads

[Hadi Moshayedi]

Dear Hackers,

We at Citus Data have been developing a columnar store extension for
PostgreSQL. Today we are excited to open source it under the Apache v2.0
license.

This columnar store extension uses the Optimized Row Columnar (ORC) format
for its data layout, which improves upon the RCFile format developed at
Facebook, and brings the following benefits:

* Compression: Reduces in-memory and on-disk data size by 2-4x. Can be
extended to support different codecs. We used the functions in
pg_lzcompress.h for compression and decompression.
* Column projections: Only reads column data relevant to the query.
Improves performance for I/O bound queries.
* Skip indexes: Stores min/max statistics for row groups, and uses them to
skip over unrelated rows.

We used the PostgreSQL FDW APIs to make this work. The extension doesn't
implement the writable FDW API, but it uses the process utility hook to
enable COPY command for the columnar tables.

This extension uses PostgreSQL's internal data type representation to store
data in the table, so this columnar store should support all data types
that PostgreSQL supports.

We tried the extension on TPC-H benchmark with 4GB scale factor on a
m1.xlarge Amazon EC2 instance, and the query performance improved by 2x-3x
compared to regular PostgreSQL table. Note that we flushed the page cache
before each test to see the impact on disk I/O.

When data is cached in memory, the performance of cstore_fdw tables were
close to the performance of regular PostgreSQL tables.

For more information, please visit:
 * our blog post:
http://citusdata.com/blog/76-postgresql-columnar-store-for-analytics
 * our github page: https://github.com/citusdata/cstore_fdw

Feedback from you is really appreciated.

Thanks,
  -- Hadi

[HACKERS] PostgreSQL Columnar Store for Analytic Workloads

[Hadi Moshayedi]

Dear Hackers,

We at Citus Data have been developing a columnar store extension for
PostgreSQL. Today we are excited to open source it under the Apache v2.0
license.

This columnar store extension uses the Optimized Row Columnar (ORC) format
for its data layout, which improves upon the RCFile format developed at
Facebook, and brings the following benefits:

* Compression: Reduces in-memory and on-disk data size by 2-4x. Can be
extended to support different codecs. We used the functions in
pg_lzcompress.h for compression and decompression.
* Column projections: Only reads column data relevant to the query.
Improves performance for I/O bound queries.
* Skip indexes: Stores min/max statistics for row groups, and uses them to
skip over unrelated rows.

We used the PostgreSQL FDW APIs to make this work. The extension doesn't
implement the writable FDW API, but it uses the process utility hook to
enable COPY command for the columnar tables.

This extension uses PostgreSQL's internal data type representation to store
data in the table, so this columnar store should support all data types
that PostgreSQL supports.

We tried the extension on TPC-H benchmark with 4GB scale factor on a
m1.xlarge Amazon EC2 instance, and the query performance improved by 2x-3x
compared to regular PostgreSQL table. Note that we flushed the page cache
before each test to see the impact on disk I/O.

When data is cached in memory, the performance of cstore_fdw tables were
close to the performance of regular PostgreSQL tables.

For more information, please visit:
 * our blog post:
http://citusdata.com/blog/76-postgresql-columnar-store-for-analytics
 * our github page: https://github.com/citusdata/cstore_fdw

Feedback from you is really appreciated.

Thanks,
  -- Hadi

[HACKERS] pglz_decompress()

[Hadi Moshayedi]

Hello,

The comments in pg_lzcompress.c say that:

 * If VARSIZE(x) == rawsize + sizeof(PGLZ_Header), then the data
 * is stored uncompressed as plain bytes. Thus, the decompressor
 * simply copies rawsize bytes from the location after the
 * header to the destination.

But pg_lzdecompress doesn't seem to check explicitly for the VARSIZE(x) ==
rawsize + sizeof(PGLZ_Header) condition. Is it caller's responsibility to
check for this condition before calling pg_lzdecompress(), or does it
happen somehow in pg_lzdecompress()?

Thanks,
  -- Hadi

[HACKERS] Cost estimation in foreign data wrappers

[Hadi Moshayedi]

Hello,

There is a callback function in fdw's which should also set estimates for
startup and total costs for each path. Assume a fdw adds only one path
(e.g. in file_fdw). I am trying to understand what can go wrong if we do a
bad job in estimating these costs.

Since we have only one scan path here, it doesn't make a difference in
choosing the best scan path.

By looking at the code and doing some experiments, I think this can
be significant in (1) underestimating a nested loop's cost, (2) not
materializing inner table in nested loop.

 * Are there any other cases that this can be significant?
 * Assume we are not sure about the exact cost, but we know that it is in
[lower_bound, upper_bound] range, where upper_bound can be 10x lower_bound
Then, what value is better to choose? lower bound? upper bound? or average?

Thanks,
  -- Hadi

Re: [HACKERS] Improving avg performance for numeric

[Hadi Moshayedi]

Hello,

 int, float, double 26829 ms (26675 ms) -- 0.5% slower .. statistic error ..
 cleaner code
 numeric sum 6490 ms (7224 ms) --  10% faster
 numeric avg 6487 ms (12023 ms) -- 46% faster

I also got very similar results.

On the other hand, initially I was receiving sigsegv's whenever I
wanted to try to run an aggregate function. gdb was telling that this
was happening somewhere in nodeAgg.c at ExecInitAgg. While trying to
find the reason, I had to reboot my computer at some point, after the
reboot the sigsegv's went away. I want to look into this and find the
reason, I think I have missed something here. Any thoughts about why
this would happen?

--Hadi


-- 
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] Improving avg performance for numeric

[Hadi Moshayedi]

Hi Tom,

Tom Lane t...@sss.pgh.pa.us wrote:
 After thinking about that for awhile: if we pursue this type of
 optimization, what would probably be appropriate is to add an aggregate
 property (stored in pg_aggregate) that allows direct specification of
 the size that the planner should assume for the aggregate's transition
 value.  We were getting away with a hardwired assumption of 8K for
 internal because the existing aggregates that used that transtype all
 had similar properties, but it was always really a band-aid not a proper
 solution.  A per-aggregate override could be useful in other cases too.

Cool.

I created a patch which adds an aggregate property to pg_aggregate, so
the transition space is can be overridden. This patch doesn't contain
the numeric optimizations. It uses 0 (meaning not-set) for all
existing aggregates.

I manual-tested it a bit, by changing this value for aggregates and
observing the changes in plan. I also updated some docs and pg_dump.

Does this look like something along the lines of what you meant?

Thanks,
  -- Hadi


aggregate-transspace.patch
Description: Binary data

-- 
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] Improving avg performance for numeric

[Hadi Moshayedi]

Hello,

I updated the patch by taking ideas from your patch, and unifying the
transition struct and update function for different aggregates. The speed
of avg improved even more. It now has 60% better performance than the
current committed version.

This patch optimizes numeric/int8 sum, avg, stddev_pop, stddev_samp,
var_pop, var_samp.

I also noticed that this patch makes matview test fail. It seems that it
just changes the ordering of rows for queries like SELECT * FROM tv;.
Does this seem like a bug in my patch, or should we add ORDER BY clauses
to this test to make it more deterministic?

I also agree with you that adding sum functions to use preallocated buffers
will make even more optimization. I'll try to see if I can find a simple
way to do this.

Thanks,
  -- Hadi

On Mon, Mar 18, 2013 at 5:25 PM, Pavel Stehule pavel.steh...@gmail.comwrote:

 Hello

 I played with sum(numeric) optimization

 Now it is based on generic numeric_add function - this code is
 relative old - now we can design aggregates with internal transition
 buffers, and probably we can do this work more effective.

 I just removed useles palloc/free operations and I got a 30% better
 performance! My patch is ugly - because I used a generic add_var
 function. Because Sum, Avg and almost all aggregates functions is
 limited by speed of sum calculation I thing so we need a new numeric
 routines optimized for calculation sum, that use a only preallocated
 buffers. A speed of numeric is more important now, because there are
 more and more warehouses, where CPU is botleneck.

 Regards

 Pavel


 2013/3/18 Hadi Moshayedi h...@moshayedi.net:
  Hi Pavel,
 
  Thanks a lot for your feedback.
 
  I'll work more on this patch this week, and will send a more complete
 patch
  later this week.
 
  I'll also try to see how much is the speed up of this method for other
  types.
 
  Thanks,
-- Hadi
 
 
  On Mon, Mar 18, 2013 at 10:36 AM, Pavel Stehule pavel.steh...@gmail.com
 
  wrote:
 
  2013/3/16 Hadi Moshayedi h...@moshayedi.net:
   Revisiting:
   http://www.postgresql.org/message-id/45661be7.4050...@paradise.net.nz
  
   I think the reasons which the numeric average was slow were:
 (1) Using Numeric for count, which is slower than int8 to increment,
 (2) Constructing/deconstructing arrays at each transition step.
  
   This is also discussed at:
  
  
 http://www.citusdata.com/blog/53-postgres-performance-to-avg-or-to-sum-divided-by-count
  
   So, I think we can improve the speed of numeric average by keeping the
   transition state as an struct in the aggregate context, and just
 passing
   the
   pointer to that struct from/to the aggregate transition function.
  
   The attached patch uses this method.
  
   I tested it using the data generated using:
   CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d
 FROM
   generate_series(1, 1000) s;
  
   After applying this patch, run time of SELECT avg(d) FROM avg_test;
   improves from 10.701 seconds to 5.204 seconds, which seems to be a
 huge
   improvement.
  
   I think we may also be able to use a similar method to improve
   performance
   of some other numeric aggregates (like stddev). But I want to know
 your
   feedback first.
  
   Is this worth working on?
 
  I checked this patch and it has a interesting speedup - and a price of
  this methoud should not be limited to numeric type only
 
  Pavel
 
  
   Thanks,
 -- Hadi
  
  
  
   --
   Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
   To make changes to your subscription:
   http://www.postgresql.org/mailpref/pgsql-hackers
  
 
 



numeric-optimize-v2.patch
Description: Binary data

-- 
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] Improving avg performance for numeric

[Hadi Moshayedi]

I am not sure how this works, but I also changed numeric sum(), and the
views in question had a numeric sum() column. Can that have any impact?

I am going to dig deeper to see why this happens.

On Tue, Mar 19, 2013 at 6:25 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Kevin Grittner kgri...@ymail.com writes:
  Hadi Moshayedi h...@moshayedi.net wrote:
  I also noticed that this patch makes matview test fail. It seems
  that it just changes the ordering of rows for queries like
  SELECT * FROM tv;. Does this seem like a bug in my patch, or
  should we add ORDER BY clauses to this test to make it more
  deterministic?

  I added some ORDER BY clauses.  That is probably a good thing
  anyway for purposes of code coverage.  Does that fix it for you?

 Uh, what?  Fooling around with the implementation of avg() should surely
 not change any planning decisions.

 regards, tom lane

Re: [HACKERS] Improving avg performance for numeric

[Hadi Moshayedi]

Hi Pavel,

Thanks a lot for your feedback.

I'll work more on this patch this week, and will send a more complete patch
later this week.

I'll also try to see how much is the speed up of this method for other
types.

Thanks,
  -- Hadi

On Mon, Mar 18, 2013 at 10:36 AM, Pavel Stehule pavel.steh...@gmail.comwrote:

 2013/3/16 Hadi Moshayedi h...@moshayedi.net:
  Revisiting:
  http://www.postgresql.org/message-id/45661be7.4050...@paradise.net.nz
 
  I think the reasons which the numeric average was slow were:
(1) Using Numeric for count, which is slower than int8 to increment,
(2) Constructing/deconstructing arrays at each transition step.
 
  This is also discussed at:
 
 http://www.citusdata.com/blog/53-postgres-performance-to-avg-or-to-sum-divided-by-count
 
  So, I think we can improve the speed of numeric average by keeping the
  transition state as an struct in the aggregate context, and just passing
 the
  pointer to that struct from/to the aggregate transition function.
 
  The attached patch uses this method.
 
  I tested it using the data generated using:
  CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d FROM
  generate_series(1, 1000) s;
 
  After applying this patch, run time of SELECT avg(d) FROM avg_test;
  improves from 10.701 seconds to 5.204 seconds, which seems to be a huge
  improvement.
 
  I think we may also be able to use a similar method to improve
 performance
  of some other numeric aggregates (like stddev). But I want to know your
  feedback first.
 
  Is this worth working on?

 I checked this patch and it has a interesting speedup - and a price of
 this methoud should not be limited to numeric type only

 Pavel

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

[HACKERS] Improving avg performance for numeric

[Hadi Moshayedi]

Revisiting:
http://www.postgresql.org/message-id/45661be7.4050...@paradise.net.nz

I think the reasons which the numeric average was slow were:
  (1) Using Numeric for count, which is slower than int8 to increment,
  (2) Constructing/deconstructing arrays at each transition step.

This is also discussed at:
http://www.citusdata.com/blog/53-postgres-performance-to-avg-or-to-sum-divided-by-count

So, I think we can improve the speed of numeric average by keeping the
transition state as an struct in the aggregate context, and just passing
the pointer to that struct from/to the aggregate transition function.

The attached patch uses this method.

I tested it using the data generated using:
CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d FROM
generate_series(1, 1000) s;

After applying this patch, run time of SELECT avg(d) FROM avg_test;
improves from 10.701 seconds to 5.204 seconds, which seems to be a huge
improvement.

I think we may also be able to use a similar method to improve performance
of some other numeric aggregates (like stddev). But I want to know your
feedback first.

Is this worth working on?

Thanks,
  -- Hadi


numeric-avg-optimize.patch
Description: Binary data

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