This is an automated email from the ASF dual-hosted git repository.
dehowef 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 5d9017b2 Fix memory leaks in MERGE (merge_edge & merge_vertex) (#958)
5d9017b2 is described below
commit 5d9017b216f2bbe77bb49f9faf2d82027cc9d233
Author: John Gemignani <[email protected]>
AuthorDate: Thu Jun 1 15:39:25 2023 -0700
Fix memory leaks in MERGE (merge_edge & merge_vertex) (#958)
Fixed memory leaks in MERGE. Specifically in the merge_edge and
merge_vertex functions. These functions will now throw error
messages rather than attempt to store values in the wrong locations.
Additionally, added some code to make it easier to debug the source
of an incorrect tuple position.
Fixed bad regression tests.
Updated code formatting.
---
regress/expected/cypher_merge.out | 15 +++--
regress/sql/cypher_merge.sql | 2 +-
src/backend/executor/cypher_merge.c | 119 +++++++++++++++++++++++-------------
src/backend/utils/adt/agtype.c | 38 +++++++-----
4 files changed, 112 insertions(+), 62 deletions(-)
diff --git a/regress/expected/cypher_merge.out
b/regress/expected/cypher_merge.out
index c4310ef6..9699794e 100644
--- a/regress/expected/cypher_merge.out
+++ b/regress/expected/cypher_merge.out
@@ -371,12 +371,19 @@ SELECT * FROM cypher('cypher_merge', $$MERGE
()-[:e]->()-[:e]->()$$) AS (a agtyp
--validate created correctly
--Returns 3. One for the data setup and 2 for the longer path in MERGE
-SELECT count(*) FROM cypher('cypher_merge', $$MATCH p=()-[e:e]->() RETURN p$$)
AS (p agtype)
+SELECT count(*) FROM cypher('cypher_merge', $$MATCH p=()-[e:e]->() RETURN p$$)
AS (p agtype);
+ count
+-------
+ 3
+(1 row)
+
-- Returns 1, the path created in MERGE
SELECT count(*) FROM cypher('cypher_merge', $$MATCH p=()-[:e]->()-[]->()
RETURN p$$) AS (p agtype);
-ERROR: syntax error at or near "SELECT"
-LINE 3: SELECT count(*) FROM cypher('cypher_merge', $$MATCH p=()-[:e...
- ^
+ count
+-------
+ 1
+(1 row)
+
-- 5 vertices total should have been created
SELECT count(*) FROM cypher('cypher_merge', $$MATCH (n) RETURN n$$) AS (n
agtype);
count
diff --git a/regress/sql/cypher_merge.sql b/regress/sql/cypher_merge.sql
index 9d04364f..8c127f63 100644
--- a/regress/sql/cypher_merge.sql
+++ b/regress/sql/cypher_merge.sql
@@ -204,7 +204,7 @@ SELECT * FROM cypher('cypher_merge', $$MERGE
()-[:e]->()-[:e]->()$$) AS (a agtyp
--validate created correctly
--Returns 3. One for the data setup and 2 for the longer path in MERGE
-SELECT count(*) FROM cypher('cypher_merge', $$MATCH p=()-[e:e]->() RETURN p$$)
AS (p agtype)
+SELECT count(*) FROM cypher('cypher_merge', $$MATCH p=()-[e:e]->() RETURN p$$)
AS (p agtype);
-- Returns 1, the path created in MERGE
SELECT count(*) FROM cypher('cypher_merge', $$MATCH p=()-[:e]->()-[]->()
RETURN p$$) AS (p agtype);
diff --git a/src/backend/executor/cypher_merge.c
b/src/backend/executor/cypher_merge.c
index 2502c977..b0247d01 100644
--- a/src/backend/executor/cypher_merge.c
+++ b/src/backend/executor/cypher_merge.c
@@ -60,14 +60,8 @@ const CustomExecMethods cypher_merge_exec_methods =
{MERGE_SCAN_STATE_NAME,
exec_cypher_merge,
end_cypher_merge,
rescan_cypher_merge,
- NULL,
- NULL,
- NULL,
- NULL,
- NULL,
- NULL,
- NULL,
- NULL};
+ NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL};
/*
* Initializes the MERGE Execution Node at the begginning of the execution
@@ -78,8 +72,8 @@ static void begin_cypher_merge(CustomScanState *node, EState
*estate,
{
cypher_merge_custom_scan_state *css =
(cypher_merge_custom_scan_state *)node;
- ListCell *lc;
- Plan *subplan;
+ ListCell *lc = NULL;
+ Plan *subplan = NULL;
Assert(list_length(css->cs->custom_plans) == 1);
@@ -87,6 +81,7 @@ static void begin_cypher_merge(CustomScanState *node, EState
*estate,
subplan = linitial(css->cs->custom_plans);
node->ss.ps.lefttree = ExecInitNode(subplan, estate, eflags);
+ /* TODO is this necessary? Removing it seems to not have an impact */
ExecAssignExprContext(estate, &node->ss.ps);
ExecInitScanTupleSlot(estate, &node->ss,
@@ -111,7 +106,7 @@ static void begin_cypher_merge(CustomScanState *node,
EState *estate,
{
cypher_target_node *cypher_node =
(cypher_target_node *)lfirst(lc);
- Relation rel;
+ Relation rel = NULL;
/*
* This entity is references an entity that is already declared. Either
@@ -149,8 +144,8 @@ static void begin_cypher_merge(CustomScanState *node,
EState *estate,
if (cypher_node->prop_expr != NULL)
{
- cypher_node->prop_expr_state =
- ExecInitExpr(cypher_node->prop_expr, (PlanState *)node);
+ cypher_node->prop_expr_state = ExecInitExpr(cypher_node->prop_expr,
+ (PlanState *)node);
}
}
@@ -162,7 +157,9 @@ static void begin_cypher_merge(CustomScanState *node,
EState *estate,
* that have modified the command id.
*/
if (estate->es_output_cid == 0)
+ {
estate->es_output_cid = estate->es_snapshot->curcid;
+ }
/* store the currentCommandId for this instance */
css->base_currentCommandId = GetCurrentCommandId(false);
@@ -179,7 +176,7 @@ static bool check_path(cypher_merge_custom_scan_state *css,
TupleTableSlot *slot)
{
cypher_create_path *path = css->path;
- ListCell *lc;
+ ListCell *lc = NULL;
foreach(lc, path->target_nodes)
{
@@ -249,7 +246,7 @@ static void process_simple_merge(CustomScanState *node)
cypher_merge_custom_scan_state *css =
(cypher_merge_custom_scan_state *)node;
EState *estate = css->css.ss.ps.state;
- TupleTableSlot *slot;
+ TupleTableSlot *slot = NULL;
/*Process the subtree first */
Decrement_Estate_CommandId(estate)
@@ -305,7 +302,7 @@ static TupleTableSlot *exec_cypher_merge(CustomScanState
*node)
(cypher_merge_custom_scan_state *)node;
EState *estate = css->css.ss.ps.state;
ExprContext *econtext = css->css.ss.ps.ps_ExprContext;
- TupleTableSlot *slot;
+ TupleTableSlot *slot = NULL;
bool terminal = CYPHER_CLAUSE_IS_TERMINAL(css->flags);
/*
@@ -462,7 +459,7 @@ static TupleTableSlot *exec_cypher_merge(CustomScanState
*node)
*/
ExprContext *econtext = node->ss.ps.ps_ExprContext;
SubqueryScanState *sss = (SubqueryScanState *)node->ss.ps.lefttree;
- HeapTuple heap_tuple;
+ HeapTuple heap_tuple = NULL;
/*
* Our child execution node is always a subquery. If not there
@@ -514,7 +511,8 @@ static TupleTableSlot *exec_cypher_merge(CustomScanState
*node)
econtext->ecxt_scantuple->tts_isnull);
// store the heap tuple
- ExecStoreTuple(heap_tuple, econtext->ecxt_scantuple,
InvalidBuffer, false);
+ ExecStoreTuple(heap_tuple, econtext->ecxt_scantuple, InvalidBuffer,
+ false);
/*
* make the subquery's projection scan slot be the tuple table we
@@ -545,7 +543,7 @@ static void end_cypher_merge(CustomScanState *node)
cypher_merge_custom_scan_state *css =
(cypher_merge_custom_scan_state *)node;
cypher_create_path *path = css->path;
- ListCell *lc;
+ ListCell *lc = NULL;
// increment the command counter
CommandCounterIncrement();
@@ -554,11 +552,12 @@ static void end_cypher_merge(CustomScanState *node)
foreach (lc, path->target_nodes)
{
- cypher_target_node *cypher_node =
- (cypher_target_node *)lfirst(lc);
+ cypher_target_node *cypher_node = (cypher_target_node *)lfirst(lc);
if (!CYPHER_TARGET_NODE_INSERT_ENTITY(cypher_node->flags))
+ {
continue;
+ }
// close all indices for the node
ExecCloseIndices(cypher_node->resultRelInfo);
@@ -576,9 +575,10 @@ static void end_cypher_merge(CustomScanState *node)
*/
static void rescan_cypher_merge(CustomScanState *node)
{
- ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cypher merge clause cannot be rescaned"),
- errhint("its unsafe to use joins in a query with a Cypher
MERGE clause")));
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cypher merge clause cannot be rescaned"),
+ errhint("its unsafe to use joins in a query with a Cypher
MERGE clause")));
}
/*
@@ -591,8 +591,8 @@ Node *create_cypher_merge_plan_state(CustomScan *cscan)
cypher_merge_custom_scan_state *cypher_css =
palloc0(sizeof(cypher_merge_custom_scan_state));
cypher_merge_information *merge_information;
- char *serialized_data;
- Const *c;
+ char *serialized_data = NULL;
+ Const *c = NULL;
cypher_css->cs = cscan;
@@ -643,6 +643,7 @@ static Datum merge_vertex(cypher_merge_custom_scan_state
*css,
{
ResultRelInfo *old_estate_es_result_relation_info = NULL;
Datum prop;
+
/*
* Set estate's result relation to the vertex's result
* relation.
@@ -723,8 +724,7 @@ static Datum merge_vertex(cypher_merge_custom_scan_state
*css,
Datum result;
/* make the vertex agtype */
- result = make_vertex(
- id, CStringGetDatum(node->label_name), prop);
+ result = make_vertex(id, CStringGetDatum(node->label_name), prop);
/* append to the path list */
if (CYPHER_TARGET_NODE_IN_PATH(node->flags))
@@ -739,19 +739,36 @@ static Datum merge_vertex(cypher_merge_custom_scan_state
*css,
*/
if (CYPHER_TARGET_NODE_IS_VARIABLE(node->flags))
{
- scanTupleSlot->tts_values[node->tuple_position - 1] = result;
- scanTupleSlot->tts_isnull[node->tuple_position - 1] = false;
+ bool debug = false;
+ int tuple_position = node->tuple_position - 1;
+
+ /*
+ * Generate an error message if the tuple position is
+ * out-of-bounds and allow for debugging.
+ */
+ if (!debug && (tuple_position >=
+ scanTupleSlot->tts_tupleDescriptor->natts))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("merge_vertex: invalid tuple position")));
+
+ }
+
+ /* store the result */
+ scanTupleSlot->tts_values[tuple_position] = result;
+ scanTupleSlot->tts_isnull[tuple_position] = false;
}
}
}
else
{
- agtype *a;
+ agtype *a = NULL;
Datum d;
- agtype_value *v;
- agtype_value *id_value;
- TupleTableSlot *scantuple;
- PlanState *ps;
+ agtype_value *v = NULL;
+ agtype_value *id_value = NULL;
+ TupleTableSlot *scantuple = NULL;
+ PlanState *ps = NULL;
ps = css->css.ss.ps.lefttree;
scantuple = ps->ps_ExprContext->ecxt_scantuple;
@@ -759,9 +776,9 @@ static Datum merge_vertex(cypher_merge_custom_scan_state
*css,
if (scantuple->tts_isnull[node->tuple_position - 1])
{
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("Existing variable %s cannot be NULL in MERGE clause",
- node->variable_name)));
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("Existing variable %s cannot be NULL in MERGE
clause",
+ node->variable_name)));
}
/* get the vertex agtype in the scanTupleSlot */
@@ -772,9 +789,11 @@ static Datum merge_vertex(cypher_merge_custom_scan_state
*css,
v = get_ith_agtype_value_from_container(&a->root, 0);
if (v->type != AGTV_VERTEX)
+ {
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("agtype must resolve to a vertex")));
+ }
/* extract the id agtype field */
id_value = GET_AGTYPE_VALUE_OBJECT_VALUE(v, "id");
@@ -922,8 +941,8 @@ static void merge_edge(cypher_merge_custom_scan_state *css,
{
Datum result;
- result = make_edge(
- id, start_id, end_id, CStringGetDatum(node->label_name), prop);
+ result = make_edge(id, start_id, end_id,
+ CStringGetDatum(node->label_name), prop);
// add the Datum to the list of entities for creating the path variable
if (CYPHER_TARGET_NODE_IN_PATH(node->flags))
@@ -936,9 +955,25 @@ static void merge_edge(cypher_merge_custom_scan_state *css,
if (CYPHER_TARGET_NODE_IS_VARIABLE(node->flags))
{
TupleTableSlot *scantuple = econtext->ecxt_scantuple;
+ bool debug = false;
+ int tuple_position = node->tuple_position - 1;
+
+ /*
+ * Generate an error message if the tuple position is
+ * out-of-bounds and allow for debugging.
+ */
+ if (!debug && (tuple_position >=
+ scantuple->tts_tupleDescriptor->natts))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("merge_edge: invalid tuple position")));
+
+ }
- scantuple->tts_values[node->tuple_position - 1] = result;
- scantuple->tts_isnull[node->tuple_position - 1] = false;
+ /* store the result */
+ scantuple->tts_values[tuple_position] = result;
+ scantuple->tts_isnull[tuple_position] = false;
}
}
}
diff --git a/src/backend/utils/adt/agtype.c b/src/backend/utils/adt/agtype.c
index 898f3d8a..6559d780 100644
--- a/src/backend/utils/adt/agtype.c
+++ b/src/backend/utils/adt/agtype.c
@@ -2179,9 +2179,11 @@ Datum _agtype_build_vertex(PG_FUNCTION_ARGS)
string_to_agtype_value("id"));
if (fcinfo->argnull[0])
+ {
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("_agtype_build_vertex() graphid cannot be NULL")));
+ }
id = AG_GETARG_GRAPHID(0);
result.res = push_agtype_value(&result.parse_state, WAGT_VALUE,
@@ -2192,8 +2194,10 @@ Datum _agtype_build_vertex(PG_FUNCTION_ARGS)
string_to_agtype_value("label"));
if (fcinfo->argnull[1])
+ {
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("_agtype_build_vertex() label cannot be
NULL")));
+ }
result.res =
push_agtype_value(&result.parse_state, WAGT_VALUE,
@@ -2216,11 +2220,11 @@ Datum _agtype_build_vertex(PG_FUNCTION_ARGS)
agtype *properties = AG_GET_ARG_AGTYPE_P(2);
if (!AGT_ROOT_IS_OBJECT(properties))
- ereport(
- ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg(
- "_agtype_build_vertex() properties argument must be an
object")));
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("_agtype_build_vertex() properties argument must
be an object")));
+ }
add_agtype((Datum)properties, false, &result, AGTYPEOID, false);
}
@@ -2234,11 +2238,7 @@ Datum _agtype_build_vertex(PG_FUNCTION_ARGS)
Datum make_vertex(Datum id, Datum label, Datum properties)
{
- return DirectFunctionCall3(_agtype_build_vertex,
- id,
- label,
- properties);
-
+ return DirectFunctionCall3(_agtype_build_vertex, id, label, properties);
}
PG_FUNCTION_INFO_V1(_agtype_build_edge);
@@ -2261,9 +2261,11 @@ Datum _agtype_build_edge(PG_FUNCTION_ARGS)
string_to_agtype_value("id"));
if (fcinfo->argnull[0])
+ {
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("_agtype_build_edge() graphid cannot be NULL")));
+ }
id = AG_GETARG_GRAPHID(0);
result.res = push_agtype_value(&result.parse_state, WAGT_VALUE,
@@ -2274,8 +2276,10 @@ Datum _agtype_build_edge(PG_FUNCTION_ARGS)
string_to_agtype_value("label"));
if (fcinfo->argnull[3])
+ {
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("_agtype_build_vertex() label cannot be
NULL")));
+ }
result.res =
push_agtype_value(&result.parse_state, WAGT_VALUE,
@@ -2286,9 +2290,11 @@ Datum _agtype_build_edge(PG_FUNCTION_ARGS)
string_to_agtype_value("end_id"));
if (fcinfo->argnull[2])
+ {
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("_agtype_build_edge() endid cannot be NULL")));
+ }
end_id = AG_GETARG_GRAPHID(2);
result.res = push_agtype_value(&result.parse_state, WAGT_VALUE,
@@ -2299,9 +2305,11 @@ Datum _agtype_build_edge(PG_FUNCTION_ARGS)
string_to_agtype_value("start_id"));
if (fcinfo->argnull[1])
+ {
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("_agtype_build_edge() startid cannot be NULL")));
+ }
start_id = AG_GETARG_GRAPHID(1);
result.res = push_agtype_value(&result.parse_state, WAGT_VALUE,
@@ -2324,11 +2332,11 @@ Datum _agtype_build_edge(PG_FUNCTION_ARGS)
agtype *properties = AG_GET_ARG_AGTYPE_P(4);
if (!AGT_ROOT_IS_OBJECT(properties))
- ereport(
- ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg(
- "_agtype_build_edge() properties argument must be an
object")));
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("_agtype_build_edge() properties argument must be
an object")));
+ }
add_agtype((Datum)properties, false, &result, AGTYPEOID, false);
}
