Copilot commented on code in PR #2375:
URL: https://github.com/apache/age/pull/2375#discussion_r3053362957
##########
regress/sql/index.sql:
##########
@@ -166,26 +164,8 @@ SELECT * FROM cypher('cypher_index', $$
(mx)<-[:has_city]-(:City {city_id: 10, name:"Tijuana", west_coast:
false, country_code:"MX"})
$$) as (n agtype);
-ALTER TABLE cypher_index."Country" ADD PRIMARY KEY (id);
-
-CREATE UNIQUE INDEX CONCURRENTLY cntry_id_idx ON cypher_index."Country" (id);
-ALTER TABLE cypher_index."Country" CLUSTER ON cntry_id_idx;
-
-ALTER TABLE cypher_index."City" ADD PRIMARY KEY (id);
-
-CREATE UNIQUE INDEX city_id_idx ON cypher_index."City" (id);
-
-ALTER TABLE cypher_index."City" CLUSTER ON city_id_idx;
-
-ALTER TABLE cypher_index.has_city
-ADD CONSTRAINT has_city_end_fk FOREIGN KEY (end_id)
-REFERENCES cypher_index."Country"(id) MATCH FULL;
-
-CREATE INDEX load_has_city_eid_idx ON cypher_index.has_city (end_id);
-
-CREATE INDEX load_has_city_sid_idx ON cypher_index.has_city (start_id);
-
-ALTER TABLE cypher_index."has_city" CLUSTER ON load_has_city_eid_idx;
+-- Verify that the incices are created on id columns
Review Comment:
Typo in comment: "incices" should be "indices".
```suggestion
-- Verify that the indices are created on id columns
```
##########
src/backend/utils/load/age_load.c:
##########
@@ -28,6 +42,153 @@ static agtype_value *csv_value_to_agtype_value(char
*csv_val);
static Oid get_or_create_graph(const Name graph_name);
static int32 get_or_create_label(Oid graph_oid, char *graph_name,
char *label_name, char label_kind);
+static char *build_safe_filename(char *name);
+static void check_file_read_permission(void);
+static void check_table_permissions(Oid relid);
+static void check_rls_for_load(Oid relid);
+
+#define AGE_BASE_CSV_DIRECTORY "/tmp/age/"
+#define AGE_CSV_FILE_EXTENSION ".csv"
+
+/*
+ * Trim leading and trailing whitespace from a string.
+ * Returns a newly allocated string with whitespace removed.
+ * Returns empty string for NULL input.
+ */
+char *trim_whitespace(const char *str)
+{
+ const char *start;
+ const char *end;
+ size_t len;
+
+ if (str == NULL)
+ {
+ return pstrdup("");
+ }
+
+ /* Find first non-whitespace character */
+ start = str;
+ while (*start && (*start == ' ' || *start == '\t' ||
+ *start == '\n' || *start == '\r'))
+ {
+ start++;
+ }
+
+ /* If string is all whitespace, return empty string */
+ if (*start == '\0')
+ {
+ return pstrdup("");
+ }
+
+ /* Find last non-whitespace character */
+ end = str + strlen(str) - 1;
+ while (end > start && (*end == ' ' || *end == '\t' ||
+ *end == '\n' || *end == '\r'))
+ {
+ end--;
+ }
+
+ /* Copy the trimmed string */
+ len = end - start + 1;
+ return pnstrdup(start, len);
+}
+
+static char *build_safe_filename(char *name)
+{
+ int length;
+ char path[PATH_MAX];
+ char *resolved;
+
+ if (name == NULL)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("file name cannot be NULL")));
+
+ }
+
+ length = strlen(name);
+
+ if (length == 0)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("file name cannot be zero length")));
+
+ }
+
+ snprintf(path, sizeof(path), "%s%s", AGE_BASE_CSV_DIRECTORY, name);
+
+ resolved = realpath(path, NULL);
+
+ if (resolved == NULL)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("File or path does not exist [%s]", path)));
+ }
+
+ if (strncmp(resolved, AGE_BASE_CSV_DIRECTORY,
+ strlen(AGE_BASE_CSV_DIRECTORY)) != 0)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("You can only load files located in [%s].",
+ AGE_BASE_CSV_DIRECTORY)));
+ }
+
+ length = strlen(resolved) - 4;
+ if (strncmp(resolved+length, AGE_CSV_FILE_EXTENSION,
+ strlen(AGE_CSV_FILE_EXTENSION)) != 0)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("You can only load files with extension [%s].",
+ AGE_CSV_FILE_EXTENSION)));
+ }
Review Comment:
`build_safe_filename()` uses `realpath(path, NULL)` (malloc) and then calls
`ereport(ERROR)` on sandbox/extension failures without freeing `resolved`.
Because `ereport(ERROR)` longjmps, this leaks the `realpath()` allocation for
each failing call. Free `resolved` before each error after it is set, or avoid
`malloc` here (e.g., use a Postgres helper that allocates in a memory context).
##########
regress/expected/index.out:
##########
@@ -264,18 +263,22 @@ $$) as (n agtype);
---
(0 rows)
-ALTER TABLE cypher_index."Country" ADD PRIMARY KEY (id);
-CREATE UNIQUE INDEX CONCURRENTLY cntry_id_idx ON cypher_index."Country" (id);
-ALTER TABLE cypher_index."Country" CLUSTER ON cntry_id_idx;
-ALTER TABLE cypher_index."City" ADD PRIMARY KEY (id);
-CREATE UNIQUE INDEX city_id_idx ON cypher_index."City" (id);
-ALTER TABLE cypher_index."City" CLUSTER ON city_id_idx;
-ALTER TABLE cypher_index.has_city
-ADD CONSTRAINT has_city_end_fk FOREIGN KEY (end_id)
-REFERENCES cypher_index."Country"(id) MATCH FULL;
-CREATE INDEX load_has_city_eid_idx ON cypher_index.has_city (end_id);
-CREATE INDEX load_has_city_sid_idx ON cypher_index.has_city (start_id);
-ALTER TABLE cypher_index."has_city" CLUSTER ON load_has_city_eid_idx;
+-- Verify that the incices are created on id columns
Review Comment:
Typo in comment: "incices" should be "indices".
```suggestion
-- Verify that the indices are created on id columns
```
##########
regress/expected/map_projection.out:
##########
@@ -152,7 +152,7 @@ $$
$$) as (a agtype);
a
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
- [{"name": "Christian Bale", "movies": [{"title": "The Prestige"}, {"title":
"The Dark Knight"}]}, {"name": "Tom Hanks", "movies": [{"title": "Forrest
Gump"}, {"title": "Finch"}, {"title": "The Circle"}]}]
+ [{"name": "Tom Hanks", "movies": [{"title": "Forrest Gump"}, {"title":
"Finch"}, {"title": "The Circle"}]}, {"name": "Christian Bale", "movies":
[{"title": "The Prestige"}, {"title": "The Dark Knight"}]}]
(1 row)
Review Comment:
This expected output update is reflecting a different row order from
`collect(...)` without any explicit ordering. Since result order is not
guaranteed without `ORDER BY`, this regression can flip again with
planner/index changes. Consider updating the underlying query (in
`regress/sql/map_projection.sql`) to impose deterministic ordering before
`collect` so the expected output stays stable.
##########
src/backend/utils/load/age_load.c:
##########
@@ -28,6 +42,153 @@ static agtype_value *csv_value_to_agtype_value(char
*csv_val);
static Oid get_or_create_graph(const Name graph_name);
static int32 get_or_create_label(Oid graph_oid, char *graph_name,
char *label_name, char label_kind);
+static char *build_safe_filename(char *name);
+static void check_file_read_permission(void);
+static void check_table_permissions(Oid relid);
+static void check_rls_for_load(Oid relid);
+
+#define AGE_BASE_CSV_DIRECTORY "/tmp/age/"
+#define AGE_CSV_FILE_EXTENSION ".csv"
+
+/*
+ * Trim leading and trailing whitespace from a string.
+ * Returns a newly allocated string with whitespace removed.
+ * Returns empty string for NULL input.
+ */
+char *trim_whitespace(const char *str)
+{
+ const char *start;
+ const char *end;
+ size_t len;
+
+ if (str == NULL)
+ {
+ return pstrdup("");
+ }
+
+ /* Find first non-whitespace character */
+ start = str;
+ while (*start && (*start == ' ' || *start == '\t' ||
+ *start == '\n' || *start == '\r'))
+ {
+ start++;
+ }
+
+ /* If string is all whitespace, return empty string */
+ if (*start == '\0')
+ {
+ return pstrdup("");
+ }
+
+ /* Find last non-whitespace character */
+ end = str + strlen(str) - 1;
+ while (end > start && (*end == ' ' || *end == '\t' ||
+ *end == '\n' || *end == '\r'))
+ {
+ end--;
+ }
+
+ /* Copy the trimmed string */
+ len = end - start + 1;
+ return pnstrdup(start, len);
+}
+
+static char *build_safe_filename(char *name)
+{
+ int length;
+ char path[PATH_MAX];
+ char *resolved;
+
+ if (name == NULL)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("file name cannot be NULL")));
+
+ }
+
+ length = strlen(name);
+
+ if (length == 0)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("file name cannot be zero length")));
+
+ }
+
+ snprintf(path, sizeof(path), "%s%s", AGE_BASE_CSV_DIRECTORY, name);
+
+ resolved = realpath(path, NULL);
+
+ if (resolved == NULL)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("File or path does not exist [%s]", path)));
+ }
+
+ if (strncmp(resolved, AGE_BASE_CSV_DIRECTORY,
+ strlen(AGE_BASE_CSV_DIRECTORY)) != 0)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("You can only load files located in [%s].",
+ AGE_BASE_CSV_DIRECTORY)));
+ }
+
+ length = strlen(resolved) - 4;
+ if (strncmp(resolved+length, AGE_CSV_FILE_EXTENSION,
+ strlen(AGE_CSV_FILE_EXTENSION)) != 0)
+ {
+ ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("You can only load files with extension [%s].",
+ AGE_CSV_FILE_EXTENSION)));
Review Comment:
The extension check computes `length = strlen(resolved) - 4` and then
dereferences `resolved + length`. If `resolved` is shorter than the extension
length (or if the extension length ever changes), this underflows/points before
the buffer. Guard with `strlen(resolved) >= strlen(AGE_CSV_FILE_EXTENSION)` and
compute the offset using `strlen(AGE_CSV_FILE_EXTENSION)` rather than a
hard-coded `4`.
##########
src/backend/utils/load/age_load.c:
##########
@@ -246,46 +476,88 @@ void insert_vertex_simple(Oid graph_oid, char
*label_name, graphid vertex_id,
if (get_label_kind(label_name, graph_oid) == LABEL_KIND_EDGE)
{
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("label %s already exists as edge label",
label_name)));
+ errmsg("label %s already exists as edge label",
+ label_name)));
}
- values[0] = GRAPHID_GET_DATUM(vertex_id);
- values[1] = AGTYPE_P_GET_DATUM((vertex_properties));
-
+ /* Open the relation */
label_relation = table_open(get_label_relation(label_name, graph_oid),
RowExclusiveLock);
+
+ /* Form the tuple */
+ values[0] = GRAPHID_GET_DATUM(vertex_id);
+ values[1] = AGTYPE_P_GET_DATUM((vertex_properties));
tuple = heap_form_tuple(RelationGetDescr(label_relation),
values, nulls);
- heap_insert(label_relation, tuple,
- GetCurrentCommandId(true), 0, NULL);
+
+ if (RelationGetForm(label_relation)->relhasindex)
+ {
+ /*
+ * CatalogTupleInsertWithInfo() is originally for PostgreSQL's
+ * catalog. However, it is used here for convenience.
+ */
+ CatalogIndexState indstate = CatalogOpenIndexes(label_relation);
+ CatalogTupleInsertWithInfo(label_relation, tuple, indstate);
+ CatalogCloseIndexes(indstate);
+ }
+ else
+ {
+ heap_insert(label_relation, tuple, GetCurrentCommandId(true),
+ 0, NULL);
+ }
+
+ /* Close the relation */
table_close(label_relation, RowExclusiveLock);
CommandCounterIncrement();
}
-void insert_batch(batch_insert_state *batch_state, char *label_name,
- Oid graph_oid)
+void insert_batch(batch_insert_state *batch_state)
{
- Relation label_relation;
- BulkInsertState bistate;
- Oid relid;
-
- // Get the relation OID
- relid = get_label_relation(label_name, graph_oid);
-
- // Open the relation
- label_relation = table_open(relid, RowExclusiveLock);
+ List *result;
+ int i;
- // Prepare the BulkInsertState
- bistate = GetBulkInsertState();
+ /* Check constraints for each tuple before inserting */
+ if (batch_state->resultRelInfo->ri_RelationDesc->rd_att->constr)
+ {
+ for (i = 0; i < batch_state->num_tuples; i++)
+ {
+ ExecConstraints(batch_state->resultRelInfo,
+ batch_state->slots[i],
+ batch_state->estate);
+ }
+ }
- // Perform the bulk insert
- heap_multi_insert(label_relation, batch_state->slots,
- batch_state->num_tuples, GetCurrentCommandId(true),
- 0, bistate);
+ /* Insert the tuples */
+ heap_multi_insert(batch_state->resultRelInfo->ri_RelationDesc,
+ batch_state->slots, batch_state->num_tuples,
+ GetCurrentCommandId(true),
+ TABLE_INSERT_SKIP_FSM, /* Skip free space map for bulk
*/
+ batch_state->bistate); /* Use bulk insert state */
- // Clean up
- FreeBulkInsertState(bistate);
- table_close(label_relation, RowExclusiveLock);
+ /* Insert index entries for the tuples */
+ if (batch_state->resultRelInfo->ri_NumIndices > 0)
+ {
+ for (i = 0; i < batch_state->num_tuples; i++)
+ {
+ result = ExecInsertIndexTuples(batch_state->resultRelInfo,
+ batch_state->slots[i],
+ batch_state->estate, false,
+ true, NULL, NIL, false);
+
+ /* Check if the unique constraint is violated */
+ if (list_length(result) != 0)
+ {
Review Comment:
`ExecInsertIndexTuples(..., false, true, NULL, NIL, false)` passes `true` as
the 5th argument, but other call sites in this repo pass `false`/`NULL` there.
In PostgreSQL, this parameter is a pointer (`bool *specConflict`), so passing
`true` would be treated as an invalid pointer and can crash. Pass `NULL` (or a
real `bool` variable address) consistently, and don't use the returned `List *`
length as a unique-violation signal—let the index AM raise the error or use the
proper conflict indicator if you intend to suppress it.
--
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]
