sandy-bes commented on code in PR #2351:
URL: https://github.com/apache/age/pull/2351#discussion_r3013699818
##########
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:
I had actually removed that assignment in a previous PR iteration to fix a
memory bug (calling heap_freetuple on the updated tuple instead of the fetched
one caused issues).
But I completely agree that leaving the return value unassigned is bad
practice and breaks consistency. I've refactored this block: we now store the
fetched tuple in a dedicated original_tuple variable for safe memory freeing at
the end, which allows us to correctly assign the update_entity_tuple result
back to heap_tuple. Fixed 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]
