This is an automated email from the ASF dual-hosted git repository.
mtaha pushed a commit to branch PG13
in repository https://gitbox.apache.org/repos/asf/age.git
The following commit(s) were added to refs/heads/PG13 by this push:
new 1a88e86d Fix issue 2046: Memory leak during btree(agtype) (#2060)
(#2072)
1a88e86d is described below
commit 1a88e86d1e3ebe01c672b40e85a19c8e41f5a8a1
Author: John Gemignani <[email protected]>
AuthorDate: Fri Aug 23 08:16:20 2024 -0700
Fix issue 2046: Memory leak during btree(agtype) (#2060) (#2072)
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.
Resolved Conflicts:
src/backend/utils/adt/agtype.c
---
src/backend/utils/adt/age_vle.c | 19 ++++++++++++--
src/backend/utils/adt/agtype.c | 50 +++++++++++++++++++++++++++++--------
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, 98 insertions(+), 27 deletions(-)
diff --git a/src/backend/utils/adt/age_vle.c b/src/backend/utils/adt/age_vle.c
index 8fcd5d5b..40d04fb3 100644
--- a/src/backend/utils/adt/age_vle.c
+++ b/src/backend/utils/adt/age_vle.c
@@ -1879,8 +1879,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;
}
/*
@@ -1940,6 +1951,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) ||
@@ -1954,6 +1967,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 d1ab0122..97418472 100644
--- a/src/backend/utils/adt/agtype.c
+++ b/src/backend/utils/adt/agtype.c
@@ -390,8 +390,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)
@@ -1928,7 +1930,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;
}
@@ -1952,6 +1954,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;
@@ -1994,8 +1997,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);
}
}
@@ -2045,6 +2050,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);
@@ -2098,6 +2105,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
@@ -2112,7 +2120,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)
@@ -2436,6 +2448,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)
@@ -2443,7 +2456,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);
@@ -4256,7 +4272,7 @@ Datum agtype_in_operator(PG_FUNCTION_ARGS)
result = (compare_agtype_scalar_values(&agtv_item, &agtv_elem)
==
0);
}
- }
+ }
}
return boolean_to_agtype(result);
@@ -4417,19 +4433,31 @@ Datum agtype_btree_cmp(PG_FUNCTION_ARGS)
{
agtype *agtype_lhs;
agtype *agtype_rhs;
+ int32 result;
if (PG_ARGISNULL(0) && PG_ARGISNULL(1))
- PG_RETURN_INT16(0);
+ {
+ PG_RETURN_INT32(0);
+ }
else if (PG_ARGISNULL(0))
- PG_RETURN_INT16(1);
+ {
+ PG_RETURN_INT32(1);
+ }
else if (PG_ARGISNULL(1))
- PG_RETURN_INT16(-1);
+ {
+ PG_RETURN_INT32(-1);
+ }
agtype_lhs = AG_GET_ARG_AGTYPE_P(0);
agtype_rhs = AG_GET_ARG_AGTYPE_P(1);
- PG_RETURN_INT16(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);
@@ -5581,7 +5609,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 73f305df..3ac8eaba 100644
--- a/src/backend/utils/load/age_load.c
+++ b/src/backend/utils/load/age_load.c
@@ -134,9 +134,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]);
@@ -156,15 +153,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;
@@ -209,15 +206,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;
@@ -497,4 +494,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 96b2300e..9cb758ec 100644
--- a/src/include/utils/agtype.h
+++ b/src/include/utils/agtype.h
@@ -552,7 +552,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);
