You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@age.apache.org by "jrgemignani (via GitHub)" <gi...@apache.org> on 2024/08/21 23:11:05 UTC
[PR] Fix memory leaks in functions part 1 [age]
jrgemignani opened a new pull request, #2066:
URL: https://github.com/apache/age/pull/2066
Fixing memory leaks in functions due to contexts not clearing out the memory soon enough.
Background: Destroying a context will free up its associated memory. However, before that happens, a process can still accumulate a lot of memory working on large data sets if it expects the context free to do it for it. For example, when PG does quicksort the context isn't destroyed until after the qs has finished. This can cause memory to be exhausted.
Put in more aggressive freeing of memory. However, there are a lot of areas that need this applied. So, this is part 1 of at least 3 or 4 parts.
Worked mainly on `agtype.c` However, dealt with linked functions in other files.
modified: src/backend/utils/adt/age_vle.c
modified: src/backend/utils/adt/agtype.c
modified: src/backend/utils/adt/agtype_util.c
modified: src/include/utils/agtype.h
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory leaks in functions part 1 [age]
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed merged PR #2066:
URL: https://github.com/apache/age/pull/2066
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory leaks in functions part 1 [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #2066:
URL: https://github.com/apache/age/pull/2066#discussion_r1728014523
##########
src/backend/utils/adt/agtype.c:
##########
@@ -2704,10 +2774,14 @@ Datum agtype_to_int8(PG_FUNCTION_ARGS)
if (temp->type != AGTV_ARRAY ||
!temp->val.array.raw_scalar)
{
+ int type = temp->type;
+
+ pfree_agtype_value(temp);
Review Comment:
`ereport(ERROR` frees memory, so cleanup isn't necessary here. Additionally, I have removed all of the cleanups preceding `ereport(ERROR`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory leaks in functions part 1 [age]
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed commented on code in PR #2066:
URL: https://github.com/apache/age/pull/2066#discussion_r1727155717
##########
src/backend/utils/adt/agtype.c:
##########
@@ -3324,8 +3416,11 @@ static agtype_value *execute_array_access_operator(agtype *array,
(errmsg("array index must resolve to an integer value")));
Review Comment:
array_index_value needs to be freed here as well?
##########
src/backend/utils/adt/agtype.c:
##########
@@ -2704,10 +2774,14 @@ Datum agtype_to_int8(PG_FUNCTION_ARGS)
if (temp->type != AGTV_ARRAY ||
!temp->val.array.raw_scalar)
{
+ int type = temp->type;
+
+ pfree_agtype_value(temp);
Review Comment:
Shouldn't agtv be freed here as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory leaks in functions part 1 [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #2066:
URL: https://github.com/apache/age/pull/2066#discussion_r1728014786
##########
src/backend/utils/adt/agtype.c:
##########
@@ -3324,8 +3416,11 @@ static agtype_value *execute_array_access_operator(agtype *array,
(errmsg("array index must resolve to an integer value")));
Review Comment:
`ereport(ERROR` frees memory, so cleanup isn't necessary here. Additionally, I have removed all of the cleanups preceding `ereport(ERROR`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory leaks in functions part 1 [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #2066:
URL: https://github.com/apache/age/pull/2066#discussion_r1729476467
##########
src/backend/utils/adt/agtype.c:
##########
@@ -3090,18 +3152,27 @@ Datum agtype_to_text(PG_FUNCTION_ARGS)
/* Return null if arg_agt is null. This covers SQL and Agtype NULLS */
if (arg_agt == NULL)
+ {
PG_RETURN_NULL();
+ }
/* check that we have a scalar value */
if (!AGT_ROOT_IS_SCALAR(arg_agt))
+ {
+ PG_FREE_IF_COPY(arg_agt, 0);
Review Comment:
Good catch, I thought that I removed all those. It is unnecessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory leaks in functions part 1 [age]
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed commented on code in PR #2066:
URL: https://github.com/apache/age/pull/2066#discussion_r1729171663
##########
src/backend/utils/adt/agtype.c:
##########
@@ -3968,8 +4093,14 @@ Datum agtype_access_operator(PG_FUNCTION_ARGS)
container = NULL;
}
+ pfree(args);
+ pfree(types);
+ pfree(nulls);
+
/* serialize and return the result */
- return AGTYPE_P_GET_DATUM(agtype_value_to_agtype(container_value));
+ result = agtype_value_to_agtype(container_value);
+
Review Comment:
`container_value` should be freed here?
##########
src/backend/utils/adt/agtype.c:
##########
@@ -3090,18 +3152,27 @@ Datum agtype_to_text(PG_FUNCTION_ARGS)
/* Return null if arg_agt is null. This covers SQL and Agtype NULLS */
if (arg_agt == NULL)
+ {
PG_RETURN_NULL();
+ }
/* check that we have a scalar value */
if (!AGT_ROOT_IS_SCALAR(arg_agt))
+ {
+ PG_FREE_IF_COPY(arg_agt, 0);
Review Comment:
We don't need this here, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix memory leaks in functions part 1 [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #2066:
URL: https://github.com/apache/age/pull/2066#discussion_r1729480535
##########
src/backend/utils/adt/agtype.c:
##########
@@ -3968,8 +4093,14 @@ Datum agtype_access_operator(PG_FUNCTION_ARGS)
container = NULL;
}
+ pfree(args);
+ pfree(types);
+ pfree(nulls);
+
/* serialize and return the result */
- return AGTYPE_P_GET_DATUM(agtype_value_to_agtype(container_value));
+ result = agtype_value_to_agtype(container_value);
+
Review Comment:
Unfortunately, due to how objects and arrays are iterated through, it can't be freed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org