Skip to content

Commit

Permalink
Fix Issue#1159: Server terminates for SET plus-equal (#1160)
Browse files Browse the repository at this point in the history 
The previous implementation of alter_properties() in
agtype.c while copying the original properties ignored
non-scalar value cases, this PR fixes that
  • Loading branch information
@Zainab-Saad
Zainab-Saad committed Sep 5, 2023
1 parent d9bc216 commit 5ceb961
Show file tree
Hide file tree
Showing 5 changed files with 181 additions and 11 deletions.
49 changes: 48 additions & 1 deletion regress/expected/cypher_set.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,11 @@ SELECT * FROM cypher('cypher_set_1', $$ CREATE (a:Robert {name:'Robert', role:'m
---
(0 rows)

SELECT * FROM cypher('cypher_set_1',$$ CREATE (a: VertexA {map: {a: 1, b: {c: 2, d: []}, c: [{d: -100, e: []}]}, list: [1, "string", [{a: []}, [[1, 2]]]], bool: true, num: -1.9::numeric, str: "string"})$$) as (a agtype);
a
---
(0 rows)

-- test copying properties between entities
SELECT * FROM cypher('cypher_set_1', $$
MATCH (at {name: 'Andy'}), (pn {name: 'Peter'})
Expand Down Expand Up @@ -869,6 +874,47 @@ $$) AS (p agtype);
{"id": 2251799813685249, "label": "Robert", "properties": {"age": 47, "city": "London", "name": "Rob"}}::vertex
(1 row)

-- test plus-equal with original properties having non-scalar values
SELECT * FROM cypher('cypher_set_1', $$
MATCH (p {map: {}})
SET p += {json: {a: -1, b: ['a', -1, true], c: {d: 'string'}}}
RETURN p
$$) AS (p agtype);
p
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
{"id": 2533274790395905, "label": "VertexA", "properties": {"map": {"a": 1, "b": {"c": 2, "d": []}, "c": [{"d": -100, "e": []}]}, "num": -1.9::numeric, "str": "string", "bool": true, "json": {"a": -1, "b": ["a", -1, true], "c": {"d": "string"}}, "list": [1, "string", [{"a": []}, [[1, 2]]]]}}::vertex
(1 row)

SELECT * FROM cypher('cypher_set_1', $$
MATCH (p: VertexA {map: {}})
SET p += {list_upd: [1, 2, 3, 4, 5]}
RETURN p
$$) AS (p agtype);
p
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
{"id": 2533274790395905, "label": "VertexA", "properties": {"map": {"a": 1, "b": {"c": 2, "d": []}, "c": [{"d": -100, "e": []}]}, "num": -1.9::numeric, "str": "string", "bool": true, "json": {"a": -1, "b": ["a", -1, true], "c": {"d": "string"}}, "list": [1, "string", [{"a": []}, [[1, 2]]]], "list_upd": [1, 2, 3, 4, 5]}}::vertex
(1 row)

SELECT * FROM cypher('cypher_set_1', $$
MATCH (p: VertexA)
SET p += {vertex: {id: 281474976710659, label: "", properties: {a: 1, b: [1, 2, 3]}}::vertex}
RETURN p
$$) AS (p agtype);
p
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
{"id": 2533274790395905, "label": "VertexA", "properties": {"map": {"a": 1, "b": {"c": 2, "d": []}, "c": [{"d": -100, "e": []}]}, "num": -1.9::numeric, "str": "string", "bool": true, "json": {"a": -1, "b": ["a", -1, true], "c": {"d": "string"}}, "list": [1, "string", [{"a": []}, [[1, 2]]]], "vertex": {"id": 281474976710659, "label": "", "properties": {"a": 1, "b": [1, 2, 3]}}::vertex, "list_upd": [1, 2, 3, 4, 5]}}::vertex
(1 row)

SELECT * FROM cypher('cypher_set_1', $$
MATCH (p {map: {}})
SET p += {}
RETURN p
$$) AS (p agtype);
p
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
{"id": 2533274790395905, "label": "VertexA", "properties": {"map": {"a": 1, "b": {"c": 2, "d": []}, "c": [{"d": -100, "e": []}]}, "num": -1.9::numeric, "str": "string", "bool": true, "json": {"a": -1, "b": ["a", -1, true], "c": {"d": "string"}}, "list": [1, "string", [{"a": []}, [[1, 2]]]], "vertex": {"id": 281474976710659, "label": "", "properties": {"a": 1, "b": [1, 2, 3]}}::vertex, "list_upd": [1, 2, 3, 4, 5]}}::vertex
(1 row)

--
-- Check passing mismatched types with SET
-- Issue 899
Expand Down Expand Up @@ -912,7 +958,7 @@ NOTICE: graph "cypher_set" has been dropped
(1 row)

SELECT drop_graph('cypher_set_1', true);
NOTICE: drop cascades to 8 other objects
NOTICE: drop cascades to 9 other objects
DETAIL: drop cascades to table cypher_set_1._ag_label_vertex
drop cascades to table cypher_set_1._ag_label_edge
drop cascades to table cypher_set_1."Andy"
Expand All @@ -921,6 +967,7 @@ drop cascades to table cypher_set_1."Kevin"
drop cascades to table cypher_set_1."Matt"
drop cascades to table cypher_set_1."Juan"
drop cascades to table cypher_set_1."Robert"
drop cascades to table cypher_set_1."VertexA"
NOTICE: graph "cypher_set_1" has been dropped
drop_graph
------------
Expand Down
26 changes: 26 additions & 0 deletions regress/sql/cypher_set.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ SELECT * FROM cypher('cypher_set_1', $$ CREATE (a:Kevin {name:'Kevin', age:32, h
SELECT * FROM cypher('cypher_set_1', $$ CREATE (a:Matt {name:'Matt', city:'Toronto'}) $$) AS (a agtype);
SELECT * FROM cypher('cypher_set_1', $$ CREATE (a:Juan {name:'Juan', role:'admin'}) $$) AS (a agtype);
SELECT * FROM cypher('cypher_set_1', $$ CREATE (a:Robert {name:'Robert', role:'manager', city:'London'}) $$) AS (a agtype);
SELECT * FROM cypher('cypher_set_1',$$ CREATE (a: VertexA {map: {a: 1, b: {c: 2, d: []}, c: [{d: -100, e: []}]}, list: [1, "string", [{a: []}, [[1, 2]]]], bool: true, num: -1.9::numeric, str: "string"})$$) as (a agtype);

-- test copying properties between entities
SELECT * FROM cypher('cypher_set_1', $$
Expand Down Expand Up @@ -316,6 +317,31 @@ SELECT * FROM cypher('cypher_set_1', $$
RETURN p
$$) AS (p agtype);

-- test plus-equal with original properties having non-scalar values
SELECT * FROM cypher('cypher_set_1', $$
MATCH (p {map: {}})
SET p += {json: {a: -1, b: ['a', -1, true], c: {d: 'string'}}}
RETURN p
$$) AS (p agtype);

SELECT * FROM cypher('cypher_set_1', $$
MATCH (p: VertexA {map: {}})
SET p += {list_upd: [1, 2, 3, 4, 5]}
RETURN p
$$) AS (p agtype);

SELECT * FROM cypher('cypher_set_1', $$
MATCH (p: VertexA)
SET p += {vertex: {id: 281474976710659, label: "", properties: {a: 1, b: [1, 2, 3]}}::vertex}
RETURN p
$$) AS (p agtype);

SELECT * FROM cypher('cypher_set_1', $$
MATCH (p {map: {}})
SET p += {}
RETURN p
$$) AS (p agtype);

--
-- Check passing mismatched types with SET
-- Issue 899
Expand Down
12 changes: 2 additions & 10 deletions src/backend/utils/adt/agtype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9241,22 +9241,14 @@ agtype_value *alter_properties(agtype_value *original_properties,
// Copy original properties.
if (original_properties)
{
int i;

if (original_properties->type != AGTV_OBJECT)
{
ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("a map is expected")));
}

for (i = 0; i < original_properties->val.object.num_pairs; i++)
{
agtype_pair* pair = original_properties->val.object.pairs + i;
parsed_agtype_value = push_agtype_value(&parse_state, WAGT_KEY,
&pair->key);
parsed_agtype_value = push_agtype_value(&parse_state, WAGT_VALUE,
&pair->value);
}
copy_agtype_value(parse_state, original_properties,
&parsed_agtype_value, true);
}

// Append new properties.
Expand Down
102 changes: 102 additions & 0 deletions src/backend/utils/adt/agtype_util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2385,3 +2385,105 @@ void pfree_agtype_in_state(agtype_in_state* value)
pfree_agtype_value(value->res);
free(value->parse_state);
}

/*
* helper function that recursively unpacks the agtype_value to be copied
* and pushes the scalar values into the copied agtype_value.
* this helps skip the serialization part at some places where the original
* properties passed to the function are in agtype_value format and
* converting it to agtype for iteration can be expensive.
* the caller of this function will need to push start and end object tokens
* on its own as this function might be used in places where pushing only start
* object token at top level is required (for example in alter_properties)
*/
void copy_agtype_value(agtype_parse_state* pstate,
agtype_value* original_agtype_value,
agtype_value **copied_agtype_value, bool is_top_level)
{
int i = 0;

/*
* guards against stack overflow due to deeply nested agtype_value
*/
check_stack_depth();

/*
* directly pass the agtype_value to be pushed into the copied result
* if type is scalar or binary (array or object) as push_agtype_value
* can unpack binary on its own
*/
if (IS_A_AGTYPE_SCALAR(original_agtype_value) ||
original_agtype_value->type == AGTV_BINARY)
{
*copied_agtype_value = push_agtype_value(&pstate, WAGT_ELEM,
original_agtype_value);
}
/*
* if the passed in type is object or array, unpack it
* until we are left with a scalar value to push to copied result
*/
else if (original_agtype_value->type == AGTV_OBJECT)
{
if (!is_top_level)
{
*copied_agtype_value = push_agtype_value(&pstate,
WAGT_BEGIN_OBJECT,
NULL);
}

for (; i < original_agtype_value->val.object.num_pairs; i ++)
{
agtype_pair *pair = original_agtype_value->val.object.pairs + i;
*copied_agtype_value = push_agtype_value(&pstate, WAGT_KEY,
&pair->key);

if (IS_A_AGTYPE_SCALAR(&pair->value))
{
*copied_agtype_value = push_agtype_value(&pstate, WAGT_VALUE,
&pair->value);
}
else
{
/* do a recursive call once a non-scalar value is reached */
copy_agtype_value(pstate, &pair->value, copied_agtype_value,
false);
}
}

if (!is_top_level)
{
*copied_agtype_value = push_agtype_value(&pstate, WAGT_END_OBJECT,
NULL);
}
}
else if (original_agtype_value->type == AGTV_ARRAY)
{
*copied_agtype_value = push_agtype_value(&pstate, WAGT_BEGIN_ARRAY,
NULL);

for (; i < original_agtype_value->val.array.num_elems; i++)
{
agtype_value elem = original_agtype_value->val.array.elems[i];

if (IS_A_AGTYPE_SCALAR(&elem))
{
*copied_agtype_value = push_agtype_value(&pstate, WAGT_ELEM,
&elem);
}
else
{
/* do a recursive call once a non-scalar value is reached */
copy_agtype_value(pstate, &elem, copied_agtype_value, false);
}
}

*copied_agtype_value = push_agtype_value(&pstate, WAGT_END_ARRAY,
NULL);
}
else
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid type provided for copy_agtype_value")));
}
}
3 changes: 3 additions & 0 deletions src/include/utils/agtype.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,9 @@ void convert_extended_object(StringInfo buffer, agtentry *pheader,
agtype_value *val);
Datum get_numeric_datum_from_agtype_value(agtype_value *agtv);
bool is_numeric_result(agtype_value *lhs, agtype_value *rhs);
void copy_agtype_value(agtype_parse_state* pstate,
agtype_value* original_agtype_value,
agtype_value **copied_agtype_value, bool is_top_level);

/* agtype.c support functions */
/*
Expand Down

0 comments on commit 5ceb961

Please sign in to comment.