Skip to content
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

Pg16 to master #1454

Merged
merged 3 commits into from
Dec 27, 2023
Merged

Pg16 to master #1454

merged 3 commits into from
Dec 27, 2023

Conversation

jrgemignani
Copy link
Contributor

@jrgemignani jrgemignani commented Dec 21, 2023

When merging PG16 into the master, there were changes that needed to be addressed. This PR applies merges those changes back into PG16.

Additionally, this PR adjusts the CI scripts to be exclusively for the PG16 branch

jrgemignani and others added 2 commits December 12, 2023 15:22
Fixed the following Docker file to reflect running under
PostgreSQL version 15.

modified:   docker/Dockerfile.dev

This impacts the development DockerHub builds.
* Initial PG16 version (apache#1237)

Fixed empty string handling.

Previously, the PG 16 outToken emitted NULL for an empty string, but now it emits an empty string "". Consequently, our cypher read function required modification to properly decode the empty string as a plain token, thereby addressing the comparison issue.

Compared the branches and added the necessary changes so that the query's rteperminfos variable doesn't stay NULL.

Fix missing include varatt.h (causing undefined symbol VARDATA_ANY) & Fixing some test cases of the failings

Added missing include varatt.h to fix the undefined symbol while loading age into postgresql because usage of VARDATA_ANY needs to import varatt.h in PG16

Modified initialisation of ResultRelInfo and removed unnecessary RTEs

Compared the branches and added the necessary changes so that the query's rteperminfos variable doesn't stay NULL.

Modified initialisation of ResultRelInfo and removed unnecessary RTEs

One of the problems that we were facing was related to the ResultRelInfo pointing at the wrong RTE via its ri_RangeTableIndex. The create_entity_result_rel_info() function does not have the capability of setting the ri_RootResultRelInfo to the correct ResultRelInfo node because it does not have access to the ModifyTableState node. The solution for this was to set the third argument in InitResultRelInfo() to be zero instead of list_length(estate->es_range_table).

In the update_entity_tuple() function, when we call table_tuple_update() and assign the returned value to the result variable, the buffer variable receives the value of 0.
Made a workaround so that the original value isn't lost.

This is a work in progress for the new field that was added to the struct Var called varnullingrels. According to the documentation, this field is responsible for marking the Vars as nullable, if they are coming from a JOIN, either LEFT JOIN, RIGHT JOIN, or FULL OUTER JOIN. The changes were made following an "optional match" clause which is being treated as a LEFT JOIN from our extension.

A function markRelsAsNulledBy is added because its internal in Postgres and doesn't belong in a header file, therefore it can't be exported. This function is added before the creation of the Vars from the make_vertex_expr and make_edge_expr, to correctly mark the specific PNSI as nullable, so later in the planner stage, the Vars will be correctly nulled.

Fix incorrect typecasting in agtype_to_graphid function.

Fix incorrect returns to the fuction _label_name, _ag_build_vertex and _ag_build_edge.
Contributors

Panagiotis Foliadis <pfoliadis@hotmail.com>
Matheus Farias <matheusfarias519@gmail.com>
Mohamed Mokhtar <m.mokhtar@outlook.it>
Hannan Aamir <hannanaamir2000@gmail.com>
John Gemignani <jrgemignani@gmail.com>
Muhammad Taha Naveed <m.taha.naveed27@gmail.com>
Wendel de Lana <sr.wendelfernandes@outlook.com>
---------

* Fix Docker files to reflect PostgreSQL version 16 (apache#1448)

Fixed the following Docker files to reflect running under
PostgreSQL version 16.

    modified:   docker/Dockerfile
    modified:   docker/Dockerfile.dev

This impacts the DockerHub builds.

* Mark null-returning RTEs in outer joins as nullable

RTEs that appear in the right side of a left join are marked as 'nullable'.
The column references to a nullable RTE within the join's RTE are also
marked as 'nullable'. This concept is introduced in Postgresql v16.

The change in Postgresql v16's pullup_replace_vars_callback() function causes
different plans to be generated depending on whether appropriate RTEs are
marked nullable. Without marking nullable, in a left join, any function call
expression containing right RTE's columns as arguments are not evaluated at
the scan level, rather it is evaluated after the join is performed. At that
point, the function call may receive null input, which was unexpected in
previous Postgresql versions.

See:
----
  - Postgresql v16's commit: Make Vars be outer-join-aware
    https://www.postgresql.org/message-id/830269.1656693747@sss.pgh.pa.us

  - The 'Vars and PlaceHolderVars' section in the Postgresql v16's
    optimizer/README.md

* Fix minor code formatting.

---------

Co-authored-by: Shoaib <muhemmed.shoaib@gmail.com>
Co-authored-by: Rafsun Masud <rafsun82@gmail.com>
Copy link
Member

@rafsun42 rafsun42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thumbs up!

Copy link
Member

@dehowef dehowef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Member

@MuhammadTahaNaveed MuhammadTahaNaveed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍🏻

.github/workflows/go-driver.yml Outdated Show resolved Hide resolved
.github/workflows/jdbc-driver.yaml Outdated Show resolved Hide resolved
.github/workflows/nodejs-driver.yaml Outdated Show resolved Hide resolved
.github/workflows/python-driver.yaml Outdated Show resolved Hide resolved
Resolved conflicts:
    src/backend/catalog/ag_catalog.c
    src/backend/catalog/ag_label.c
    src/backend/executor/cypher_utils.c
    src/backend/nodes/cypher_readfuncs.c
    src/backend/parser/cypher_expr.c
    src/backend/parser/cypher_item.c
    src/backend/parser/cypher_parse_agg.c
    src/backend/utils/adt/agtype.c

Modified specifically for PG16 branch:
    .github/workflows/go-driver.yml
    .github/workflows/installcheck.yaml
    .github/workflows/jdbc-driver.yaml
    .github/workflows/nodejs-driver.yaml
    .github/workflows/python-driver.yaml
    README.md
@rafsun42 rafsun42 merged commit 127b367 into apache:PG16 Dec 27, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants