Pg16 to master (#1454)

* Fix Docker file to reflect PostgreSQL version 15 (#1449) Fixed the following Docker file to reflect running under PostgreSQL version 15. modified: docker/Dockerfile.dev This impacts the development DockerHub builds. * Master to PostgreSQL version 16 (#1451) * Initial PG16 version (#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 (#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> --------- Co-authored-by: Shoaib <muhemmed.shoaib@gmail.com> Co-authored-by: Rafsun Masud <rafsun82@gmail.com>
apache · Dec 27, 2023 · 127b367 · 127b367
1 parent 8e56db9
commit 127b367
Show file tree

Hide file tree

Showing 16 changed files with 197 additions and 112 deletions.
diff --git a/.github/workflows/go-driver.yml b/.github/workflows/go-driver.yml
@@ -2,10 +2,10 @@ name: Go Driver Tests
 
 on:
   push:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
   pull_request:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
 jobs:
   build:
@@ -23,19 +23,7 @@ jobs:
 
     - name: Set tag based on branch
       run: |
-        if [[ "$GITHUB_EVENT_NAME" == "push" ]]; then
-          if [[ "$GITHUB_REF" == "refs/heads/master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_REF" == "refs/heads/PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        elif [[ "$GITHUB_EVENT_NAME" == "pull_request" ]]; then
-          if [[ "$GITHUB_BASE_REF" == "master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_BASE_REF" == "PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        fi
+        echo "TAG=PG16_latest" >> $GITHUB_ENV
 
     - name: Run apache/age docker image
       run: |

diff --git a/.github/workflows/installcheck.yaml b/.github/workflows/installcheck.yaml
@@ -2,9 +2,9 @@ name: Build / Regression
 
 on:
   push:
-    branches: [ 'master', 'PG16' ]
+    branches: [ 'PG16' ]
   pull_request:
-    branches: [ 'master', 'PG16' ]
+    branches: [ 'PG16' ]
 
 jobs:
   build:

diff --git a/.github/workflows/jdbc-driver.yaml b/.github/workflows/jdbc-driver.yaml
@@ -2,10 +2,10 @@ name: JDBC Driver Tests
 
 on:
   push:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
   pull_request:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
 jobs:
   build:
@@ -25,19 +25,7 @@ jobs:
 
     - name: Set tag based on branch
       run: |
-        if [[ "$GITHUB_EVENT_NAME" == "push" ]]; then
-          if [[ "$GITHUB_REF" == "refs/heads/master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_REF" == "refs/heads/PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        elif [[ "$GITHUB_EVENT_NAME" == "pull_request" ]]; then
-          if [[ "$GITHUB_BASE_REF" == "master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_BASE_REF" == "PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        fi
+        echo "TAG=PG16_latest" >> $GITHUB_ENV
 
     - name: Build and Test
       run: |

diff --git a/.github/workflows/nodejs-driver.yaml b/.github/workflows/nodejs-driver.yaml
@@ -2,10 +2,10 @@ name: Nodejs Driver Tests
 
 on:
   push:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
   pull_request:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
 jobs:
   build:
@@ -20,19 +20,7 @@ jobs:
 
     - name: Set tag based on branch
       run: |
-        if [[ "$GITHUB_EVENT_NAME" == "push" ]]; then
-          if [[ "$GITHUB_REF" == "refs/heads/master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_REF" == "refs/heads/PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        elif [[ "$GITHUB_EVENT_NAME" == "pull_request" ]]; then
-          if [[ "$GITHUB_BASE_REF" == "master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_BASE_REF" == "PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        fi
+        echo "TAG=PG16_latest" >> $GITHUB_ENV
 
     - name: Run apache/age docker image
       run: |

diff --git a/.github/workflows/python-driver.yaml b/.github/workflows/python-driver.yaml
@@ -2,10 +2,10 @@ name: Python Driver Tests
 
 on:
   push:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
   pull_request:
-    branches: [ "master", "PG16" ]
+    branches: [ "PG16" ]
 
 jobs:
   build:
@@ -20,19 +20,7 @@ jobs:
 
     - name: Set tag based on branch
       run: |
-        if [[ "$GITHUB_EVENT_NAME" == "push" ]]; then
-          if [[ "$GITHUB_REF" == "refs/heads/master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_REF" == "refs/heads/PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        elif [[ "$GITHUB_EVENT_NAME" == "pull_request" ]]; then
-          if [[ "$GITHUB_BASE_REF" == "master" ]]; then
-            echo "TAG=latest" >> $GITHUB_ENV
-          elif [[ "$GITHUB_BASE_REF" == "PG16" ]]; then
-            echo "TAG=PG16_latest" >> $GITHUB_ENV
-          fi
-        fi
+        echo "TAG=PG16_latest" >> $GITHUB_ENV
 
     - name: Run apache/age docker image
       run: |

diff --git a/README.md b/README.md
@@ -33,8 +33,8 @@
     <img src="https://img.shields.io/badge/Release-v1.4.0-FFA500?labelColor=gray&style=flat&link=https://github.com/apache/age/releases"/>
   </a>
   &nbsp;
-  <a href="https://www.postgresql.org/docs/15/index.html">
-    <img src="https://img.shields.io/badge/Version-Postgresql 15-00008B?labelColor=gray&style=flat&link=https://www.postgresql.org/docs/15/index.html"/>
+  <a href="https://www.postgresql.org/docs/16/index.html">
+    <img src="https://img.shields.io/badge/Version-Postgresql 16-00008B?labelColor=gray&style=flat&link=https://www.postgresql.org/docs/16/index.html"/>
   </a>
   &nbsp;
   <a href="https://github.com/apache/age/issues">
@@ -131,7 +131,7 @@ Apache AGE is intended to be simple to install and run. It can be installed with
 &nbsp;Install PostgreSQL
 </h4>
 
-You will need to install an AGE compatible version of Postgres<a>, for now AGE supports Postgres 11, 12, 13, 14 & 15. Supporting the latest versions is on AGE roadmap.
+You will need to install an AGE compatible version of Postgres<a>, for now AGE supports Postgres 11, 12, 13, 14, 15 & 16. Supporting the latest versions is on AGE roadmap.
 
 <h4>
 &nbsp;Installation via Package Manager
@@ -149,7 +149,7 @@ sudo apt install postgresql
 &nbsp;Installation From Source Code
 </h4>
 
-You can <a href="https://www.postgresql.org/ftp/source/"> download the Postgres </a> source code and install your own instance of Postgres. You can read instructions on how to install from source code for different versions on the <a href="https://www.postgresql.org/docs/15/installation.html">official Postgres Website.</a>
+You can <a href="https://www.postgresql.org/ftp/source/"> download the Postgres </a> source code and install your own instance of Postgres. You can read instructions on how to install from source code for different versions on the <a href="https://www.postgresql.org/docs/16/installation.html">official Postgres Website.</a>
 
 
 
@@ -158,7 +158,7 @@ You can <a href="https://www.postgresql.org/ftp/source/"> download the Postgres
 
 Clone the <a href="https://github.com/apache/age">github repository</a> or download the <a href="https://github.com/apache/age/releases">download an official release.
 </a>
-Run the pg_config utility and check the version of PostgreSQL. Currently, only PostgreSQL versions 11, 12, 13, 14 & 15 are supported. If you have any other version of Postgres, you will need to install PostgreSQL version 11, 12, 13, 14, or 15.
+Run the pg_config utility and check the version of PostgreSQL. Currently, only PostgreSQL versions 11, 12, 13, 14, 15 & 16 are supported. If you have any other version of Postgres, you will need to install PostgreSQL version 11, 12, 13, 14, 15 or 16.
 <br>
 
 ```bash

diff --git a/src/backend/catalog/ag_catalog.c b/src/backend/catalog/ag_catalog.c
@@ -86,26 +86,29 @@ void process_utility_hook_fini(void)
  * from being thrown, we need to disable the object_access_hook before dropping
  * the extension.
  */
-void ag_ProcessUtility_hook(PlannedStmt *pstmt, const char *queryString, bool readOnlyTree,
-                             ProcessUtilityContext context, ParamListInfo params,
-                             QueryEnvironment *queryEnv, DestReceiver *dest,
-                             QueryCompletion *qc)
+void ag_ProcessUtility_hook(PlannedStmt *pstmt, const char *queryString,
+                            bool readOnlyTree, ProcessUtilityContext context,
+                            ParamListInfo params, QueryEnvironment *queryEnv,
+                            DestReceiver *dest, QueryCompletion *qc)
 {
     if (is_age_drop(pstmt))
+    {
         drop_age_extension((DropStmt *)pstmt->utilityStmt);
+    }
     else if (prev_process_utility_hook)
-        (*prev_process_utility_hook) (pstmt, queryString, readOnlyTree, context, params,
-                                      queryEnv, dest, qc);
+    {
+        (*prev_process_utility_hook) (pstmt, queryString, readOnlyTree, context,
+                                      params, queryEnv, dest, qc);
+    }
     else
     {
         Assert(IsA(pstmt, PlannedStmt));
         Assert(pstmt->commandType == CMD_UTILITY);
         Assert(queryString != NULL);	/* required as of 8.4 */
         Assert(qc == NULL || qc->commandTag == CMDTAG_UNKNOWN);
-        standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv,
-                                dest, qc);
+        standard_ProcessUtility(pstmt, queryString, readOnlyTree, context,
+                                params, queryEnv, dest, qc);
     }
-
 }
 
 static void drop_age_extension(DropStmt *stmt)

diff --git a/src/backend/catalog/ag_label.c b/src/backend/catalog/ag_label.c
@@ -188,9 +188,10 @@ Datum _label_name(PG_FUNCTION_ARGS)
     uint32 label_id;
 
     if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
-        PG_RETURN_NULL(); 
-        //ereport(ERROR, (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
-        //                errmsg("graph_oid and label_id must not be null")));
+    {
+        ereport(ERROR, (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
+                        errmsg("graph_oid and label_id must not be null")));
+    }
 
     graph = PG_GETARG_OID(0);
 

diff --git a/src/backend/executor/cypher_set.c b/src/backend/executor/cypher_set.c
@@ -485,7 +485,8 @@ static void process_update_list(CustomScanState *node)
         }
 
         // Alter the properties Agtype value.
-        if (strcmp(update_item->prop_name, ""))
+        if (update_item->prop_name != NULL &&
+            strcmp(update_item->prop_name, "") != 0)
         {
             altered_properties = alter_property_value(original_properties,
                                                       update_item->prop_name,

diff --git a/src/backend/executor/cypher_utils.c b/src/backend/executor/cypher_utils.c
@@ -51,16 +51,20 @@
 
 /*
  * Given the graph name and the label name, create a ResultRelInfo for the table
- * those to variables represent. Open the Indices too.
+ * those two variables represent. Open the Indices too.
  */
 ResultRelInfo *create_entity_result_rel_info(EState *estate, char *graph_name,
                                              char *label_name)
 {
-    RangeVar *rv;
-    Relation label_relation;
-    ResultRelInfo *resultRelInfo;
+    RangeVar *rv = NULL;
+    Relation label_relation = NULL;
+    ResultRelInfo *resultRelInfo = NULL;
+    ParseState *pstate = NULL;
+    RangeTblEntry *rte = NULL;
+    int pii = 0;
 
-    ParseState *pstate = make_parsestate(NULL);
+    /* create a new parse state for this operation */
+    pstate = make_parsestate(NULL);
 
     resultRelInfo = palloc(sizeof(ResultRelInfo));
 
@@ -75,12 +79,33 @@ ResultRelInfo *create_entity_result_rel_info(EState *estate, char *graph_name,
 
     label_relation = parserOpenTable(pstate, rv, RowExclusiveLock);
 
-    // initialize the resultRelInfo
-    InitResultRelInfo(resultRelInfo, label_relation,
-                      0, NULL,
+    /*
+     * Get the rte to determine the correct perminfoindex value. Some rtes
+     * may have it set up, some created here (executor) may not.
+     *
+     * Note: The RTEPermissionInfo structure was added in PostgreSQL version 16.
+     *
+     * Note: We use the list_length because exec_rt_fetch starts at 1, not 0.
+     *       Doing this gives us the last rte in the es_range_table list, which
+     *       is the rte in question.
+     *
+     *       If the rte is created here and doesn't have a perminfoindex, we
+     *       need to pass on a 0. Otherwise, later on GetResultRTEPermissionInfo
+     *       will attempt to get the rte's RTEPermissionInfo data, which doesn't
+     *       exist.
+     *
+     * TODO: Ideally, we should consider creating the RTEPermissionInfo data,
+     *       but as this is just a read of the label relation, it is likely
+     *       unnecessary.
+     */
+    rte = exec_rt_fetch(list_length(estate->es_range_table), estate);
+    pii = (rte->perminfoindex == 0) ? 0 : list_length(estate->es_range_table);
+
+    /* initialize the resultRelInfo */
+    InitResultRelInfo(resultRelInfo, label_relation, pii, NULL,
                       estate->es_instrument);
 
-    // open the parse state
+    /* open the indices */
     ExecOpenIndices(resultRelInfo, false);
 
     free_parsestate(pstate);

diff --git a/src/backend/nodes/cypher_readfuncs.c b/src/backend/nodes/cypher_readfuncs.c
@@ -24,6 +24,7 @@
 #include "nodes/cypher_readfuncs.h"
 #include "nodes/cypher_nodes.h"
 
+static char *nullable_string(const char *token, int length);
 /*
  * Copied From Postgres
  *
@@ -111,7 +112,7 @@
 #define READ_STRING_FIELD(fldname) \
         token = pg_strtok(&length); \
         token = pg_strtok(&length); \
-        local_node->fldname = non_nullable_string(token, length)
+        local_node->fldname = nullable_string(token, length)
 
 // Read a parse location field (and throw away the value, per notes above)
 #define READ_LOCATION_FIELD(fldname) \
@@ -162,11 +163,22 @@
 
 #define strtobool(x)  ((*(x) == 't') ? true : false)
 
-#define nullable_string(token,length)  \
-        ((length) == 0 ? NULL : debackslash(token, length))
-
-#define non_nullable_string(token,length)  \
-        ((length == 2 && token[0] == '"' && token[1] == '"') ? "" : debackslash(token, length))
+/* copied from PG16 function of the same name for consistency */
+static char *nullable_string(const char *token, int length)
+{
+    /* outToken emits <> for NULL, and pg_strtok makes that an empty string */
+    if (length == 0)
+    {
+        return NULL;
+    }
+    /* outToken emits "" for empty string */
+    if (length == 2 && token[0] == '"' && token[1] == '"')
+    {
+        return pstrdup("");
+    }
+    /* otherwise, we must remove protective backslashes added by outToken */
+    return debackslash(token, length);
+}
 
 /*
  * Default read function for cypher nodes. For most nodes, we don't expect