Skip to content

Commit

Permalink
Fix issue 2046: Memory leak during btree(agtype) (#2060)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jrgemignani
jrgemignani committed Aug 22, 2024
1 parent 2bcd095 commit 048faba
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 27 deletions.
19 changes: 17 additions & 2 deletions src/backend/utils/adt/age_vle.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/*
Expand Down Expand Up @@ -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) ||
Expand All @@ -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);
Expand Down
50 changes: 39 additions & 11 deletions src/backend/utils/adt/agtype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -2436,14 +2448,18 @@ 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)
{
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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
38 changes: 35 additions & 3 deletions src/backend/utils/adt/agtype_util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
Expand All @@ -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)
Expand Down Expand Up @@ -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;

Expand All @@ -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:
Expand Down
17 changes: 7 additions & 10 deletions src/backend/utils/load/age_load.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -497,4 +494,4 @@ static int32 get_or_create_label(Oid graph_oid, char *graph_name,
}

return label_id;
}
}
1 change: 0 additions & 1 deletion src/include/utils/agtype.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit 048faba

Please sign in to comment.