subject:"Re\: \[HACKERS\] Function array

Re: [HACKERS] Function array_agg(array)

2014-11-25 Thread Tom Lane

Pavel Stehule pavel.steh...@gmail.com writes:
 2014-10-27 11:20 GMT+01:00 Ali Akbar the.ap...@gmail.com:
 [ array_agg_anyarray-13.patch ]

 This patch is ready for commit

I've committed this after some significant modifications.

I did not like the API chosen, specifically the business about callers
needing to deal with both accumArrayResult and accumMdArray, because that
seemed pretty messy and error-prone.  There was in fact at least one
calling-logic error, namely that the empty array created by nodeSubplan.c
when there were zero inputs would have the wrong element type for the
array-input case.

It seemed to me that the best fix for that would be to push the empty
array creation special case into the accumArrayResult function family.
After some thought about how to do that, I settled on adding an init
function that can create an ArrayBuildState with no data yet put into
it; the main purpose is just to save the info about the input datatype.
Then, if makeArrayResult receives the ArrayBuildState with still no
data in it, it can create the appropriate empty array.  (This turned
out to be a better idea than I first realized, as both xml.c and plperl.c
had coding patterns that could be made significantly less ugly with a
convention that the ArrayBuildState is always there.)

After doing that, I adjusted your new functions to have similar APIs,
and then added a switching layer on top for use by callers that want
to support both the scalar and array cases.  This made the adjustments
for array subplans more or less one-liner changes in the relevant
places.  We could possibly have unified the array_agg support functions
as well, but I didn't see much point in that given that we need two
sets of pg_proc entries to make type resolution work properly.

There were a number of other smaller issues too, like not being cautious
about memory allocation (the submitted code could both fail to allocate
enough, and compute an allocation request that would overflow an int).

regards, tom lane


-- 
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] Function array_agg(array)

2014-11-25 Thread Ali Akbar

2014-11-26 0:38 GMT+07:00 Tom Lane t...@sss.pgh.pa.us:

 Pavel Stehule pavel.steh...@gmail.com writes:
  2014-10-27 11:20 GMT+01:00 Ali Akbar the.ap...@gmail.com:
  [ array_agg_anyarray-13.patch ]

  This patch is ready for commit

 I've committed this after some significant modifications.

 I did not like the API chosen, specifically the business about callers
 needing to deal with both accumArrayResult and accumMdArray, because that
 seemed pretty messy and error-prone.


Thanks!

When in the reviewing process, we tried to implement in existing API, and
it was messy, so the last patch is with two API. We didn't think what you
eventually did. 3 API: existing for scalar, a new API for array, _and_ a
new API for both. Great!!

Just curious, in accumArrayResultArr, while enlarging array and
nullbitmaps, why it's implemented with:

astate-abytes = Max(astate-abytes * 2,
  astate-nbytes + ndatabytes);


and

astate-aitems = Max(astate-aitems * 2, newnitems);


won't it be more consistent if it's implemented just like in the first
allocation?:

while (astate-aitems = newnitems)
 astate-aitems *= 2;


Anyway, thanks for the big modifications. I learned a lot from that.

Regards,
-- 
Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-11-25 Thread Tom Lane

Ali Akbar the.ap...@gmail.com writes:
 Just curious, in accumArrayResultArr, while enlarging array and
 nullbitmaps, why it's implemented with:

 astate-abytes = Max(astate-abytes * 2,
 astate-nbytes + ndatabytes);
 and
 astate-aitems = Max(astate-aitems * 2, newnitems);

 won't it be more consistent if it's implemented just like in the first
 allocation?:

 while (astate-aitems = newnitems)
 astate-aitems *= 2;

The idea was to try to force the initial allocation to be a power of 2,
while not insisting on that for later enlargements.  I can't point to any
hard reasons for doing it that way, but it seemed like a good idea.
Power-of-2 allocations are good up to a certain point but after that they
tend to get wasteful ...

regards, tom lane


-- 
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] Function array_agg(array)

2014-10-27 Thread Ali Akbar

2014-10-27 9:11 GMT+07:00 Ali Akbar the.ap...@gmail.com:


 2014-10-27 1:38 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 My idea is using new ArrayBuilder optimized for building multidimensional
 arrays with own State type. I think so casting to ArrayBuildState is base
 of our problems, so I don't would to do. Code in array_agg_* is simple,
 little bit more complex code is in nodeSubplan.c. Some schematic changes
 are in attachments.


 Thanks! The structure looks clear, and thanks for the example on
 nodeSubplan.c. I will restructure the v10 of the patch to this structure.


Patch attached.

Regards,
-- 
Ali Akbar
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..f59738a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12046,6 +12046,22 @@ NULL baz/literallayout(3 rows)/entry
  row
   entry
indexterm
+primaryarray_agg/primary
+   /indexterm
+   functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+  /entry
+  entry
+   any
+  /entry
+  entry
+   the same array type as input type
+  /entry
+  entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+ /row
+
+ row
+  entry
+   indexterm
 primaryaverage/primary
/indexterm
indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 2f0680f..8c182a4 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2238,6 +2238,11 @@ SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
  array
 ---
  {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+
+SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+ array
+---
+ {{1},{2},{3},{4},{5}}
 (1 row)
 /programlisting
The subquery must return a single column. The resulting
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 401bad4..eb4de3b 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -231,7 +231,10 @@ ExecScanSubPlan(SubPlanState *node,
 	bool		found = false;	/* TRUE if got at least one subplan tuple */
 	ListCell   *pvar;
 	ListCell   *l;
+	bool		use_md_array_builder;
+	Oid			md_array_element_type;
 	ArrayBuildState *astate = NULL;
+	MdArrayBuildState *mdastate = NULL;
 
 	/*
 	 * MULTIEXPR subplans, when executed, just return NULL; but first we
@@ -366,8 +369,25 @@ ExecScanSubPlan(SubPlanState *node,
 			/* stash away current value */
 			Assert(subplan-firstColType == tdesc-attrs[0]-atttypid);
 			dvalue = slot_getattr(slot, 1, disnull);
-			astate = accumArrayResult(astate, dvalue, disnull,
-	  subplan-firstColType, oldcontext);
+			/*
+			 * use a fast array multidimensional builder when input is a array
+			 * only check on first iteration. On subsequent, use the cached values
+			 */
+			if (astate == NULL  mdastate == NULL)
+			{
+md_array_element_type = get_element_type(subplan-firstColType);
+use_md_array_builder = OidIsValid(md_array_element_type);
+			}
+
+			if (use_md_array_builder)
+mdastate = accumMdArray(mdastate,
+		disnull? NULL :
+ DatumGetArrayTypeP(dvalue),
+		disnull, md_array_element_type,
+		oldcontext);
+			else
+astate = accumArrayResult(astate, dvalue, disnull,
+		  subplan-firstColType, oldcontext);
 			/* keep scanning subplan to collect all values */
 			continue;
 		}
@@ -439,6 +459,8 @@ ExecScanSubPlan(SubPlanState *node,
 		/* We return the result in the caller's context */
 		if (astate != NULL)
 			result = makeArrayResult(astate, oldcontext);
+		else if (mdastate != NULL)
+			result = makeMdArray(mdastate, oldcontext);
 		else
 			result = PointerGetDatum(construct_empty_array(subplan-firstColType));
 	}
@@ -951,7 +973,10 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 	ListCell   *pvar;
 	ListCell   *l;
 	bool		found = false;
+	bool		use_md_array_builder;
+	Oid			md_array_element_type;
 	ArrayBuildState *astate = NULL;
+	MdArrayBuildState *mdastate = NULL;
 
 	if (subLinkType == ANY_SUBLINK ||
 		subLinkType == ALL_SUBLINK)
@@ -1018,8 +1043,25 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 			/* stash away current value */
 			Assert(subplan-firstColType == tdesc-attrs[0]-atttypid);
 			dvalue = slot_getattr(slot, 1, disnull);
-			astate = accumArrayResult(astate, dvalue, disnull,
-	  subplan-firstColType, oldcontext);
+			/*
+			 * use a fast array multidimensional builder when input is a array
+			 * only check on first iteration. On subsequent, use the cached values
+			 */
+			if (astate == NULL  mdastate == NULL)
+			{
+md_array_element_type = get_element_type(subplan-firstColType);
+use_md_array_builder =

Re: [HACKERS] Function array_agg(array)

2014-10-27 Thread Pavel Stehule

Hi

I did some minor changes in code

* move tests of old or new builder style for array sublink out of main
cycles
* some API simplification of new builder - we should not to create
identical API, mainly it has no sense

Regards

Pavel Stehule


2014-10-27 8:12 GMT+01:00 Ali Akbar the.ap...@gmail.com:

 2014-10-27 9:11 GMT+07:00 Ali Akbar the.ap...@gmail.com:


 2014-10-27 1:38 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 My idea is using new ArrayBuilder optimized for building
 multidimensional arrays with own State type. I think so casting to
 ArrayBuildState is base of our problems, so I don't would to do. Code in
 array_agg_* is simple, little bit more complex code is in nodeSubplan.c.
 Some schematic changes are in attachments.


 Thanks! The structure looks clear, and thanks for the example on
 nodeSubplan.c. I will restructure the v10 of the patch to this structure.


 Patch attached.

 Regards,
 --
 Ali Akbar

commit f09f41f5fe780bed4cf25949961a4e68e6402ff0
Author: Pavel Stehule pavel.steh...@gooddata.com
Date:   Mon Oct 27 10:03:07 2014 +0100

initial

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..f59738a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12046,6 +12046,22 @@ NULL baz/literallayout(3 rows)/entry
  row
   entry
indexterm
+primaryarray_agg/primary
+   /indexterm
+   functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+  /entry
+  entry
+   any
+  /entry
+  entry
+   the same array type as input type
+  /entry
+  entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+ /row
+
+ row
+  entry
+   indexterm
 primaryaverage/primary
/indexterm
indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 2f0680f..8c182a4 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2238,6 +2238,11 @@ SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
  array
 ---
  {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+
+SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+ array
+---
+ {{1},{2},{3},{4},{5}}
 (1 row)
 /programlisting
The subquery must return a single column. The resulting
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 401bad4..21b8de1 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -231,7 +231,10 @@ ExecScanSubPlan(SubPlanState *node,
 	bool		found = false;	/* TRUE if got at least one subplan tuple */
 	ListCell   *pvar;
 	ListCell   *l;
+	bool		use_md_array_builder = false;
+	Oid			md_array_element_type = InvalidOid;
 	ArrayBuildState *astate = NULL;
+	MdArrayBuildState *mdastate = NULL;
 
 	/*
 	 * MULTIEXPR subplans, when executed, just return NULL; but first we
@@ -260,6 +263,16 @@ ExecScanSubPlan(SubPlanState *node,
 	}
 
 	/*
+	 * use a fast array multidimensional builder when input is a array
+	 * only check on first iteration. On subsequent, use the cached values
+	 */
+	if (subLinkType == ARRAY_SUBLINK)
+	{
+		md_array_element_type = get_element_type(subplan-firstColType);
+		use_md_array_builder = OidIsValid(md_array_element_type);
+	}
+
+	/*
 	 * We are probably in a short-lived expression-evaluation context. Switch
 	 * to the per-query context for manipulating the child plan's chgParam,
 	 * calling ExecProcNode on it, etc.
@@ -366,8 +379,16 @@ ExecScanSubPlan(SubPlanState *node,
 			/* stash away current value */
 			Assert(subplan-firstColType == tdesc-attrs[0]-atttypid);
 			dvalue = slot_getattr(slot, 1, disnull);
-			astate = accumArrayResult(astate, dvalue, disnull,
-	  subplan-firstColType, oldcontext);
+
+			if (use_md_array_builder)
+mdastate = accumMdArray(mdastate,
+		disnull? NULL :
+ DatumGetArrayTypeP(dvalue),
+		disnull, md_array_element_type,
+		oldcontext);
+			else
+astate = accumArrayResult(astate, dvalue, disnull,
+		  subplan-firstColType, oldcontext);
 			/* keep scanning subplan to collect all values */
 			continue;
 		}
@@ -439,6 +460,8 @@ ExecScanSubPlan(SubPlanState *node,
 		/* We return the result in the caller's context */
 		if (astate != NULL)
 			result = makeArrayResult(astate, oldcontext);
+		else if (mdastate != NULL)
+			result = PointerGetDatum(makeMdArray(mdastate, oldcontext, true));
 		else
 			result = PointerGetDatum(construct_empty_array(subplan-firstColType));
 	}
@@ -951,7 +974,10 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 	ListCell   *pvar;
 	ListCell   *l;
 	bool		found = false;
+	bool		use_md_array_builder = false;
+	Oid			md_array_element_type = InvalidOid;
 	ArrayBuildState *astate =

Re: [HACKERS] Function array_agg(array)

2014-10-27 Thread Ali Akbar

2014-10-27 16:15 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 I did some minor changes in code

 * move tests of old or new builder style for array sublink out of main
 cycles
 * some API simplification of new builder - we should not to create
 identical API, mainly it has no sense


minor changes in the patch:
* remove array_agg_finalfn_internal declaration in top of array_userfuncs.c
* fix comment of makeMdArray
* fix return of makeMdArray
* remove unnecesary changes to array_agg_finalfn

Regards,
--
Ali Akbar
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..f59738a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12046,6 +12046,22 @@ NULL baz/literallayout(3 rows)/entry
  row
   entry
indexterm
+primaryarray_agg/primary
+   /indexterm
+   functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+  /entry
+  entry
+   any
+  /entry
+  entry
+   the same array type as input type
+  /entry
+  entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+ /row
+
+ row
+  entry
+   indexterm
 primaryaverage/primary
/indexterm
indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 2f0680f..8c182a4 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2238,6 +2238,11 @@ SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
  array
 ---
  {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+
+SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+ array
+---
+ {{1},{2},{3},{4},{5}}
 (1 row)
 /programlisting
The subquery must return a single column. The resulting
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 401bad4..21b8de1 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -231,7 +231,10 @@ ExecScanSubPlan(SubPlanState *node,
 	bool		found = false;	/* TRUE if got at least one subplan tuple */
 	ListCell   *pvar;
 	ListCell   *l;
+	bool		use_md_array_builder = false;
+	Oid			md_array_element_type = InvalidOid;
 	ArrayBuildState *astate = NULL;
+	MdArrayBuildState *mdastate = NULL;
 
 	/*
 	 * MULTIEXPR subplans, when executed, just return NULL; but first we
@@ -260,6 +263,16 @@ ExecScanSubPlan(SubPlanState *node,
 	}
 
 	/*
+	 * use a fast array multidimensional builder when input is a array
+	 * only check on first iteration. On subsequent, use the cached values
+	 */
+	if (subLinkType == ARRAY_SUBLINK)
+	{
+		md_array_element_type = get_element_type(subplan-firstColType);
+		use_md_array_builder = OidIsValid(md_array_element_type);
+	}
+
+	/*
 	 * We are probably in a short-lived expression-evaluation context. Switch
 	 * to the per-query context for manipulating the child plan's chgParam,
 	 * calling ExecProcNode on it, etc.
@@ -366,8 +379,16 @@ ExecScanSubPlan(SubPlanState *node,
 			/* stash away current value */
 			Assert(subplan-firstColType == tdesc-attrs[0]-atttypid);
 			dvalue = slot_getattr(slot, 1, disnull);
-			astate = accumArrayResult(astate, dvalue, disnull,
-	  subplan-firstColType, oldcontext);
+
+			if (use_md_array_builder)
+mdastate = accumMdArray(mdastate,
+		disnull? NULL :
+ DatumGetArrayTypeP(dvalue),
+		disnull, md_array_element_type,
+		oldcontext);
+			else
+astate = accumArrayResult(astate, dvalue, disnull,
+		  subplan-firstColType, oldcontext);
 			/* keep scanning subplan to collect all values */
 			continue;
 		}
@@ -439,6 +460,8 @@ ExecScanSubPlan(SubPlanState *node,
 		/* We return the result in the caller's context */
 		if (astate != NULL)
 			result = makeArrayResult(astate, oldcontext);
+		else if (mdastate != NULL)
+			result = PointerGetDatum(makeMdArray(mdastate, oldcontext, true));
 		else
 			result = PointerGetDatum(construct_empty_array(subplan-firstColType));
 	}
@@ -951,7 +974,10 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 	ListCell   *pvar;
 	ListCell   *l;
 	bool		found = false;
+	bool		use_md_array_builder = false;
+	Oid			md_array_element_type = InvalidOid;
 	ArrayBuildState *astate = NULL;
+	MdArrayBuildState *mdastate = NULL;
 
 	if (subLinkType == ANY_SUBLINK ||
 		subLinkType == ALL_SUBLINK)
@@ -960,6 +986,16 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 		elog(ERROR, CTE subplans should not be executed via ExecSetParamPlan);
 
 	/*
+	 * use a fast array multidimensional builder when input is a array
+	 * only check on first iteration. On subsequent, use the cached values
+	 */
+	if (subLinkType == ARRAY_SUBLINK)
+	{
+		md_array_element_type = get_element_type(subplan-firstColType);
+		use_md_array_builder =

Re: [HACKERS] Function array_agg(array)

2014-10-27 Thread Pavel Stehule

2014-10-27 11:20 GMT+01:00 Ali Akbar the.ap...@gmail.com:


 2014-10-27 16:15 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 I did some minor changes in code

 * move tests of old or new builder style for array sublink out of main
 cycles
 * some API simplification of new builder - we should not to create
 identical API, mainly it has no sense


 minor changes in the patch:
 * remove array_agg_finalfn_internal declaration in top of array_userfuncs.c
 * fix comment of makeMdArray
 * fix return of makeMdArray
 * remove unnecesary changes to array_agg_finalfn


super

I tested last version and I have not any objections.

1. We would to have this feature - it is long time number of our ToDo List

2. Proposal and design of multidimensional aggregation is clean and nobody
has objection here.

3. There is zero impact on current implementation. From performance reasons
it uses new array optimized aggregator - 30% faster for this purpose than
current (scalar) array aggregator

4. Code is clean and respect PostgreSQL coding rules

5. There are documentation and necessary regress tests

6. Patching and compilation is clean without warnings.

This patch is ready for commit

Thank you for patch

Regards

Pavel



 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-27 Thread Ali Akbar


 super

 I tested last version and I have not any objections.

 1. We would to have this feature - it is long time number of our ToDo List

 2. Proposal and design of multidimensional aggregation is clean and nobody
 has objection here.

 3. There is zero impact on current implementation. From performance
 reasons it uses new array optimized aggregator - 30% faster for this
 purpose than current (scalar) array aggregator

 4. Code is clean and respect PostgreSQL coding rules

 5. There are documentation and necessary regress tests

 6. Patching and compilation is clean without warnings.

 This patch is ready for commit

 Thank you for patch


Thank you for the thorough review process.

Regards,
-- 
Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-26 Thread Pavel Stehule

Hi

My idea is using new ArrayBuilder optimized for building multidimensional
arrays with own State type. I think so casting to ArrayBuildState is base
of our problems, so I don't would to do. Code in array_agg_* is simple,
little bit more complex code is in nodeSubplan.c. Some schematic changes
are in attachments.

Regards

Pavel



2014-10-25 15:58 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 you can check it? We can test, how performance lost we get. As second
 benefit we can get numbers for introduction new optimized array builder


 array_agg(anyarray) with deconstruct_array, unchanged accumArrayResult and
 makeMdArrayResult:

 INSERT 0 1
 Time: 852,527 ms
 INSERT 0 1
 Time: 844,275 ms
 INSERT 0 1
 Time: 858,855 ms
 INSERT 0 1
 Time: 861,072 ms
 INSERT 0 1
 Time: 952,006 ms
 INSERT 0 1
 Time: 953,918 ms
 INSERT 0 1
 Time: 926,945 ms
 INSERT 0 1
 Time: 923,692 ms
 INSERT 0 1
 Time: 940,916 ms
 INSERT 0 1
 Time: 948,700 ms
 INSERT 0 1
 Time: 933,333 ms
 INSERT 0 1
 Time: 948,869 ms
 INSERT 0 1
 Time: 847,113 ms
 INSERT 0 1
 Time: 908,572 ms



 Total: 12776.83

 Avg: 912,63


 with last patch (v10):

 INSERT 0 1
 Time: 643,339 ms
 INSERT 0 1
 Time: 608,010 ms
 INSERT 0 1
 Time: 610,465 ms
 INSERT 0 1
 Time: 613,931 ms
 INSERT 0 1
 Time: 616,466 ms
 INSERT 0 1
 Time: 634,754 ms
 INSERT 0 1
 Time: 683,566 ms
 INSERT 0 1
 Time: 656,665 ms
 INSERT 0 1
 Time: 630,096 ms
 INSERT 0 1
 Time: 607,564 ms
 INSERT 0 1
 Time: 610,353 ms
 INSERT 0 1
 Time: 626,816 ms
 INSERT 0 1
 Time: 610,450 ms
 INSERT 0 1
 Time: 614,342 ms



 Total: 8842,7
 Avg: 631,6


 It's 30% faster (i tried varlena element - text). I tried several times
 and it's consistent in +/- 30%.


 quick  dirty non-optimized patch and the test script attached.

 Regards,
 --
 Ali Akbar

ExecScanSubPlan(...)

   ArrayBuildState *astate = NULL;
   MdArrayBuildState *mdastate = NULL;
   bool use_md_array_builder;

   if (subLinkType == MULTIEXPR_SUBLINK)
   {
   }

   if (subLinkType == ARRAY_SUBLINK)
   {
  Assert(subplan-firstColType == tdesc-attrs[0]-attypid);

  /* use a fast array multidimensional builder when input is a array */
  use_md_array_builder = OidIsValid(get_element_type(subplan-firstColType);
   }
   
   ...
   
   for (slot = ExecProcNode() ...)
   {


  if (subLinkType == ARRAY_SUBLINK)
  {
 Datum   dvalue;
 booldisnull;

 found = true;
 dvalue = slot_getattr(slot, 1, disnull);

 if (use_md_array_builder)
mdastate = accumMdArray(...);
 else
astate = accumArrayResult(...);
  }

   } /* endfor */

   if (subLinkType == ARRAY_SUBLINK)
   {
  ..
  if (astate != NULL)
node-curArray = makeArrayResult(astate, ...);
  else if (mdastate != NULL)
node-curArray = makeMdArray(mdastate, ...);
  else
  {
  }
   }

   

-- 
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] Function array_agg(array)

2014-10-26 Thread Ali Akbar

2014-10-27 1:38 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 My idea is using new ArrayBuilder optimized for building multidimensional
 arrays with own State type. I think so casting to ArrayBuildState is base
 of our problems, so I don't would to do. Code in array_agg_* is simple,
 little bit more complex code is in nodeSubplan.c. Some schematic changes
 are in attachments.


Thanks! The structure looks clear, and thanks for the example on
nodeSubplan.c. I will restructure the v10 of the patch to this structure.

Regards,
--
Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Ali Akbar

2014-10-25 10:29 GMT+07:00 Ali Akbar the.ap...@gmail.com:

 I fixed small issue in regress tests and I enhanced tests for varlena
 types and null values.

 Thanks.

 it is about 15% faster than original implementation.

 15% faster than array_agg(scalar)? I haven't verify the performance, but
 because the internal array data and null bitmap is copied as-is, that will
 be faster.

 2014-10-25 1:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi Ali


 I checked a code. I am thinking so code organization is not good.
 accumArrayResult is too long now. makeMdArrayResult will not work, when
 arrays was joined (it is not consistent now). I don't like a usage of
 state-is_array_accum in array_userfunc.c -- it is signal of wrong
 wrapping.

 Yes, i was thinking the same. Attached WIP patch to reorganizate the code.
 makeMdArrayResult works now, with supplied arguments act as override from
 default values calculated from ArrayBuildStateArray.

 In array_userfunc.c, because we don't want to override ndims, dims and
 lbs, i copied array_agg_finalfn and only change the call to
 makeMdArrayResult (we don't uses makeArrayResult because we want to set
 release to false). Another alternative is to create new
 makeArrayResult-like function that has parameter bool release.


adding makeArrayResult1 (do you have better name for this?), that accepts
bool release parameter. array_agg_finalfn becomes more clean, and no
duplicate code in array_agg_anyarray_finalfn.


 next question: there is function array_append(anyarray, anyelement). Isn't
 time to define array_append(anyarray, anyarray) now?


 There is array_cat(anyarray, anyarray):

 /*-
  * array_cat :
  * concatenate two nD arrays to form an nD array, or
  * push an (n-1)D array onto the end of an nD array

  *
  */


Regards,
-- 
Ali Akbar
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..f59738a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12046,6 +12046,22 @@ NULL baz/literallayout(3 rows)/entry
  row
   entry
indexterm
+primaryarray_agg/primary
+   /indexterm
+   functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+  /entry
+  entry
+   any
+  /entry
+  entry
+   the same array type as input type
+  /entry
+  entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+ /row
+
+ row
+  entry
+   indexterm
 primaryaverage/primary
/indexterm
indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 2f0680f..8c182a4 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2238,6 +2238,11 @@ SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
  array
 ---
  {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+
+SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+ array
+---
+ {{1},{2},{3},{4},{5}}
 (1 row)
 /programlisting
The subquery must return a single column. The resulting
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 41e973b..0261fcb 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -108,12 +108,16 @@ exprType(const Node *expr)
 	type = exprType((Node *) tent-expr);
 	if (sublink-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-			format_type_be(exprType((Node *) tent-expr);
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+		(errcode(ERRCODE_UNDEFINED_OBJECT),
+		 errmsg(could not find array type for data type %s,
+format_type_be(exprType((Node *) tent-expr);
+		}
 	}
 }
 else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
@@ -139,12 +143,16 @@ exprType(const Node *expr)
 	type = subplan-firstColType;
 	if (subplan-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-	format_type_be(subplan-firstColType;
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Pavel Stehule

2014-10-25 8:19 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 2014-10-25 10:29 GMT+07:00 Ali Akbar the.ap...@gmail.com:

 I fixed small issue in regress tests and I enhanced tests for varlena
 types and null values.

 Thanks.

 it is about 15% faster than original implementation.

 15% faster than array_agg(scalar)? I haven't verify the performance, but
 because the internal array data and null bitmap is copied as-is, that will
 be faster.

 2014-10-25 1:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi Ali


 I checked a code. I am thinking so code organization is not good.
 accumArrayResult is too long now. makeMdArrayResult will not work, when
 arrays was joined (it is not consistent now). I don't like a usage of
 state-is_array_accum in array_userfunc.c -- it is signal of wrong
 wrapping.

 Yes, i was thinking the same. Attached WIP patch to reorganizate the
 code. makeMdArrayResult works now, with supplied arguments act as override
 from default values calculated from ArrayBuildStateArray.

 In array_userfunc.c, because we don't want to override ndims, dims and
 lbs, i copied array_agg_finalfn and only change the call to
 makeMdArrayResult (we don't uses makeArrayResult because we want to set
 release to false). Another alternative is to create new
 makeArrayResult-like function that has parameter bool release.


 adding makeArrayResult1 (do you have better name for this?), that accepts
 bool release parameter. array_agg_finalfn becomes more clean, and no
 duplicate code in array_agg_anyarray_finalfn.


 makeArrayResult1 - I have no better name now

I found one next minor detail.

you reuse a array_agg_transfn function. Inside is a message
array_agg_transfn called in non-aggregate context. It is not correct for
array_agg_anyarray_transfn

probably specification dim and lbs in array_agg_finalfn is useless, when
you expect a natural result. A automatic similar to
array_agg_anyarray_finalfn should work there too.

makeMdArrayResultArray and appendArrayDatum  should be marked as static



 next question: there is function array_append(anyarray, anyelement).
 Isn't time to define array_append(anyarray, anyarray) now?


 There is array_cat(anyarray, anyarray):

 /*-
  * array_cat :
  * concatenate two nD arrays to form an nD array, or
  * push an (n-1)D array onto the end of an nD array

  
 *
  */


 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Pavel Stehule

2014-10-25 8:33 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2014-10-25 8:19 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 2014-10-25 10:29 GMT+07:00 Ali Akbar the.ap...@gmail.com:

 I fixed small issue in regress tests and I enhanced tests for varlena
 types and null values.

 Thanks.

 it is about 15% faster than original implementation.

 15% faster than array_agg(scalar)? I haven't verify the performance, but
 because the internal array data and null bitmap is copied as-is, that will
 be faster.

 2014-10-25 1:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi Ali


 I checked a code. I am thinking so code organization is not good.
 accumArrayResult is too long now. makeMdArrayResult will not work, when
 arrays was joined (it is not consistent now). I don't like a usage of
 state-is_array_accum in array_userfunc.c -- it is signal of wrong
 wrapping.

 Yes, i was thinking the same. Attached WIP patch to reorganizate the
 code. makeMdArrayResult works now, with supplied arguments act as override
 from default values calculated from ArrayBuildStateArray.

 In array_userfunc.c, because we don't want to override ndims, dims and
 lbs, i copied array_agg_finalfn and only change the call to
 makeMdArrayResult (we don't uses makeArrayResult because we want to set
 release to false). Another alternative is to create new
 makeArrayResult-like function that has parameter bool release.


 adding makeArrayResult1 (do you have better name for this?), that accepts
 bool release parameter. array_agg_finalfn becomes more clean, and no
 duplicate code in array_agg_anyarray_finalfn.


  makeArrayResult1 - I have no better name now

 I found one next minor detail.

 you reuse a array_agg_transfn function. Inside is a message
 array_agg_transfn called in non-aggregate context. It is not correct for
 array_agg_anyarray_transfn

 probably specification dim and lbs in array_agg_finalfn is useless, when
 you expect a natural result. A automatic similar to
 array_agg_anyarray_finalfn should work there too.

 makeMdArrayResultArray and appendArrayDatum  should be marked as static


I am thinking so change of makeArrayResult is wrong. It is developed for 1D
array. When we expect somewhere ND result now, then we should to use
makeMdArrayResult there. So makeArrayResult should to return 1D array in
all cases. Else a difference between makeArrayResult and makeMdArrayResult
hasn't big change and we have a problem with naming.

Regards

Pavel






 next question: there is function array_append(anyarray, anyelement).
 Isn't time to define array_append(anyarray, anyarray) now?


 There is array_cat(anyarray, anyarray):

 /*-
  * array_cat :
  * concatenate two nD arrays to form an nD array, or
  * push an (n-1)D array onto the end of an nD array

  
 *
  */


 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Ali Akbar


  makeArrayResult1 - I have no better name now

 I found one next minor detail.

 you reuse a array_agg_transfn function. Inside is a message
 array_agg_transfn called in non-aggregate context. It is not correct for
 array_agg_anyarray_transfn


Fixed.


 probably specification dim and lbs in array_agg_finalfn is useless, when
 you expect a natural result. A automatic similar to
 array_agg_anyarray_finalfn should work there too.

 makeMdArrayResultArray and appendArrayDatum  should be marked as static


ok, marked all of that as static.


 I am thinking so change of makeArrayResult is wrong. It is developed for
 1D array. When we expect somewhere ND result now, then we should to use
 makeMdArrayResult there. So makeArrayResult should to return 1D array in
 all cases. Else a difference between makeArrayResult and makeMdArrayResult
 hasn't big change and we have a problem with naming.


I'm thinking it like this:
- if we want to accumulate array normally, use accumArrayResult and
makeArrayResult. If we accumulate scalar the result will be 1D array. if we
accumulate array, the resulting dimension is incremented by 1.
- if we want, somehow to affect the normal behavior, and change the
dimensions, use makeMdArrayResult.

Searching through the postgres' code, other than internal use in
arrayfuncs.c, makeMdArrayResult is used only
in src/pl/plperl/plperl.c:plperl_array_to_datum, while converting perl
array to postgres array.

So if somehow we will accumulate array other than in array_agg, i think the
most natural way is using accumArrayResult and then makeArrayResult.

CMIIW

Regards,
--
Ali Akbar
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..f59738a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12046,6 +12046,22 @@ NULL baz/literallayout(3 rows)/entry
  row
   entry
indexterm
+primaryarray_agg/primary
+   /indexterm
+   functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+  /entry
+  entry
+   any
+  /entry
+  entry
+   the same array type as input type
+  /entry
+  entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+ /row
+
+ row
+  entry
+   indexterm
 primaryaverage/primary
/indexterm
indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 2f0680f..8c182a4 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2238,6 +2238,11 @@ SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
  array
 ---
  {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+
+SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+ array
+---
+ {{1},{2},{3},{4},{5}}
 (1 row)
 /programlisting
The subquery must return a single column. The resulting
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 41e973b..0261fcb 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -108,12 +108,16 @@ exprType(const Node *expr)
 	type = exprType((Node *) tent-expr);
 	if (sublink-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-			format_type_be(exprType((Node *) tent-expr);
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+		(errcode(ERRCODE_UNDEFINED_OBJECT),
+		 errmsg(could not find array type for data type %s,
+format_type_be(exprType((Node *) tent-expr);
+		}
 	}
 }
 else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
@@ -139,12 +143,16 @@ exprType(const Node *expr)
 	type = subplan-firstColType;
 	if (subplan-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-	format_type_be(subplan-firstColType;
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+		(errcode(ERRCODE_UNDEFINED_OBJECT),
+		 errmsg(could not find array type for data type %s,
+		format_type_be(subplan-firstColType;
+		}
 	}
 }
 else if (subplan-subLinkType == MULTIEXPR_SUBLINK)
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Pavel Stehule

2014-10-25 10:16 GMT+02:00 Ali Akbar the.ap...@gmail.com:

  makeArrayResult1 - I have no better name now

 I found one next minor detail.

 you reuse a array_agg_transfn function. Inside is a message
 array_agg_transfn called in non-aggregate context. It is not correct for
 array_agg_anyarray_transfn


 Fixed.


 probably specification dim and lbs in array_agg_finalfn is useless, when
 you expect a natural result. A automatic similar to
 array_agg_anyarray_finalfn should work there too.

 makeMdArrayResultArray and appendArrayDatum  should be marked as static


 ok, marked all of that as static.


 I am thinking so change of makeArrayResult is wrong. It is developed for
 1D array. When we expect somewhere ND result now, then we should to use
 makeMdArrayResult there. So makeArrayResult should to return 1D array in
 all cases. Else a difference between makeArrayResult and makeMdArrayResult
 hasn't big change and we have a problem with naming.


 I'm thinking it like this:
 - if we want to accumulate array normally, use accumArrayResult and
 makeArrayResult. If we accumulate scalar the result will be 1D array. if we
 accumulate array, the resulting dimension is incremented by 1.
 - if we want, somehow to affect the normal behavior, and change the
 dimensions, use makeMdArrayResult.

 Searching through the postgres' code, other than internal use in
 arrayfuncs.c, makeMdArrayResult is used only
 in src/pl/plperl/plperl.c:plperl_array_to_datum, while converting perl
 array to postgres array.

 So if somehow we will accumulate array other than in array_agg, i think
 the most natural way is using accumArrayResult and then makeArrayResult.



ok, there is more variants and I can't to decide. But I am not satisfied
with this API. We do some wrong in structure. makeMdArrayResult is now
ugly.





 CMIIW

 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Ali Akbar

2014-10-25 15:43 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:



 2014-10-25 10:16 GMT+02:00 Ali Akbar the.ap...@gmail.com:

  makeArrayResult1 - I have no better name now

 I found one next minor detail.

 you reuse a array_agg_transfn function. Inside is a message
 array_agg_transfn called in non-aggregate context. It is not correct for
 array_agg_anyarray_transfn


 Fixed.


 probably specification dim and lbs in array_agg_finalfn is useless, when
 you expect a natural result. A automatic similar to
 array_agg_anyarray_finalfn should work there too.

 makeMdArrayResultArray and appendArrayDatum  should be marked as
 static


 ok, marked all of that as static.


 I am thinking so change of makeArrayResult is wrong. It is developed for
 1D array. When we expect somewhere ND result now, then we should to use
 makeMdArrayResult there. So makeArrayResult should to return 1D array in
 all cases. Else a difference between makeArrayResult and makeMdArrayResult
 hasn't big change and we have a problem with naming.


 I'm thinking it like this:
 - if we want to accumulate array normally, use accumArrayResult and
 makeArrayResult. If we accumulate scalar the result will be 1D array. if we
 accumulate array, the resulting dimension is incremented by 1.
 - if we want, somehow to affect the normal behavior, and change the
 dimensions, use makeMdArrayResult.

 Searching through the postgres' code, other than internal use in
 arrayfuncs.c, makeMdArrayResult is used only
 in src/pl/plperl/plperl.c:plperl_array_to_datum, while converting perl
 array to postgres array.

 So if somehow we will accumulate array other than in array_agg, i think
 the most natural way is using accumArrayResult and then makeArrayResult.


 ok, there is more variants and I can't to decide. But I am not satisfied
 with this API. We do some wrong in structure. makeMdArrayResult is now
 ugly.


One approach that i can think is we cleanly separate the structures and
API. We don't touch existing ArrayBuildState, accumArrayResult,
makeArrayResult and makeMdArrayResult, and we create new functions:
ArrayBuildStateArray, accumArrayResultArray, makeArrayResultArray and
makeArrayResultArrayCustom.

But if we do that, the array(subselect) implementation will not be as
simple as current patch. We must also change all the ARRAY_SUBLINK-related
accumArrayResult call.

Regarding current patch implementation, the specific typedefs are declared
as:
+typedef struct ArrayBuildStateScalar
+{
+ ArrayBuildState astate;
...
+} ArrayBuildStateArray;

so it necessities rather ugly code like this:
+ result-elemtype = astate-astate.element_type;

Can we use C11 feature of unnamed struct like this? :
+typedef struct ArrayBuildStateScalar
+{
+ ArrayBuildState;
...
+} ArrayBuildStateArray;

so the code can be a little less ugly by doing it like this:
+ result-elemtype = astate-element_type;

I don't know whether all currently supported compilers implements this
feature..


Can you suggest a better structure for this?

If we can't settle on a better structure, i think i'll reimplement
array_agg without the performance improvement, using deconstruct_array and
unchanged accumArrayResult  makeMdArrayResult.

Thanks,
-- 
Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Pavel Stehule

2014-10-25 12:20 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 2014-10-25 15:43 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:



 2014-10-25 10:16 GMT+02:00 Ali Akbar the.ap...@gmail.com:

  makeArrayResult1 - I have no better name now

 I found one next minor detail.

 you reuse a array_agg_transfn function. Inside is a message
 array_agg_transfn called in non-aggregate context. It is not correct for
 array_agg_anyarray_transfn


 Fixed.


 probably specification dim and lbs in array_agg_finalfn is useless,
 when you expect a natural result. A automatic similar to
 array_agg_anyarray_finalfn should work there too.

 makeMdArrayResultArray and appendArrayDatum  should be marked as
 static


 ok, marked all of that as static.


 I am thinking so change of makeArrayResult is wrong. It is developed
 for 1D array. When we expect somewhere ND result now, then we should to use
 makeMdArrayResult there. So makeArrayResult should to return 1D array in
 all cases. Else a difference between makeArrayResult and makeMdArrayResult
 hasn't big change and we have a problem with naming.


 I'm thinking it like this:
 - if we want to accumulate array normally, use accumArrayResult and
 makeArrayResult. If we accumulate scalar the result will be 1D array. if we
 accumulate array, the resulting dimension is incremented by 1.
 - if we want, somehow to affect the normal behavior, and change the
 dimensions, use makeMdArrayResult.

 Searching through the postgres' code, other than internal use in
 arrayfuncs.c, makeMdArrayResult is used only
 in src/pl/plperl/plperl.c:plperl_array_to_datum, while converting perl
 array to postgres array.

 So if somehow we will accumulate array other than in array_agg, i think
 the most natural way is using accumArrayResult and then makeArrayResult.


 ok, there is more variants and I can't to decide. But I am not satisfied
 with this API. We do some wrong in structure. makeMdArrayResult is now
 ugly.


 One approach that i can think is we cleanly separate the structures and
 API. We don't touch existing ArrayBuildState, accumArrayResult,
 makeArrayResult and makeMdArrayResult, and we create new functions:
 ArrayBuildStateArray, accumArrayResultArray, makeArrayResultArray and
 makeArrayResultArrayCustom.


yes, I am thinking about separatate path, that will be joined in
constructMdArray

In reality, there are two different array builders - with different API and
better to respect it.



 But if we do that, the array(subselect) implementation will not be as
 simple as current patch. We must also change all the ARRAY_SUBLINK-related
 accumArrayResult call.

 Regarding current patch implementation, the specific typedefs are declared
 as:
 +typedef struct ArrayBuildStateScalar
 +{
 + ArrayBuildState astate;
 ...
 +} ArrayBuildStateArray;

 so it necessities rather ugly code like this:
 + result-elemtype = astate-astate.element_type;

 Can we use C11 feature of unnamed struct like this? :


no, what I know a C11 is prohibited


 +typedef struct ArrayBuildStateScalar
 +{
 + ArrayBuildState;
 ...
 +} ArrayBuildStateArray;

 so the code can be a little less ugly by doing it like this:
 + result-elemtype = astate-element_type;

 I don't know whether all currently supported compilers implements this
 feature..


 Can you suggest a better structure for this?

 If we can't settle on a better structure, i think i'll reimplement
 array_agg without the performance improvement, using deconstruct_array and
 unchanged accumArrayResult  makeMdArrayResult.


you can check it? We can test, how performance lost we get. As second
benefit we can get numbers for introduction new optimized array builder

Regards

Pavel



 Thanks,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-25 Thread Ali Akbar


 you can check it? We can test, how performance lost we get. As second
 benefit we can get numbers for introduction new optimized array builder


array_agg(anyarray) with deconstruct_array, unchanged accumArrayResult and
makeMdArrayResult:

 INSERT 0 1
 Time: 852,527 ms
 INSERT 0 1
 Time: 844,275 ms
 INSERT 0 1
 Time: 858,855 ms
 INSERT 0 1
 Time: 861,072 ms
 INSERT 0 1
 Time: 952,006 ms
 INSERT 0 1
 Time: 953,918 ms
 INSERT 0 1
 Time: 926,945 ms
 INSERT 0 1
 Time: 923,692 ms
 INSERT 0 1
 Time: 940,916 ms
 INSERT 0 1
 Time: 948,700 ms
 INSERT 0 1
 Time: 933,333 ms
 INSERT 0 1
 Time: 948,869 ms
 INSERT 0 1
 Time: 847,113 ms
 INSERT 0 1
 Time: 908,572 ms



Total: 12776.83

Avg: 912,63


with last patch (v10):

 INSERT 0 1
 Time: 643,339 ms
 INSERT 0 1
 Time: 608,010 ms
 INSERT 0 1
 Time: 610,465 ms
 INSERT 0 1
 Time: 613,931 ms
 INSERT 0 1
 Time: 616,466 ms
 INSERT 0 1
 Time: 634,754 ms
 INSERT 0 1
 Time: 683,566 ms
 INSERT 0 1
 Time: 656,665 ms
 INSERT 0 1
 Time: 630,096 ms
 INSERT 0 1
 Time: 607,564 ms
 INSERT 0 1
 Time: 610,353 ms
 INSERT 0 1
 Time: 626,816 ms
 INSERT 0 1
 Time: 610,450 ms
 INSERT 0 1
 Time: 614,342 ms



Total: 8842,7
 Avg: 631,6


It's 30% faster (i tried varlena element - text). I tried several times and
it's consistent in +/- 30%.


quick  dirty non-optimized patch and the test script attached.

Regards,
--
Ali Akbar
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..f59738a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12046,6 +12046,22 @@ NULL baz/literallayout(3 rows)/entry
  row
   entry
indexterm
+primaryarray_agg/primary
+   /indexterm
+   functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+  /entry
+  entry
+   any
+  /entry
+  entry
+   the same array type as input type
+  /entry
+  entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+ /row
+
+ row
+  entry
+   indexterm
 primaryaverage/primary
/indexterm
indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 2f0680f..8c182a4 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2238,6 +2238,11 @@ SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
  array
 ---
  {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+
+SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+ array
+---
+ {{1},{2},{3},{4},{5}}
 (1 row)
 /programlisting
The subquery must return a single column. The resulting
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 41e973b..0261fcb 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -108,12 +108,16 @@ exprType(const Node *expr)
 	type = exprType((Node *) tent-expr);
 	if (sublink-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-			format_type_be(exprType((Node *) tent-expr);
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+		(errcode(ERRCODE_UNDEFINED_OBJECT),
+		 errmsg(could not find array type for data type %s,
+format_type_be(exprType((Node *) tent-expr);
+		}
 	}
 }
 else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
@@ -139,12 +143,16 @@ exprType(const Node *expr)
 	type = subplan-firstColType;
 	if (subplan-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-	format_type_be(subplan-firstColType;
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+		(errcode(ERRCODE_UNDEFINED_OBJECT),
+		 errmsg(could not find array type for data type %s,
+		format_type_be(subplan-firstColType;
+		}
 	}
 }
 else if (subplan-subLinkType == MULTIEXPR_SUBLINK)
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 3e7dc85..8fc8b49 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -668,10 +668,16 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
 
 		Assert(!te-resjunk);
 		Assert(testexpr == NULL);
-

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Ali Akbar

Updated patch attached.

2014-10-22 20:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 I agree with your proposal. I have a few comments to design:


 1. patch doesn't hold documentation and regress tests, please append
 it.


i've added some regression tests in arrays.sql and aggregate.sql.

I've only found the documentation of array_agg. Where is the doc for
array(subselect) defined?


 2. this functionality (multidimensional aggregation) can be interesting
 more times, so maybe some interface like array builder should be 
 preferred.

 We already have accumArrayResult and makeArrayResult/makeMdArrayResult,
 haven't we?

 Actually array_agg(anyarray) can be implemented by using
 accumArrayResult and makeMdArrayResult, but in the process we will
 deconstruct the array data and null bit-flags into ArrayBuildState-dvalues
 and dnulls. And we will reconstruct it through makeMdArrayResult. I think
 this way it's not efficient, so i decided to reimplement it and memcpy the
 array data and null flags as-is.


 aha, so isn't better to fix a performance for accumArrayResult ?


 Ok, i'll go this route. While reading the code of array(subselect) as you
 pointed below, i think the easiest way to implement support for both
 array_agg(anyarray) and array(subselect) is to change accumArrayResult so
 it operates both with scalar datum(s) and array datums, with performance
 optimization for the latter.


implemented it by modifying ArrayBuildState to ArrayBuildStateArray and
ArrayBuildStateScalar (do you have any idea for better struct naming?)


 In other places, i think it's clearer if we just use accumArrayResult and
 makeArrayResult/makeMdArrayResult as API for building (multidimensional)
 arrays.


 3. array_agg was consistent with array(subselect), so it should be
 fixed too

 postgres=# select array_agg(a) from test;
array_agg
 ---
  {{1,2,3,4},{1,2,3,4}}
 (1 row)

 postgres=# select array(select a from test);
 ERROR:  could not find array type for data type integer[]


 I'm pretty new in postgresql development. Can you point out where is
 array(subselect) implemented?


 where you can start?

 postgres=# explain select array(select a from test);
 ERROR:  42704: could not find array type for data type integer[]
 LOCATION:  exprType, nodeFuncs.c:116

 look to code ... your magic keyword is a ARRAY_SUBLINK .. search in
 postgresql sources this keyword


 attention: probably we don't would to allow arrays everywhere.


I've changed the places where i think it's appropriate.


 4. why you use a magic constant (64) there?

 + astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
 + astate-aitems = 64 * nitems;

 + astate-nullbitmap = (bits8 *)
 + repalloc(astate-nullbitmap, (astate-aitems + 7) /
 8);


 just follow the arbitrary size choosen in accumArrayResult
 (arrayfuncs.c):
 astate-alen = 64; /* arbitrary starting array size */

 it can be any number not too small and too big. Too small, and we will
 realloc shortly. Too big, we will end up wasting memory.


 you can try to alloc 1KB instead as start -- it is used more times in
 Postgres. Then a overhead is max 1KB per agg call - what is acceptable.

 You take this value from accumArrayResult, but it is targeted for
 shorted scalars - you should to expect so any array will be much larger.


this patch allocates 1KB (1024 bytes) if the ndatabytes is  512bytes. If
it is larger, it allocates 4 * size. For nullbitmap, it allocates 4 *
number of items in array.

Regards,
--
Ali Akbar
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 12046,12051  NULL baz/literallayout(3 rows)/entry
--- 12046,12067 
   row
entry
 indexterm
+ primaryarray_agg/primary
+/indexterm
+functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+   /entry
+   entry
+any
+   /entry
+   entry
+the same array type as input type
+   /entry
+   entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+  /row
+ 
+  row
+   entry
+indexterm
  primaryaverage/primary
 /indexterm
 indexterm
*** a/src/backend/nodes/nodeFuncs.c
--- b/src/backend/nodes/nodeFuncs.c
***
*** 108,119  exprType(const Node *expr)
  	type = exprType((Node *) tent-expr);
  	if (sublink-subLinkType == ARRAY_SUBLINK)
  	{
! 		type = get_array_type(type);
! 		if (!OidIsValid(type))
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_OBJECT),
! 	 errmsg(could not find array type for data type %s,
! 			format_type_be(exprType((Node *) tent-expr);
  	}
  }
  else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
--- 108,123 
  	type = exprType((Node *) tent-expr);
  	if (sublink-subLinkType == ARRAY_SUBLINK)
  	{
! 		if

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Pavel Stehule

Hi

it looks well

doc:
http://www.postgresql.org/docs/9.4/static/sql-expressions.html#SQL-SYNTAX-ARRAY-CONSTRUCTORS
it should be fixed too

Regards

Pavel

2014-10-24 10:24 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 Updated patch attached.

 2014-10-22 20:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 I agree with your proposal. I have a few comments to design:


 1. patch doesn't hold documentation and regress tests, please append
 it.


 i've added some regression tests in arrays.sql and aggregate.sql.

 I've only found the documentation of array_agg. Where is the doc for
 array(subselect) defined?


 2. this functionality (multidimensional aggregation) can be interesting
 more times, so maybe some interface like array builder should be 
 preferred.

 We already have accumArrayResult and
 makeArrayResult/makeMdArrayResult, haven't we?

 Actually array_agg(anyarray) can be implemented by using
 accumArrayResult and makeMdArrayResult, but in the process we will
 deconstruct the array data and null bit-flags into 
 ArrayBuildState-dvalues
 and dnulls. And we will reconstruct it through makeMdArrayResult. I think
 this way it's not efficient, so i decided to reimplement it and memcpy the
 array data and null flags as-is.


 aha, so isn't better to fix a performance for accumArrayResult ?


 Ok, i'll go this route. While reading the code of array(subselect) as
 you pointed below, i think the easiest way to implement support for both
 array_agg(anyarray) and array(subselect) is to change accumArrayResult so
 it operates both with scalar datum(s) and array datums, with performance
 optimization for the latter.


 implemented it by modifying ArrayBuildState to ArrayBuildStateArray and
 ArrayBuildStateScalar (do you have any idea for better struct naming?)


 In other places, i think it's clearer if we just use accumArrayResult and
 makeArrayResult/makeMdArrayResult as API for building (multidimensional)
 arrays.


 3. array_agg was consistent with array(subselect), so it should be
 fixed too

 postgres=# select array_agg(a) from test;
array_agg
 ---
  {{1,2,3,4},{1,2,3,4}}
 (1 row)

 postgres=# select array(select a from test);
 ERROR:  could not find array type for data type integer[]


 I'm pretty new in postgresql development. Can you point out where is
 array(subselect) implemented?


 where you can start?

 postgres=# explain select array(select a from test);
 ERROR:  42704: could not find array type for data type integer[]
 LOCATION:  exprType, nodeFuncs.c:116

 look to code ... your magic keyword is a ARRAY_SUBLINK .. search in
 postgresql sources this keyword


 attention: probably we don't would to allow arrays everywhere.


 I've changed the places where i think it's appropriate.


 4. why you use a magic constant (64) there?

 + astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
 + astate-aitems = 64 * nitems;

 + astate-nullbitmap = (bits8 *)
 + repalloc(astate-nullbitmap, (astate-aitems + 7) /
 8);


 just follow the arbitrary size choosen in accumArrayResult
 (arrayfuncs.c):
 astate-alen = 64; /* arbitrary starting array size */

 it can be any number not too small and too big. Too small, and we will
 realloc shortly. Too big, we will end up wasting memory.


 you can try to alloc 1KB instead as start -- it is used more times in
 Postgres. Then a overhead is max 1KB per agg call - what is acceptable.

 You take this value from accumArrayResult, but it is targeted for
 shorted scalars - you should to expect so any array will be much larger.


 this patch allocates 1KB (1024 bytes) if the ndatabytes is  512bytes. If
 it is larger, it allocates 4 * size. For nullbitmap, it allocates 4 *
 number of items in array.

 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Ali Akbar

2014-10-24 15:48 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 it looks well

 doc:
 http://www.postgresql.org/docs/9.4/static/sql-expressions.html#SQL-SYNTAX-ARRAY-CONSTRUCTORS
 it should be fixed too

 Regards

 Pavel


doc updated with additional example for array(subselect). patch attached.

Regards,
--
Ali Akbar
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 12046,12051  NULL baz/literallayout(3 rows)/entry
--- 12046,12067 
   row
entry
 indexterm
+ primaryarray_agg/primary
+/indexterm
+functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+   /entry
+   entry
+any
+   /entry
+   entry
+the same array type as input type
+   /entry
+   entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+  /row
+ 
+  row
+   entry
+indexterm
  primaryaverage/primary
 /indexterm
 indexterm
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
***
*** 2238,2243  SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
--- 2238,2248 
   array
  ---
   {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+ 
+ SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+  array
+ ---
+  {{1},{2},{3},{4},{5}}
  (1 row)
  /programlisting
 The subquery must return a single column. The resulting
*** a/src/backend/nodes/nodeFuncs.c
--- b/src/backend/nodes/nodeFuncs.c
***
*** 108,119  exprType(const Node *expr)
  	type = exprType((Node *) tent-expr);
  	if (sublink-subLinkType == ARRAY_SUBLINK)
  	{
! 		type = get_array_type(type);
! 		if (!OidIsValid(type))
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_OBJECT),
! 	 errmsg(could not find array type for data type %s,
! 			format_type_be(exprType((Node *) tent-expr);
  	}
  }
  else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
--- 108,123 
  	type = exprType((Node *) tent-expr);
  	if (sublink-subLinkType == ARRAY_SUBLINK)
  	{
! 		if (!OidIsValid(get_element_type(type)))
! 		{
! 			/* not array, so check for its array type */
! 			type = get_array_type(type);
! 			if (!OidIsValid(type))
! ereport(ERROR,
! 		(errcode(ERRCODE_UNDEFINED_OBJECT),
! 		 errmsg(could not find array type for data type %s,
! format_type_be(exprType((Node *) tent-expr);
! 		}
  	}
  }
  else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
***
*** 139,150  exprType(const Node *expr)
  	type = subplan-firstColType;
  	if (subplan-subLinkType == ARRAY_SUBLINK)
  	{
! 		type = get_array_type(type);
! 		if (!OidIsValid(type))
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_OBJECT),
! 	 errmsg(could not find array type for data type %s,
! 	format_type_be(subplan-firstColType;
  	}
  }
  else if (subplan-subLinkType == MULTIEXPR_SUBLINK)
--- 143,158 
  	type = subplan-firstColType;
  	if (subplan-subLinkType == ARRAY_SUBLINK)
  	{
! 		if (!OidIsValid(get_element_type(type)))
! 		{
! 			/* not array, so check for its array type */
! 			type = get_array_type(type);
! 			if (!OidIsValid(type))
! ereport(ERROR,
! 		(errcode(ERRCODE_UNDEFINED_OBJECT),
! 		 errmsg(could not find array type for data type %s,
! 		format_type_be(subplan-firstColType;
! 		}
  	}
  }
  else if (subplan-subLinkType == MULTIEXPR_SUBLINK)
*** a/src/backend/optimizer/plan/subselect.c
--- b/src/backend/optimizer/plan/subselect.c
***
*** 668,677  build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
  
  		Assert(!te-resjunk);
  		Assert(testexpr == NULL);
! 		arraytype = get_array_type(exprType((Node *) te-expr));
! 		if (!OidIsValid(arraytype))
! 			elog(ERROR, could not find array type for datatype %s,
!  format_type_be(exprType((Node *) te-expr)));
  		prm = generate_new_param(root,
   arraytype,
   exprTypmod((Node *) te-expr),
--- 668,683 
  
  		Assert(!te-resjunk);
  		Assert(testexpr == NULL);
! 
! 		arraytype = exprType((Node *) te-expr);
! 		if (!OidIsValid(get_element_type(arraytype)))
! 		{
! 			/* not array, so get the array type */
! 			arraytype = get_array_type(exprType((Node *) te-expr));
! 			if (!OidIsValid(arraytype))
! elog(ERROR, could not find array type for datatype %s,
! 	 format_type_be(exprType((Node *) te-expr)));
! 		}
  		prm = generate_new_param(root,
   arraytype,
   exprTypmod((Node *) te-expr),
*** a/src/backend/utils/adt/array_userfuncs.c
---

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Pavel Stehule

Hi

some in last patch is wrong, I cannot to compile it:

arrayfuncs.c: In function ‘accumArrayResult’:
arrayfuncs.c:4603:9: error: ‘ArrayBuildState’ has no member named ‘alen’
   astate-alen = 64;  /* arbitrary starting array size */
 ^
arrayfuncs.c:4604:9: error: ‘ArrayBuildState’ has no member named ‘dvalues’
   astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
 ^
arrayfuncs.c:4604:44: error: ‘ArrayBuildState’ has no member named ‘alen’
   astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
^
arrayfuncs.c:4605:9: error: ‘ArrayBuildState’ has no member named ‘dnulls’
   astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
 ^
arrayfuncs.c:4605:42: error: ‘ArrayBuildState’ has no member named ‘alen’
   astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
  ^
arrayfuncs.c:4606:9: error: ‘ArrayBuildState’ has no member named ‘nelems’
   astate-nelems = 0;
 ^
arrayfuncs.c:4618:13: error: ‘ArrayBuildState’ has no member named ‘nelems’
   if (astate-nelems = astate-alen)
 ^
arrayfuncs.c:4618:31: error: ‘ArrayBuildState’ has no member named ‘alen’
   if (astate-nelems = astate-alen)
   ^
arrayfuncs.c:4620:10: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-alen *= 2;


2014-10-24 11:24 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 2014-10-24 15:48 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 it looks well

 doc:
 http://www.postgresql.org/docs/9.4/static/sql-expressions.html#SQL-SYNTAX-ARRAY-CONSTRUCTORS
 it should be fixed too

 Regards

 Pavel


 doc updated with additional example for array(subselect). patch attached.

 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Ali Akbar

2014-10-24 16:26 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 some in last patch is wrong, I cannot to compile it:

 arrayfuncs.c: In function ‘accumArrayResult’:
 arrayfuncs.c:4603:9: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-alen = 64;  /* arbitrary starting array size */
  ^
 arrayfuncs.c:4604:9: error: ‘ArrayBuildState’ has no member named ‘dvalues’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
  ^
 arrayfuncs.c:4604:44: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
 ^
 arrayfuncs.c:4605:9: error: ‘ArrayBuildState’ has no member named ‘dnulls’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
  ^
 arrayfuncs.c:4605:42: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
   ^
 arrayfuncs.c:4606:9: error: ‘ArrayBuildState’ has no member named ‘nelems’
astate-nelems = 0;
  ^
 arrayfuncs.c:4618:13: error: ‘ArrayBuildState’ has no member named ‘nelems’
if (astate-nelems = astate-alen)
  ^
 arrayfuncs.c:4618:31: error: ‘ArrayBuildState’ has no member named ‘alen’
if (astate-nelems = astate-alen)
^
 arrayfuncs.c:4620:10: error: ‘ArrayBuildState’ has no member named ‘alen’
 astate-alen *= 2;


Sorry,  correct patch attached.

This patch is in patience format (git --patience ..). In previous patches,
i use context format (git --patience ... | filterdiff --format=context),
but it turns out that some modification is lost.

-- 
Ali Akbar
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7e5bcd9..f59738a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12046,6 +12046,22 @@ NULL baz/literallayout(3 rows)/entry
  row
   entry
indexterm
+primaryarray_agg/primary
+   /indexterm
+   functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+  /entry
+  entry
+   any
+  /entry
+  entry
+   the same array type as input type
+  /entry
+  entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+ /row
+
+ row
+  entry
+   indexterm
 primaryaverage/primary
/indexterm
indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 2f0680f..8c182a4 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2238,6 +2238,11 @@ SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
  array
 ---
  {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+
+SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+ array
+---
+ {{1},{2},{3},{4},{5}}
 (1 row)
 /programlisting
The subquery must return a single column. The resulting
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 41e973b..0261fcb 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -108,12 +108,16 @@ exprType(const Node *expr)
 	type = exprType((Node *) tent-expr);
 	if (sublink-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-			format_type_be(exprType((Node *) tent-expr);
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+		(errcode(ERRCODE_UNDEFINED_OBJECT),
+		 errmsg(could not find array type for data type %s,
+format_type_be(exprType((Node *) tent-expr);
+		}
 	}
 }
 else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
@@ -139,12 +143,16 @@ exprType(const Node *expr)
 	type = subplan-firstColType;
 	if (subplan-subLinkType == ARRAY_SUBLINK)
 	{
-		type = get_array_type(type);
-		if (!OidIsValid(type))
-			ereport(ERROR,
-	(errcode(ERRCODE_UNDEFINED_OBJECT),
-	 errmsg(could not find array type for data type %s,
-	format_type_be(subplan-firstColType;
+		if (!OidIsValid(get_element_type(type)))
+		{
+			/* not array, so check for its array type */
+			type = get_array_type(type);
+			if (!OidIsValid(type))
+ereport(ERROR,
+		(errcode(ERRCODE_UNDEFINED_OBJECT),
+		 errmsg(could not find array type for data type %s,
+		format_type_be(subplan-firstColType;
+		}
 	}
 }
 else if (subplan-subLinkType ==

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Michael Paquier

On Fri, Oct 24, 2014 at 11:43 AM, Ali Akbar the.ap...@gmail.com wrote:


 2014-10-24 16:26 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 some in last patch is wrong, I cannot to compile it:

 arrayfuncs.c: In function 'accumArrayResult':
 arrayfuncs.c:4603:9: error: 'ArrayBuildState' has no member named 'alen'
astate-alen = 64;  /* arbitrary starting array size */
  ^
 arrayfuncs.c:4604:9: error: 'ArrayBuildState' has no member named
 'dvalues'
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
  ^
 arrayfuncs.c:4604:44: error: 'ArrayBuildState' has no member named 'alen'
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
 ^
 arrayfuncs.c:4605:9: error: 'ArrayBuildState' has no member named 'dnulls'
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
  ^
 arrayfuncs.c:4605:42: error: 'ArrayBuildState' has no member named 'alen'
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
   ^
 arrayfuncs.c:4606:9: error: 'ArrayBuildState' has no member named 'nelems'
astate-nelems = 0;
  ^
 arrayfuncs.c:4618:13: error: 'ArrayBuildState' has no member named
 'nelems'
if (astate-nelems = astate-alen)
  ^
 arrayfuncs.c:4618:31: error: 'ArrayBuildState' has no member named 'alen'
if (astate-nelems = astate-alen)
^
 arrayfuncs.c:4620:10: error: 'ArrayBuildState' has no member named 'alen'
 astate-alen *= 2;


 Sorry,  correct patch attached.

 This patch is in patience format (git --patience ..). In previous patches,
 i use context format (git --patience ... | filterdiff --format=context),
 but it turns out that some modification is lost.

That's not surprising, sometimes filterdiff misses the shot.
-- 
Michael

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Pavel Stehule

2014-10-24 11:43 GMT+02:00 Ali Akbar the.ap...@gmail.com:


 2014-10-24 16:26 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 some in last patch is wrong, I cannot to compile it:

 arrayfuncs.c: In function ‘accumArrayResult’:
 arrayfuncs.c:4603:9: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-alen = 64;  /* arbitrary starting array size */
  ^
 arrayfuncs.c:4604:9: error: ‘ArrayBuildState’ has no member named
 ‘dvalues’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
  ^
 arrayfuncs.c:4604:44: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
 ^
 arrayfuncs.c:4605:9: error: ‘ArrayBuildState’ has no member named ‘dnulls’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
  ^
 arrayfuncs.c:4605:42: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
   ^
 arrayfuncs.c:4606:9: error: ‘ArrayBuildState’ has no member named ‘nelems’
astate-nelems = 0;
  ^
 arrayfuncs.c:4618:13: error: ‘ArrayBuildState’ has no member named
 ‘nelems’
if (astate-nelems = astate-alen)
  ^
 arrayfuncs.c:4618:31: error: ‘ArrayBuildState’ has no member named ‘alen’
if (astate-nelems = astate-alen)
^
 arrayfuncs.c:4620:10: error: ‘ArrayBuildState’ has no member named ‘alen’
 astate-alen *= 2;


 Sorry,  correct patch attached.

 This patch is in patience format (git --patience ..). In previous patches,
 i use context format (git --patience ... | filterdiff --format=context),
 but it turns out that some modification is lost.


last version is compileable, but some is still broken

postgres=# select array_agg(array[i, i+1, i-1])
from generate_series(1,2) a(i);
ERROR:  could not find array type for data type integer[]

but array(subselect) works

postgres=# select array(select a from xx);
   array
---
 {{1,2,3},{1,2,3}}
(1 row)

Regards

Pavel




 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Pavel Stehule

Hi

I did some performance tests and it is interesting:

it is about 15% faster than original implementation.

Regards

Pavel




2014-10-24 13:58 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2014-10-24 11:43 GMT+02:00 Ali Akbar the.ap...@gmail.com:


 2014-10-24 16:26 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 some in last patch is wrong, I cannot to compile it:

 arrayfuncs.c: In function ‘accumArrayResult’:
 arrayfuncs.c:4603:9: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-alen = 64;  /* arbitrary starting array size */
  ^
 arrayfuncs.c:4604:9: error: ‘ArrayBuildState’ has no member named
 ‘dvalues’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
  ^
 arrayfuncs.c:4604:44: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
 ^
 arrayfuncs.c:4605:9: error: ‘ArrayBuildState’ has no member named
 ‘dnulls’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
  ^
 arrayfuncs.c:4605:42: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
   ^
 arrayfuncs.c:4606:9: error: ‘ArrayBuildState’ has no member named
 ‘nelems’
astate-nelems = 0;
  ^
 arrayfuncs.c:4618:13: error: ‘ArrayBuildState’ has no member named
 ‘nelems’
if (astate-nelems = astate-alen)
  ^
 arrayfuncs.c:4618:31: error: ‘ArrayBuildState’ has no member named ‘alen’
if (astate-nelems = astate-alen)
^
 arrayfuncs.c:4620:10: error: ‘ArrayBuildState’ has no member named ‘alen’
 astate-alen *= 2;


 Sorry,  correct patch attached.

 This patch is in patience format (git --patience ..). In previous
 patches, i use context format (git --patience ... | filterdiff
 --format=context), but it turns out that some modification is lost.


 last version is compileable, but some is still broken

 postgres=# select array_agg(array[i, i+1, i-1])
 from generate_series(1,2) a(i);
 ERROR:  could not find array type for data type integer[]

 but array(subselect) works

 postgres=# select array(select a from xx);
array
 ---
  {{1,2,3},{1,2,3}}
 (1 row)

 Regards

 Pavel




 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Alvaro Herrera

Michael Paquier wrote:

 That's not surprising, sometimes filterdiff misses the shot.

Really?  Wow, that's bad news.  I've been using it to submit patches
from time to time ...

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


-- 
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] Function array_agg(array)

2014-10-24 Thread Pavel Stehule

2014-10-24 13:58 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2014-10-24 11:43 GMT+02:00 Ali Akbar the.ap...@gmail.com:


 2014-10-24 16:26 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 some in last patch is wrong, I cannot to compile it:

 arrayfuncs.c: In function ‘accumArrayResult’:
 arrayfuncs.c:4603:9: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-alen = 64;  /* arbitrary starting array size */
  ^
 arrayfuncs.c:4604:9: error: ‘ArrayBuildState’ has no member named
 ‘dvalues’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
  ^
 arrayfuncs.c:4604:44: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
 ^
 arrayfuncs.c:4605:9: error: ‘ArrayBuildState’ has no member named
 ‘dnulls’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
  ^
 arrayfuncs.c:4605:42: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
   ^
 arrayfuncs.c:4606:9: error: ‘ArrayBuildState’ has no member named
 ‘nelems’
astate-nelems = 0;
  ^
 arrayfuncs.c:4618:13: error: ‘ArrayBuildState’ has no member named
 ‘nelems’
if (astate-nelems = astate-alen)
  ^
 arrayfuncs.c:4618:31: error: ‘ArrayBuildState’ has no member named ‘alen’
if (astate-nelems = astate-alen)
^
 arrayfuncs.c:4620:10: error: ‘ArrayBuildState’ has no member named ‘alen’
 astate-alen *= 2;


 Sorry,  correct patch attached.

 This patch is in patience format (git --patience ..). In previous
 patches, i use context format (git --patience ... | filterdiff
 --format=context), but it turns out that some modification is lost.


 last version is compileable, but some is still broken

 postgres=# select array_agg(array[i, i+1, i-1])
 from generate_series(1,2) a(i);
 ERROR:  could not find array type for data type integer[]


I am sorry, it works - I had a problem with broken database

I fixed small issue in regress tests and I enhanced tests for varlena types
and null values.

Regards

Pavel



 but array(subselect) works

 postgres=# select array(select a from xx);
array
 ---
  {{1,2,3},{1,2,3}}
 (1 row)

 Regards

 Pavel




 --
 Ali Akbar



diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index 7e5bcd9..f59738a
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** NULL baz/literallayout(3 rows)/entry
*** 12046,12051 
--- 12046,12067 
   row
entry
 indexterm
+ primaryarray_agg/primary
+/indexterm
+functionarray_agg(replaceable class=parameteranyarray/replaceable)/function
+   /entry
+   entry
+any
+   /entry
+   entry
+the same array type as input type
+   /entry
+   entryinput arrays, aggregated into higher-order multidimesional array. Rejects NULL and empty array as input./entry
+  /row
+ 
+  row
+   entry
+indexterm
  primaryaverage/primary
 /indexterm
 indexterm
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
new file mode 100644
index 2f0680f..8c182a4
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
*** SELECT ARRAY(SELECT oid FROM pg_proc WHE
*** 2238,2243 
--- 2238,2248 
   array
  ---
   {2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31,2412,2413}
+ 
+ SELECT ARRAY(SELECT array(select i) FROM generate_series(1,5) a(i));
+  array
+ ---
+  {{1},{2},{3},{4},{5}}
  (1 row)
  /programlisting
 The subquery must return a single column. The resulting
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
new file mode 100644
index 41e973b..0261fcb
*** a/src/backend/nodes/nodeFuncs.c
--- b/src/backend/nodes/nodeFuncs.c
*** exprType(const Node *expr)
*** 108,119 
  	type = exprType((Node *) tent-expr);
  	if (sublink-subLinkType == ARRAY_SUBLINK)
  	{
! 		type = get_array_type(type);
! 		if (!OidIsValid(type))
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_OBJECT),
! 	 errmsg(could not find array type for data type %s,
! 			format_type_be(exprType((Node *) tent-expr);
  	}
  }
  else if (sublink-subLinkType == MULTIEXPR_SUBLINK)
--- 108,123 
  	type = exprType((Node *) tent-expr);
  	if (sublink-subLinkType == ARRAY_SUBLINK)
  	{
! 		if (!OidIsValid(get_element_type(type)))
! 		{
! 			/* not array, so check for its array type */
! 			type = get_array_type(type);
! 			if (!OidIsValid(type))
! ereport(ERROR,
! 		(errcode(ERRCODE_UNDEFINED_OBJECT),
!

Re: [HACKERS] Function array_agg(array)

2014-10-24 Thread Pavel Stehule

Hi Ali

I checked a code. I am thinking so code organization is not good.
accumArrayResult is too long now. makeMdArrayResult will not work, when
arrays was joined (it is not consistent now). I don't like a usage of
state-is_array_accum in array_userfunc.c -- it is signal of wrong
wrapping.

next question: there is function array_append(anyarray, anyelement). Isn't
time to define array_append(anyarray, anyarray) now?

Regards

Pavel

2014-10-24 15:05 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2014-10-24 13:58 GMT+02:00 Pavel Stehule pavel.steh...@gmail.com:



 2014-10-24 11:43 GMT+02:00 Ali Akbar the.ap...@gmail.com:


 2014-10-24 16:26 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 Hi

 some in last patch is wrong, I cannot to compile it:

 arrayfuncs.c: In function ‘accumArrayResult’:
 arrayfuncs.c:4603:9: error: ‘ArrayBuildState’ has no member named ‘alen’
astate-alen = 64;  /* arbitrary starting array size */
  ^
 arrayfuncs.c:4604:9: error: ‘ArrayBuildState’ has no member named
 ‘dvalues’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
  ^
 arrayfuncs.c:4604:44: error: ‘ArrayBuildState’ has no member named
 ‘alen’
astate-dvalues = (Datum *) palloc(astate-alen * sizeof(Datum));
 ^
 arrayfuncs.c:4605:9: error: ‘ArrayBuildState’ has no member named
 ‘dnulls’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
  ^
 arrayfuncs.c:4605:42: error: ‘ArrayBuildState’ has no member named
 ‘alen’
astate-dnulls = (bool *) palloc(astate-alen * sizeof(bool));
   ^
 arrayfuncs.c:4606:9: error: ‘ArrayBuildState’ has no member named
 ‘nelems’
astate-nelems = 0;
  ^
 arrayfuncs.c:4618:13: error: ‘ArrayBuildState’ has no member named
 ‘nelems’
if (astate-nelems = astate-alen)
  ^
 arrayfuncs.c:4618:31: error: ‘ArrayBuildState’ has no member named
 ‘alen’
if (astate-nelems = astate-alen)
^
 arrayfuncs.c:4620:10: error: ‘ArrayBuildState’ has no member named
 ‘alen’
 astate-alen *= 2;


 Sorry,  correct patch attached.

 This patch is in patience format (git --patience ..). In previous
 patches, i use context format (git --patience ... | filterdiff
 --format=context), but it turns out that some modification is lost.


 last version is compileable, but some is still broken

 postgres=# select array_agg(array[i, i+1, i-1])
 from generate_series(1,2) a(i);
 ERROR:  could not find array type for data type integer[]


 I am sorry, it works - I had a problem with broken database

 I fixed small issue in regress tests and I enhanced tests for varlena
 types and null values.

 Regards

 Pavel



 but array(subselect) works

 postgres=# select array(select a from xx);
array
 ---
  {{1,2,3},{1,2,3}}
 (1 row)

 Regards

 Pavel




 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-22 Thread Pavel Stehule

Hi

2014-10-19 8:02 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 So, is there any idea how we will handle NULL and empty array in
 array_agg(anyarray)?
 I propose we just reject those input because the output will make no
 sense:
 - array_agg(NULL::int[]) -- the result will be indistinguished from
 array_agg of NULL ints.
 - array_agg('{}'::int[]) -- how we determine the dimension of the
 result? is it 0? Or the result will be just an empty array {} ?


 This updated patch rejects NULL and {} arrays as noted above.



I agree with your proposal. I have a few comments to design:

1. patch doesn't hold documentation and regress tests, please append it.

2. this functionality (multidimensional aggregation) can be interesting
more times, so maybe some interface like array builder should be preferred.

3. array_agg was consistent with array(subselect), so it should be fixed too

postgres=# select array_agg(a) from test;
   array_agg
---
 {{1,2,3,4},{1,2,3,4}}
(1 row)

postgres=# select array(select a from test);
ERROR:  could not find array type for data type integer[]

4. why you use a magic constant (64) there?

+ astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
+ astate-aitems = 64 * nitems;

+ astate-nullbitmap = (bits8 *)
+ repalloc(astate-nullbitmap, (astate-aitems + 7) / 8);

Regards

Pavel



 Regards,
 --
 Ali Akbar


 --
 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] Function array_agg(array)

2014-10-22 Thread Ali Akbar

Thanks for the review

2014-10-22 20:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 I agree with your proposal. I have a few comments to design:


 1. patch doesn't hold documentation and regress tests, please append it.

OK, i'll add the documentation and regression test


 2. this functionality (multidimensional aggregation) can be interesting
 more times, so maybe some interface like array builder should be preferred.

We already have accumArrayResult and makeArrayResult/makeMdArrayResult,
haven't we?

Actually array_agg(anyarray) can be implemented by using accumArrayResult
and makeMdArrayResult, but in the process we will deconstruct the array
data and null bit-flags into ArrayBuildState-dvalues and dnulls. And we
will reconstruct it through makeMdArrayResult. I think this way it's not
efficient, so i decided to reimplement it and memcpy the array data and
null flags as-is.

In other places, i think it's clearer if we just use accumArrayResult and
makeArrayResult/makeMdArrayResult as API for building (multidimensional)
arrays.


 3. array_agg was consistent with array(subselect), so it should be fixed
 too

 postgres=# select array_agg(a) from test;
array_agg
 ---
  {{1,2,3,4},{1,2,3,4}}
 (1 row)

 postgres=# select array(select a from test);
 ERROR:  could not find array type for data type integer[]


I'm pretty new in postgresql development. Can you point out where is
array(subselect) implemented?



 4. why you use a magic constant (64) there?

 + astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
 + astate-aitems = 64 * nitems;

 + astate-nullbitmap = (bits8 *)
 + repalloc(astate-nullbitmap, (astate-aitems + 7) / 8);


just follow the arbitrary size choosen in accumArrayResult (arrayfuncs.c):
astate-alen = 64; /* arbitrary starting array size */

it can be any number not too small and too big. Too small, and we will
realloc shortly. Too big, we will end up wasting memory.

Regards,
-- 
Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-22 Thread Pavel Stehule

2014-10-22 16:58 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 Thanks for the review

 2014-10-22 20:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 I agree with your proposal. I have a few comments to design:


 1. patch doesn't hold documentation and regress tests, please append it.

 OK, i'll add the documentation and regression test


 2. this functionality (multidimensional aggregation) can be interesting
 more times, so maybe some interface like array builder should be preferred.

 We already have accumArrayResult and makeArrayResult/makeMdArrayResult,
 haven't we?

 Actually array_agg(anyarray) can be implemented by using accumArrayResult
 and makeMdArrayResult, but in the process we will deconstruct the array
 data and null bit-flags into ArrayBuildState-dvalues and dnulls. And we
 will reconstruct it through makeMdArrayResult. I think this way it's not
 efficient, so i decided to reimplement it and memcpy the array data and
 null flags as-is.


aha, so isn't better to fix a performance for accumArrayResult ?



 In other places, i think it's clearer if we just use accumArrayResult and
 makeArrayResult/makeMdArrayResult as API for building (multidimensional)
 arrays.


 3. array_agg was consistent with array(subselect), so it should be fixed
 too

 postgres=# select array_agg(a) from test;
array_agg
 ---
  {{1,2,3,4},{1,2,3,4}}
 (1 row)

 postgres=# select array(select a from test);
 ERROR:  could not find array type for data type integer[]


 I'm pretty new in postgresql development. Can you point out where is
 array(subselect) implemented?


where you can start?

postgres=# explain select array(select a from test);
ERROR:  42704: could not find array type for data type integer[]
LOCATION:  exprType, nodeFuncs.c:116

look to code ... your magic keyword is a ARRAY_SUBLINK .. search in
postgresql sources this keyword





 4. why you use a magic constant (64) there?

 + astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
 + astate-aitems = 64 * nitems;

 + astate-nullbitmap = (bits8 *)
 + repalloc(astate-nullbitmap, (astate-aitems + 7) / 8);


 just follow the arbitrary size choosen in accumArrayResult (arrayfuncs.c):
 astate-alen = 64; /* arbitrary starting array size */

 it can be any number not too small and too big. Too small, and we will
 realloc shortly. Too big, we will end up wasting memory.


you can try to alloc 1KB instead as start -- it is used more times in
Postgres. Then a overhead is max 1KB per agg call - what is acceptable.

You take this value from accumArrayResult, but it is targeted for shorted
scalars - you should to expect so any array will be much larger.





 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-22 Thread Ali Akbar

2014-10-22 22:48 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 2014-10-22 16:58 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 Thanks for the review

 2014-10-22 20:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 I agree with your proposal. I have a few comments to design:


 1. patch doesn't hold documentation and regress tests, please append it.

 OK, i'll add the documentation and regression test


 2. this functionality (multidimensional aggregation) can be interesting
 more times, so maybe some interface like array builder should be preferred.

 We already have accumArrayResult and makeArrayResult/makeMdArrayResult,
 haven't we?

 Actually array_agg(anyarray) can be implemented by using accumArrayResult
 and makeMdArrayResult, but in the process we will deconstruct the array
 data and null bit-flags into ArrayBuildState-dvalues and dnulls. And we
 will reconstruct it through makeMdArrayResult. I think this way it's not
 efficient, so i decided to reimplement it and memcpy the array data and
 null flags as-is.


 aha, so isn't better to fix a performance for accumArrayResult ?


Ok, i'll go this route. While reading the code of array(subselect) as you
pointed below, i think the easiest way to implement support for both
array_agg(anyarray) and array(subselect) is to change accumArrayResult so
it operates both with scalar datum(s) and array datums, with performance
optimization for the latter.

In other places, i think it's clearer if we just use accumArrayResult and
 makeArrayResult/makeMdArrayResult as API for building (multidimensional)
 arrays.


 3. array_agg was consistent with array(subselect), so it should be fixed
 too

 postgres=# select array_agg(a) from test;
array_agg
 ---
  {{1,2,3,4},{1,2,3,4}}
 (1 row)

 postgres=# select array(select a from test);
 ERROR:  could not find array type for data type integer[]


 I'm pretty new in postgresql development. Can you point out where is
 array(subselect) implemented?


 where you can start?

 postgres=# explain select array(select a from test);
 ERROR:  42704: could not find array type for data type integer[]
 LOCATION:  exprType, nodeFuncs.c:116

 look to code ... your magic keyword is a ARRAY_SUBLINK .. search in
 postgresql sources this keyword


Found it. Thanks.

On a side note in postgres array type for integer[] is still integer[], but
in pg_type, integer[] has no array type. I propose we consider to change it
so array type of anyarray is itself (not in this patch, of course, because
it is a big change). Consider the following code in nodeFuncs.c:109

if (sublink-subLinkType == ARRAY_SUBLINK)
{
type = get_array_type(type);
if (!OidIsValid(type))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
 errmsg(could not find array type for data type %s,
format_type_be(exprType((Node *) tent-expr);
}

to implement array(subselect) you pointed above, we must specially checks
for array types. Quick search on get_array_type returns 10 places.

I'll think more about this later. For this patch, i'll go without changes
in pg_type.h.

4. why you use a magic constant (64) there?

 + astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
 + astate-aitems = 64 * nitems;

 + astate-nullbitmap = (bits8 *)
 + repalloc(astate-nullbitmap, (astate-aitems + 7) / 8);


 just follow the arbitrary size choosen in accumArrayResult (arrayfuncs.c):
 astate-alen = 64; /* arbitrary starting array size */

 it can be any number not too small and too big. Too small, and we will
 realloc shortly. Too big, we will end up wasting memory.


 you can try to alloc 1KB instead as start -- it is used more times in
 Postgres. Then a overhead is max 1KB per agg call - what is acceptable.

 You take this value from accumArrayResult, but it is targeted for shorted
 scalars - you should to expect so any array will be much larger.


Ok.


Regards,
--
Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-22 Thread Pavel Stehule

2014-10-23 4:00 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 2014-10-22 22:48 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 2014-10-22 16:58 GMT+02:00 Ali Akbar the.ap...@gmail.com:

 Thanks for the review

 2014-10-22 20:51 GMT+07:00 Pavel Stehule pavel.steh...@gmail.com:

 I agree with your proposal. I have a few comments to design:


 1. patch doesn't hold documentation and regress tests, please append it.

 OK, i'll add the documentation and regression test


 2. this functionality (multidimensional aggregation) can be interesting
 more times, so maybe some interface like array builder should be preferred.

 We already have accumArrayResult and makeArrayResult/makeMdArrayResult,
 haven't we?

 Actually array_agg(anyarray) can be implemented by using
 accumArrayResult and makeMdArrayResult, but in the process we will
 deconstruct the array data and null bit-flags into ArrayBuildState-dvalues
 and dnulls. And we will reconstruct it through makeMdArrayResult. I think
 this way it's not efficient, so i decided to reimplement it and memcpy the
 array data and null flags as-is.


 aha, so isn't better to fix a performance for accumArrayResult ?


 Ok, i'll go this route. While reading the code of array(subselect) as you
 pointed below, i think the easiest way to implement support for both
 array_agg(anyarray) and array(subselect) is to change accumArrayResult so
 it operates both with scalar datum(s) and array datums, with performance
 optimization for the latter.

 In other places, i think it's clearer if we just use accumArrayResult and
 makeArrayResult/makeMdArrayResult as API for building (multidimensional)
 arrays.


 3. array_agg was consistent with array(subselect), so it should be
 fixed too

 postgres=# select array_agg(a) from test;
array_agg
 ---
  {{1,2,3,4},{1,2,3,4}}
 (1 row)

 postgres=# select array(select a from test);
 ERROR:  could not find array type for data type integer[]


 I'm pretty new in postgresql development. Can you point out where is
 array(subselect) implemented?


 where you can start?

 postgres=# explain select array(select a from test);
 ERROR:  42704: could not find array type for data type integer[]
 LOCATION:  exprType, nodeFuncs.c:116

 look to code ... your magic keyword is a ARRAY_SUBLINK .. search in
 postgresql sources this keyword


 Found it. Thanks.

 On a side note in postgres array type for integer[] is still integer[],
 but in pg_type, integer[] has no array type. I propose we consider to
 change it so array type of anyarray is itself (not in this patch, of
 course, because it is a big change). Consider the following code in
 nodeFuncs.c:109


yes, it is true - this is really big change and maybe needs separate
discuss - ***if we allow cycle there. I am not sure about possible side
effects***.

Maybe this change is not necessary, you can fix a check only ... if type
is not array or if get_array_type is null raise a error



 if (sublink-subLinkType == ARRAY_SUBLINK)
 {
 type = get_array_type(type);
 if (!OidIsValid(type))
 ereport(ERROR,
 (errcode(ERRCODE_UNDEFINED_OBJECT),
  errmsg(could not find array type for data type %s,
 format_type_be(exprType((Node *) tent-expr);
 }

 to implement array(subselect) you pointed above, we must specially checks
 for array types. Quick search on get_array_type returns 10 places.


attention: probably we don't would to allow arrays everywhere.



 I'll think more about this later. For this patch, i'll go without changes
 in pg_type.h.

 4. why you use a magic constant (64) there?

 + astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
 + astate-aitems = 64 * nitems;

 + astate-nullbitmap = (bits8 *)
 + repalloc(astate-nullbitmap, (astate-aitems + 7) /
 8);


 just follow the arbitrary size choosen in accumArrayResult
 (arrayfuncs.c):
 astate-alen = 64; /* arbitrary starting array size */

 it can be any number not too small and too big. Too small, and we will
 realloc shortly. Too big, we will end up wasting memory.


 you can try to alloc 1KB instead as start -- it is used more times in
 Postgres. Then a overhead is max 1KB per agg call - what is acceptable.

 You take this value from accumArrayResult, but it is targeted for shorted
 scalars - you should to expect so any array will be much larger.


 Ok.


 Regards,
 --
 Ali Akbar

Re: [HACKERS] Function array_agg(array)

2014-10-19 Thread Ali Akbar


 So, is there any idea how we will handle NULL and empty array in
 array_agg(anyarray)?
 I propose we just reject those input because the output will make no sense:
 - array_agg(NULL::int[]) -- the result will be indistinguished from
 array_agg of NULL ints.
 - array_agg('{}'::int[]) -- how we determine the dimension of the result?
 is it 0? Or the result will be just an empty array {} ?


This updated patch rejects NULL and {} arrays as noted above.

Regards,
-- 
Ali Akbar
*** a/src/backend/utils/adt/array_userfuncs.c
--- b/src/backend/utils/adt/array_userfuncs.c
***
*** 16,21 
--- 16,51 
  #include utils/builtins.h
  #include utils/lsyscache.h
  
+ #include utils/memutils.h
+ 
+ /*-
+  * ArrayAggAnyArrayState:
+  *		aggregate state for array_agg(anyarray)
+  *-
+  */
+ typedef struct
+ {
+ 	MemoryContext mcontext;		/* where all the temp stuff is kept */
+ 	char	   *data;			/* array of accumulated data */
+ 	bits8	   *nullbitmap;		/* bitmap of is-null flags for data */
+ 
+ 	int			abytes;			/* allocated length of above arrays */
+ 	int			aitems;			/* allocated length of above arrays */
+ 	int			nbytes;			/* number of used bytes in above arrays */
+ 	int			nitems;			/* number of elements in above arrays */
+ 	int			narray;			/* number of array accumulated */
+ 	Oid			element_type;	/* data type of the Datums */
+ 	int16		typlen;			/* needed info about datatype */
+ 	bool		typbyval;
+ 	char		typalign;
+ 
+ 	int			ndims;			/* element dimensions */
+ 	int		   *dims;
+ 	int		   *lbs;
+ 
+ 	bool		hasnull;		/* any element has null */
+ } ArrayAggAnyArrayState;
+ 
  
  /*-
   * array_push :
***
*** 544,546  array_agg_finalfn(PG_FUNCTION_ARGS)
--- 574,821 
  
  	PG_RETURN_DATUM(result);
  }
+ 
+ /*
+  * ARRAY_AGG(anyarray) aggregate function
+  */
+ Datum
+ array_agg_anyarray_transfn(PG_FUNCTION_ARGS)
+ {
+ 	MemoryContext aggcontext,
+ arr_context,
+ oldcontext;
+ 	ArrayAggAnyArrayState *astate;
+ 
+ 	Oid			arg_typeid = get_fn_expr_argtype(fcinfo-flinfo, 1);
+ 	Oid			arg_elemtype = get_element_type(arg_typeid);
+ 	ArrayType  *arg;
+ 	int		   *dims,
+ 			   *lbs,
+ ndims,
+ nitems,
+ ndatabytes;
+ 	char	   *data;
+ 
+ 	int			i;
+ 
+ 	if (arg_elemtype == InvalidOid)
+ 		ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(could not determine input data type)));
+ 
+ 	if (!AggCheckCallContext(fcinfo, aggcontext))
+ 		elog(ERROR, array_agg_anyarray_transfn called in non-aggregate context);
+ 
+ 	if (PG_ARGISNULL(1))
+ 		ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(cannot aggregate null arrays)));
+ 
+ 	astate = PG_ARGISNULL(0) ? NULL : (ArrayAggAnyArrayState *) PG_GETARG_POINTER(0);
+ 
+ 	if (astate == NULL)
+ 	{
+ 		arr_context = AllocSetContextCreate(aggcontext,
+ 			array_agg_anyarray_transfn,
+ 			ALLOCSET_DEFAULT_MINSIZE,
+ 			ALLOCSET_DEFAULT_INITSIZE,
+ 			ALLOCSET_DEFAULT_MAXSIZE);
+ 		oldcontext = MemoryContextSwitchTo(arr_context);
+ 		astate = (ArrayAggAnyArrayState *) palloc(sizeof(ArrayAggAnyArrayState));
+ 
+ 		astate-mcontext = arr_context;
+ 		astate-abytes = 0;
+ 		astate-aitems = 0;
+ 		astate-data = NULL;
+ 		astate-nullbitmap = NULL;
+ 		astate-nitems = 0;
+ 		astate-narray = 0;
+ 		astate-element_type = arg_elemtype;
+ 		get_typlenbyvalalign(arg_elemtype,
+ 			 astate-typlen,
+ 			 astate-typbyval,
+ 			 astate-typalign);
+ 	}
+ 	else
+ 	{
+ 		oldcontext = MemoryContextSwitchTo(astate-mcontext);
+ 		Assert(astate-element_type == arg_elemtype);
+ 	}
+ 
+ 	arg = PG_GETARG_ARRAYTYPE_P(1);
+ 
+ 	ndims = ARR_NDIM(arg);
+ 	dims = ARR_DIMS(arg);
+ 	lbs = ARR_LBOUND(arg);
+ 	data = ARR_DATA_PTR(arg);
+ 	nitems = ArrayGetNItems(ndims, dims);
+ 
+ 	ndatabytes = ARR_SIZE(arg) - ARR_DATA_OFFSET(arg);
+ 
+ 	if (astate-data == NULL)
+ 	{
+ 		if (ndims == 0)
+ 			ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(cannot aggregate empty arrays)));
+ 
+ 		if (ndims + 1  MAXDIM)
+ 			ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+  errmsg(number of array dimensions (%d) exceeds the maximum allowed (%d),
+ 		ndims + 1, MAXDIM)));
+ 
+ 		astate-ndims = ndims;
+ 		astate-dims = (int *) palloc(ndims * sizeof(int));
+ 		astate-lbs = (int *) palloc(ndims * sizeof(int));
+ 		memcpy(astate-dims, dims, ndims * sizeof(int));
+ 		memcpy(astate-lbs, lbs, ndims * sizeof(int));
+ 
+ 		astate-abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
+ 		astate-aitems = 64 * nitems;
+ 		astate-data = (char *) palloc(astate-abytes);
+ 
+ 		memcpy(astate-data, data, ndatabytes);
+ 		astate-nbytes = ndatabytes;
+ 		astate-nitems = nitems;
+ 
+ 		if (ARR_HASNULL(arg))
+ 		{
+ 			astate-hasnull = true;
+

Re: [HACKERS] Function array_agg(array)

2014-10-12 Thread Ali Akbar

2014-10-11 22:28 GMT+07:00 Tom Lane t...@sss.pgh.pa.us:

 Seems dangerous as heck; certainly it would have side-effects far more
 wide-ranging than just making this particular function work.

 A safer answer is to split array_agg into two functions,
 array_agg(anynonarray) - anyarray
 array_agg(anyarray) - anyarray

 I rather imagine you should do that anyway, because I really doubt
 that this hack is operating quite as intended.  I suspect you are
 producing arrays containing arrays as elements, not true 2-D arrays.
 That's not a direction we want to go in I think; certainly there are
 no other operations that produce such things.


Thanks for the review. Yes, it looks like the patch produced array as the
elements. So, all array operations behaves wierdly.

In this quick  dirty patch, I am trying to implement the
array_agg(anyarray), introducing two new functions:
- array_agg_anyarray_transfn
- array_agg_anyarray_finalfn

At first, i want to use accumArrayResult and makeMdArrayResult, but it's
complicated to work with multi-dimensional arrays with those two functions.
So i combined array_cat with those function.

Currently, it cannot handle NULL arrays:
backend select array_agg(a) from (values(null::int[])) a(a);
 1: array_agg(typeid = 1007, len = -1, typmod = -1, byval = f)

ERROR:  cannot aggregate null arrays

Regards,
-- 
Ali Akbar
*** a/src/backend/utils/adt/array_userfuncs.c
--- b/src/backend/utils/adt/array_userfuncs.c
***
*** 16,21 
--- 16,51 
  #include utils/builtins.h
  #include utils/lsyscache.h
  
+ #include utils/memutils.h
+ 
+ /*-
+  * ArrayAggAnyArrayState:
+  *		aggregate state for array_agg(anyarray)
+  *-
+  */
+ typedef struct
+ {
+ 	MemoryContext mcontext;		/* where all the temp stuff is kept */
+ 	char	   *data;			/* array of accumulated data */
+ 	bits8	   *nullbitmap;		/* bitmap of is-null flags for data */
+ 
+ 	int			abytes;			/* allocated length of above arrays */
+ 	int			aitems;			/* allocated length of above arrays */
+ 	int			nbytes;			/* number of used bytes in above arrays */
+ 	int			nitems;			/* number of elements in above arrays */
+ 	int			narray;			/* number of array accumulated */
+ 	Oid			element_type;	/* data type of the Datums */
+ 	int16		typlen;			/* needed info about datatype */
+ 	bool		typbyval;
+ 	char		typalign;
+ 
+ 	int			ndims;			/* element dimensions */
+ 	int		   *dims;
+ 	int		   *lbs;
+ 
+ 	bool		hasnull;		/* any element has null */
+ } ArrayAggAnyArrayState;
+ 
  
  /*-
   * array_push :
***
*** 544,546  array_agg_finalfn(PG_FUNCTION_ARGS)
--- 574,814 
  
  	PG_RETURN_DATUM(result);
  }
+ 
+ /*
+  * ARRAY_AGG(anyarray) aggregate function
+  */
+ Datum
+ array_agg_anyarray_transfn(PG_FUNCTION_ARGS)
+ {
+ 	MemoryContext aggcontext,
+ arr_context,
+ oldcontext;
+ 	ArrayAggAnyArrayState *astate;
+ 
+ 	Oid			arg_typeid = get_fn_expr_argtype(fcinfo-flinfo, 1);
+ 	Oid			arg_elemtype = get_element_type(arg_typeid);
+ 	ArrayType  *arg;
+ 	int		   *dims,
+ 			   *lbs,
+ ndims,
+ nitems,
+ ndatabytes;
+ 	char	   *data;
+ 
+ 	int			i;
+ 
+ 	if (arg_elemtype == InvalidOid)
+ 		ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+  errmsg(could not determine input data type)));
+ 
+ 	if (!AggCheckCallContext(fcinfo, aggcontext))
+ 		elog(ERROR, array_agg_anyarray_transfn called in non-aggregate context);
+ 
+ 	if (PG_ARGISNULL(1))
+ 		elog(ERROR, cannot aggregate null arrays);
+ 
+ 	astate = PG_ARGISNULL(0) ? NULL : (ArrayAggAnyArrayState *) PG_GETARG_POINTER(0);
+ 
+ 	if (astate == NULL)
+ 	{
+ 		arr_context = AllocSetContextCreate(aggcontext,
+ 			array_agg_anyarray_transfn,
+ 			ALLOCSET_DEFAULT_MINSIZE,
+ 			ALLOCSET_DEFAULT_INITSIZE,
+ 			ALLOCSET_DEFAULT_MAXSIZE);
+ 		oldcontext = MemoryContextSwitchTo(arr_context);
+ 		astate = (ArrayAggAnyArrayState *) palloc(sizeof(ArrayAggAnyArrayState));
+ 
+ 		astate-mcontext = arr_context;
+ 		astate-abytes = 0;
+ 		astate-aitems = 0;
+ 		astate-data = NULL;
+ 		astate-nullbitmap = NULL;
+ 		astate-nitems = 0;
+ 		astate-narray = 0;
+ 		astate-element_type = arg_elemtype;
+ 		get_typlenbyvalalign(arg_elemtype,
+ 			 astate-typlen,
+ 			 astate-typbyval,
+ 			 astate-typalign);
+ 	}
+ 	else
+ 	{
+ 		oldcontext = MemoryContextSwitchTo(astate-mcontext);
+ 		Assert(astate-element_type == arg_elemtype);
+ 	}
+ 
+ 	arg = PG_GETARG_ARRAYTYPE_P(1);
+ 
+ 	ndims = ARR_NDIM(arg);
+ 	dims = ARR_DIMS(arg);
+ 	lbs = ARR_LBOUND(arg);
+ 	data = ARR_DATA_PTR(arg);
+ 	nitems = ArrayGetNItems(ndims, dims);
+ 
+ 	ndatabytes = ARR_SIZE(arg) - ARR_DATA_OFFSET(arg);
+ 
+ 	if (astate-data == NULL)
+ 	{
+ 		if (ndims + 1  MAXDIM)
+ 			ereport(ERROR,
+

Re: [HACKERS] Function array_agg(array)

2014-10-11 Thread Tom Lane

Ali Akbar the.ap...@gmail.com writes:
 So, can't we just set the typarray of array types to its self oid?

Seems dangerous as heck; certainly it would have side-effects far more
wide-ranging than just making this particular function work.

A safer answer is to split array_agg into two functions,
array_agg(anynonarray) - anyarray
array_agg(anyarray) - anyarray

I rather imagine you should do that anyway, because I really doubt
that this hack is operating quite as intended.  I suspect you are
producing arrays containing arrays as elements, not true 2-D arrays.
That's not a direction we want to go in I think; certainly there are
no other operations that produce such things.

regards, tom lane


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

37 matches

Mail list logo