pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedRemoved extra memory allocations from create_list_bounds
20+ messages / 5 participants
[nested] [flat]
* Removed extra memory allocations from create_list_bounds
@ 2021-05-15 09:10 Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-15 09:10 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
While working on [1], I observed that extra memory is allocated in
'create_list_bounds'
function which can be avoided. So the attached patch removes extra memory
allocations done inside 'create_list_bounds' function and also removes the
unused variable 'cell'.
In the existing code, in create_list_bounds(),
1. It iterates through all the partitions and for each partition,
- It iterates through the list of datums named 'listdatums'.
- For each non null value of 'listdatums', it allocates a memory
for 'list_value' whose type is 'PartitionListValue' and
stores value and
index information.
- Appends 'list_value' to a list named 'non_null_values'.
2. Allocates memory to 'all_values' variable which contains
information of all the list bounds of all the partitions. The count
allocated for 'all_values' is nothing but the total number of non null
values which is populated from the previous step (1).
3. Iterates through each item of 'non_null_values' list.
- It allocates a memory for 'all_values[i]' whose type is
'PartitionListValue' and copies the information from 'list_value'.
The above logic is changed to following,
1. Call function 'get_non_null_count_list_bounds()' which iterates
through all the partitions and for each partition, it iterates through a
list of datums and calculates the count of all non null bound values.
2. Allocates memory to 'all_values' variable which contains information
of all the list bounds of all the partitions. The count allocated for
'all_values' is nothing but the total number of non null values which is
populated from the previous step (1).
3. Iterates through all the partitions and for each partition,
- It iterates through the list of datums named 'listdatums'.
- For each non null value of 'listdatums', it allocates a memory
for 'all_values[i]' whose type is 'PartitionListValue' and stores
value and index information directly.
The above fix, removes the extra memory allocations. Let's consider an
example.
If there are 10 partitions and each partition contains 11 bounds including
NULL value.
Parameters Existing code With patch
Memory allocation of 'PartitionListValue' 100+100 = 200 times 100 times
Total number of iterations 110 + 100 = 210 110 + 110 = 220
As we can see in the above data, the total number of iterations are
increased slightly
(When it contains NULL values. Otherwise no change) but it improves in case
of
memory allocations. As memory allocations are costly operations, I feel we
should
consider changing the existing code.
Please share your thoughts.
[1] -
https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compos...
Thanks & Regards,
Nitin Jadhav
Attachments:
v1_remove_extra_mem_alloc_from_list_bounds.patch (application/octet-stream, 3.1K) download
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index d3dedfd..b0854e2 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -433,6 +433,32 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
/*
+ * get_non_null_count_list_bounds
+ * Calculates the total number of non null bound values of
+ * all the partitions.
+ */
+static int
+get_non_null_count_list_bounds(PartitionBoundSpec **boundspecs, int nparts)
+{
+ int i = 0;
+ int count = 0;
+
+ for (i = 0; i < nparts; i++)
+ {
+ ListCell *c;
+
+ foreach(c, boundspecs[i]->listdatums)
+ {
+ Const *val = castNode(Const, lfirst(c));
+
+ if (!val->constisnull)
+ count++;
+ }
+ }
+
+ return count;
+}
+/*
* create_list_bounds
* Create a PartitionBoundInfo for a list partitioned table
*/
@@ -442,13 +468,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
{
PartitionBoundInfo boundinfo;
PartitionListValue **all_values = NULL;
- ListCell *cell;
int i = 0;
+ int j = 0;
int ndatums = 0;
int next_index = 0;
int default_index = -1;
int null_index = -1;
- List *non_null_values = NIL;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -457,6 +482,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->null_index = -1;
boundinfo->default_index = -1;
+ ndatums = get_non_null_count_list_bounds(boundspecs, nparts);
+ all_values = (PartitionListValue **)
+ palloc(ndatums * sizeof(PartitionListValue *));
+
/* Create a unified list of non-null values across all partitions. */
for (i = 0; i < nparts; i++)
{
@@ -480,14 +509,14 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
foreach(c, spec->listdatums)
{
Const *val = castNode(Const, lfirst(c));
- PartitionListValue *list_value = NULL;
if (!val->constisnull)
{
- list_value = (PartitionListValue *)
+ all_values[j] = (PartitionListValue *)
palloc0(sizeof(PartitionListValue));
- list_value->index = i;
- list_value->value = val->constvalue;
+ all_values[j]->index = i;
+ all_values[j]->value = val->constvalue;
+ j++;
}
else
{
@@ -499,32 +528,9 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
elog(ERROR, "found null more than once");
null_index = i;
}
-
- if (list_value)
- non_null_values = lappend(non_null_values, list_value);
}
}
- ndatums = list_length(non_null_values);
-
- /*
- * Collect all list values in one array. Alongside the value, we also save
- * the index of partition the value comes from.
- */
- all_values = (PartitionListValue **)
- palloc(ndatums * sizeof(PartitionListValue *));
- i = 0;
- foreach(cell, non_null_values)
- {
- PartitionListValue *src = lfirst(cell);
-
- all_values[i] = (PartitionListValue *)
- palloc(sizeof(PartitionListValue));
- all_values[i]->value = src->value;
- all_values[i]->index = src->index;
- i++;
- }
-
qsort_arg(all_values, ndatums, sizeof(PartitionListValue *),
qsort_partition_list_value_cmp, (void *) key);
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-16 17:00 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
0 siblings, 1 reply; 20+ messages in thread
From: Justin Pryzby @ 2021-05-16 17:00 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On Sat, May 15, 2021 at 02:40:45PM +0530, Nitin Jadhav wrote:
> While working on [1], I observed that extra memory is allocated in
> [1] https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compos...
This is a link to your gmail, not to anything public.
If it's worth counting list elements in advance, then you can also allocate the
PartitionListValue as a single chunk, rather than palloc in a loop.
This may help large partition heirarchies.
And the same thing in create_hash_bounds with hbounds.
create_range_bounds() already doesn't call palloc in a loop. However, then
there's an asymmetry in create_range_bounds, which is still takes a
double-indirect pointer.
I'm not able to detect that this is saving more than about ~1% less RAM, to
create or select from 1000 partitions, probably because other data structures
are using much more, and savings here are relatively small.
I'm going to add to the next CF. You can add yourself as an author, and watch
that the patch passes tests in cfbot.
https://commitfest.postgresql.org/
http://cfbot.cputube.org/
Thanks,
--
Justin
Attachments: download all (5 files, .tar.gz)
0001-Removed-extra-memory-allocations-from-create_list_bo.patch (text/x-diff, 3.5K) download
From eccac01b63c983ba97859a4893e4115de19fda95 Mon Sep 17 00:00:00 2001
From: Nitin Jadhav <nitinjadhavpostgres@gmail.com>
Date: Sat, 15 May 2021 14:40:45 +0530
Subject: [PATCH 1/5] Removed extra memory allocations from create_list_bounds
---
src/backend/partitioning/partbounds.c | 64 +++++++++++++++------------
1 file changed, 35 insertions(+), 29 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7925fcce3b..d0c263d435 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -432,6 +432,32 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
return boundinfo;
}
+/*
+ * get_non_null_count_list_bounds
+ * Calculates the total number of non null bound values of
+ * all the partitions.
+ */
+static int
+get_non_null_count_list_bounds(PartitionBoundSpec **boundspecs, int nparts)
+{
+ int i = 0;
+ int count = 0;
+
+ for (i = 0; i < nparts; i++)
+ {
+ ListCell *c;
+
+ foreach(c, boundspecs[i]->listdatums)
+ {
+ Const *val = castNode(Const, lfirst(c));
+
+ if (!val->constisnull)
+ count++;
+ }
+ }
+
+ return count;
+}
/*
* create_list_bounds
* Create a PartitionBoundInfo for a list partitioned table
@@ -442,13 +468,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
{
PartitionBoundInfo boundinfo;
PartitionListValue **all_values = NULL;
- ListCell *cell;
int i = 0;
+ int j = 0;
int ndatums = 0;
int next_index = 0;
int default_index = -1;
int null_index = -1;
- List *non_null_values = NIL;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -457,6 +482,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->null_index = -1;
boundinfo->default_index = -1;
+ ndatums = get_non_null_count_list_bounds(boundspecs, nparts);
+ all_values = (PartitionListValue **)
+ palloc(ndatums * sizeof(PartitionListValue *));
+
/* Create a unified list of non-null values across all partitions. */
for (i = 0; i < nparts; i++)
{
@@ -480,14 +509,14 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
foreach(c, spec->listdatums)
{
Const *val = castNode(Const, lfirst(c));
- PartitionListValue *list_value = NULL;
if (!val->constisnull)
{
- list_value = (PartitionListValue *)
+ all_values[j] = (PartitionListValue *)
palloc0(sizeof(PartitionListValue));
- list_value->index = i;
- list_value->value = val->constvalue;
+ all_values[j]->index = i;
+ all_values[j]->value = val->constvalue;
+ j++;
}
else
{
@@ -499,32 +528,9 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
elog(ERROR, "found null more than once");
null_index = i;
}
-
- if (list_value)
- non_null_values = lappend(non_null_values, list_value);
}
}
- ndatums = list_length(non_null_values);
-
- /*
- * Collect all list values in one array. Alongside the value, we also save
- * the index of partition the value comes from.
- */
- all_values = (PartitionListValue **)
- palloc(ndatums * sizeof(PartitionListValue *));
- i = 0;
- foreach(cell, non_null_values)
- {
- PartitionListValue *src = lfirst(cell);
-
- all_values[i] = (PartitionListValue *)
- palloc(sizeof(PartitionListValue));
- all_values[i]->value = src->value;
- all_values[i]->index = src->index;
- i++;
- }
-
qsort_arg(all_values, ndatums, sizeof(PartitionListValue *),
qsort_partition_list_value_cmp, (void *) key);
--
2.17.0
0002-allocate-the-PartitionListValue-as-a-single-chunk.patch (text/x-diff, 2.9K) download
From 0aeec1e5c9ddf2dea0045ed3ddbf0f7ae09d06a6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 15 May 2021 15:57:12 -0500
Subject: [PATCH 2/5] allocate the PartitionListValue as a single chunk
---
src/backend/partitioning/partbounds.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index d0c263d435..c57156d150 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -467,7 +467,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
PartitionKey key, int **mapping)
{
PartitionBoundInfo boundinfo;
- PartitionListValue **all_values = NULL;
+ PartitionListValue *all_values;
int i = 0;
int j = 0;
int ndatums = 0;
@@ -483,8 +483,8 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->default_index = -1;
ndatums = get_non_null_count_list_bounds(boundspecs, nparts);
- all_values = (PartitionListValue **)
- palloc(ndatums * sizeof(PartitionListValue *));
+ all_values = (PartitionListValue *)
+ palloc(ndatums * sizeof(PartitionListValue));
/* Create a unified list of non-null values across all partitions. */
for (i = 0; i < nparts; i++)
@@ -512,10 +512,8 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
if (!val->constisnull)
{
- all_values[j] = (PartitionListValue *)
- palloc0(sizeof(PartitionListValue));
- all_values[j]->index = i;
- all_values[j]->value = val->constvalue;
+ all_values[j].index = i;
+ all_values[j].value = val->constvalue;
j++;
}
else
@@ -531,7 +529,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
}
- qsort_arg(all_values, ndatums, sizeof(PartitionListValue *),
+ qsort_arg(all_values, ndatums, sizeof(PartitionListValue),
qsort_partition_list_value_cmp, (void *) key);
boundinfo->ndatums = ndatums;
@@ -547,10 +545,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
*/
for (i = 0; i < ndatums; i++)
{
- int orig_index = all_values[i]->index;
+ int orig_index = all_values[i].index;
boundinfo->datums[i] = (Datum *) palloc(sizeof(Datum));
- boundinfo->datums[i][0] = datumCopy(all_values[i]->value,
+ boundinfo->datums[i][0] = datumCopy(all_values[i].value,
key->parttypbyval[0],
key->parttyplen[0]);
@@ -3693,8 +3691,8 @@ qsort_partition_hbound_cmp(const void *a, const void *b)
static int32
qsort_partition_list_value_cmp(const void *a, const void *b, void *arg)
{
- Datum val1 = (*(PartitionListValue *const *) a)->value,
- val2 = (*(PartitionListValue *const *) b)->value;
+ Datum val1 = ((PartitionListValue *const) a)->value,
+ val2 = ((PartitionListValue *const) b)->value;
PartitionKey key = (PartitionKey) arg;
return DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[0],
--
2.17.0
0003-Same-for-create_hash_bounds-PartitionHashBound.patch (text/x-diff, 3.3K) download
From 883cf386966d88b5dae9c91a165481189dbd1dde Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 16 May 2021 10:36:58 -0500
Subject: [PATCH 3/5] Same for create_hash_bounds PartitionHashBound
---
src/backend/partitioning/partbounds.c | 28 +++++++++++++--------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index c57156d150..490fe89eb8 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -358,7 +358,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
PartitionKey key, int **mapping)
{
PartitionBoundInfo boundinfo;
- PartitionHashBound **hbounds = NULL;
+ PartitionHashBound *hbounds;
int i;
int ndatums = 0;
int greatest_modulus;
@@ -371,8 +371,8 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->default_index = -1;
ndatums = nparts;
- hbounds = (PartitionHashBound **)
- palloc(nparts * sizeof(PartitionHashBound *));
+ hbounds = (PartitionHashBound *)
+ palloc(nparts * sizeof(PartitionHashBound));
/* Convert from node to the internal representation */
for (i = 0; i < nparts; i++)
@@ -382,18 +382,17 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
if (spec->strategy != PARTITION_STRATEGY_HASH)
elog(ERROR, "invalid strategy in partition bound spec");
- hbounds[i] = (PartitionHashBound *) palloc(sizeof(PartitionHashBound));
- hbounds[i]->modulus = spec->modulus;
- hbounds[i]->remainder = spec->remainder;
- hbounds[i]->index = i;
+ hbounds[i].modulus = spec->modulus;
+ hbounds[i].remainder = spec->remainder;
+ hbounds[i].index = i;
}
/* Sort all the bounds in ascending order */
- qsort(hbounds, nparts, sizeof(PartitionHashBound *),
+ qsort(hbounds, nparts, sizeof(PartitionHashBound),
qsort_partition_hbound_cmp);
/* After sorting, moduli are now stored in ascending order. */
- greatest_modulus = hbounds[ndatums - 1]->modulus;
+ greatest_modulus = hbounds[ndatums - 1].modulus;
boundinfo->ndatums = ndatums;
boundinfo->datums = (Datum **) palloc0(ndatums * sizeof(Datum *));
@@ -409,8 +408,8 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
*/
for (i = 0; i < nparts; i++)
{
- int modulus = hbounds[i]->modulus;
- int remainder = hbounds[i]->remainder;
+ int modulus = hbounds[i].modulus;
+ int remainder = hbounds[i].remainder;
boundinfo->datums[i] = (Datum *) palloc(2 * sizeof(Datum));
boundinfo->datums[i][0] = Int32GetDatum(modulus);
@@ -424,8 +423,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
remainder += modulus;
}
- (*mapping)[hbounds[i]->index] = i;
- pfree(hbounds[i]);
+ (*mapping)[hbounds[i].index] = i;
}
pfree(hbounds);
@@ -3676,8 +3674,8 @@ partition_hash_bsearch(PartitionBoundInfo boundinfo,
static int32
qsort_partition_hbound_cmp(const void *a, const void *b)
{
- PartitionHashBound *h1 = (*(PartitionHashBound *const *) a);
- PartitionHashBound *h2 = (*(PartitionHashBound *const *) b);
+ PartitionHashBound *const h1 = (PartitionHashBound *const) a;
+ PartitionHashBound *const h2 = (PartitionHashBound *const) b;
return partition_hbound_cmp(h1->modulus, h1->remainder,
h2->modulus, h2->remainder);
--
2.17.0
0004-Allocate-datum-arrays-in-bulk-to-avoid-palloc-overhe.patch (text/x-diff, 3.1K) download
From f7eb3ee22ae86df2b1fec2e06727e77db5b4d12b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 16 May 2021 11:21:14 -0500
Subject: [PATCH 4/5] Allocate datum arrays in bulk, to avoid palloc overhead
---
src/backend/partitioning/partbounds.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 490fe89eb8..961447ba5d 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -362,6 +362,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
int i;
int ndatums = 0;
int greatest_modulus;
+ Datum *datumptr;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -406,12 +407,14 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
* pairs) as there are partitions. Indexes are simply values ranging from
* 0 to (nparts - 1).
*/
+ datumptr = (Datum *) palloc0(ndatums * 2 * sizeof(Datum));
for (i = 0; i < nparts; i++)
{
int modulus = hbounds[i].modulus;
int remainder = hbounds[i].remainder;
- boundinfo->datums[i] = (Datum *) palloc(2 * sizeof(Datum));
+ boundinfo->datums[i] = datumptr;
+ datumptr += 2;
boundinfo->datums[i][0] = Int32GetDatum(modulus);
boundinfo->datums[i][1] = Int32GetDatum(remainder);
@@ -472,6 +475,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
int next_index = 0;
int default_index = -1;
int null_index = -1;
+ Datum *datumptr;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -541,11 +545,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
* receive the same value. The value for a given partition is the index of
* that partition's smallest datum in the all_values[] array.
*/
+ datumptr = (Datum *) palloc0(ndatums * sizeof(Datum));
for (i = 0; i < ndatums; i++)
{
int orig_index = all_values[i].index;
- boundinfo->datums[i] = (Datum *) palloc(sizeof(Datum));
+ boundinfo->datums[i] = datumptr++;
boundinfo->datums[i][0] = datumCopy(all_values[i].value,
key->parttypbyval[0],
key->parttyplen[0]);
@@ -609,6 +614,7 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
int ndatums = 0;
int default_index = -1;
int next_index = 0;
+ Datum *datumptr;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -733,12 +739,13 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->nindexes = ndatums + 1;
boundinfo->indexes = (int *) palloc((ndatums + 1) * sizeof(int));
+ datumptr = (Datum *) palloc0(ndatums * key->partnatts * sizeof(Datum));
for (i = 0; i < ndatums; i++)
{
int j;
- boundinfo->datums[i] = (Datum *) palloc(key->partnatts *
- sizeof(Datum));
+ boundinfo->datums[i] = datumptr;
+ datumptr += key->partnatts;
boundinfo->kind[i] = (PartitionRangeDatumKind *)
palloc(key->partnatts *
sizeof(PartitionRangeDatumKind));
--
2.17.0
0005-create_range_bounds-pfree-intermediate-results.patch (text/x-diff, 949B) download
From 208ed37da054e7ffc82180bcca01134ac7ea7786 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 16 May 2021 11:22:59 -0500
Subject: [PATCH 5/5] create_range_bounds: pfree() intermediate results
---
src/backend/partitioning/partbounds.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 961447ba5d..e3b2256693 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -715,6 +715,8 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
prev = cur;
}
+ pfree(all_bounds);
+
/* Update ndatums to hold the count of distinct datums. */
ndatums = k;
@@ -781,6 +783,8 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
}
+ pfree(rbounds);
+
/* Set the canonical value for default_index, if any. */
if (default_index != -1)
{
--
2.17.0
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
@ 2021-05-16 17:26 ` Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
0 siblings, 1 reply; 20+ messages in thread
From: Zhihong Yu @ 2021-05-16 17:26 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Sun, May 16, 2021 at 10:00 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Sat, May 15, 2021 at 02:40:45PM +0530, Nitin Jadhav wrote:
> > While working on [1], I observed that extra memory is allocated in
> > [1]
> https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compos...
>
> This is a link to your gmail, not to anything public.
>
> If it's worth counting list elements in advance, then you can also
> allocate the
> PartitionListValue as a single chunk, rather than palloc in a loop.
> This may help large partition heirarchies.
>
> And the same thing in create_hash_bounds with hbounds.
>
> create_range_bounds() already doesn't call palloc in a loop. However, then
> there's an asymmetry in create_range_bounds, which is still takes a
> double-indirect pointer.
>
> I'm not able to detect that this is saving more than about ~1% less RAM, to
> create or select from 1000 partitions, probably because other data
> structures
> are using much more, and savings here are relatively small.
>
> I'm going to add to the next CF. You can add yourself as an author, and
> watch
> that the patch passes tests in cfbot.
> https://commitfest.postgresql.org/
> http://cfbot.cputube.org/
>
> Thanks,
> --
> Justin
>
Hi,
For 0001-Removed-extra-memory-allocations-from-create_list_bo.patch :
+static int
+get_non_null_count_list_bounds(PartitionBoundSpec **boundspecs, int nparts)
Since the function returns the total number of non null bound values,
should it be named get_non_null_list_bounds_count ?
It can also be named get_count_of_... but that's longer.
+ all_values = (PartitionListValue **)
+ palloc(ndatums * sizeof(PartitionListValue *));
The palloc() call would take place even if ndatums is 0. I think in that
case, palloc() doesn't need to be called.
Cheers
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
@ 2021-05-17 14:52 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:00 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-17 14:52 UTC (permalink / raw)
To: Zhihong Yu <zyu@yugabyte.com>; Justin Pryzby <pryzby@telsasoft.com>; +Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
> > While working on [1], I observed that extra memory is allocated in
> > [1] https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compos...
I am really sorry for this. I wanted to point to the thread subjected
to 'Multi-Column List Partitioning'.
> If it's worth counting list elements in advance, then you can also allocate the
> PartitionListValue as a single chunk, rather than palloc in a loop.
> This may help large partition heirarchies.
>
> And the same thing in create_hash_bounds with hbounds.
I agree and thanks for creating those patches. I am not able to apply
the patch on the latest HEAD. Kindly check and upload the modified
patches.
> I'm not able to detect that this is saving more than about ~1% less RAM, to
> create or select from 1000 partitions, probably because other data structures
> are using much more, and savings here are relatively small.
Yes it does not save huge memory but it's an improvement.
> I'm going to add to the next CF. You can add yourself as an author, and watch
> that the patch passes tests in cfbot.
> https://commitfest.postgresql.org/
> http://cfbot.cputube.org/
Thanks for creating the commitfest entry.
> Since the function returns the total number of non null bound values, should it be named get_non_null_list_bounds_count ?
> It can also be named get_count_of_... but that's longer.
Changed it to 'get_non_null_list_bounds_count'.
> The palloc() call would take place even if ndatums is 0. I think in that case, palloc() doesn't need to be called.
I feel there is no such case where the 'ndatums' is 0 because as we
can see below, there is an assert in the 'partition_bounds_create'
function from where we call the 'create_list_bounds' function. Kindly
provide such a case if I am wrong.
PartitionBoundInfo
partition_bounds_create(PartitionBoundSpec **boundspecs, int nparts,
PartitionKey key, int **mapping)
{
int i;
Assert(nparts > 0);
Thanks & Regards,
Nitin Jadhav
On Sun, May 16, 2021 at 10:52 PM Zhihong Yu <zyu@yugabyte.com> wrote:
>
>
>
> On Sun, May 16, 2021 at 10:00 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>>
>> On Sat, May 15, 2021 at 02:40:45PM +0530, Nitin Jadhav wrote:
>> > While working on [1], I observed that extra memory is allocated in
>> > [1] https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compos...
>>
>> This is a link to your gmail, not to anything public.
>>
>> If it's worth counting list elements in advance, then you can also allocate the
>> PartitionListValue as a single chunk, rather than palloc in a loop.
>> This may help large partition heirarchies.
>>
>> And the same thing in create_hash_bounds with hbounds.
>>
>> create_range_bounds() already doesn't call palloc in a loop. However, then
>> there's an asymmetry in create_range_bounds, which is still takes a
>> double-indirect pointer.
>>
>> I'm not able to detect that this is saving more than about ~1% less RAM, to
>> create or select from 1000 partitions, probably because other data structures
>> are using much more, and savings here are relatively small.
>>
>> I'm going to add to the next CF. You can add yourself as an author, and watch
>> that the patch passes tests in cfbot.
>> https://commitfest.postgresql.org/
>> http://cfbot.cputube.org/
>>
>> Thanks,
>> --
>> Justin
>
> Hi,
> For 0001-Removed-extra-memory-allocations-from-create_list_bo.patch :
>
> +static int
> +get_non_null_count_list_bounds(PartitionBoundSpec **boundspecs, int nparts)
>
> Since the function returns the total number of non null bound values, should it be named get_non_null_list_bounds_count ?
> It can also be named get_count_of_... but that's longer.
>
> + all_values = (PartitionListValue **)
> + palloc(ndatums * sizeof(PartitionListValue *));
>
> The palloc() call would take place even if ndatums is 0. I think in that case, palloc() doesn't need to be called.
>
> Cheers
>
Attachments:
v2_removed_extra_mem_alloc_from_create_list_bounds.patch (application/x-patch, 3.1K) download
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7925fcc..e0a04fe 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -433,6 +433,32 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
/*
+ * get_non_null_list_bounds_count
+ * Calculates the total number of non null bound values of
+ * all the partitions.
+ */
+static int
+get_non_null_list_bounds_count(PartitionBoundSpec **boundspecs, int nparts)
+{
+ int i = 0;
+ int count = 0;
+
+ for (i = 0; i < nparts; i++)
+ {
+ ListCell *c;
+
+ foreach(c, boundspecs[i]->listdatums)
+ {
+ Const *val = castNode(Const, lfirst(c));
+
+ if (!val->constisnull)
+ count++;
+ }
+ }
+
+ return count;
+}
+/*
* create_list_bounds
* Create a PartitionBoundInfo for a list partitioned table
*/
@@ -442,13 +468,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
{
PartitionBoundInfo boundinfo;
PartitionListValue **all_values = NULL;
- ListCell *cell;
int i = 0;
+ int j = 0;
int ndatums = 0;
int next_index = 0;
int default_index = -1;
int null_index = -1;
- List *non_null_values = NIL;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -457,6 +482,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->null_index = -1;
boundinfo->default_index = -1;
+ ndatums = get_non_null_list_bounds_count(boundspecs, nparts);
+ all_values = (PartitionListValue **)
+ palloc(ndatums * sizeof(PartitionListValue *));
+
/* Create a unified list of non-null values across all partitions. */
for (i = 0; i < nparts; i++)
{
@@ -480,14 +509,14 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
foreach(c, spec->listdatums)
{
Const *val = castNode(Const, lfirst(c));
- PartitionListValue *list_value = NULL;
if (!val->constisnull)
{
- list_value = (PartitionListValue *)
+ all_values[j] = (PartitionListValue *)
palloc0(sizeof(PartitionListValue));
- list_value->index = i;
- list_value->value = val->constvalue;
+ all_values[j]->index = i;
+ all_values[j]->value = val->constvalue;
+ j++;
}
else
{
@@ -499,32 +528,9 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
elog(ERROR, "found null more than once");
null_index = i;
}
-
- if (list_value)
- non_null_values = lappend(non_null_values, list_value);
}
}
- ndatums = list_length(non_null_values);
-
- /*
- * Collect all list values in one array. Alongside the value, we also save
- * the index of partition the value comes from.
- */
- all_values = (PartitionListValue **)
- palloc(ndatums * sizeof(PartitionListValue *));
- i = 0;
- foreach(cell, non_null_values)
- {
- PartitionListValue *src = lfirst(cell);
-
- all_values[i] = (PartitionListValue *)
- palloc(sizeof(PartitionListValue));
- all_values[i]->value = src->value;
- all_values[i]->index = src->index;
- i++;
- }
-
qsort_arg(all_values, ndatums, sizeof(PartitionListValue *),
qsort_partition_list_value_cmp, (void *) key);
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-17 15:00 ` Zhihong Yu <zyu@yugabyte.com>
1 sibling, 0 replies; 20+ messages in thread
From: Zhihong Yu @ 2021-05-17 15:00 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Mon, May 17, 2021 at 7:52 AM Nitin Jadhav <nitinjadhavpostgres@gmail.com>
wrote:
> > > While working on [1], I observed that extra memory is allocated in
> > > [1]
> https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compos...
>
> I am really sorry for this. I wanted to point to the thread subjected
> to 'Multi-Column List Partitioning'.
>
> > If it's worth counting list elements in advance, then you can also
> allocate the
> > PartitionListValue as a single chunk, rather than palloc in a loop.
> > This may help large partition heirarchies.
> >
> > And the same thing in create_hash_bounds with hbounds.
>
> I agree and thanks for creating those patches. I am not able to apply
> the patch on the latest HEAD. Kindly check and upload the modified
> patches.
>
> > I'm not able to detect that this is saving more than about ~1% less RAM,
> to
> > create or select from 1000 partitions, probably because other data
> structures
> > are using much more, and savings here are relatively small.
>
> Yes it does not save huge memory but it's an improvement.
>
> > I'm going to add to the next CF. You can add yourself as an author, and
> watch
> > that the patch passes tests in cfbot.
> > https://commitfest.postgresql.org/
> > http://cfbot.cputube.org/
>
> Thanks for creating the commitfest entry.
>
> > Since the function returns the total number of non null bound values,
> should it be named get_non_null_list_bounds_count ?
> > It can also be named get_count_of_... but that's longer.
>
> Changed it to 'get_non_null_list_bounds_count'.
>
> > The palloc() call would take place even if ndatums is 0. I think in that
> case, palloc() doesn't need to be called.
>
> I feel there is no such case where the 'ndatums' is 0 because as we
> can see below, there is an assert in the 'partition_bounds_create'
> function from where we call the 'create_list_bounds' function. Kindly
> provide such a case if I am wrong.
>
> PartitionBoundInfo
> partition_bounds_create(PartitionBoundSpec **boundspecs, int nparts,
> PartitionKey key, int **mapping)
> {
> int i;
>
> Assert(nparts > 0);
>
Hi,
Thanks for pointing out the assertion.
My corresponding comment can be dropped.
Cheers
>
> Thanks & Regards,
> Nitin Jadhav
> On Sun, May 16, 2021 at 10:52 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> >
> >
> >
> > On Sun, May 16, 2021 at 10:00 AM Justin Pryzby <pryzby@telsasoft.com>
> wrote:
> >>
> >> On Sat, May 15, 2021 at 02:40:45PM +0530, Nitin Jadhav wrote:
> >> > While working on [1], I observed that extra memory is allocated in
> >> > [1]
> https://mail.google.com/mail/u/2/#search/multi+column+list/KtbxLxgZZTjRxNrBWvmHzDTHXCHLssSprg?compos...
> >>
> >> This is a link to your gmail, not to anything public.
> >>
> >> If it's worth counting list elements in advance, then you can also
> allocate the
> >> PartitionListValue as a single chunk, rather than palloc in a loop.
> >> This may help large partition heirarchies.
> >>
> >> And the same thing in create_hash_bounds with hbounds.
> >>
> >> create_range_bounds() already doesn't call palloc in a loop. However,
> then
> >> there's an asymmetry in create_range_bounds, which is still takes a
> >> double-indirect pointer.
> >>
> >> I'm not able to detect that this is saving more than about ~1% less
> RAM, to
> >> create or select from 1000 partitions, probably because other data
> structures
> >> are using much more, and savings here are relatively small.
> >>
> >> I'm going to add to the next CF. You can add yourself as an author,
> and watch
> >> that the patch passes tests in cfbot.
> >> https://commitfest.postgresql.org/
> >> http://cfbot.cputube.org/
> >>
> >> Thanks,
> >> --
> >> Justin
> >
> > Hi,
> > For 0001-Removed-extra-memory-allocations-from-create_list_bo.patch :
> >
> > +static int
> > +get_non_null_count_list_bounds(PartitionBoundSpec **boundspecs, int
> nparts)
> >
> > Since the function returns the total number of non null bound values,
> should it be named get_non_null_list_bounds_count ?
> > It can also be named get_count_of_... but that's longer.
> >
> > + all_values = (PartitionListValue **)
> > + palloc(ndatums * sizeof(PartitionListValue *));
> >
> > The palloc() call would take place even if ndatums is 0. I think in that
> case, palloc() doesn't need to be called.
> >
> > Cheers
> >
>
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-17 15:03 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Justin Pryzby @ 2021-05-17 15:03 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers@lists.postgresql.org
On Mon, May 17, 2021 at 08:22:25PM +0530, Nitin Jadhav wrote:
> I agree and thanks for creating those patches. I am not able to apply
> the patch on the latest HEAD. Kindly check and upload the modified
> patches.
The CFBOT had no issues with the patches, so I suspect an issue on your side.
http://cfbot.cputube.org/nitin-jadhav.html
--
Justin
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
@ 2021-05-18 17:28 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
2021-07-05 13:48 ` Re: Removed extra memory allocations from create_list_bounds David Rowley <dgrowleyml@gmail.com>
0 siblings, 2 replies; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-18 17:28 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
> The CFBOT had no issues with the patches, so I suspect an issue on your side.
> http://cfbot.cputube.org/nitin-jadhav.html
I am getting the following error when I try to apply in my machine.
$ git apply ../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:18:
trailing whitespace.
/*
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:19:
trailing whitespace.
* get_non_null_count_list_bounds
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:20:
trailing whitespace.
* Calculates the total number of non null bound values of
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:21:
trailing whitespace.
* all the partitions.
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:22:
trailing whitespace.
*/
error: patch failed: src/backend/partitioning/partbounds.c:432
error: src/backend/partitioning/partbounds.c: patch does not apply
However I was able to apply it by adding '--reject --whitespace=fix'.
$ git apply --reject --whitespace=fix
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:18:
trailing whitespace.
/*
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:19:
trailing whitespace.
* get_non_null_count_list_bounds
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:20:
trailing whitespace.
* Calculates the total number of non null bound values of
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:21:
trailing whitespace.
* all the partitions.
../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:22:
trailing whitespace.
*/
Checking patch src/backend/partitioning/partbounds.c...
Applied patch src/backend/partitioning/partbounds.c cleanly.
warning: squelched 30 whitespace errors
warning: 35 lines add whitespace errors.
I have rebased all the patches on top of
'v2_0001-removed_extra_mem_alloc_from_create_list_bounds.patch'.
Attaching all the patches here.
--
Thanks & Regards,
Nitin Jadhav
On Mon, May 17, 2021 at 8:33 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Mon, May 17, 2021 at 08:22:25PM +0530, Nitin Jadhav wrote:
> > I agree and thanks for creating those patches. I am not able to apply
> > the patch on the latest HEAD. Kindly check and upload the modified
> > patches.
>
> The CFBOT had no issues with the patches, so I suspect an issue on your side.
> http://cfbot.cputube.org/nitin-jadhav.html
>
> --
> Justin
Attachments: download all (5 files, .tar.gz)
v2_0001-removed_extra_mem_alloc_from_create_list_bounds.patch (application/octet-stream, 3.5K) download
From ee6a19144c6aa9f623353916d7a560df31ae42e6 Mon Sep 17 00:00:00 2001
From: Nitin <nitin.jadhav@enterprisedb.com>
Date: Tue, 18 May 2021 22:31:55 +0530
Subject: [PATCH 1/5] removed_extra_mem_alloc_from_create_list_bounds
---
src/backend/partitioning/partbounds.c | 64 +++++++++++++++++++----------------
1 file changed, 35 insertions(+), 29 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7925fcc..e0a04fe 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -433,6 +433,32 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
/*
+ * get_non_null_list_bounds_count
+ * Calculates the total number of non null bound values of
+ * all the partitions.
+ */
+static int
+get_non_null_list_bounds_count(PartitionBoundSpec **boundspecs, int nparts)
+{
+ int i = 0;
+ int count = 0;
+
+ for (i = 0; i < nparts; i++)
+ {
+ ListCell *c;
+
+ foreach(c, boundspecs[i]->listdatums)
+ {
+ Const *val = castNode(Const, lfirst(c));
+
+ if (!val->constisnull)
+ count++;
+ }
+ }
+
+ return count;
+}
+/*
* create_list_bounds
* Create a PartitionBoundInfo for a list partitioned table
*/
@@ -442,13 +468,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
{
PartitionBoundInfo boundinfo;
PartitionListValue **all_values = NULL;
- ListCell *cell;
int i = 0;
+ int j = 0;
int ndatums = 0;
int next_index = 0;
int default_index = -1;
int null_index = -1;
- List *non_null_values = NIL;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -457,6 +482,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->null_index = -1;
boundinfo->default_index = -1;
+ ndatums = get_non_null_list_bounds_count(boundspecs, nparts);
+ all_values = (PartitionListValue **)
+ palloc(ndatums * sizeof(PartitionListValue *));
+
/* Create a unified list of non-null values across all partitions. */
for (i = 0; i < nparts; i++)
{
@@ -480,14 +509,14 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
foreach(c, spec->listdatums)
{
Const *val = castNode(Const, lfirst(c));
- PartitionListValue *list_value = NULL;
if (!val->constisnull)
{
- list_value = (PartitionListValue *)
+ all_values[j] = (PartitionListValue *)
palloc0(sizeof(PartitionListValue));
- list_value->index = i;
- list_value->value = val->constvalue;
+ all_values[j]->index = i;
+ all_values[j]->value = val->constvalue;
+ j++;
}
else
{
@@ -499,32 +528,9 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
elog(ERROR, "found null more than once");
null_index = i;
}
-
- if (list_value)
- non_null_values = lappend(non_null_values, list_value);
}
}
- ndatums = list_length(non_null_values);
-
- /*
- * Collect all list values in one array. Alongside the value, we also save
- * the index of partition the value comes from.
- */
- all_values = (PartitionListValue **)
- palloc(ndatums * sizeof(PartitionListValue *));
- i = 0;
- foreach(cell, non_null_values)
- {
- PartitionListValue *src = lfirst(cell);
-
- all_values[i] = (PartitionListValue *)
- palloc(sizeof(PartitionListValue));
- all_values[i]->value = src->value;
- all_values[i]->index = src->index;
- i++;
- }
-
qsort_arg(all_values, ndatums, sizeof(PartitionListValue *),
qsort_partition_list_value_cmp, (void *) key);
--
1.8.3.1
v2_0003-Same-for-create_hash_bounds-PartitionHashBound.patch (application/octet-stream, 3.3K) download
From 9d8782e0213fea5d677a00abe78bd406bfc231fa Mon Sep 17 00:00:00 2001
From: Nitin <nitin.jadhav@enterprisedb.com>
Date: Tue, 18 May 2021 22:40:04 +0530
Subject: [PATCH 3/5] Same-for-create_hash_bounds-PartitionHashBound
---
src/backend/partitioning/partbounds.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index f91efba..5cf7672 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -358,7 +358,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
PartitionKey key, int **mapping)
{
PartitionBoundInfo boundinfo;
- PartitionHashBound **hbounds = NULL;
+ PartitionHashBound *hbounds;
int i;
int ndatums = 0;
int greatest_modulus;
@@ -371,8 +371,8 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->default_index = -1;
ndatums = nparts;
- hbounds = (PartitionHashBound **)
- palloc(nparts * sizeof(PartitionHashBound *));
+ hbounds = (PartitionHashBound *)
+ palloc(nparts * sizeof(PartitionHashBound));
/* Convert from node to the internal representation */
for (i = 0; i < nparts; i++)
@@ -382,18 +382,17 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
if (spec->strategy != PARTITION_STRATEGY_HASH)
elog(ERROR, "invalid strategy in partition bound spec");
- hbounds[i] = (PartitionHashBound *) palloc(sizeof(PartitionHashBound));
- hbounds[i]->modulus = spec->modulus;
- hbounds[i]->remainder = spec->remainder;
- hbounds[i]->index = i;
+ hbounds[i].modulus = spec->modulus;
+ hbounds[i].remainder = spec->remainder;
+ hbounds[i].index = i;
}
/* Sort all the bounds in ascending order */
- qsort(hbounds, nparts, sizeof(PartitionHashBound *),
+ qsort(hbounds, nparts, sizeof(PartitionHashBound),
qsort_partition_hbound_cmp);
/* After sorting, moduli are now stored in ascending order. */
- greatest_modulus = hbounds[ndatums - 1]->modulus;
+ greatest_modulus = hbounds[ndatums - 1].modulus;
boundinfo->ndatums = ndatums;
boundinfo->datums = (Datum **) palloc0(ndatums * sizeof(Datum *));
@@ -409,8 +408,8 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
*/
for (i = 0; i < nparts; i++)
{
- int modulus = hbounds[i]->modulus;
- int remainder = hbounds[i]->remainder;
+ int modulus = hbounds[i].modulus;
+ int remainder = hbounds[i].remainder;
boundinfo->datums[i] = (Datum *) palloc(2 * sizeof(Datum));
boundinfo->datums[i][0] = Int32GetDatum(modulus);
@@ -424,8 +423,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
remainder += modulus;
}
- (*mapping)[hbounds[i]->index] = i;
- pfree(hbounds[i]);
+ (*mapping)[hbounds[i].index] = i;
}
pfree(hbounds);
@@ -3676,8 +3674,8 @@ partition_hash_bsearch(PartitionBoundInfo boundinfo,
static int32
qsort_partition_hbound_cmp(const void *a, const void *b)
{
- PartitionHashBound *h1 = (*(PartitionHashBound *const *) a);
- PartitionHashBound *h2 = (*(PartitionHashBound *const *) b);
+ PartitionHashBound *const h1 = (PartitionHashBound *const) a;
+ PartitionHashBound *const h2 = (PartitionHashBound *const) b;
return partition_hbound_cmp(h1->modulus, h1->remainder,
h2->modulus, h2->remainder);
--
1.8.3.1
v2_0004-Allocate-datum-arrays-in-bulk-to-avoid-palloc-overhead.patch (application/octet-stream, 3.1K) download
From 838f97cd1c36dbbfa05f6a813c56042acd012d5b Mon Sep 17 00:00:00 2001
From: Nitin <nitin.jadhav@enterprisedb.com>
Date: Tue, 18 May 2021 22:42:12 +0530
Subject: [PATCH 4/5] Allocate-datum-arrays-in-bulk-to-avoid-palloc-overhe
---
src/backend/partitioning/partbounds.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 5cf7672..f1a692c 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -362,6 +362,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
int i;
int ndatums = 0;
int greatest_modulus;
+ Datum *datumptr;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -406,12 +407,14 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
* pairs) as there are partitions. Indexes are simply values ranging from
* 0 to (nparts - 1).
*/
+ datumptr = (Datum *) palloc0(ndatums * 2 * sizeof(Datum));
for (i = 0; i < nparts; i++)
{
int modulus = hbounds[i].modulus;
int remainder = hbounds[i].remainder;
- boundinfo->datums[i] = (Datum *) palloc(2 * sizeof(Datum));
+ boundinfo->datums[i] = datumptr;
+ datumptr += 2;
boundinfo->datums[i][0] = Int32GetDatum(modulus);
boundinfo->datums[i][1] = Int32GetDatum(remainder);
@@ -472,6 +475,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
int next_index = 0;
int default_index = -1;
int null_index = -1;
+ Datum *datumptr;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -541,11 +545,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
* receive the same value. The value for a given partition is the index of
* that partition's smallest datum in the all_values[] array.
*/
+ datumptr = (Datum *) palloc0(ndatums * sizeof(Datum));
for (i = 0; i < ndatums; i++)
{
int orig_index = all_values[i].index;
- boundinfo->datums[i] = (Datum *) palloc(sizeof(Datum));
+ boundinfo->datums[i] = datumptr++;
boundinfo->datums[i][0] = datumCopy(all_values[i].value,
key->parttypbyval[0],
key->parttyplen[0]);
@@ -609,6 +614,7 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
int ndatums = 0;
int default_index = -1;
int next_index = 0;
+ Datum *datumptr;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -733,12 +739,13 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->nindexes = ndatums + 1;
boundinfo->indexes = (int *) palloc((ndatums + 1) * sizeof(int));
+ datumptr = (Datum *) palloc0(ndatums * key->partnatts * sizeof(Datum));
for (i = 0; i < ndatums; i++)
{
int j;
- boundinfo->datums[i] = (Datum *) palloc(key->partnatts *
- sizeof(Datum));
+ boundinfo->datums[i] = datumptr;
+ datumptr += key->partnatts;
boundinfo->kind[i] = (PartitionRangeDatumKind *)
palloc(key->partnatts *
sizeof(PartitionRangeDatumKind));
--
1.8.3.1
v2_0005-create_range_bounds-pfree-intermediate-results.patch (application/octet-stream, 943B) download
From 382237eae1e143f60da9a4424992327c89a89b10 Mon Sep 17 00:00:00 2001
From: Nitin <nitin.jadhav@enterprisedb.com>
Date: Tue, 18 May 2021 22:43:58 +0530
Subject: [PATCH 5/5] create_range_bounds-pfree-intermediate-results
---
src/backend/partitioning/partbounds.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index f1a692c..67a0fce 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -715,6 +715,8 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
prev = cur;
}
+ pfree(all_bounds);
+
/* Update ndatums to hold the count of distinct datums. */
ndatums = k;
@@ -781,6 +783,8 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
}
+ pfree(rbounds);
+
/* Set the canonical value for default_index, if any. */
if (default_index != -1)
{
--
1.8.3.1
v2_0002-allocate-the-PartitionListValue-as-a-single-chunk.patch (application/octet-stream, 2.9K) download
From 433eb17cbcca7e2200a1416109097d91158f4b31 Mon Sep 17 00:00:00 2001
From: Nitin <nitin.jadhav@enterprisedb.com>
Date: Tue, 18 May 2021 22:37:08 +0530
Subject: [PATCH 2/5] allocate-the-PartitionListValue-as-a-single-chunk
---
src/backend/partitioning/partbounds.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index e0a04fe..f91efba 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -467,7 +467,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
PartitionKey key, int **mapping)
{
PartitionBoundInfo boundinfo;
- PartitionListValue **all_values = NULL;
+ PartitionListValue *all_values;
int i = 0;
int j = 0;
int ndatums = 0;
@@ -483,8 +483,8 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->default_index = -1;
ndatums = get_non_null_list_bounds_count(boundspecs, nparts);
- all_values = (PartitionListValue **)
- palloc(ndatums * sizeof(PartitionListValue *));
+ all_values = (PartitionListValue *)
+ palloc(ndatums * sizeof(PartitionListValue));
/* Create a unified list of non-null values across all partitions. */
for (i = 0; i < nparts; i++)
@@ -512,10 +512,8 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
if (!val->constisnull)
{
- all_values[j] = (PartitionListValue *)
- palloc0(sizeof(PartitionListValue));
- all_values[j]->index = i;
- all_values[j]->value = val->constvalue;
+ all_values[j].index = i;
+ all_values[j].value = val->constvalue;
j++;
}
else
@@ -531,7 +529,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
}
- qsort_arg(all_values, ndatums, sizeof(PartitionListValue *),
+ qsort_arg(all_values, ndatums, sizeof(PartitionListValue),
qsort_partition_list_value_cmp, (void *) key);
boundinfo->ndatums = ndatums;
@@ -547,10 +545,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
*/
for (i = 0; i < ndatums; i++)
{
- int orig_index = all_values[i]->index;
+ int orig_index = all_values[i].index;
boundinfo->datums[i] = (Datum *) palloc(sizeof(Datum));
- boundinfo->datums[i][0] = datumCopy(all_values[i]->value,
+ boundinfo->datums[i][0] = datumCopy(all_values[i].value,
key->parttypbyval[0],
key->parttyplen[0]);
@@ -3693,8 +3691,8 @@ qsort_partition_hbound_cmp(const void *a, const void *b)
static int32
qsort_partition_list_value_cmp(const void *a, const void *b, void *arg)
{
- Datum val1 = (*(PartitionListValue *const *) a)->value,
- val2 = (*(PartitionListValue *const *) b)->value;
+ Datum val1 = ((PartitionListValue *const) a)->value,
+ val2 = ((PartitionListValue *const) b)->value;
PartitionKey key = (PartitionKey) arg;
return DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[0],
--
1.8.3.1
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-18 17:49 ` Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Robert Haas @ 2021-05-18 17:49 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, May 18, 2021 at 1:29 PM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
> > The CFBOT had no issues with the patches, so I suspect an issue on your side.
> > http://cfbot.cputube.org/nitin-jadhav.html
>
> I am getting the following error when I try to apply in my machine.
>
> $ git apply ../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch
> ../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:18:
> trailing whitespace.
'git apply' is very picky. Use 'patch -p1' to apply your patches instead.
Also, use 'git diff --check' or 'git log --check' before generating
patches to send, and fix any whitespace errors before submitting.
I see that you have made a theoretical argument for why this should be
good for performance, but it would be better to have some test results
that show that it works out in practice. Sometimes things seem like
they ought to be more efficient but turn out to be less efficient when
they are actually tried.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
@ 2021-05-19 18:51 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:10 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:16 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-19 18:51 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
> 'git apply' is very picky. Use 'patch -p1' to apply your patches instead.
>
> Also, use 'git diff --check' or 'git log --check' before generating
> patches to send, and fix any whitespace errors before submitting.
Thanks for the suggestion. I will follow these.
> I see that you have made a theoretical argument for why this should be
> good for performance, but it would be better to have some test results
> that show that it works out in practice. Sometimes things seem like
> they ought to be more efficient but turn out to be less efficient when
> they are actually tried.
Created a table with one column of type 'int' and partitioned by that
column. Created 1 million partitions using following queries.
create table t(a int) partition by list(a);
select 'create table t_' || i || ' partition of t for
values in (' || i || ');'
from generate_series(1, 10000) i
\gexec
After this I tried to create 10 partitions and observed the time taken
to execute. Here is the data for 'with patch'.
postgres@34077=#select 'create table t_' || i || ' partition of t for
postgres'# values in (' || i || ');'
postgres-# from generate_series(10001, 10010) i
postgres-# \gexec
CREATE TABLE
Time: 36.863 ms
CREATE TABLE
Time: 46.645 ms
CREATE TABLE
Time: 44.915 ms
CREATE TABLE
Time: 39.660 ms
CREATE TABLE
Time: 42.188 ms
CREATE TABLE
Time: 43.163 ms
CREATE TABLE
Time: 44.374 ms
CREATE TABLE
Time: 45.117 ms
CREATE TABLE
Time: 40.340 ms
CREATE TABLE
Time: 38.604 ms
The data for 'without patch' looks like this.
postgres@31718=#select 'create table t_' || i || ' partition of t for
postgres'# values in (' || i || ');'
postgres-# from generate_series(10001, 10010) i
postgres-# \gexec
CREATE TABLE
Time: 45.917 ms
CREATE TABLE
Time: 46.815 ms
CREATE TABLE
Time: 44.180 ms
CREATE TABLE
Time: 48.163 ms
CREATE TABLE
Time: 45.884 ms
CREATE TABLE
Time: 48.330 ms
CREATE TABLE
Time: 48.614 ms
CREATE TABLE
Time: 48.376 ms
CREATE TABLE
Time: 46.380 ms
CREATE TABLE
Time: 48.233 ms
If we observe above data, we can see the improvement with the patch.
The average time taken to execute for the last 10 partitions is.
With patch - 42.1869 ms
Without patch - 47.0892 ms.
With respect to memory usage, AFAIK the allocated memory gets cleaned
during deallocation of the memory used by the memory context. So at
the end of the query, we may see no difference in the memory usage but
during the query execution it tries to get less memory than before.
Maybe during some worst case scenario, if there is less memory
available, we may see 'out of memory' errors without the patch but it
may work with the patch. I have not done experiments in this angle. I
am happy to do it if required.
Please share your thoughts.
--
Thanks & Regards,
Nitin Jadhav
On Tue, May 18, 2021 at 11:19 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, May 18, 2021 at 1:29 PM Nitin Jadhav
> <nitinjadhavpostgres@gmail.com> wrote:
> > > The CFBOT had no issues with the patches, so I suspect an issue on your side.
> > > http://cfbot.cputube.org/nitin-jadhav.html
> >
> > I am getting the following error when I try to apply in my machine.
> >
> > $ git apply ../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch
> > ../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:18:
> > trailing whitespace.
>
> 'git apply' is very picky. Use 'patch -p1' to apply your patches instead.
>
> Also, use 'git diff --check' or 'git log --check' before generating
> patches to send, and fix any whitespace errors before submitting.
>
> I see that you have made a theoretical argument for why this should be
> good for performance, but it would be better to have some test results
> that show that it works out in practice. Sometimes things seem like
> they ought to be more efficient but turn out to be less efficient when
> they are actually tried.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-19 19:10 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 0 replies; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-19 19:10 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
> Created a table with one column of type 'int' and partitioned by that
> column. Created 1 million partitions using following queries.
Sorry. It's not 1 million. Its 10,000 partitions.
--
Thanks & Regards,
Nitin Jadhav
On Thu, May 20, 2021 at 12:21 AM Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
>
> > 'git apply' is very picky. Use 'patch -p1' to apply your patches instead.
> >
> > Also, use 'git diff --check' or 'git log --check' before generating
> > patches to send, and fix any whitespace errors before submitting.
>
> Thanks for the suggestion. I will follow these.
>
> > I see that you have made a theoretical argument for why this should be
> > good for performance, but it would be better to have some test results
> > that show that it works out in practice. Sometimes things seem like
> > they ought to be more efficient but turn out to be less efficient when
> > they are actually tried.
>
> Created a table with one column of type 'int' and partitioned by that
> column. Created 1 million partitions using following queries.
>
> create table t(a int) partition by list(a);
> select 'create table t_' || i || ' partition of t for
> values in (' || i || ');'
> from generate_series(1, 10000) i
> \gexec
>
> After this I tried to create 10 partitions and observed the time taken
> to execute. Here is the data for 'with patch'.
>
> postgres@34077=#select 'create table t_' || i || ' partition of t for
> postgres'# values in (' || i || ');'
> postgres-# from generate_series(10001, 10010) i
> postgres-# \gexec
> CREATE TABLE
> Time: 36.863 ms
> CREATE TABLE
> Time: 46.645 ms
> CREATE TABLE
> Time: 44.915 ms
> CREATE TABLE
> Time: 39.660 ms
> CREATE TABLE
> Time: 42.188 ms
> CREATE TABLE
> Time: 43.163 ms
> CREATE TABLE
> Time: 44.374 ms
> CREATE TABLE
> Time: 45.117 ms
> CREATE TABLE
> Time: 40.340 ms
> CREATE TABLE
> Time: 38.604 ms
>
> The data for 'without patch' looks like this.
>
> postgres@31718=#select 'create table t_' || i || ' partition of t for
> postgres'# values in (' || i || ');'
> postgres-# from generate_series(10001, 10010) i
> postgres-# \gexec
> CREATE TABLE
> Time: 45.917 ms
> CREATE TABLE
> Time: 46.815 ms
> CREATE TABLE
> Time: 44.180 ms
> CREATE TABLE
> Time: 48.163 ms
> CREATE TABLE
> Time: 45.884 ms
> CREATE TABLE
> Time: 48.330 ms
> CREATE TABLE
> Time: 48.614 ms
> CREATE TABLE
> Time: 48.376 ms
> CREATE TABLE
> Time: 46.380 ms
> CREATE TABLE
> Time: 48.233 ms
>
> If we observe above data, we can see the improvement with the patch.
> The average time taken to execute for the last 10 partitions is.
> With patch - 42.1869 ms
> Without patch - 47.0892 ms.
>
> With respect to memory usage, AFAIK the allocated memory gets cleaned
> during deallocation of the memory used by the memory context. So at
> the end of the query, we may see no difference in the memory usage but
> during the query execution it tries to get less memory than before.
> Maybe during some worst case scenario, if there is less memory
> available, we may see 'out of memory' errors without the patch but it
> may work with the patch. I have not done experiments in this angle. I
> am happy to do it if required.
>
> Please share your thoughts.
>
> --
> Thanks & Regards,
> Nitin Jadhav
>
> On Tue, May 18, 2021 at 11:19 PM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Tue, May 18, 2021 at 1:29 PM Nitin Jadhav
> > <nitinjadhavpostgres@gmail.com> wrote:
> > > > The CFBOT had no issues with the patches, so I suspect an issue on your side.
> > > > http://cfbot.cputube.org/nitin-jadhav.html
> > >
> > > I am getting the following error when I try to apply in my machine.
> > >
> > > $ git apply ../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch
> > > ../patches/0001-Removed-extra-memory-allocations-from-create_list_bo.patch:18:
> > > trailing whitespace.
> >
> > 'git apply' is very picky. Use 'patch -p1' to apply your patches instead.
> >
> > Also, use 'git diff --check' or 'git log --check' before generating
> > patches to send, and fix any whitespace errors before submitting.
> >
> > I see that you have made a theoretical argument for why this should be
> > good for performance, but it would be better to have some test results
> > that show that it works out in practice. Sometimes things seem like
> > they ought to be more efficient but turn out to be less efficient when
> > they are actually tried.
> >
> > --
> > Robert Haas
> > EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-19 19:16 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-23 17:10 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Justin Pryzby @ 2021-05-19 19:16 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers@lists.postgresql.org
On Tue, May 18, 2021 at 01:49:12PM -0400, Robert Haas wrote:
> I see that you have made a theoretical argument for why this should be
> good for performance, but it would be better to have some test results
> that show that it works out in practice. Sometimes things seem like
> they ought to be more efficient but turn out to be less efficient when
> they are actually tried.
I see this as a code cleanup more than an performance optimization.
I couldn't see a measurable difference in my tests, involving CREATE TABLE and
SELECT.
I think some of my patches could *increase* memory use, due to power-of-two
allocation logic. I think it's still a good idea, since it doesn't seem to be
the dominant memory allocation.
On Thu, May 20, 2021 at 12:21:19AM +0530, Nitin Jadhav wrote:
> > I see that you have made a theoretical argument for why this should be
> > good for performance, but it would be better to have some test results
> > that show that it works out in practice. Sometimes things seem like
> > they ought to be more efficient but turn out to be less efficient when
> > they are actually tried.
>
> After this I tried to create 10 partitions and observed the time taken
> to execute. Here is the data for 'with patch'.
>
> postgres@34077=#select 'create table t_' || i || ' partition of t for
> postgres'# values in (' || i || ');'
> postgres-# from generate_series(10001, 10010) i
> postgres-# \gexec
I think you should be sure to do this within a transaction, without cassert,
and maybe with fsync=off full_page_writes=off.
> If we observe above data, we can see the improvement with the patch.
> The average time taken to execute for the last 10 partitions is.
It'd be interesting to know which patch(es) contributed to the improvement.
It's possible that (say) patch 0001 alone gives all the gain, or that 0002
diminishes the gains.
I think there'll be an interest in committing the smallest possible patch to
realize the gains, to minimize code churn an unrelated changes.
LIST and RANGE might need to be checked separately..
> With respect to memory usage, AFAIK the allocated memory gets cleaned
> during deallocation of the memory used by the memory context. So at
> the end of the query, we may see no difference in the memory usage but
> during the query execution it tries to get less memory than before.
You can check MAXRSS (at least on linux) if you enable log_executor_stats,
like:
\set QUIET \\ SET client_min_messages=debug; SET log_executor_stats=on; DROP TABLE p; CREATE TABLE p(i int) PARTITION BY LIST(i); CREATE TABLE pd PARTITION OF p DEFAULT;
SELECT format('CREATE TABLE p%s PARTITION OF p FOR VALUES IN (%s)', a,a) FROM generate_series(1,999)a;\gexec \\ SELECT;
--
Justin
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:16 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
@ 2021-05-23 17:10 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-23 17:14 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-23 17:46 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-23 17:10 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
> I see this as a code cleanup more than an performance optimization.
I agree with this. This is like a code cleanup but it also improves
performance.
I have done the performance testing, Just to confirm whether it really
improves
performance.
> I think some of my patches could *increase* memory use, due to
power-of-two
> allocation logic. I think it's still a good idea, since it doesn't seem
to be
> the dominant memory allocation.
I don't think that it will increase performance rather it adds to the
improvement.
> I think you should be sure to do this within a transaction, without
cassert,
> and maybe with fsync=off full_page_writes=off
Thanks for sharing this. I have done the above settings and collected the
below data.
> It'd be interesting to know which patch(es) contributed to the
improvement.
> It's possible that (say) patch 0001 alone gives all the gain, or that 0002
> diminishes the gains.
>
> I think there'll be an interest in committing the smallest possible patch
to
> realize the gains, to minimize code churn an unrelated changes.
In order to answer the above points, I have divided the patches into 2 sets.
1. Only 0001 and 0002 - These are related to list partitioning and do
not contain
changes related power-of-two allocation logic.
2. This contains all the 5 patches.
I have used the same testing procedure as explained in the previous mail.
Please find the timing information of the last 10 creation of partitioned
tables as given below.
Without patch With 0001 and 0002 With all patch
17.105 14.722 13.878
15.897 14.427 13.493
15.991 15.424 14.751
17.965 16.487 19.491
19.704 19.042 21.278
18.98 18.949 18.123
18.986 21.713 17.585
21.273 20.596 19.623
18.839 18.521 17.605
20.724 18.774 19.242
18.5464 17.8655 17.5069
As we can see in the above data, there is an improvement with both of the
patch sets.
> You can check MAXRSS (at least on linux) if you enable log_executor_stats,
> like:
>
> \set QUIET \\ SET client_min_messages=debug; SET log_executor_stats=on;
DROP TABLE p; CREATE TABLE p(i int) PARTITION BY LIST(i); CREATE TABLE pd
PARTITION OF p DEFAULT;
> SELECT format('CREATE TABLE p%s PARTITION OF p FOR VALUES IN (%s)', a,a)
FROM generate_series(1,999)a;\gexec \\ SELECT;
Thanks a lot for sharing this information. It was really helpful.
I have collected the stat information for creation of 1000
partitions. Please find the stat information for the 'without patch'
case below.
LOG: EXECUTOR STATISTICS
DETAIL: ! system usage stats:
! 0.000012 s user, 0.000000 s system, 0.000011 s elapsed
! [71.599426 s user, 4.362552 s system total]
! 63872 kB max resident size
! 0/0 [0/231096] filesystem blocks in/out
! 0/0 [0/2388074] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 0/0 [9982/6709] voluntary/involuntary context switches
Please find the stat information for 'with patch (all 5 patches)' case
below.
LOG: EXECUTOR STATISTICS
DETAIL: ! system usage stats:
! 0.000018 s user, 0.000002 s system, 0.000013 s elapsed
! [73.529715 s user, 4.219172 s system total]
! 63152 kB max resident size
! 0/0 [0/204840] filesystem blocks in/out
! 0/0 [0/2066377] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 0/0 [9956/4129] voluntary/involuntary context switches
Please share your thoughts.
--
Thanks & Regards,
Nitin Jadhav
On Thu, May 20, 2021 at 12:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Tue, May 18, 2021 at 01:49:12PM -0400, Robert Haas wrote:
> > I see that you have made a theoretical argument for why this should be
> > good for performance, but it would be better to have some test results
> > that show that it works out in practice. Sometimes things seem like
> > they ought to be more efficient but turn out to be less efficient when
> > they are actually tried.
>
> I see this as a code cleanup more than an performance optimization.
> I couldn't see a measurable difference in my tests, involving CREATE TABLE
> and
> SELECT.
>
> I think some of my patches could *increase* memory use, due to power-of-two
> allocation logic. I think it's still a good idea, since it doesn't seem
> to be
> the dominant memory allocation.
>
> On Thu, May 20, 2021 at 12:21:19AM +0530, Nitin Jadhav wrote:
> > > I see that you have made a theoretical argument for why this should be
> > > good for performance, but it would be better to have some test results
> > > that show that it works out in practice. Sometimes things seem like
> > > they ought to be more efficient but turn out to be less efficient when
> > > they are actually tried.
> >
> > After this I tried to create 10 partitions and observed the time taken
> > to execute. Here is the data for 'with patch'.
> >
> > postgres@34077=#select 'create table t_' || i || ' partition of t for
> > postgres'# values in (' || i || ');'
> > postgres-# from generate_series(10001, 10010) i
> > postgres-# \gexec
>
> I think you should be sure to do this within a transaction, without
> cassert,
> and maybe with fsync=off full_page_writes=off.
>
> > If we observe above data, we can see the improvement with the patch.
> > The average time taken to execute for the last 10 partitions is.
>
> It'd be interesting to know which patch(es) contributed to the improvement.
> It's possible that (say) patch 0001 alone gives all the gain, or that 0002
> diminishes the gains.
>
> I think there'll be an interest in committing the smallest possible patch
> to
> realize the gains, to minimize code churn an unrelated changes.
>
> LIST and RANGE might need to be checked separately..
>
> > With respect to memory usage, AFAIK the allocated memory gets cleaned
> > during deallocation of the memory used by the memory context. So at
> > the end of the query, we may see no difference in the memory usage but
> > during the query execution it tries to get less memory than before.
>
> You can check MAXRSS (at least on linux) if you enable log_executor_stats,
> like:
>
> \set QUIET \\ SET client_min_messages=debug; SET log_executor_stats=on;
> DROP TABLE p; CREATE TABLE p(i int) PARTITION BY LIST(i); CREATE TABLE pd
> PARTITION OF p DEFAULT;
> SELECT format('CREATE TABLE p%s PARTITION OF p FOR VALUES IN (%s)', a,a)
> FROM generate_series(1,999)a;\gexec \\ SELECT;
>
> --
> Justin
>
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:16 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-23 17:10 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-23 17:14 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 0 replies; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-23 17:14 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
> > I think some of my patches could *increase* memory use, due to
power-of-two
> > allocation logic. I think it's still a good idea, since it doesn't
seem to be
> > the dominant memory allocation.
>
> I don't think that it will increase performance rather it adds to the
improvement.
Sorry. Kindly ignore the above comment. I had misunderstood the statement.
Thanks & Regards,
Nitin Jadhav
On Sun, May 23, 2021 at 10:40 PM Nitin Jadhav <nitinjadhavpostgres@gmail.com>
wrote:
> > I see this as a code cleanup more than an performance optimization.
>
> I agree with this. This is like a code cleanup but it also improves
> performance.
> I have done the performance testing, Just to confirm whether it really
> improves
> performance.
>
> > I think some of my patches could *increase* memory use, due to
> power-of-two
> > allocation logic. I think it's still a good idea, since it doesn't seem
> to be
> > the dominant memory allocation.
>
> I don't think that it will increase performance rather it adds to the
> improvement.
>
> > I think you should be sure to do this within a transaction, without
> cassert,
> > and maybe with fsync=off full_page_writes=off
>
> Thanks for sharing this. I have done the above settings and collected the
> below data.
>
> > It'd be interesting to know which patch(es) contributed to the
> improvement.
> > It's possible that (say) patch 0001 alone gives all the gain, or that
> 0002
> > diminishes the gains.
> >
> > I think there'll be an interest in committing the smallest possible
> patch to
> > realize the gains, to minimize code churn an unrelated changes.
>
> In order to answer the above points, I have divided the patches into 2
> sets.
> 1. Only 0001 and 0002 - These are related to list partitioning and do
> not contain
> changes related power-of-two allocation logic.
> 2. This contains all the 5 patches.
>
> I have used the same testing procedure as explained in the previous mail.
> Please find the timing information of the last 10 creation of partitioned
> tables as given below.
>
> Without patch With 0001 and 0002 With all patch
> 17.105 14.722 13.878
> 15.897 14.427 13.493
> 15.991 15.424 14.751
> 17.965 16.487 19.491
> 19.704 19.042 21.278
> 18.98 18.949 18.123
> 18.986 21.713 17.585
> 21.273 20.596 19.623
> 18.839 18.521 17.605
> 20.724 18.774 19.242
> 18.5464 17.8655 17.5069
> As we can see in the above data, there is an improvement with both of the
> patch sets.
>
> > You can check MAXRSS (at least on linux) if you enable
> log_executor_stats,
> > like:
> >
> > \set QUIET \\ SET client_min_messages=debug; SET log_executor_stats=on;
> DROP TABLE p; CREATE TABLE p(i int) PARTITION BY LIST(i); CREATE TABLE pd
> PARTITION OF p DEFAULT;
> > SELECT format('CREATE TABLE p%s PARTITION OF p FOR VALUES IN (%s)', a,a)
> FROM generate_series(1,999)a;\gexec \\ SELECT;
>
> Thanks a lot for sharing this information. It was really helpful.
> I have collected the stat information for creation of 1000
> partitions. Please find the stat information for the 'without patch'
> case below.
>
> LOG: EXECUTOR STATISTICS
> DETAIL: ! system usage stats:
> ! 0.000012 s user, 0.000000 s system, 0.000011 s elapsed
> ! [71.599426 s user, 4.362552 s system total]
> ! 63872 kB max resident size
> ! 0/0 [0/231096] filesystem blocks in/out
> ! 0/0 [0/2388074] page faults/reclaims, 0 [0] swaps
> ! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
> ! 0/0 [9982/6709] voluntary/involuntary context switches
>
> Please find the stat information for 'with patch (all 5 patches)' case
> below.
>
> LOG: EXECUTOR STATISTICS
> DETAIL: ! system usage stats:
> ! 0.000018 s user, 0.000002 s system, 0.000013 s elapsed
> ! [73.529715 s user, 4.219172 s system total]
> ! 63152 kB max resident size
> ! 0/0 [0/204840] filesystem blocks in/out
> ! 0/0 [0/2066377] page faults/reclaims, 0 [0] swaps
> ! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
> ! 0/0 [9956/4129] voluntary/involuntary context switches
>
> Please share your thoughts.
>
> --
> Thanks & Regards,
> Nitin Jadhav
>
>
>
> On Thu, May 20, 2021 at 12:47 AM Justin Pryzby <pryzby@telsasoft.com>
> wrote:
>
>> On Tue, May 18, 2021 at 01:49:12PM -0400, Robert Haas wrote:
>> > I see that you have made a theoretical argument for why this should be
>> > good for performance, but it would be better to have some test results
>> > that show that it works out in practice. Sometimes things seem like
>> > they ought to be more efficient but turn out to be less efficient when
>> > they are actually tried.
>>
>> I see this as a code cleanup more than an performance optimization.
>> I couldn't see a measurable difference in my tests, involving CREATE
>> TABLE and
>> SELECT.
>>
>> I think some of my patches could *increase* memory use, due to
>> power-of-two
>> allocation logic. I think it's still a good idea, since it doesn't seem
>> to be
>> the dominant memory allocation.
>>
>> On Thu, May 20, 2021 at 12:21:19AM +0530, Nitin Jadhav wrote:
>> > > I see that you have made a theoretical argument for why this should be
>> > > good for performance, but it would be better to have some test results
>> > > that show that it works out in practice. Sometimes things seem like
>> > > they ought to be more efficient but turn out to be less efficient when
>> > > they are actually tried.
>> >
>> > After this I tried to create 10 partitions and observed the time taken
>> > to execute. Here is the data for 'with patch'.
>> >
>> > postgres@34077=#select 'create table t_' || i || ' partition of t for
>> > postgres'# values in (' || i || ');'
>> > postgres-# from generate_series(10001, 10010) i
>> > postgres-# \gexec
>>
>> I think you should be sure to do this within a transaction, without
>> cassert,
>> and maybe with fsync=off full_page_writes=off.
>>
>> > If we observe above data, we can see the improvement with the patch.
>> > The average time taken to execute for the last 10 partitions is.
>>
>> It'd be interesting to know which patch(es) contributed to the
>> improvement.
>> It's possible that (say) patch 0001 alone gives all the gain, or that 0002
>> diminishes the gains.
>>
>> I think there'll be an interest in committing the smallest possible patch
>> to
>> realize the gains, to minimize code churn an unrelated changes.
>>
>> LIST and RANGE might need to be checked separately..
>>
>> > With respect to memory usage, AFAIK the allocated memory gets cleaned
>> > during deallocation of the memory used by the memory context. So at
>> > the end of the query, we may see no difference in the memory usage but
>> > during the query execution it tries to get less memory than before.
>>
>> You can check MAXRSS (at least on linux) if you enable log_executor_stats,
>> like:
>>
>> \set QUIET \\ SET client_min_messages=debug; SET log_executor_stats=on;
>> DROP TABLE p; CREATE TABLE p(i int) PARTITION BY LIST(i); CREATE TABLE pd
>> PARTITION OF p DEFAULT;
>> SELECT format('CREATE TABLE p%s PARTITION OF p FOR VALUES IN (%s)', a,a)
>> FROM generate_series(1,999)a;\gexec \\ SELECT;
>>
>> --
>> Justin
>>
>
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:16 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-23 17:10 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-05-23 17:46 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-24 14:42 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Justin Pryzby @ 2021-05-23 17:46 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers@lists.postgresql.org
On Sun, May 23, 2021 at 10:40:16PM +0530, Nitin Jadhav wrote:
> I have used the same testing procedure as explained in the previous mail.
> Please find the timing information of the last 10 creation of partitioned
> tables as given below.
> Without patch With 0001 and 0002 With all patch
...
> 18.5464 17.8655 17.5069
For anyone reading non-HTML email, the last line shows the averages of the
previous 10 lines.
>> LIST and RANGE might need to be checked separately..
You checked LIST but not HASH (patches 3-4) or RANGE (patch 4-5), right?
Another test is to show the time/memory used by SELECT. That's far more
important than DDL, but I think the same results would apply here, so I think
it's not needed to test each of LIST/RANGE/HASH, nor to test every combination
of patches. Mostly it's nice to see if the memory use is more visibly
different, or if there's an impressive improvement for this case.
Note that for the MAXRSS test, you must a different postgres backend process
for each of the tests (or else each test would never show a lower number than
the previous test).
Thanks,
--
Justin
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Re: Removed extra memory allocations from create_list_bounds Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:16 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-23 17:10 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-23 17:46 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
@ 2021-05-24 14:42 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: Nitin Jadhav @ 2021-05-24 14:42 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
> You checked LIST but not HASH (patches 3-4) or RANGE (patch 4-5), right?
Yes. I did not check about HASH and RANGE partitioning related patches
as the changes are mostly similar to the list partitioning related
changes.
> Another test is to show the time/memory used by SELECT. That's far more
> important than DDL, but I think the same results would apply here, so I think
> it's not needed to test each of LIST/RANGE/HASH, nor to test every combination
> of patches.
Yes. I also feel that the same result would apply there as well.
> Note that for the MAXRSS test, you must a different postgres backend process
> for each of the tests (or else each test would never show a lower number than
> the previous test).
I have used different backend processes for each of the tests.
> Mostly it's nice to see if the memory use is more visibly
> different, or if there's an impressive improvement for this case.
I did not get this point. Kindly explain for which scenario the memory
usage test has to be done.
Thanks & Regards,
Nitin Jadhav
On Sun, May 23, 2021 at 11:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Sun, May 23, 2021 at 10:40:16PM +0530, Nitin Jadhav wrote:
> > I have used the same testing procedure as explained in the previous mail.
> > Please find the timing information of the last 10 creation of partitioned
> > tables as given below.
>
> > Without patch With 0001 and 0002 With all patch
> ...
> > 18.5464 17.8655 17.5069
>
> For anyone reading non-HTML email, the last line shows the averages of the
> previous 10 lines.
>
> >> LIST and RANGE might need to be checked separately..
>
> You checked LIST but not HASH (patches 3-4) or RANGE (patch 4-5), right?
>
> Another test is to show the time/memory used by SELECT. That's far more
> important than DDL, but I think the same results would apply here, so I think
> it's not needed to test each of LIST/RANGE/HASH, nor to test every combination
> of patches. Mostly it's nice to see if the memory use is more visibly
> different, or if there's an impressive improvement for this case.
>
> Note that for the MAXRSS test, you must a different postgres backend process
> for each of the tests (or else each test would never show a lower number than
> the previous test).
>
> Thanks,
> --
> Justin
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
@ 2021-07-05 13:48 ` David Rowley <dgrowleyml@gmail.com>
2021-07-05 16:45 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-07-05 17:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 2 replies; 20+ messages in thread
From: David Rowley @ 2021-07-05 13:48 UTC (permalink / raw)
To: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; Justin Pryzby <pryzby@telsasoft.com>; +Cc: Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, 19 May 2021 at 05:28, Nitin Jadhav
<nitinjadhavpostgres@gmail.com> wrote:
> I have rebased all the patches on top of
> 'v2_0001-removed_extra_mem_alloc_from_create_list_bounds.patch'.
> Attaching all the patches here.
I had a look over these and I think what's being done here is fine.
I think this will help speed up building the partition bound.
Unfortunately, it won't help any for speeding up things like finding
the correct partition during SELECT or DML on partitioned tables.
The reason for this is that RelationBuildPartitionDesc first builds
the partition bound using the functions being modified here, but it
then copies it into the relcache into a memory context for the
partition using partition_bounds_copy(). It looks like
partition_bounds_copy() suffers from the same palloc explosion type
problem as is being fixed in each of the create_*_bounds() functions
here. The good news is, we can just give partition_bounds_copy() the
same treatment. 0004 does that.
I tried to see if the better cache locality of the
PartitionBoundInfo's datum array would help speed up inserts into a
partitioned table. I figured a fairly small binary search in a LIST
partitioned table of 10 partitions might have all Datum visited all on
the same cache line. However, I was unable to see any performance
gains. I think the other work being done is likely just going to drown
out any gains in cache efficiency in the binary search. COPY into a
partitioned table might have more chance of becoming a little faster,
but I didn't try.
I've attached another set of patches. I squashed all the changes to
each create_*_bounds function into a patch of their own. Perhaps 0002
and 0003 which handle range and hash partitioning can be one patch
since Justin seemed to write that one. I kept 0001 separate since
that's Nitin's patch plus Justin's extra parts. It seems easier to
credit properly having the patches broken out like this. I think it's
excessive to break down 0001 into Nitin and Justin's individual parts.
I did make a few adjustments to the patches renaming a variable or two
and I changed how we assign the boundinfo->datums[i] pointers to take
the address of the Nth element rather than incrementing the variable
pointing to the array each item. I personally like p = &array[i];
more than p = array; array++, others may not feel the same.
Nitin and Justin, are you both able to have another look over these
and let me know what you think. If all is well I'd like to push all 4
patches.
David
Attachments: download all (4 files, .tar.gz)
v3-0001-Reduce-the-number-of-pallocs-in-create_list_bound.patch (application/octet-stream, 6.7K) download
From 9e731e20f396ec2f1b9fd2d12e69093170b0816e Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Mon, 5 Jul 2021 21:52:09 +1200
Subject: [PATCH v3 1/4] Reduce the number of pallocs in create_list_bounds
create_list_bounds is rather inefficient in regards to the number of
pallocs it performs. The code would allocate a single element Datum
pointer array for each Datum in the LIST partitioned table. It would be
much more efficient in terms of number of pallocs used if we just
allocated a ndatum sized array of Datums and instead of pallocing a single
element Datum pointer array, just point to an element in that Datum array.
Here we do just that.
Also, to further reduce the number of pallocs, change the transient
"all_values" array by making this a single array of PartitionListValue
rather than an array of PartitionListValue pointers pointing into
individually allocated chunks. This requires some prior knowledge of the
number of Datums that will go in the array, which the new
get_non_null_list_datum_count function takes care of for us.
While we're here, don't forget to pfree the all_values array after we're
done. create_hash_bounds() already does this with its equivalent array.
Let's be consistent.
There's now no longer any need for the transient List data structure.
Getting rid of that should further speed up this function.
Author: Nitin Jadhav, Justin Pryzby
Reviewed-by: Justin Pryzby, Zhihong Yu
Discussion: https://postgr.es/m/flat/CAMm1aWYFTqEio3bURzZh47jveiHRwgQTiSDvBORczNEz2duZ1Q@mail.gmail.com
---
src/backend/partitioning/partbounds.c | 89 +++++++++++++++------------
1 file changed, 50 insertions(+), 39 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7096d3bf45..12ad45fedf 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -432,6 +432,31 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
return boundinfo;
}
+/*
+ * get_non_null_list_datum_count
+ * Counts the number of non-null Datums in each partition.
+ */
+static int
+get_non_null_list_datum_count(PartitionBoundSpec **boundspecs, int nparts)
+{
+ int i = 0;
+ int count = 0;
+
+ for (i = 0; i < nparts; i++)
+ {
+ ListCell *c;
+
+ foreach(c, boundspecs[i]->listdatums)
+ {
+ Const *val = castNode(Const, lfirst(c));
+
+ if (!val->constisnull)
+ count++;
+ }
+ }
+
+ return count;
+}
/*
* create_list_bounds
* Create a PartitionBoundInfo for a list partitioned table
@@ -441,14 +466,14 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
PartitionKey key, int **mapping)
{
PartitionBoundInfo boundinfo;
- PartitionListValue **all_values = NULL;
- ListCell *cell;
- int i = 0;
- int ndatums = 0;
+ PartitionListValue *all_values;
+ int i;
+ int j;
+ int ndatums;
int next_index = 0;
int default_index = -1;
int null_index = -1;
- List *non_null_values = NIL;
+ Datum *boundDatums;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -457,8 +482,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->null_index = -1;
boundinfo->default_index = -1;
+ ndatums = get_non_null_list_datum_count(boundspecs, nparts);
+ all_values = (PartitionListValue *)
+ palloc(ndatums * sizeof(PartitionListValue));
+
/* Create a unified list of non-null values across all partitions. */
- for (i = 0; i < nparts; i++)
+ for (j = 0, i = 0; i < nparts; i++)
{
PartitionBoundSpec *spec = boundspecs[i];
ListCell *c;
@@ -480,14 +509,12 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
foreach(c, spec->listdatums)
{
Const *val = castNode(Const, lfirst(c));
- PartitionListValue *list_value = NULL;
if (!val->constisnull)
{
- list_value = (PartitionListValue *)
- palloc0(sizeof(PartitionListValue));
- list_value->index = i;
- list_value->value = val->constvalue;
+ all_values[j].index = i;
+ all_values[j].value = val->constvalue;
+ j++;
}
else
{
@@ -499,33 +526,13 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
elog(ERROR, "found null more than once");
null_index = i;
}
-
- if (list_value)
- non_null_values = lappend(non_null_values, list_value);
}
}
- ndatums = list_length(non_null_values);
-
- /*
- * Collect all list values in one array. Alongside the value, we also save
- * the index of partition the value comes from.
- */
- all_values = (PartitionListValue **)
- palloc(ndatums * sizeof(PartitionListValue *));
- i = 0;
- foreach(cell, non_null_values)
- {
- PartitionListValue *src = lfirst(cell);
-
- all_values[i] = (PartitionListValue *)
- palloc(sizeof(PartitionListValue));
- all_values[i]->value = src->value;
- all_values[i]->index = src->index;
- i++;
- }
+ /* ensure we found a Datum for every slot in the all_values array */
+ Assert(j == ndatums);
- qsort_arg(all_values, ndatums, sizeof(PartitionListValue *),
+ qsort_arg(all_values, ndatums, sizeof(PartitionListValue),
qsort_partition_list_value_cmp, (void *) key);
boundinfo->ndatums = ndatums;
@@ -533,6 +540,8 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->nindexes = ndatums;
boundinfo->indexes = (int *) palloc(ndatums * sizeof(int));
+ boundDatums = (Datum *) palloc(ndatums * sizeof(Datum));
+
/*
* Copy values. Canonical indexes are values ranging from 0 to (nparts -
* 1) assigned to each partition such that all datums of a given partition
@@ -541,10 +550,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
*/
for (i = 0; i < ndatums; i++)
{
- int orig_index = all_values[i]->index;
+ int orig_index = all_values[i].index;
- boundinfo->datums[i] = (Datum *) palloc(sizeof(Datum));
- boundinfo->datums[i][0] = datumCopy(all_values[i]->value,
+ boundinfo->datums[i] = &boundDatums[i];
+ boundinfo->datums[i][0] = datumCopy(all_values[i].value,
key->parttypbyval[0],
key->parttyplen[0]);
@@ -555,6 +564,8 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->indexes[i] = (*mapping)[orig_index];
}
+ pfree(all_values);
+
/*
* Set the canonical value for null_index, if any.
*
@@ -3697,8 +3708,8 @@ qsort_partition_hbound_cmp(const void *a, const void *b)
static int32
qsort_partition_list_value_cmp(const void *a, const void *b, void *arg)
{
- Datum val1 = (*(PartitionListValue *const *) a)->value,
- val2 = (*(PartitionListValue *const *) b)->value;
+ Datum val1 = ((PartitionListValue *const) a)->value,
+ val2 = ((PartitionListValue *const) b)->value;
PartitionKey key = (PartitionKey) arg;
return DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[0],
--
2.30.2
v3-0004-Reduce-the-number-of-pallocs-in-partition_bounds_.patch (application/octet-stream, 1.5K) download
From c759c42b0fb9d9c0555965d93f6d2dc31cd99a0d Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 6 Jul 2021 00:54:54 +1200
Subject: [PATCH v3 4/4] Reduce the number of pallocs in partition_bounds_copy
The Datum pointer arrays in the resulting PartitionBoundInfo's datums
array can be allocated as a single chunk of memory rather than allocating
a series of small arrays for each Datum.
Author: David Rowley
Discussion: https://postgr.es/m/flat/CAMm1aWYFTqEio3bURzZh47jveiHRwgQTiSDvBORczNEz2duZ1Q@mail.gmail.com
---
src/backend/partitioning/partbounds.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 64bbec05ea..2a1ed5a49d 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -928,6 +928,7 @@ partition_bounds_copy(PartitionBoundInfo src,
int partnatts;
bool hash_part;
int natts;
+ Datum *boundDatums;
dest = (PartitionBoundInfo) palloc(sizeof(PartitionBoundInfoData));
@@ -963,12 +964,13 @@ partition_bounds_copy(PartitionBoundInfo src,
*/
hash_part = (key->strategy == PARTITION_STRATEGY_HASH);
natts = hash_part ? 2 : partnatts;
+ boundDatums = palloc(ndatums * natts * sizeof(Datum));
for (i = 0; i < ndatums; i++)
{
int j;
- dest->datums[i] = (Datum *) palloc(sizeof(Datum) * natts);
+ dest->datums[i] = &boundDatums[i * natts];
for (j = 0; j < natts; j++)
{
--
2.30.2
v3-0003-Reduce-the-number-of-palloc-calls-in-create_range.patch (application/octet-stream, 1.9K) download
From f9d8e87b98f8a28c18a29e373a0ede76019c99fb Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 6 Jul 2021 00:47:05 +1200
Subject: [PATCH v3 3/4] Reduce the number of palloc calls in
create_range_bounds
The resulting PartitionBoundInfo's datum array elements can be made to
point to a single allocated chunk of memory rather than allocating a
small Datum pointer array for each element.
Author: Justin Pryzby
Reviewed-by: Nitin Jadhav
Discussion: https://postgr.es/m/flat/CAMm1aWYFTqEio3bURzZh47jveiHRwgQTiSDvBORczNEz2duZ1Q@mail.gmail.com
---
src/backend/partitioning/partbounds.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index e809fb9dda..64bbec05ea 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -617,6 +617,7 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
int ndatums = 0;
int default_index = -1;
int next_index = 0;
+ Datum *boundDatums;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -741,12 +742,13 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->nindexes = ndatums + 1;
boundinfo->indexes = (int *) palloc((ndatums + 1) * sizeof(int));
+ boundDatums = (Datum *) palloc(ndatums * key->partnatts * sizeof(Datum));
+
for (i = 0; i < ndatums; i++)
{
int j;
- boundinfo->datums[i] = (Datum *) palloc(key->partnatts *
- sizeof(Datum));
+ boundinfo->datums[i] = &boundDatums[i * key->partnatts];
boundinfo->kind[i] = (PartitionRangeDatumKind *)
palloc(key->partnatts *
sizeof(PartitionRangeDatumKind));
@@ -782,6 +784,8 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
}
}
+ pfree(rbounds);
+
/* Set the canonical value for default_index, if any. */
if (default_index != -1)
{
--
2.30.2
v3-0002-Reduce-the-number-of-pallocs-in-create_hash_bound.patch (application/octet-stream, 4.5K) download
From b089c9ceabc68d1937cab3185fdfb50f8039c69b Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 6 Jul 2021 00:31:57 +1200
Subject: [PATCH v3 2/4] Reduce the number of pallocs in create_hash_bounds
Make the hbounds array an array of PartitionHashBound rather than array of
PartitionHashBound pointers with individually allocated elements.
Also, make the resulting PartitionBoundInfo's datum array elements point
to elements of a single allocation rather than allocating an array of 2
Datum pointers for each partition.
Author: Justin Pryzby
Reviewed-by: Nitin Jadhav
Discussion: https://postgr.es/m/flat/CAMm1aWYFTqEio3bURzZh47jveiHRwgQTiSDvBORczNEz2duZ1Q@mail.gmail.com
---
src/backend/partitioning/partbounds.c | 39 +++++++++++++--------------
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 12ad45fedf..e809fb9dda 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -358,10 +358,10 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
PartitionKey key, int **mapping)
{
PartitionBoundInfo boundinfo;
- PartitionHashBound **hbounds = NULL;
+ PartitionHashBound *hbounds;
int i;
- int ndatums = 0;
int greatest_modulus;
+ Datum *boundDatums;
boundinfo = (PartitionBoundInfoData *)
palloc0(sizeof(PartitionBoundInfoData));
@@ -370,9 +370,8 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->null_index = -1;
boundinfo->default_index = -1;
- ndatums = nparts;
- hbounds = (PartitionHashBound **)
- palloc(nparts * sizeof(PartitionHashBound *));
+ hbounds = (PartitionHashBound *)
+ palloc(nparts * sizeof(PartitionHashBound));
/* Convert from node to the internal representation */
for (i = 0; i < nparts; i++)
@@ -382,26 +381,27 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
if (spec->strategy != PARTITION_STRATEGY_HASH)
elog(ERROR, "invalid strategy in partition bound spec");
- hbounds[i] = (PartitionHashBound *) palloc(sizeof(PartitionHashBound));
- hbounds[i]->modulus = spec->modulus;
- hbounds[i]->remainder = spec->remainder;
- hbounds[i]->index = i;
+ hbounds[i].modulus = spec->modulus;
+ hbounds[i].remainder = spec->remainder;
+ hbounds[i].index = i;
}
/* Sort all the bounds in ascending order */
- qsort(hbounds, nparts, sizeof(PartitionHashBound *),
+ qsort(hbounds, nparts, sizeof(PartitionHashBound),
qsort_partition_hbound_cmp);
/* After sorting, moduli are now stored in ascending order. */
- greatest_modulus = hbounds[ndatums - 1]->modulus;
+ greatest_modulus = hbounds[nparts - 1].modulus;
- boundinfo->ndatums = ndatums;
- boundinfo->datums = (Datum **) palloc0(ndatums * sizeof(Datum *));
+ boundinfo->ndatums = nparts;
+ boundinfo->datums = (Datum **) palloc0(nparts * sizeof(Datum *));
boundinfo->nindexes = greatest_modulus;
boundinfo->indexes = (int *) palloc(greatest_modulus * sizeof(int));
for (i = 0; i < greatest_modulus; i++)
boundinfo->indexes[i] = -1;
+ boundDatums = (Datum *) palloc(nparts * 2 * sizeof(Datum));
+
/*
* For hash partitioning, there are as many datums (modulus and remainder
* pairs) as there are partitions. Indexes are simply values ranging from
@@ -409,10 +409,10 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
*/
for (i = 0; i < nparts; i++)
{
- int modulus = hbounds[i]->modulus;
- int remainder = hbounds[i]->remainder;
+ int modulus = hbounds[i].modulus;
+ int remainder = hbounds[i].remainder;
- boundinfo->datums[i] = (Datum *) palloc(2 * sizeof(Datum));
+ boundinfo->datums[i] = &boundDatums[i * 2];
boundinfo->datums[i][0] = Int32GetDatum(modulus);
boundinfo->datums[i][1] = Int32GetDatum(remainder);
@@ -424,8 +424,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
remainder += modulus;
}
- (*mapping)[hbounds[i]->index] = i;
- pfree(hbounds[i]);
+ (*mapping)[hbounds[i].index] = i;
}
pfree(hbounds);
@@ -3693,8 +3692,8 @@ partition_hash_bsearch(PartitionBoundInfo boundinfo,
static int32
qsort_partition_hbound_cmp(const void *a, const void *b)
{
- PartitionHashBound *h1 = (*(PartitionHashBound *const *) a);
- PartitionHashBound *h2 = (*(PartitionHashBound *const *) b);
+ PartitionHashBound *const h1 = (PartitionHashBound *const) a;
+ PartitionHashBound *const h2 = (PartitionHashBound *const) b;
return partition_hbound_cmp(h1->modulus, h1->remainder,
h2->modulus, h2->remainder);
--
2.30.2
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-07-05 13:48 ` Re: Removed extra memory allocations from create_list_bounds David Rowley <dgrowleyml@gmail.com>
@ 2021-07-05 16:45 ` Justin Pryzby <pryzby@telsasoft.com>
2021-07-06 00:26 ` Re: Removed extra memory allocations from create_list_bounds David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Justin Pryzby @ 2021-07-05 16:45 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers@lists.postgresql.org
On Tue, Jul 06, 2021 at 01:48:52AM +1200, David Rowley wrote:
> On Wed, 19 May 2021 at 05:28, Nitin Jadhav <nitinjadhavpostgres@gmail.com> wrote:
> > I have rebased all the patches on top of
> > 'v2_0001-removed_extra_mem_alloc_from_create_list_bounds.patch'.
> > Attaching all the patches here.
>
> I had a look over these and I think what's being done here is fine.
Thanks for loooking.
0001 is missing a newline before create_list_bounds()
0003 is missing pfree(all_bounds), which I had as 0005.
It 1) allocates all_bounds; 2) allocates rbounds; 3) copies all_bounds into
rbounds; 4) allocates boundDatums; 5) copies rbounds into boundDatums; 6) frees
rbounds; 7) returns boundInfo with boundinfo->datums.
> The good news is, we can just give partition_bounds_copy() the same
> treatment. 0004 does that.
+1
> I've attached another set of patches. I squashed all the changes to
> each create_*_bounds function into a patch of their own. Perhaps 0002
> and 0003 which handle range and hash partitioning can be one patch
> since Justin seemed to write that one. I kept 0001 separate since
> that's Nitin's patch plus Justin's extra parts. It seems easier to
> credit properly having the patches broken out like this. I think it's
> excessive to break down 0001 into Nitin and Justin's individual parts.
If you wanted to further squish the patches together, I don't mind being a
co-author.
Cheers,
--
Justin
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-07-05 13:48 ` Re: Removed extra memory allocations from create_list_bounds David Rowley <dgrowleyml@gmail.com>
2021-07-05 16:45 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
@ 2021-07-06 00:26 ` David Rowley <dgrowleyml@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: David Rowley @ 2021-07-06 00:26 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Tue, 6 Jul 2021 at 04:45, Justin Pryzby <pryzby@telsasoft.com> wrote:
> If you wanted to further squish the patches together, I don't mind being a
> co-author.
Thanks for looking at the patches.
I fixed the couple of things that you mentioned and pushed all 4
patches as a single commit (53d86957e)
David
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-07-05 13:48 ` Re: Removed extra memory allocations from create_list_bounds David Rowley <dgrowleyml@gmail.com>
@ 2021-07-05 17:03 ` Justin Pryzby <pryzby@telsasoft.com>
2021-07-06 00:27 ` Re: Removed extra memory allocations from create_list_bounds David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 20+ messages in thread
From: Justin Pryzby @ 2021-07-05 17:03 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; pgsql-hackers@lists.postgresql.org
Also, if you're going to remove the initializations here, maybe you'd also
change i and j to C99 "for" declarations like "for (int i=0, j=0; ...)"
- PartitionListValue **all_values = NULL;
- ListCell *cell;
- int i = 0;
- int ndatums = 0;
+ PartitionListValue *all_values;
+ int i;
+ int j;
+ int ndatums;
Same in get_non_null_list_datum_count()
--
Justin
^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: Removed extra memory allocations from create_list_bounds
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Re: Removed extra memory allocations from create_list_bounds Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Re: Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-07-05 13:48 ` Re: Removed extra memory allocations from create_list_bounds David Rowley <dgrowleyml@gmail.com>
2021-07-05 17:03 ` Re: Removed extra memory allocations from create_list_bounds Justin Pryzby <pryzby@telsasoft.com>
@ 2021-07-06 00:27 ` David Rowley <dgrowleyml@gmail.com>
0 siblings, 0 replies; 20+ messages in thread
From: David Rowley @ 2021-07-06 00:27 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Nitin Jadhav <nitinjadhavpostgres@gmail.com>; Zhihong Yu <zyu@yugabyte.com>; PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Tue, 6 Jul 2021 at 05:03, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> Also, if you're going to remove the initializations here, maybe you'd also
> change i and j to C99 "for" declarations like "for (int i=0, j=0; ...)"
>
> - PartitionListValue **all_values = NULL;
> - ListCell *cell;
> - int i = 0;
> - int ndatums = 0;
> + PartitionListValue *all_values;
> + int i;
> + int j;
> + int ndatums;
>
> Same in get_non_null_list_datum_count()
I tend to only get motivated to use that for new code that does not
exist in back-branches. I'll maybe stop doing that when we no longer
have to support the pre-C99 versions of the code.
David
^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2021-07-06 00:27 UTC | newest]
Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-05-15 09:10 Removed extra memory allocations from create_list_bounds Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-16 17:00 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-16 17:26 ` Zhihong Yu <zyu@yugabyte.com>
2021-05-17 14:52 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-17 15:00 ` Zhihong Yu <zyu@yugabyte.com>
2021-05-17 15:03 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-18 17:28 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-18 17:49 ` Robert Haas <robertmhaas@gmail.com>
2021-05-19 18:51 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:10 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-19 19:16 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-23 17:10 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-23 17:14 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-05-23 17:46 ` Justin Pryzby <pryzby@telsasoft.com>
2021-05-24 14:42 ` Nitin Jadhav <nitinjadhavpostgres@gmail.com>
2021-07-05 13:48 ` David Rowley <dgrowleyml@gmail.com>
2021-07-05 16:45 ` Justin Pryzby <pryzby@telsasoft.com>
2021-07-06 00:26 ` David Rowley <dgrowleyml@gmail.com>
2021-07-05 17:03 ` Justin Pryzby <pryzby@telsasoft.com>
2021-07-06 00:27 ` David Rowley <dgrowleyml@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox