-
Notifications
You must be signed in to change notification settings - Fork 467
Fixes #2164: Adds support for PostgreSQL 18 #2165
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
Fixes #2164: Adds support for PostgreSQL 18 #2165
Conversation
- A new node type is introduced for JSON support, that is JsonConstructorExpr - wrapper over FuncExpr/Aggref/WindowFunc for SQL/JSON constructors. - Added additional checks for JsonConstructorExpr expression node for which the walker would crash. - Removed palloc0fast function call (which is not available in PG17)
- Currently, all workflows are targeting the `PG17_prepare` branch, which will be changed to `PG17` once the branch is renamed. - Updated all the github workflows - Updated the README - Updated repo settings - Updated the Dockerfiles
c96fa43 to
ace7bf7
Compare
|
ace7bf7 to
7e3a642
Compare
|
@jrgemignani @MuhammadTahaNaveed I didn't get the option to request reviewers. I would greatly appreciate if you could review my code? Always happy to incorporate any improvements. Thanks! |
- Removed stale bot. (https://lists.apache.org/thread/qh4h2z6hsjy2v7wg8mwfnl6cbjp28y08) - Decrease required PR approvals by one. (https://lists.apache.org/thread/kmz155t6k0h3b26fjpz36924zthqjlpm) - Fixed a warning reported by apache infra i.e. "An error occurred while processing the github feature in .asf.yaml: GitHub discussions can only be enabled if a mailing list target exists for it."
Adjust workflow/labeler.yml to add permissions.
Adjusted the following CI files (workflows) for PG17, they originally pointed to PG17_prepare - 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: drivers/docker-compose.yml modified: .github/labeler.yml
|
@sanchayanghosh Sorry for the delay. This would need to be added to a branch specifically for PG18 support. |
|
@sanchayanghosh I have created PG18_prepare for such work. It will need to be adjusted a bit before it actually is ready, but it would be where you would apply this PR. |
@sanchayanghosh could you apply this against PG18_prepare ? |
|
@sanchayanghosh ty for the pr and sorry for the delay. I have changed the base branch to |
|
I removed the macro checks. Thanks to you both for reviews. I am sorry I was in middle of some other projects I couldn't check last month and got free this week onwards. |
1. In PostgreSQL 18, TupleDescAttr is now used to access ScanTuple->tts_tupleDescriptor->attrs since attrs is now replaced by compact_attrs to save on memory in PostgreSQL 18. In PostgreSQL 16, 17 and 18 we have a function TupleDescAttr which allows to acess pg_attrs 2. In PostgreSQL palloc0fast is now merged into palloc0. 3. Few funcitons now reuiqre executor/executor.h and are no longer present in the other includes.
15d8908 to
a66436a
Compare
…ly provide compatbility going forward from postgreSQL 16 onwards.
13fa2b4 to
02c372a
Compare
No worries. This is a volunteer project and we all have other things to do. |
I added Taha and myself as reviewers. |
|
@sanchayanghosh Regression tests fail and there's a crash in |
Same for me. How did you build this prior to submitting it? Did it work for you? |
|
@sanchayanghosh @MuhammadTahaNaveed Is this necessary to add to this PR? Could, should, it be its own PR?
|
|
@sanchayanghosh Any updates? |
|
Sorry for delay. Had a big project that took my time I will split the tests into its own PR and illoom into the crash once more |
@sanchayanghosh No worries. We did move this to an updated PG18 branch. You will need to rebase your work off of it. |
-- load 'age';
-- SET search_path = ag_catalog, "$user", public;
select * from create_graph('knowledge_graph');
SELECT
*
FROM
cypher ('knowledge_graph', $$
MERGE (n:TOPIC {id: '3tbr5xae-topic-ft1a', kg_id: '3tbr5xae'})
RETURN n$$) AS (n agtype);
PREPARE age_merge_topic (agtype) AS
SELECT * FROM cypher('knowledge_graph', $$
MATCH (n:TOPIC {id: $id, kg_id: $kg_id})
SET n.name = $name
RETURN n
$$, $1) AS (n agtype);
-- crash in postgres 18.1
EXECUTE age_merge_topic('{"id": "3tbr5xae-topic-ft1a","kg_id": "3tbr5xae", "name": "12"}'::agtype);
-- fine
PREPARE cypher_stored_procedure(agtype) AS
SELECT *
FROM cypher('knowledge_graph', $$
MATCH (v:Person)
WHERE v.name = $name
RETURN v
$$, $1)
AS (v agtype);
EXECUTE cypher_stored_procedure('{"name": "Tobias"}');
The above statements will cause the database to crash: Segmentation fault: 11, currently only found in the set operation. But this statement runs normally when using the Docker version of apache/age (Debian 17.7-3.pgdg13+, age 1.6.0). macbook pro with with m4 , macOS 26.1 |
|
@optionals Apache AGE PG18 isn't working with PostgreSQL 18 yet. |
I used the code from this PR, Isn't this PR just for adapting to PostgreSQL 18? I tried the branch "sanchayanghosh:feature/2164/provide-support-for-postgresql-16-18," and also tried merging it into the latest master code: 571c19 |
PG18 is still a test branch and this particular PR currently does not allow PG18 to build with Apache AGE. |
I understand. This issue was discovered in a local development environment. The comments above are simply to report that this PR has this problem. Before merging, it should be verified whether the issue indeed exists, and if so, a targeted fix will be necessary. In addition, I used this approach because the issue described in #1722 currently has no suitable solution. If a workaround or an updated fix becomes available, it would be greatly appreciated. After multiple attempts locally, I have not found any alternative besides fully manual SQL construction or using PREPARE as a workaround. |
You may want to try PR #2251. It is another PR to bring PG18 into the fold and it looks good so far. Your test could be valuable there as well. |
I tried #2251, the above statement works fine, thanks. (only the above scenarios have been tested.) |
|
The other PR solved the problem and that solution was more complete than mine. I will close this PR until further issue. |
|
@sanchayanghosh Regardless, I want to thank you for your efforts! |
Fixes #2164