Copilot commented on code in PR #2351:
URL: https://github.com/apache/age/pull/2351#discussion_r2989842420
##########
src/backend/utils/adt/agtype.c:
##########
@@ -12020,6 +12065,38 @@ static int64 get_int64_from_int_datums(Datum d, Oid
type, char *funcname,
return result;
}
+/*
+ * Helper function to find a valid index for a specific attribute.
+ * Returns the OID of the index, or InvalidOid if none is found.
+ */
+Oid find_usable_index_for_attr(Relation rel, AttrNumber attnum)
+{
+List *index_list = RelationGetIndexList(rel);
+ ListCell *ilc;
+ Oid index_oid = InvalidOid;
+
+ foreach(ilc, index_list)
+ {
+ Oid curr_idx_oid = lfirst_oid(ilc);
+ Relation curr_idx_rel = index_open(curr_idx_oid, AccessShareLock);
+
+ if (curr_idx_rel->rd_index->indisvalid &&
+ curr_idx_rel->rd_index->indnatts >= 1 &&
+ curr_idx_rel->rd_index->indkey.values[0] == attnum)
Review Comment:
`find_usable_index_for_attr()` can select any valid index whose first key
matches `attnum`, but all call sites build scan keys with
`BTEqualStrategyNumber` (btree strategy numbering). If a non-btree index exists
on that column (or an index AM with different strategy semantics), the scan can
behave incorrectly or fail. Restrict the helper to only return btree indexes
(and optionally skip partial/expression indexes), or change the scan-key
construction to be compatible with the selected index AM/opclass.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -5646,19 +5649,52 @@ static Datum get_vertex(const char *graph, const char
*vertex_label,
aclcheck_error(aclresult, OBJECT_TABLE, vertex_label);
}
- /* initialize the scan key */
- ScanKeyInit(&scan_keys[0], 1, BTEqualStrategyNumber, F_OIDEQ,
- Int64GetDatum(graphid));
-
- /* open the relation (table), begin the scan, and get the tuple */
+ /* open the relation (table) */
graph_vertex_label = table_open(vertex_label_table_oid, ShareLock);
- scan_desc = table_beginscan(graph_vertex_label, snapshot, 1, scan_keys);
- tuple = heap_getnext(scan_desc, ForwardScanDirection);
+
+ index_oid = find_usable_index_for_attr(graph_vertex_label, 1);
+
+ if (OidIsValid(index_oid))
+ {
+ IndexScanDesc index_scan_desc;
+ Relation index_rel;
+
+ index_rel = index_open(index_oid, ShareLock);
+ slot = table_slot_create(graph_vertex_label, NULL);
+
+ /* initialize the scan key using GRAPHIDEQ for index */
+ ScanKeyInit(&scan_keys[0], 1, BTEqualStrategyNumber,
+ F_GRAPHIDEQ, Int64GetDatum(graphid));
+
+ index_scan_desc = index_beginscan(graph_vertex_label, index_rel,
+ snapshot, NULL, 1, 0);
+ index_rescan(index_scan_desc, scan_keys, 1, NULL, 0);
+
+ if (index_getnext_slot(index_scan_desc, ForwardScanDirection, slot))
+ {
+ tuple = ExecCopySlotHeapTuple(slot);
+ should_free_tuple = true;
+ }
+
+ index_endscan(index_scan_desc);
+ index_close(index_rel, ShareLock);
+ ExecDropSingleTupleTableSlot(slot);
+ }
+ else
+ {
+ /* fallback to sequential scan */
+ ScanKeyInit(&scan_keys[0], 1, BTEqualStrategyNumber, F_OIDEQ,
+ Int64GetDatum(graphid));
+
+ scan_desc = table_beginscan(graph_vertex_label, snapshot, 1,
scan_keys);
+ tuple = heap_getnext(scan_desc, ForwardScanDirection);
Review Comment:
The index path uses `F_GRAPHIDEQ` for the same attribute, but the seq-scan
fallback still uses `F_OIDEQ`. That makes correctness depend on whether an
index exists (and may be wrong if the column type/operator is `graphid`). Use
the same operator/function for both paths (e.g., `F_GRAPHIDEQ` with the
appropriate datum conversion) so results are consistent regardless of index
availability.
##########
src/backend/executor/cypher_set.c:
##########
@@ -628,60 +652,129 @@ static void process_update_list(CustomScanState *node)
if (luindex[update_item->entity_position - 1] == lidx)
{
- /*
- * Setup the scan key to require the id field on-disc to match the
- * entity's graphid.
- */
- ScanKeyInit(&scan_keys[0], 1, BTEqualStrategyNumber, F_GRAPHIDEQ,
- GRAPHID_GET_DATUM(id->val.int_value));
- /*
- * Setup the scan description, with the correct snapshot and scan
- * keys.
- */
- scan_desc = table_beginscan(resultRelInfo->ri_RelationDesc,
- estate->es_snapshot, 1, scan_keys);
- /* Retrieve the tuple. */
- heap_tuple = heap_getnext(scan_desc, ForwardScanDirection);
-
- /*
- * If the heap tuple still exists (It wasn't deleted between the
- * match and this SET/REMOVE) update the heap_tuple.
- */
- if (HeapTupleIsValid(heap_tuple))
- {
- bool should_update = true;
- Oid relid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
-
- /* Check RLS security quals (USING policy) before update */
- if (check_enable_rls(relid, InvalidOid, true) == RLS_ENABLED)
+ if (OidIsValid(index_oid))
+ {
+ Relation index_rel;
+ IndexScanDesc idx_scan_desc;
+ TupleTableSlot *index_slot;
+
+ index_rel = index_open(index_oid, RowExclusiveLock);
+
+ /*
+ * Setup the scan key to require the id field on-disc to match
the
+ * entity's graphid.
+ */
+ ScanKeyInit(&scan_keys[0], 1, BTEqualStrategyNumber,
F_GRAPHIDEQ,
+ GRAPHID_GET_DATUM(id->val.int_value));
+
+ index_slot = table_slot_create(rel, NULL);
+ idx_scan_desc = index_beginscan(rel, index_rel,
estate->es_snapshot, NULL, 1, 0);
+ index_rescan(idx_scan_desc, scan_keys, 1, NULL, 0);
+
+ if (index_getnext_slot(idx_scan_desc, ForwardScanDirection,
index_slot))
{
- RLSCacheEntry *entry;
+ bool shouldFree;
+
+ /* Retrieve the tuple from the slot */
+ heap_tuple = ExecFetchSlotHeapTuple(index_slot, true,
&shouldFree);
- /* Entry was already created earlier when setting up WCOs
*/
- entry = hash_search(qual_cache, &relid, HASH_FIND, NULL);
- if (!entry)
+ if (HeapTupleIsValid(heap_tuple))
{
- ereport(ERROR,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("missing RLS cache entry for relation
%u",
- relid)));
+ bool should_update = true;
+
+ /* Check RLS security quals (USING policy) before
update */
+ if (check_enable_rls(relid, InvalidOid, true) ==
RLS_ENABLED)
+ {
+ RLSCacheEntry *entry;
+
+ /* Entry was already created earlier when setting
up WCOs */
+ entry = hash_search(qual_cache, &relid, HASH_FIND,
NULL);
+ if (!entry)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("missing RLS cache entry for
relation %u",
+ relid)));
+ }
+
+ ExecStoreHeapTuple(heap_tuple, entry->slot, false);
+ should_update =
check_security_quals(entry->qualExprs,
+ entry->slot,
+ econtext);
+ }
+
+ /* Silently skip if USING policy filters out this row
*/
+ if (should_update)
+ {
+ update_entity_tuple(resultRelInfo, slot, estate,
+ heap_tuple);
Review Comment:
In the seq-scan path, the return value of `update_entity_tuple(...)` is
assigned back to `heap_tuple`, but in the index-scan path it is ignored. If
`update_entity_tuple` can return a different `HeapTuple` (or if callers rely on
the returned value for ownership/lifetime), this inconsistency can cause
incorrect behavior or memory/tuple lifecycle issues. Make the index-scan path
match the existing pattern (assign the return value and handle any required
freeing/ownership consistently).
```suggestion
heap_tuple = update_entity_tuple(resultRelInfo,
slot,
estate,
heap_tuple);
```
##########
src/backend/executor/cypher_utils.c:
##########
@@ -238,20 +240,47 @@ bool entity_exists(EState *estate, Oid graph_oid, graphid
id)
GetCurrentCommandId(false));
rel = table_open(label->relation, RowExclusiveLock);
- scan_desc = table_beginscan(rel, estate->es_snapshot, 1, scan_keys);
- tuple = heap_getnext(scan_desc, ForwardScanDirection);
+ index_oid = find_usable_index_for_attr(rel, 1);
- /*
- * If a single tuple was returned, the tuple is still valid, otherwise'
- * set to false.
- */
- if (!HeapTupleIsValid(tuple))
+ if (OidIsValid(index_oid))
{
- result = false;
+ IndexScanDesc index_scan_desc;
+ Relation index_rel;
+
+ slot = table_slot_create(rel, NULL);
+
+ index_rel = index_open(index_oid, RowExclusiveLock);
Review Comment:
`entity_exists()` is a read-only existence check, but it opens the index
with `RowExclusiveLock`. This lock mode is stronger than necessary for scanning
and can increase lock contention under concurrent workloads (especially since
`entity_exists` is in the hot insertion path). Prefer `AccessShareLock` for
`index_open` (while keeping the table lock as required by the surrounding
logic).
##########
src/backend/catalog/ag_label.c:
##########
@@ -297,49 +298,111 @@ List *get_all_edge_labels_per_graph(EState *estate, Oid
graph_oid)
HeapTuple tuple;
TupleTableSlot *slot;
ResultRelInfo *resultRelInfo;
-
- /* setup scan keys to get all edges for the given graph oid */
- ScanKeyInit(&scan_keys[1], Anum_ag_label_graph, BTEqualStrategyNumber,
- F_OIDEQ, ObjectIdGetDatum(graph_oid));
- ScanKeyInit(&scan_keys[0], Anum_ag_label_kind, BTEqualStrategyNumber,
- F_CHAREQ, CharGetDatum(LABEL_TYPE_EDGE));
+ Oid index_oid;
/* setup the table to be scanned */
- ag_label = table_open(ag_label_relation_id(), RowExclusiveLock);
- scan_desc = table_beginscan(ag_label, estate->es_snapshot, 2, scan_keys);
+ ag_label = table_open(ag_label_relation_id(), AccessShareLock);
+
+ index_oid = find_usable_index_for_attr(ag_label, Anum_ag_label_graph);
resultRelInfo = create_entity_result_rel_info(estate, "ag_catalog",
"ag_label");
Review Comment:
This function opens `ag_label` via `table_open(...)` but later closes
`resultRelInfo->ri_RelationDesc` instead of the `ag_label` relation it opened.
This double-open pattern is easy to get wrong and risks leaking a relation
reference/lock if the descriptors are not the same instance. Prefer using a
single relation handle throughout (e.g., use `resultRelInfo->ri_RelationDesc`
for scanning and close that, or explicitly close `ag_label` if it remains
separately opened).
```suggestion
resultRelInfo = create_entity_result_rel_info(estate, "ag_catalog",
"ag_label");
ag_label = resultRelInfo->ri_RelationDesc;
index_oid = find_usable_index_for_attr(ag_label, Anum_ag_label_graph);
```
##########
src/backend/utils/adt/age_global_graph.c:
##########
@@ -204,43 +204,117 @@ static List *get_ag_labels_names(Snapshot snapshot, Oid
graph_oid,
TableScanDesc scan_desc;
HeapTuple tuple;
TupleDesc tupdesc;
+ Oid index_oid = InvalidOid;
/* we need a valid snapshot */
Assert(snapshot != NULL);
- /* setup scan keys to get all edges for the given graph oid */
- ScanKeyInit(&scan_keys[1], Anum_ag_label_graph, BTEqualStrategyNumber,
- F_OIDEQ, ObjectIdGetDatum(graph_oid));
- ScanKeyInit(&scan_keys[0], Anum_ag_label_kind, BTEqualStrategyNumber,
- F_CHAREQ, CharGetDatum(label_type));
-
/* setup the table to be scanned, ag_label in this case */
ag_label = table_open(ag_label_relation_id(), AccessShareLock);
- scan_desc = table_beginscan(ag_label, snapshot, 2, scan_keys);
/* get the tupdesc - we don't need to release this one */
tupdesc = RelationGetDescr(ag_label);
/* bail if the number of columns differs - this table has 5 */
Assert(tupdesc->natts == Natts_ag_label);
- /* get all of the label names */
- while((tuple = heap_getnext(scan_desc, ForwardScanDirection)) != NULL)
+ /* We look for 'ag_label_graph_oid_index' or any index starting with
'graph' */
Review Comment:
The comment is inaccurate: the code does not look for an index by
name/prefix; it selects the first “usable” index whose first key matches the
attribute number. Update the comment to reflect the actual selection logic to
avoid misleading future maintainers.
```suggestion
/* Find a usable index whose first key column is ag_label.graph
(Anum_ag_label_graph) */
```
--
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]
