Copilot commented on code in PR #2351:
URL: https://github.com/apache/age/pull/2351#discussion_r2879242495
##########
src/backend/executor/cypher_set.c:
##########
@@ -628,60 +656,100 @@ 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 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);
+ scan_desc = index_beginscan(rel, index_rel,
estate->es_snapshot, NULL, 1, 0);
+ index_rescan(scan_desc, scan_keys, 1, NULL, 0);
+
+ if (index_getnext_slot(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)));
+ heap_tuple = update_entity_tuple(resultRelInfo, slot,
estate, heap_tuple);
Review Comment:
The index-scan update path skips the RLS USING-policy check
(check_security_quals) that the sequential-scan path performs. This can allow
SET/REMOVE to update rows that should be filtered by RLS, and also changes
behavior (it should silently skip when filtered). Apply the same RLS gating
logic in the index-scan branch before calling update_entity_tuple().
```suggestion
/*
* Apply the same RLS USING-policy gating as in the
* sequential-scan path: if the tuple is filtered by
* RLS, silently skip the update.
*/
if (check_security_quals(estate, resultRelInfo,
heap_tuple))
heap_tuple = update_entity_tuple(resultRelInfo,
slot, estate, heap_tuple);
```
##########
src/backend/executor/cypher_set.c:
##########
@@ -628,60 +656,100 @@ 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 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);
+ scan_desc = index_beginscan(rel, index_rel,
estate->es_snapshot, NULL, 1, 0);
+ index_rescan(scan_desc, scan_keys, 1, NULL, 0);
+
+ if (index_getnext_slot(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)));
+ heap_tuple = update_entity_tuple(resultRelInfo, slot,
estate, heap_tuple);
}
- ExecStoreHeapTuple(heap_tuple, entry->slot, false);
- should_update = check_security_quals(entry->qualExprs,
- entry->slot,
- econtext);
+ if (shouldFree)
+ heap_freetuple(heap_tuple);
Review Comment:
In the index-scan branch, heap_tuple is overwritten with the HeapTuple
returned by update_entity_tuple(), but the code then calls
heap_freetuple(heap_tuple) based on shouldFree from
ExecFetchSlotHeapTuple(index_slot,...). This can free the wrong tuple (and
potentially double-free or corrupt memory). Keep the fetched tuple in a
separate variable and only free that one when shouldFree is true.
##########
src/backend/catalog/ag_label.c:
##########
@@ -297,46 +298,104 @@ List *get_all_edge_labels_per_graph(EState *estate, Oid
graph_oid)
HeapTuple tuple;
TupleTableSlot *slot;
ResultRelInfo *resultRelInfo;
+ Oid index_oid;
- /* 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));
+ index_oid = get_relname_relid("ag_label_graph_oid_index",
+ get_namespace_oid("ag_catalog", false));
/* 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);
resultRelInfo = create_entity_result_rel_info(estate, "ag_catalog",
"ag_label");
- slot = ExecInitExtraTupleSlot(
- estate, RelationGetDescr(resultRelInfo->ri_RelationDesc),
- &TTSOpsHeapTuple);
-
- /* scan through the results and get all the label names. */
- while(true)
+ if (OidIsValid(index_oid))
+ {
+ Relation index_rel;
+ IndexScanDesc index_scan_desc;
+
+ slot = ExecInitExtraTupleSlot(
+ estate, RelationGetDescr(resultRelInfo->ri_RelationDesc),
+ &TTSOpsBufferHeapTuple);
+
+ index_rel = index_open(index_oid, RowExclusiveLock);
+
+ ScanKeyInit(&scan_keys[0], Anum_ag_label_name, BTEqualStrategyNumber,
+ F_OIDEQ, ObjectIdGetDatum(graph_oid));
+
+ index_scan_desc = index_beginscan(ag_label, index_rel,
estate->es_snapshot, NULL, 1, 0);
+ index_rescan(index_scan_desc, scan_keys, 1, NULL, 0);
+
+ while (index_getnext_slot(index_scan_desc, ForwardScanDirection, slot))
+ {
+ Name label;
+ Name lval;
+ bool isNull;
+ Datum datum;
+ char kind;
+
+ /*There isn't field kind in index. So we should check it by hands*/
+ datum = slot_getattr(slot, Anum_ag_label_kind, &isNull);
+ if (isNull)
+ continue;
+
+ kind = DatumGetChar(datum);
+
+ if (kind != LABEL_TYPE_EDGE)
+ continue;
+
+ datum = slot_getattr(slot, Anum_ag_label_name, &isNull);
+ if (!isNull)
+ {
+ label = DatumGetName(datum);
+ lval = (Name) palloc(NAMEDATALEN);
+ namestrcpy(lval, NameStr(*label));
+ labels = lappend(labels, lval);
+ }
+ }
+
+ index_endscan(index_scan_desc);
+ index_close(index_rel, RowExclusiveLock);
+ } else
{
- Name label;
- bool isNull;
- Datum datum;
+ slot = ExecInitExtraTupleSlot(
+ estate, RelationGetDescr(resultRelInfo->ri_RelationDesc),
+ &TTSOpsHeapTuple);
- tuple = heap_getnext(scan_desc, ForwardScanDirection);
+ // 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));
Review Comment:
This file uses C++-style '//' comments in newly added code. The rest of the
backend C codebase follows C90/C-style block comments, and using '//' can break
builds under stricter C90 settings. Please convert these to /* ... */ comments
for consistency and portability.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -5646,19 +5649,74 @@ 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 = RelationGetPrimaryKeyIndex(graph_vertex_label, false);
+
+ if (!OidIsValid(index_oid))
+ {
+ List *idx_list = RelationGetIndexList(graph_vertex_label);
+ ListCell *lc;
+ foreach(lc, idx_list)
+ {
+ Oid curr = lfirst_oid(lc);
+ Relation idx_rel = index_open(curr, ShareLock);
+
+ if (idx_rel->rd_index->indisvalid &&
+ idx_rel->rd_index->indnatts >= 1 &&
+ idx_rel->rd_index->indkey.values[0] == 1)
+ {
+ index_oid = curr;
+ index_close(idx_rel, ShareLock);
+ break;
+ }
+ index_close(idx_rel, ShareLock);
+ }
+ list_free(idx_list);
+ }
+
+ 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));
Review Comment:
The sequential-scan fallback still uses F_OIDEQ to compare the graphid
column. Since the index path uses F_GRAPHIDEQ (and the column type is
GRAPHIDOID), the fallback should also use F_GRAPHIDEQ/GRAPHID_GET_DATUM to
avoid incorrect comparisons when no suitable index is found.
```suggestion
ScanKeyInit(&scan_keys[0], 1, BTEqualStrategyNumber, F_GRAPHIDEQ,
GRAPHID_GET_DATUM(graphid));
```
--
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]
