Add index scan

[sandy-bes]

Motivation / Problem:
As a result of load testing, a significant performance degradation was found in insertion scenarios. The scenarios used were taken from an open-source benchmark and rewritten in pure SQL. Examples of the queries can be found here:

1. https://github.com/ldbc/ldbc_snb_interactive_v1_impls/blob/main/cypher/queries/interactive-update-1.cypher
2. https://github.com/ldbc/ldbc_snb_interactive_v1_impls/blob/main/cypher/queries/interactive-update-6.cypher
3. https://github.com/ldbc/ldbc_snb_interactive_v1_impls/blob/main/cypher/queries/interactive-update-7.cypher

Perf analysis showed that the main bottleneck is the entity_exists function. The root cause lies in the use of a Sequential Scan (SeqScan) to check for the existence of an entity prior to insertion. The time complexity of a SeqScan is O(N), meaning the search time grows linearly as the number of rows in the table increases. The larger the graph became, the longer each individual insertion took. This led to a drop in TPS regardless of the concurrency level (the issue was consistently reproduced with both 1 and 30 threads).

Profile Analysis (Flame Graphs & perf): I am attaching Flame Graphs for comparison.

• Before Patch: The Flame Graph shows a massive "plateau" for the entity_exists function and its underlying sequential scan operations.

• After Patch: The "plateau" of entity_exists has disappeared, confirming that the bottleneck is resolved.

Changes Made:

• Added Index Scan (IndexScan / time complexity O(log N)) inside the entity_exists function.
• Refactored other functions utilizing SeqScan — they were also migrated to use IndexScan wherever applicable.

Performance Impact:
Benchmarks were conducted on a server with 30 CPU cores and 32 GB of RAM, using a graph ranging from 20,000 to 200,000 objects over a 2-minute duration. The transition to index access completely eliminated the performance degradation associated with data volume growth:

• Before: ~1,500 TPS (at peak, with subsequent degradation as the table grew).
• After: Stable ~15,000 TPS (a 10x speedup).

Here are the scripts and commands for local reproduction:

1. Graph creation and population:
The sf parameter here defines the scale factor of the generated graph.
psql -d your_database -f generate_graph.sql -v sf=1
generate_graph.sql

2. Creating wrapper functions for the workload:
This script sets up the environment and creates functions with the correct parameter mapping for Apache AGE.
psql -d your_database -f setup_func_for_workload.sql
setup_func_for_workload.sql

3. Running the load test:
Running 8 clients for 120 seconds.
pgbench -d your_database -f workload_update.sql -D sf=1 -c 8 -T 120 -P
workload_update.sql

Acknowledgments:

• Huge thanks to Daria Barsukova for conducting the load testing and isolating the issue.
• Implementation of index scanning: Alexandra Bondar.

[Copilot]

Pull request overview

This PR targets a load-test regression where insertion throughput degraded as label tables grew, by replacing O(N) sequential scans with index-backed lookups (primarily around entity_exists and other hot paths that check entity/label presence).

Changes:

• Switch entity_exists() to use an index scan when a suitable index is available (fallback to seq scan otherwise).
• Refactor several other table lookups (vertex fetch, label enumeration, SET/DELETE code paths) to prefer index scans.
• Add index discovery logic to locate a usable index when a PK index isn’t present.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/backend/utils/adt/agtype.c	Uses index scan to fetch a vertex tuple by graphid, with fallback to sequential scan.
src/backend/utils/adt/age_global_graph.c	Uses index scan to enumerate labels for a graph_oid, filtering kind in the heap.
src/backend/executor/cypher_utils.c	Reworks entity_exists() to prefer index scans over seq scans.
src/backend/executor/cypher_set.c	Uses index scan to find the tuple to update (SET/REMOVE) instead of seq scan.
src/backend/executor/cypher_delete.c	Uses index scan to find tuples for DELETE and to accelerate connected-edge checks where possible.
src/backend/catalog/ag_label.c	Uses ag_label_graph_oid_index to accelerate edge-label enumeration for a graph.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jrgemignani]

@sandy-bes Please address the above Copilot suggestions. Additionally, please make sure to note any AI usage, if used.

[jrgemignani]

@sandy-bes Please see this as well (Opus 4.6 review). All of these items need to be address.

Review of PR #2351 — Add index scan

The core idea — replacing O(N) sequential scans with O(log N) index scans in entity_exists and related hot paths — is excellent, and the performance numbers are compelling. However, there are several correctness, security, and code quality issues that need to be addressed before this can be merged.

Critical Issues

1. RLS bypass in cypher_set.c index-scan path (security bug)

The index-scan branch in process_update_list() calls update_entity_tuple() directly without checking check_security_quals. The seq-scan fallback correctly gates the update on RLS USING-policy evaluation. This means SET/REMOVE will bypass row-level security when an index is present. The same RLS gating logic must be applied in the index-scan branch.

2. heap_freetuple on wrong tuple in cypher_set.c (memory corruption)

heap_tuple = ExecFetchSlotHeapTuple(index_slot, true, &shouldFree);
// ...
heap_tuple = update_entity_tuple(resultRelInfo, slot, estate, heap_tuple);
// ...
if (shouldFree)
    heap_freetuple(heap_tuple);  // <-- frees the UPDATE result, not the fetched tuple

After update_entity_tuple(), heap_tuple points to the new tuple, but shouldFree applies to the originally fetched tuple. This can free the wrong memory. Keep the fetched tuple in a separate variable and only free that one.

Correctness Issues

3. F_OIDEQ vs F_GRAPHIDEQ mismatch in agtype.c fallback path

The index path correctly uses F_GRAPHIDEQ, but the seq-scan fallback at line ~5709 still uses F_OIDEQ. The column type is graphid, so both paths should use F_GRAPHIDEQ / GRAPHID_GET_DATUM.

4. Index attno vs heap attno confusion in ag_label.c

ScanKeyInit for the index scan uses Anum_ag_label_name as the attribute number. For index scans, the attno should be the index column number (typically 1), not a heap attribute-number macro. Additionally, the index is on graph_oid, so referencing Anum_ag_label_name is semantically incorrect — it only works by coincidence if the value happens to equal 1. Consider using ag_label_graph_oid_index_id() instead of get_relname_relid() string lookup as well.

Performance Issue

5. Index discovery runs per-tuple in hot paths

In process_delete_list() and process_update_list(), the index discovery block (RelationGetIndexList + open/close each index) runs inside the per-entity loop. For a DELETE or SET touching N entities of the same label, this runs N times. The index OID should be cached per-label or discovered once outside the inner loop.

Code Quality Issues

6. Massive code duplication

The index discovery pattern (iterate RelationGetIndexList, open each, check indisvalid/indnatts/indkey.values[0]) is copy-pasted 6+ times across the changed files. This should be factored into a shared utility function, e.g.:

Oid find_usable_index_for_attr(Relation rel, AttrNumber attnum);

7. DETACH DELETE duplication in cypher_delete.c

The two-pass connected-edge check (START_ID pass + END_ID pass) duplicates ~120 lines of nearly identical ACL check + RLS check + delete + error logic. A helper function would reduce this significantly.

8. C++ style comments

New code uses // comments in ag_label.c and elsewhere. The AGE/PostgreSQL codebase uses /* ... */ exclusively. Please convert for consistency and C90 portability.

9. Brace style inconsistency

The PR mixes } else { with the existing codebase convention of:

}
else
{

Please follow the existing style.

10. Inconsistent lock levels

Index open calls use RowExclusiveLock in some read-only scan paths (ag_label.c label enumeration, age_global_graph.c) where AccessShareLock would be more appropriate. The lock levels for index access during reads vs writes should be reviewed for consistency.

Summary

#	Issue	Severity
1	RLS bypass in SET index path	Critical
2	heap_freetuple on wrong tuple	High
3	F_OIDEQ vs F_GRAPHIDEQ mismatch	Medium
4	Index attno vs heap attno confusion	Medium
5	Index discovery per-tuple in loops	Medium
6	Index discovery code duplication (6x)	High
7	DETACH DELETE logic duplication	Medium
8	C++ style comments	Low
9	Brace style inconsistency	Low
10	Inconsistent lock levels	Medium

The performance improvement is real and valuable. With the security fix (#1), memory safety fix (#2), correctness fixes (#3-4), and a refactoring pass to deduplicate the index discovery logic (#5-7), this will be in good shape.

[sandy-bes]

I've updated the PR description with the steps to reproduce the tests locally.

[jrgemignani]

@sandy-bes

I've updated the PR description with the steps to reproduce the tests locally.

I was able to validate the perf claims with Opus earlier.

But, more importantly, I can't do anything with this PR until all of the above issues (Copilot & Opus) have been addressed. Including noting any use of AI. Btw, using AI is fine, it just needs to be noted.

[sandy-bes]

Yes, absolutely, I understand that the review process needs to be finalized first. I will do my best to address all the feedback over the next few days.

Regarding the use of AI agents: I did not use any AI agents while writing this code.

[sandy-bes]

All review comments and suggestions provided by GitHub Copilot and Opus 4.6 have been addressed and integrated into this patch.

[jrgemignani]

@sandy-bes A lot of regression tests failed. Are you running the installcheck locally to verify the build?

[sandy-bes]

I forgot to run installcheck for the latest change in the commit, my bad! It turned out there was a lock mismatch. I just fixed it, the tests are passing now, and I force-pushed the update.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jrgemignani]

@sandy-bes Sorry, you know the drill by now, more Copilot,... I keep hoping it stops complaining :( But, I think we're close.

[sandy-bes]

@jrgemignani No worries at all, I completely understand! As much as it complains, I have to admit Copilot actually gives some really good hints and catches subtle things.

[jrgemignani]

@sandy-bes I think if I keep having Copilot review it, it will never stop finding "something" to complain about. So, I think I'm done with it for this PR. Now I'll review the changes manually and go from there.

[jrgemignani]

@sandy-bes Okay, one small-ish ask. We used to not have this coding rule, so code exists that doesn't follow it. However, we try to enforce that all new code always uses braces {} even for one line of code. If you could make that change in your code, please :)