From 4e01626e1707d0bacab5031583a8a2ae32d1da33 Mon Sep 17 00:00:00 2001 From: John Gemignani Date: Mon, 19 Aug 2024 10:35:37 -0700 Subject: [PATCH] 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. Resolved Conflicts: src/backend/utils/adt/agtype.c --- src/backend/utils/adt/age_vle.c | 19 +++++++++-- src/backend/utils/adt/agtype.c | 53 ++++++++++++++++++++++------- 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, 100 insertions(+), 28 deletions(-) diff --git a/src/backend/utils/adt/age_vle.c b/src/backend/utils/adt/age_vle.c index b3eb80f23..46f354231 100644 --- a/src/backend/utils/adt/age_vle.c +++ b/src/backend/utils/adt/age_vle.c @@ -1834,8 +1834,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; } /* @@ -1895,6 +1906,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) || @@ -1909,6 +1922,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 3d40578fc..da8ee9a99 100644 --- a/src/backend/utils/adt/agtype.c +++ b/src/backend/utils/adt/agtype.c @@ -389,8 +389,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) @@ -1925,7 +1927,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; } @@ -1949,6 +1951,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; @@ -1991,8 +1994,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); } } @@ -2042,6 +2047,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); @@ -2095,6 +2102,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 @@ -2109,7 +2117,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) @@ -2424,14 +2436,19 @@ PG_FUNCTION_INFO_V1(agtype_build_map); */ Datum agtype_build_map(PG_FUNCTION_ARGS) { - agtype_value *result = agtype_build_map_as_agtype_value(fcinfo); + agtype_value *result = NULL; + agtype *agt_result = NULL; + result = agtype_build_map_as_agtype_value(fcinfo); if (result == NULL) { 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); @@ -4239,7 +4256,7 @@ Datum agtype_in_operator(PG_FUNCTION_ARGS) result = (compare_agtype_scalar_values(&agtv_item, &agtv_elem) == 0); } - } + } } return boolean_to_agtype(result); @@ -4400,19 +4417,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); @@ -5561,7 +5590,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 eef916fdc..fbd4e36b8 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 fd90b6045..9b7406ea9 100644 --- a/src/backend/utils/load/age_load.c +++ b/src/backend/utils/load/age_load.c @@ -158,9 +158,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