Cherry-pick of upstream PR #2117: Add index on id columns (#2117) - #1
Conversation
jasonyb
left a comment
jasonyb
left a comment
There was a problem hiding this comment.
Merge resolutions should be marked somewhere in the commit. I see cypher_subquery changes are dropped. Conflict appears to be due to lack of commit ffc9869. Either justify with logic why no changes are needed, or ideally run the tests to validate that what you have makes sense.
|
The cypher_subquery changes were intentionally not carried over. Commit ffc9869 was never backported to the PG15 branch, it only exists on master/PG17/PG18. The only cypher_subquery.out change in 5aed9ec was a trivial 2-line row-reorder in one coalesce query result, caused by the new index affecting plan ordering on master's test data (which includes the extra "Lucy" node from ffc9869). On PG15, that test data doesn't exist and the reorder doesn't apply, so the conflict was resolved by keeping PG15's version as-is. Have ran the Apache Age test suite locally and verified the expected output. |
jasonyb
left a comment
jasonyb
left a comment
There was a problem hiding this comment.
Documentation states
At the end, you should be n commits ahead of the commit in upstream_repositories.csv, where n is the number of commits you are point-importing, unless more new commits are authored.
I see two commits. They should be squashed to one. Documentation states
For the upstream repository, there should only be the cherry-picked commits. If there are any other commits, there should be a good reason for them, and the commit titles should start with YB:. Merge conflicts (including logical ones) should generally be resolved and amended into the same commit being cherry-picked. The Git author information should be preserved for cherry-picked commits.
The only cypher_subquery.out change in 5aed9ec was a trivial 2-line row-reorder in one coalesce query result, caused by the new index affecting plan ordering on master's test data (which includes the extra "Lucy" node from ffc9869). On PG15, that test data doesn't exist and the reorder doesn't apply, so the conflict was resolved by keeping PG15's version as-is.
Have ran the Apache Age test suite locally and verified the expected output.
This is great context that should be in the one local commit message.
Documentation states
if encountering any merge conflicts, put such details somewhere in the commit message
- Whenever a label will be created, indices on id columns will be created by default. In case of vertex, a unique index on id column will be created, which will also serve as a unique constraint. In case of edge, a non-unique index on start_id and end_id columns will be created. - This change is expected to improve the performance of queries that involve joins. From some performance tests, it was observed that the performance of queries improved alot. - Loader was updated to insert tuples in indices as well. This has caused to slow the loader down a bit, but it was necessary. - A bug related to command ids in cypher_delete executor was also fixed. (cherry picked from commit 5aed9ec) Merge conflict resolutions: - regress/expected/cypher_subquery.out: The only change in 5aed9ec was a trivial 2-line row-reorder in one coalesce query result, caused by the new index affecting plan ordering on master's test data (which includes the extra "Lucy" node from ffc9869). On PG15, that test data doesn't exist and the reorder doesn't apply, so the conflict was resolved by keeping PG15's version as-is. - src/backend/utils/load/ag_load_labels.c: Accepted incoming (upstream). The upstream commit removes vertex-specific batch functions (insert_vertex_batch, setup_temp_table_for_vertex_ids, insert_batch_in_temp_table, init_vertex_batch_insert, finish_vertex_batch_insert) in favor of shared batch functions defined in age_load.c. Ran the Apache Age test suite locally and verified the expected output.
c2367f7 to
a9dc043
Compare
Summary: Subtree point-import of upstream apache/age PR #2117 "Add index on id columns" into src/postgres/third-party-extensions/age. Upstream commit: 5aed9ecc5b492a47dc0e449421cf344de58b5edd Fork cherry-pick: a9dc043 (yugabyte/age yb-age-pg15) Fork PR: yugabyte/age#1 Changes from upstream: - Automatically create indexes on id columns when a label is created. For vertices, a unique index on the id column is created (also serves as a unique constraint). For edges, non-unique indexes on start_id and end_id columns are created. - Loader updated to insert tuples into indexes. - Bug fix for command ids in cypher_delete executor. Fork-side conflict resolutions (in a9dc043): - regress/expected/cypher_subquery.out: Kept PG15 baseline. The upstream change was a trivial 2-line row-reorder caused by the new index affecting plan ordering on master's test data (which includes the extra "Lucy" node from ffc9869). On PG15, that test data doesn't exist and the reorder doesn't apply. - src/backend/utils/load/ag_load_labels.c: Accepted incoming (upstream). Upstream removes vertex-specific batch functions in favor of shared batch functions defined in age_load.c. Subtree merge conflict resolutions (in yugabyte-db): - src/backend/utils/load/age_load.c: - insert_edge_simple(): Accepted upstream's relocation of table_open() and new index-aware insertion (CatalogTupleInsertWithInfo). Preserved YB-specific IsYBRelation early-return branch that uses yb_insert_edge_simple(). - insert_vertex_simple(): Accepted upstream's relocated "Form the tuple" values assignment. Preserved YB-specific IsYBRelation early-return branch that uses yb_insert_vertex_simple(). - insert_batch(): Accepted upstream entirely. Old code used label_name/graph_oid parameters with BulkInsertState; upstream rewrites to use batch_state->resultRelInfo with index insertion via ExecInsertIndexTuples. - src/backend/utils/load/ag_load_labels.c: Accepted incoming (upstream). Logical resolutions (YB-modified PG function signatures): - src/backend/commands/label_commands.c: - Changed IndexElem.collation from InvalidOid to NIL. YB changed the collation field type from Oid to List*. - src/backend/utils/load/age_load.c: - CatalogTupleInsertWithInfo(): Added missing fourth argument (false). YB's PG15 fork adds an extra bool parameter to this function. - ExecInsertIndexTuples(): Removed extra eighth argument. YB's PG15 fork has one fewer parameter than upstream PG expects. Subtree merge structure can be verified at: https://github.com/HarshDaryani896/yugabyte-db/tree/age-index-creation-v2 Test Plan: Jenkins Reviewers: jason Reviewed By: jason Subscribers: yql Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D52107
3 participants
Cherry-pick of upstream commit 5aed9ecc5b (apache/age PR #2117) onto the PG15 base fa1af8d.
This commit is not part of the latest PG15 release of Apache AGE, so a cherry-pick is necessary rather than a fast-forward.
Changes from upstream
Conflict Resolutions
regress/expected/cypher_subquery.out
The upstream commit only reorders existing rows on master (3 rows, id=977 for Charlie). PG15 has different test data — there is no "Lucy" person node, which means Charlie has only one outgoing edge instead of two. This results in a different baseline on PG15 (2 rows, id=976 for Charlie) compared to master (3 rows, id=977). The upstream change does not affect the PG15 row count or data here, only the ordering on master, so PG15's existing output is correct as-is.
src/backend/utils/load/ag_load_labels.c
The upstream commit removes vertex-specific batch functions (insert_vertex_batch, setup_temp_table_for_vertex_ids, insert_batch_in_temp_table, init_vertex_batch_insert, finish_vertex_batch_insert) in favor of shared batch functions defined in age_load.c.
Trailing newlines were preserved in ag_load_labels.c, ag_load_edges.c and age_load.c where the upstream commit inadvertently removed them.