-
Notifications
You must be signed in to change notification settings - Fork 467
PG18 port for AGE #2251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PG18 port for AGE #2251
Conversation
|
@MuhammadTahaNaveed @kk-src I built this locally and it builds without errors. I have not reviewed the code, though. |
|
@kk-src It looks like this PR isn't up-to-date with this [PG18] branch and is based off the master branch. I would recommend rebasing it against this branch to be safe. Or, we use the rebase and merge option. @MuhammadTahaNaveed thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kk-src Looks good less minor changes requested.
Include executor/executor.h for PG18 header reorganization and use TupleDescAttr() accessor macro instead of direct attrs[] access. Note: Assisted by GitHub Copilot Agent mode.
- Add VarReturningType parameter to expandRTE() calls using VAR_RETURNING_DEFAULT. - Replace pg_attribute_noreturn() with pg_noreturn prefix specifier. Note: Assisted by GitHub Copilot Agent mode.
PG18 enforces stricter assertions in ExecOpenIndices, requiring ri_IndexRelationDescs to be NULL when called. In update_entity_tuple(), indices may already be opened by the caller (create_entity_result_rel_info), causing assertion failures. Add a check to only open indices if not already open, and track ownership with a boolean flag to ensure we only close what we opened. Found when regression tests failed with assertions, which this change resolves. Note: Assisted by GitHub Copilot Agent mode.
PG18's implementation changes result in different row ordering for queries without explicit ORDER BY clauses. Update expected output files to reflect the new ordering while maintaining identical result content. Note: Assisted by GitHub Copilot Agent mode.
jrgemignani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - approve.
@MuhammadTahaNaveed Still needs to approve it before merging it.
|
@kk-src @MuhammadTahaNaveed It also ran the issue the other PR had, and, without issue - |
|
The commits with signing has been updated as well. Please let me know next steps. |
Right now, the next steps are our reviews. |
MuhammadTahaNaveed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
|
@kk-src Thank you for your valuable contribution. |
* Updated CI, Labeler, Docker, and branch security files for PG18 (#2246) Updated the CI and Docker files for the PG18 Updated the labeler and branch security files for PG18. Some of these only apply to the master branch but are updated for consistency. modified: .asf.yaml modified: .github/labeler.yml modified: .github/workflows/go-driver.yml modified: .github/workflows/installcheck.yaml modified: .github/workflows/jdbc-driver.yaml modified: .github/workflows/nodejs-driver.yaml modified: .github/workflows/python-driver.yaml modified: docker/Dockerfile modified: docker/Dockerfile.dev modified: drivers/docker-compose.yml * PG18 port for AGE (#2251) * [PG18 port][Set1] Fix header dependencies and use TupleDescAttr macro - Include executor/executor.h for PG18 header reorganization and use TupleDescAttr() accessor macro instead of direct attrs[] access. * [PG18 port][Set2] Adapt to expandRTE signature change and pg_noreturn - Add VarReturningType parameter to expandRTE() calls using VAR_RETURNING_DEFAULT. - Replace pg_attribute_noreturn() with pg_noreturn prefix specifier. * [PG18 port][Set3] Fix double ExecOpenIndices call for PG18 compatibility - PG18 enforces stricter assertions in ExecOpenIndices, requiring ri_IndexRelationDescs to be NULL when called. - In update_entity_tuple(), indices may already be opened by the caller (create_entity_result_rel_info), causing assertion failures. - Add a check to only open indices if not already open, and track ownership with a boolean flag to ensure we only close what we opened. - Found when regression tests failed with assertions, which this change resolves. * [PG18 port][Set4] Update regression test expected output for ordering PG18's implementation changes result in different row ordering for queries without explicit ORDER BY clauses. Update expected output files to reflect the new ordering while maintaining identical result content. * [PG18 port][Set5] Address review comments - coding standard fix Note: Assisted by GitHub Copilot Agent mode. * Fix DockerHub build warning messages (#2252) PR fixes build warning messages on DockerHub and on my local build. No regression tests needed. modified: src/include/nodes/ag_nodes.h modified: src/include/optimizer/cypher_createplan.h modified: src/include/optimizer/cypher_pathnode.h modified: tools/gen_keywordlist.pl --------- Co-authored-by: Krishnakumar R (KK) <65895020+kk-src@users.noreply.github.com>
Summary:
This PR ports Apache AGE to work with PG18
[PG18 port][Set1] Fix header dependencies and use TupleDescAttr macro
[PG18 port][Set2] Adapt to expandRTE signature change and pg_noreturn
[PG18 port][Set3] Fix double ExecOpenIndices call for PG18 compatibility
[PG18 port][Set4] Update regression test expected output for ordering
Testing: