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?compose=CllgCHrjDqKgWCBNMmLqhzKhmrvHhSRlRVZxPCVcLkLmFQwrccpTpqLNgbWqKkTkTFCHMtZjWnV
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
>From eccac01b63c983ba97859a4893e4115de19fda95 Mon Sep 17 00:00:00 2001
From: Nitin Jadhav <nitinjadhavpostg...@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
>From 0aeec1e5c9ddf2dea0045ed3ddbf0f7ae09d06a6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From 883cf386966d88b5dae9c91a165481189dbd1dde Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From f7eb3ee22ae86df2b1fec2e06727e77db5b4d12b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
>From 208ed37da054e7ffc82180bcca01134ac7ea7786 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@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
