jrgemignani commented on code in PR #1266:
URL: https://github.com/apache/age/pull/1266#discussion_r1353047900
##########
src/backend/executor/cypher_set.c:
##########
@@ -255,7 +255,7 @@ static agtype_value *replace_entity_in_path(agtype_value
*path,
agtype *prop_agtype;
int i;
- r = palloc(sizeof(agtype_value));
+ r = palloc0(sizeof(agtype_value));
Review Comment:
While it is generally good form to pre-initialize to zero, is it really
necessary if the routine that uses it, sets it up properly? In the case of
these iterator functions, which are generally slow, it takes up additional cpu
cycles. So, I'm not sure this is necessary. Can you justify adding this?
##########
src/backend/utils/load/ag_load_edges.c:
##########
@@ -160,7 +160,7 @@ int create_edges_from_csv_file(char *file_path,
struct csv_parser p;
char buf[1024];
size_t bytes_read;
- unsigned char options = 0;
+ unsigned char options = CSV_APPEND_NULL;
Review Comment:
Please elaborate why. This is not dismissing it, but inquiring why.
##########
src/backend/utils/load/ag_load_labels.c:
##########
@@ -194,7 +194,7 @@ int create_labels_from_csv_file(char *file_path,
struct csv_parser p;
char buf[1024];
size_t bytes_read;
- unsigned char options = 0;
+ unsigned char options = CSV_APPEND_NULL;
Review Comment:
Please elaborate why. This is not dismissing it, but inquiring why.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -9043,7 +9043,7 @@ agtype_value
*agtype_composite_to_agtype_value_binary(agtype *a)
errmsg("cannot convert agtype scalar objects to binary
agtype_value objects")));
}
- result = palloc(sizeof(agtype_value));
+ result = palloc0(sizeof(agtype_value));
Review Comment:
While it is generally good form to pre-initialize to zero, is it really
necessary if the routine that uses it, sets it up properly? In the case of
these iterator functions, which are generally slow, it takes up additional cpu
cycles. So, I'm not sure this is necessary. Can you justify adding this?
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -460,7 +460,7 @@ agtype_value
*find_agtype_value_from_container(agtype_container *container,
return NULL;
}
- result = palloc(sizeof(agtype_value));
+ result = palloc0(sizeof(agtype_value));
Review Comment:
Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm
just not sure that it is necessary.
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -554,7 +554,7 @@ agtype_value
*get_ith_agtype_value_from_container(agtype_container *container,
if (i >= nelements)
return NULL;
- result = palloc(sizeof(agtype_value));
+ result = palloc0(sizeof(agtype_value));
Review Comment:
Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm
just not sure that it is necessary.
##########
src/backend/utils/adt/agtype_ext.c:
##########
@@ -195,7 +195,7 @@ static void ag_deserialize_composite(char *base, enum
agtype_value_type type,
//offset container by the extended type header
char *container_base = base + AGT_HEADER_SIZE;
- r = palloc(sizeof(agtype_value));
+ r = palloc0(sizeof(agtype_value));
Review Comment:
While it is generally good form to pre-initialize to zero, is it really
necessary if the routine that uses it, sets it up properly? In the case of
these iterator functions, which are generally slow, it takes up additional cpu
cycles. So, I'm not sure this is necessary. Can you justify adding this?
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -727,7 +727,7 @@ static agtype_value
*push_agtype_value_scalar(agtype_parse_state **pstate,
(*pstate)->cont_val.val.object.num_pairs = 0;
(*pstate)->size = 4;
(*pstate)->cont_val.val.object.pairs =
- palloc(sizeof(agtype_pair) * (*pstate)->size);
+ palloc0(sizeof(agtype_pair) * (*pstate)->size);
Review Comment:
Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm
just not sure that it is necessary.
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1306,7 +1306,7 @@ bool agtype_deep_contains(agtype_iterator **val,
agtype_iterator **m_contained)
uint32 j = 0;
/* Make room for all possible values */
- lhs_conts = palloc(sizeof(agtype_value) * num_lhs_elems);
+ lhs_conts = palloc0(sizeof(agtype_value) * num_lhs_elems);
Review Comment:
Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm
just not sure that it is necessary.
##########
src/backend/parser/cypher_clause.c:
##########
@@ -6760,7 +6760,7 @@ static FuncExpr *make_clause_func_expr(char
*function_name,
*/
outNode(str, clause_information);
- clause_information_const = makeConst(INTERNALOID, -1, InvalidOid, str->len,
+ clause_information_const = makeConst(INTERNALOID, -1, InvalidOid, str->len
+ 1,
Review Comment:
Please justify why there needs to be an additional +1 to the length?
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -716,7 +716,7 @@ static agtype_value
*push_agtype_value_scalar(agtype_parse_state **pstate,
(*pstate)->size = 4;
}
(*pstate)->cont_val.val.array.elems =
- palloc(sizeof(agtype_value) * (*pstate)->size);
+ palloc0(sizeof(agtype_value) * (*pstate)->size);
Review Comment:
Similar to the above palloc0 comment. I'm not opposed to adding this in, I'm
just not sure that it is necessary.
##########
src/backend/utils/adt/agtype_ext.c:
##########
@@ -59,7 +59,7 @@ bool ag_serialize_extended_type(StringInfo buffer, agtentry
*agtentry,
/* copy in the int_value data */
numlen = sizeof(int64);
offset = reserve_from_buffer(buffer, numlen);
- *((int64 *)(buffer->data + offset)) = scalar_val->val.int_value;
+ memcpy( buffer->data + offset, &scalar_val->val.int_value, numlen);
Review Comment:
I don't see why this is necessary, maybe it is just me, could you please
elaborate?
##########
src/backend/utils/adt/agtype_ext.c:
##########
@@ -156,12 +156,12 @@ void ag_deserialize_extended_type(char *base_addr, uint32
offset,
{
case AGT_HEADER_INTEGER:
result->type = AGTV_INTEGER;
- result->val.int_value = *((int64 *)(base + AGT_HEADER_SIZE));
+ memcpy(&result->val.int_value, base + AGT_HEADER_SIZE, sizeof(int64));
Review Comment:
I don't see why this is necessary, maybe it is just me, could you please
elaborate?
##########
src/backend/utils/adt/agtype_ext.c:
##########
@@ -70,7 +70,7 @@ bool ag_serialize_extended_type(StringInfo buffer, agtentry
*agtentry,
/* copy in the float_value data */
numlen = sizeof(scalar_val->val.float_value);
offset = reserve_from_buffer(buffer, numlen);
- *((float8 *)(buffer->data + offset)) = scalar_val->val.float_value;
+ memcpy(buffer->data + offset, &scalar_val->val.int_value, numlen);
Review Comment:
I don't see why this is necessary, maybe it is just me, could you please
elaborate?
##########
src/backend/utils/adt/agtype_ext.c:
##########
@@ -156,12 +156,12 @@ void ag_deserialize_extended_type(char *base_addr, uint32
offset,
{
case AGT_HEADER_INTEGER:
result->type = AGTV_INTEGER;
- result->val.int_value = *((int64 *)(base + AGT_HEADER_SIZE));
+ memcpy(&result->val.int_value, base + AGT_HEADER_SIZE, sizeof(int64));
break;
case AGT_HEADER_FLOAT:
result->type = AGTV_FLOAT;
- result->val.float_value = *((float8 *)(base + AGT_HEADER_SIZE));
+ memcpy(&result->val.float_value, base + AGT_HEADER_SIZE,
sizeof(float8));
Review Comment:
I don't see why this is necessary, maybe it is just me, could you please
elaborate?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
