This is an automated email from the ASF dual-hosted git repository.
mtaha pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/age.git
The following commit(s) were added to refs/heads/master by this push:
new 05487a08 Fix issue 2046: Memory leak during btree(agtype) (#2060)
05487a08 is described below
commit 05487a08f7b1985c5a815f55918905ede47454da
Author: John Gemignani <[email protected]>
AuthorDate: Mon Aug 19 10:35:37 2024 -0700
Fix issue 2046: Memory leak during btree(agtype) (#2060)
Fixed the memory leaks during a btree index build.
The issue has to do with how PostgreSQL allocates and frees its
memory. Usually, contexts are destroyed, freeing up the memory in
that context for the functions. However, sometimes this destruction
happens way too late for it to be helpful. In the case of btree
compare, the context destruction doesn't happen until after the sort
has completed. The solution is to add in pfrees to manage the memory
we use, ourselves.
Additionally, propagated these changes to a few other functions. More
PRs will be created to address other locations where this applies.
No regression tests were impacted.
No additionall regression tests are needed.
---
src/backend/utils/adt/age_vle.c | 19 +++++++++++++++++--
src/backend/utils/adt/agtype.c | 38 +++++++++++++++++++++++++++++--------
src/backend/utils/adt/agtype_util.c | 38 ++++++++++++++++++++++++++++++++++---
src/backend/utils/load/age_load.c | 17 +++++++----------
src/include/utils/agtype.h | 1 -
5 files changed, 89 insertions(+), 24 deletions(-)
diff --git a/src/backend/utils/adt/age_vle.c b/src/backend/utils/adt/age_vle.c
index e6bdec78..ed094e95 100644
--- a/src/backend/utils/adt/age_vle.c
+++ b/src/backend/utils/adt/age_vle.c
@@ -1878,8 +1878,19 @@ Datum age_vle(PG_FUNCTION_ARGS)
*/
agtype *agt_materialize_vle_path(agtype *agt_arg_vpc)
{
- /* convert the agtype_value to agtype and return it */
- return agtype_value_to_agtype(agtv_materialize_vle_path(agt_arg_vpc));
+ agtype *agt_path = NULL;
+ agtype_value *agtv_path = NULL;
+
+ /* get the path */
+ agtv_path = agtv_materialize_vle_path(agt_arg_vpc);
+
+ /* convert agtype_value to agtype */
+ agt_path = agtype_value_to_agtype(agtv_path);
+
+ /* free in memory path */
+ pfree_agtype_value(agtv_path);
+
+ return agt_path;
}
/*
@@ -1939,6 +1950,8 @@ Datum age_match_two_vle_edges(PG_FUNCTION_ARGS)
left_array_size = left_path->graphid_array_size;
left_array = GET_GRAPHID_ARRAY_FROM_CONTAINER(left_path);
+ PG_FREE_IF_COPY(agt_arg_vpc, 0);
+
agt_arg_vpc = AG_GET_ARG_AGTYPE_P(1);
if (!AGT_ROOT_IS_BINARY(agt_arg_vpc) ||
@@ -1953,6 +1966,8 @@ Datum age_match_two_vle_edges(PG_FUNCTION_ARGS)
right_path = (VLE_path_container *)agt_arg_vpc;
right_array = GET_GRAPHID_ARRAY_FROM_CONTAINER(right_path);
+ PG_FREE_IF_COPY(agt_arg_vpc, 1);
+
if (left_array[left_array_size - 1] != right_array[0])
{
PG_RETURN_BOOL(false);
diff --git a/src/backend/utils/adt/agtype.c b/src/backend/utils/adt/agtype.c
index f44d1750..2651a846 100644
--- a/src/backend/utils/adt/agtype.c
+++ b/src/backend/utils/adt/agtype.c
@@ -391,8 +391,10 @@ agtype_value *agtype_value_from_cstring(char *str, int len)
static inline Datum agtype_from_cstring(char *str, int len)
{
agtype_value *agtv = agtype_value_from_cstring(str, len);
+ agtype *agt = agtype_value_to_agtype(agtv);
- PG_RETURN_POINTER(agtype_value_to_agtype(agtv));
+ pfree_agtype_value(agtv);
+ PG_RETURN_POINTER(agt);
}
size_t check_string_length(size_t len)
@@ -1929,7 +1931,7 @@ agtype_value *string_to_agtype_value(char *s)
agtv->type = AGTV_STRING;
agtv->val.string.len = check_string_length(strlen(s));
- agtv->val.string.val = s;
+ agtv->val.string.val = pnstrdup(s, agtv->val.string.len);
return agtv;
}
@@ -1953,6 +1955,7 @@ PG_FUNCTION_INFO_V1(_agtype_build_path);
Datum _agtype_build_path(PG_FUNCTION_ARGS)
{
agtype_in_state result;
+ agtype *agt_result;
Datum *args = NULL;
bool *nulls = NULL;
Oid *types = NULL;
@@ -1995,8 +1998,10 @@ Datum _agtype_build_path(PG_FUNCTION_ARGS)
AGT_ROOT_BINARY_FLAGS(agt) == AGT_FBINARY_TYPE_VLE_PATH)
{
agtype *path = agt_materialize_vle_path(agt);
+ PG_FREE_IF_COPY(agt, i);
PG_RETURN_POINTER(path);
}
+ PG_FREE_IF_COPY(agt, i);
}
}
@@ -2046,6 +2051,8 @@ Datum _agtype_build_path(PG_FUNCTION_ARGS)
/* get the VLE path from the container as an agtype_value */
agtv_path = agtv_materialize_vle_path(agt);
+ PG_FREE_IF_COPY(agt, i);
+
/* it better be an AGTV_PATH */
Assert(agtv_path->type == AGTV_PATH);
@@ -2099,6 +2106,7 @@ Datum _agtype_build_path(PG_FUNCTION_ARGS)
{
add_agtype(AGTYPE_P_GET_DATUM(agt), false, &result, types[i],
false);
+ PG_FREE_IF_COPY(agt, i);
}
/* If we got here, we had a zero boundary case. So, clear it */
else
@@ -2113,7 +2121,11 @@ Datum _agtype_build_path(PG_FUNCTION_ARGS)
/* set it to a path type */
result.res->type = AGTV_PATH;
- PG_RETURN_POINTER(agtype_value_to_agtype(result.res));
+ agt_result = agtype_value_to_agtype(result.res);
+
+ pfree_agtype_in_state(&result);
+
+ PG_RETURN_POINTER(agt_result);
}
Datum make_path(List *path)
@@ -2437,6 +2449,7 @@ PG_FUNCTION_INFO_V1(agtype_build_map);
Datum agtype_build_map(PG_FUNCTION_ARGS)
{
agtype_value *result = NULL;
+ agtype *agt_result = NULL;
result = agtype_build_map_as_agtype_value(fcinfo);
if (result == NULL)
@@ -2444,7 +2457,10 @@ Datum agtype_build_map(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
- PG_RETURN_POINTER(agtype_value_to_agtype(result));
+ agt_result = agtype_value_to_agtype(result);
+ pfree_agtype_value(result);
+
+ PG_RETURN_POINTER(agt_result);
}
PG_FUNCTION_INFO_V1(agtype_build_map_noargs);
@@ -4264,7 +4280,7 @@ Datum agtype_in_operator(PG_FUNCTION_ARGS)
result = (compare_agtype_scalar_values(&agtv_item, &agtv_elem)
==
0);
}
- }
+ }
}
return boolean_to_agtype(result);
@@ -4428,6 +4444,7 @@ Datum agtype_btree_cmp(PG_FUNCTION_ARGS)
{
agtype *agtype_lhs;
agtype *agtype_rhs;
+ int32 result;
/* this function returns INTEGER which is 32bits */
if (PG_ARGISNULL(0) && PG_ARGISNULL(1))
@@ -4446,8 +4463,13 @@ Datum agtype_btree_cmp(PG_FUNCTION_ARGS)
agtype_lhs = AG_GET_ARG_AGTYPE_P(0);
agtype_rhs = AG_GET_ARG_AGTYPE_P(1);
- PG_RETURN_INT32(compare_agtype_containers_orderability(&agtype_lhs->root,
- &agtype_rhs->root));
+ result = compare_agtype_containers_orderability(&agtype_lhs->root,
+ &agtype_rhs->root);
+
+ PG_FREE_IF_COPY(agtype_lhs, 0);
+ PG_FREE_IF_COPY(agtype_rhs, 1);
+
+ PG_RETURN_INT32(result);
}
PG_FUNCTION_INFO_V1(agtype_typecast_numeric);
@@ -5598,7 +5620,7 @@ Datum age_tail(PG_FUNCTION_ARGS)
WAGT_END_ARRAY, NULL);
agt_result = agtype_value_to_agtype(agis_result.res);
- pfree_agtype_value(agis_result.res);
+ pfree_agtype_in_state(&agis_result);
PG_RETURN_POINTER(agt_result);
}
diff --git a/src/backend/utils/adt/agtype_util.c
b/src/backend/utils/adt/agtype_util.c
index 89aedeac..9982efc6 100644
--- a/src/backend/utils/adt/agtype_util.c
+++ b/src/backend/utils/adt/agtype_util.c
@@ -87,6 +87,9 @@ static agtype_value
*push_agtype_value_scalar(agtype_parse_state **pstate,
agtype_value *scalar_val);
static int compare_two_floats_orderability(float8 lhs, float8 rhs);
static int get_type_sort_priority(enum agtype_value_type type);
+static void pfree_agtype_value_content(agtype_value* value);
+static void pfree_iterator_agtype_value_token(agtype_iterator_token token,
+ agtype_value *agtv);
/*
* Turn an in-memory agtype_value into an agtype for on-disk storage.
@@ -234,6 +237,17 @@ static int get_type_sort_priority(enum agtype_value_type
type)
return -1;
}
+static void pfree_iterator_agtype_value_token(agtype_iterator_token token,
+ agtype_value *agtv)
+{
+ if (token == WAGT_KEY ||
+ token == WAGT_VALUE ||
+ token == WAGT_ELEM)
+ {
+ pfree_agtype_value_content(agtv);
+ }
+}
+
/*
* BT comparator worker function. Returns an integer less than, equal to, or
* greater than zero, indicating whether a is less than, equal to, or greater
@@ -269,6 +283,10 @@ int
compare_agtype_containers_orderability(agtype_container *a,
if (ra == WAGT_DONE)
{
/* Decisively equal */
+
+ /* free the agtype_values associated with the tokens */
+ pfree_iterator_agtype_value_token(ra, &va);
+ pfree_iterator_agtype_value_token(rb, &vb);
break;
}
@@ -280,6 +298,10 @@ int
compare_agtype_containers_orderability(agtype_container *a,
* initially, at the WAGT_BEGIN_ARRAY and WAGT_BEGIN_OBJECT
* tokens.
*/
+
+ /* free the agtype_values associated with the tokens */
+ pfree_iterator_agtype_value_token(ra, &va);
+ pfree_iterator_agtype_value_token(rb, &vb);
continue;
}
@@ -367,12 +389,18 @@ int
compare_agtype_containers_orderability(agtype_container *a,
if (ra == WAGT_END_ARRAY || ra == WAGT_END_OBJECT)
{
res = -1;
+ /* free the agtype_values associated with the tokens */
+ pfree_iterator_agtype_value_token(ra, &va);
+ pfree_iterator_agtype_value_token(rb, &vb);
break;
}
/* If right side is shorter, greater than */
if (rb == WAGT_END_ARRAY || rb == WAGT_END_OBJECT)
{
res = 1;
+ /* free the agtype_values associated with the tokens */
+ pfree_iterator_agtype_value_token(ra, &va);
+ pfree_iterator_agtype_value_token(rb, &vb);
break;
}
@@ -387,7 +415,7 @@ int compare_agtype_containers_orderability(agtype_container
*a,
{
rb = agtype_iterator_next(&itb, &vb, false);
}
-
+
Assert(va.type != vb.type);
Assert(va.type != AGTV_BINARY);
Assert(vb.type != AGTV_BINARY);
@@ -397,6 +425,9 @@ int compare_agtype_containers_orderability(agtype_container
*a,
-1 :
1;
}
+ /* free the agtype_values associated with the tokens */
+ pfree_iterator_agtype_value_token(ra, &va);
+ pfree_iterator_agtype_value_token(rb, &vb);
} while (res == 0);
while (ita != NULL)
@@ -2330,11 +2361,11 @@ void pfree_agtype_value(agtype_value* value)
}
/*
- * Helper function that recursively deallocates the contents
+ * Helper function that recursively deallocates the contents
* of the passed agtype_value only. It does not deallocate
* `value` itself.
*/
-void pfree_agtype_value_content(agtype_value* value)
+static void pfree_agtype_value_content(agtype_value* value)
{
int i;
@@ -2352,6 +2383,7 @@ void pfree_agtype_value_content(agtype_value* value)
* The char pointer (val.string.val) is not free'd because
* it is not allocated by an agtype helper function.
*/
+ pfree(value->val.string.val);
break;
case AGTV_ARRAY:
diff --git a/src/backend/utils/load/age_load.c
b/src/backend/utils/load/age_load.c
index 859116a8..815a53ba 100644
--- a/src/backend/utils/load/age_load.c
+++ b/src/backend/utils/load/age_load.c
@@ -114,9 +114,6 @@ agtype *create_agtype_from_list(char **header, char
**fields, size_t fields_len,
WAGT_VALUE,
value_agtype);
- pfree_agtype_value(key_agtype);
- pfree_agtype_value(value_agtype);
-
for (i = 0; i<fields_len; i++)
{
key_agtype = string_to_agtype_value(header[i]);
@@ -136,15 +133,15 @@ agtype *create_agtype_from_list(char **header, char
**fields, size_t fields_len,
result.res = push_agtype_value(&result.parse_state,
WAGT_VALUE,
value_agtype);
-
- pfree_agtype_value(key_agtype);
- pfree_agtype_value(value_agtype);
}
result.res = push_agtype_value(&result.parse_state,
WAGT_END_OBJECT, NULL);
+ /* serialize it */
out = agtype_value_to_agtype(result.res);
+
+ /* now that it is serialized we can free the in memory structure */
pfree_agtype_in_state(&result);
return out;
@@ -189,15 +186,15 @@ agtype* create_agtype_from_list_i(char **header, char
**fields,
result.res = push_agtype_value(&result.parse_state,
WAGT_VALUE,
value_agtype);
-
- pfree_agtype_value(key_agtype);
- pfree_agtype_value(value_agtype);
}
result.res = push_agtype_value(&result.parse_state,
WAGT_END_OBJECT, NULL);
+ /* serialize it */
out = agtype_value_to_agtype(result.res);
+
+ /* now that it is serialized we can free the in memory structure */
pfree_agtype_in_state(&result);
return out;
@@ -477,4 +474,4 @@ static int32 get_or_create_label(Oid graph_oid, char
*graph_name,
}
return label_id;
-}
\ No newline at end of file
+}
diff --git a/src/include/utils/agtype.h b/src/include/utils/agtype.h
index c5a2fe95..5905ba04 100644
--- a/src/include/utils/agtype.h
+++ b/src/include/utils/agtype.h
@@ -551,7 +551,6 @@ agtype_iterator *get_next_list_element(agtype_iterator *it,
agtype_container *agtc,
agtype_value *elem);
void pfree_agtype_value(agtype_value* value);
-void pfree_agtype_value_content(agtype_value* value);
void pfree_agtype_in_state(agtype_in_state* value);
agtype_value *agtype_value_from_cstring(char *str, int len);
