This is an automated email from the ASF dual-hosted git repository.
rafsun42 pushed a commit to branch PG13
in repository https://gitbox.apache.org/repos/asf/age.git
The following commit(s) were added to refs/heads/PG13 by this push:
new 2e4a72d2 Fix issue 1878 - Regression test failure with delete global
graphs (#1881) (#1889)
2e4a72d2 is described below
commit 2e4a72d2a1702a6fbcfe99b11874c9e29a90000f
Author: John Gemignani <[email protected]>
AuthorDate: Fri May 17 17:01:51 2024 -0700
Fix issue 1878 - Regression test failure with delete global graphs (#1881)
(#1889)
Fixed issue 1878, an issue with delete_global_graphs during parallel
regression tests.
Added locking and some more defensive copies.
No regression tests were impacted.
---
src/backend/utils/adt/age_global_graph.c | 140 +++++++++++++++++++++++++------
1 file changed, 115 insertions(+), 25 deletions(-)
diff --git a/src/backend/utils/adt/age_global_graph.c
b/src/backend/utils/adt/age_global_graph.c
index d4a02242..aeae5e32 100644
--- a/src/backend/utils/adt/age_global_graph.c
+++ b/src/backend/utils/adt/age_global_graph.c
@@ -32,6 +32,8 @@
#include "catalog/ag_graph.h"
#include "catalog/ag_label.h"
+#include <pthread.h>
+
/* defines */
#define VERTEX_HTAB_NAME "Vertex to edge lists " /* added a space at end for */
#define EDGE_HTAB_NAME "Edge to vertex mapping " /* the graph name to follow */
@@ -81,12 +83,22 @@ typedef struct GRAPH_global_context
struct GRAPH_global_context *next; /* next graph */
} GRAPH_global_context;
-/* global variable to hold the per process GRAPH global context */
-static GRAPH_global_context *global_graph_contexts = NULL;
+/* container for GRAPH_global_context and its mutex */
+typedef struct GRAPH_global_context_container
+{
+ /* head of the list */
+ GRAPH_global_context *contexts;
+
+ /* mutex to protect the list */
+ pthread_mutex_t mutex_lock;
+} GRAPH_global_context_container;
+
+/* global variable to hold the per process GRAPH global contexts */
+static GRAPH_global_context_container global_graph_contexts_container = {0};
/* declarations */
/* GRAPH global context functions */
-static void free_specific_GRAPH_global_context(GRAPH_global_context *ggctx);
+static bool free_specific_GRAPH_global_context(GRAPH_global_context *ggctx);
static bool delete_specific_GRAPH_global_contexts(char *graph_name);
static bool delete_GRAPH_global_contexts(void);
static void create_GRAPH_global_hashtables(GRAPH_global_context *ggctx);
@@ -681,14 +693,14 @@ static void
freeze_GRAPH_global_hashtables(GRAPH_global_context *ggctx)
* Helper function to free the entire specified GRAPH global context. After
* running this you should not use the pointer in ggctx.
*/
-static void free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
+static bool free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
{
GraphIdNode *curr_vertex = NULL;
/* don't do anything if NULL */
if (ggctx == NULL)
{
- return;
+ return true;
}
/* free the graph name */
@@ -717,8 +729,11 @@ static void
free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
value = (vertex_entry *)hash_search(ggctx->vertex_hashtable,
(void *)&vertex_id, HASH_FIND,
&found);
- /* this is bad if it isn't found */
- Assert(found);
+ /* this is bad if it isn't found, but leave that to the caller */
+ if (found == false)
+ {
+ return false;
+ }
/* free the edge list associated with this vertex */
free_ListGraphId(value->edges_in);
@@ -747,6 +762,8 @@ static void
free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
/* free the context */
pfree(ggctx);
ggctx = NULL;
+
+ return true;
}
/*
@@ -754,6 +771,9 @@ static void
free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
* context for the graph specified, provided it isn't already built and valid.
* During processing it will free (delete) all invalid GRAPH contexts. It
* returns the GRAPH global context for the specified graph.
+ *
+ * NOTE: Function uses a MUTEX for global_graph_contexts
+ *
*/
GRAPH_global_context *manage_GRAPH_global_contexts(char *graph_name,
Oid graph_oid)
@@ -777,9 +797,12 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
* 5) One or more other contexts do exist but, one or more are invalid.
*/
+ /* lock the global contexts list */
+ pthread_mutex_lock(&global_graph_contexts_container.mutex_lock);
+
/* free the invalidated GRAPH global contexts first */
prev_ggctx = NULL;
- curr_ggctx = global_graph_contexts;
+ curr_ggctx = global_graph_contexts_container.contexts;
while (curr_ggctx != NULL)
{
GRAPH_global_context *next_ggctx = curr_ggctx->next;
@@ -787,14 +810,16 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
/* if the transaction ids have changed, we have an invalid graph */
if (is_ggctx_invalid(curr_ggctx))
{
+ bool success = false;
+
/*
* If prev_ggctx is NULL then we are freeing the top of the
- * contexts. So, we need to point the global variable to the
+ * contexts. So, we need to point the contexts variable to the
* new (next) top context, if there is one.
*/
if (prev_ggctx == NULL)
{
- global_graph_contexts = next_ggctx;
+ global_graph_contexts_container.contexts = next_ggctx;
}
else
{
@@ -802,7 +827,17 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
}
/* free the current graph context */
- free_specific_GRAPH_global_context(curr_ggctx);
+ success = free_specific_GRAPH_global_context(curr_ggctx);
+
+ /* if it wasn't successfull, there was a missing vertex entry */
+ if (!success)
+ {
+ /* unlock the mutex so we don't get a deadlock */
+
pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
+ ereport(ERROR, (errcode(ERRCODE_DATA_EXCEPTION),
+ errmsg("missing vertex_entry during free")));
+ }
}
else
{
@@ -814,14 +849,17 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
}
/* find our graph's context. if it exists, we are done */
- curr_ggctx = global_graph_contexts;
+ curr_ggctx = global_graph_contexts_container.contexts;
while (curr_ggctx != NULL)
{
if (curr_ggctx->graph_oid == graph_oid)
{
/* switch our context back */
MemoryContextSwitchTo(oldctx);
- /* we are done */
+
+ /* we are done unlock the global contexts list */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
return curr_ggctx;
}
curr_ggctx = curr_ggctx->next;
@@ -830,9 +868,9 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
/* otherwise, we need to create one and possibly attach it */
new_ggctx = palloc0(sizeof(GRAPH_global_context));
- if (global_graph_contexts != NULL)
+ if (global_graph_contexts_container.contexts != NULL)
{
- new_ggctx->next = global_graph_contexts;
+ new_ggctx->next = global_graph_contexts_container.contexts;
}
else
{
@@ -840,7 +878,7 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
}
/* set the global context variable */
- global_graph_contexts = new_ggctx;
+ global_graph_contexts_container.contexts = new_ggctx;
/* set the graph name and oid */
new_ggctx->graph_name = pstrdup(graph_name);
@@ -859,6 +897,9 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
load_GRAPH_global_hashtables(new_ggctx);
freeze_GRAPH_global_hashtables(new_ggctx);
+ /* unlock the global contexts list */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
/* switch back to the previous memory context */
MemoryContextSwitchTo(oldctx);
@@ -868,22 +909,38 @@ GRAPH_global_context *manage_GRAPH_global_contexts(char
*graph_name,
/*
* Helper function to delete all of the global graph contexts used by the
* process. When done the global global_graph_contexts will be NULL.
+ *
+ * NOTE: Function uses a MUTEX global_graph_contexts_mutex
*/
static bool delete_GRAPH_global_contexts(void)
{
GRAPH_global_context *curr_ggctx = NULL;
bool retval = false;
+ /* lock contexts list */
+ pthread_mutex_lock(&global_graph_contexts_container.mutex_lock);
+
/* get the first context, if any */
- curr_ggctx = global_graph_contexts;
+ curr_ggctx = global_graph_contexts_container.contexts;
/* free all GRAPH global contexts */
while (curr_ggctx != NULL)
{
GRAPH_global_context *next_ggctx = curr_ggctx->next;
+ bool success = false;
/* free the current graph context */
- free_specific_GRAPH_global_context(curr_ggctx);
+ success = free_specific_GRAPH_global_context(curr_ggctx);
+
+ /* if it wasn't successfull, there was a missing vertex entry */
+ if (!success)
+ {
+ /* unlock the mutex so we don't get a deadlock */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
+ ereport(ERROR, (errcode(ERRCODE_DATA_EXCEPTION),
+ errmsg("missing vertex_entry during free")));
+ }
/* advance to the next context */
curr_ggctx = next_ggctx;
@@ -891,8 +948,11 @@ static bool delete_GRAPH_global_contexts(void)
retval = true;
}
- /* clear the global variable */
- global_graph_contexts = NULL;
+ /* reset the head of the contexts to NULL */
+ global_graph_contexts_container.contexts = NULL;
+
+ /* unlock the global contexts list */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
return retval;
}
@@ -915,8 +975,11 @@ static bool delete_specific_GRAPH_global_contexts(char
*graph_name)
/* get the graph oid */
graph_oid = get_graph_oid(graph_name);
+ /* lock the global contexts list */
+ pthread_mutex_lock(&global_graph_contexts_container.mutex_lock);
+
/* get the first context, if any */
- curr_ggctx = global_graph_contexts;
+ curr_ggctx = global_graph_contexts_container.contexts;
/* find the specified GRAPH global context */
while (curr_ggctx != NULL)
@@ -925,6 +988,7 @@ static bool delete_specific_GRAPH_global_contexts(char
*graph_name)
if (curr_ggctx->graph_oid == graph_oid)
{
+ bool success = false;
/*
* If prev_ggctx is NULL then we are freeing the top of the
* contexts. So, we need to point the global variable to the
@@ -932,7 +996,7 @@ static bool delete_specific_GRAPH_global_contexts(char
*graph_name)
*/
if (prev_ggctx == NULL)
{
- global_graph_contexts = next_ggctx;
+ global_graph_contexts_container.contexts = next_ggctx;
}
else
{
@@ -940,7 +1004,17 @@ static bool delete_specific_GRAPH_global_contexts(char
*graph_name)
}
/* free the current graph context */
- free_specific_GRAPH_global_context(curr_ggctx);
+ success = free_specific_GRAPH_global_context(curr_ggctx);
+
+ /* unlock the global contexts list */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
+ /* if it wasn't successfull, there was a missing vertex entry */
+ if (!success)
+ {
+ ereport(ERROR, (errcode(ERRCODE_DATA_EXCEPTION),
+ errmsg("missing vertex_entry during free")));
+ }
/* we found and freed it, return true */
return true;
@@ -951,6 +1025,9 @@ static bool delete_specific_GRAPH_global_contexts(char
*graph_name)
curr_ggctx = next_ggctx;
}
+ /* unlock the global contexts list */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
/* we didn't find it, return false */
return false;
}
@@ -994,14 +1071,20 @@ GRAPH_global_context *find_GRAPH_global_context(Oid
graph_oid)
{
GRAPH_global_context *ggctx = NULL;
+ /* lock the global contexts lists */
+ pthread_mutex_lock(&global_graph_contexts_container.mutex_lock);
+
/* get the root */
- ggctx = global_graph_contexts;
+ ggctx = global_graph_contexts_container.contexts;
while(ggctx != NULL)
{
/* if we found it return it */
if (ggctx->graph_oid == graph_oid)
{
+ /* unlock the global contexts lists */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
return ggctx;
}
@@ -1009,6 +1092,9 @@ GRAPH_global_context *find_GRAPH_global_context(Oid
graph_oid)
ggctx = ggctx->next;
}
+ /* unlock the global contexts lists */
+ pthread_mutex_unlock(&global_graph_contexts_container.mutex_lock);
+
/* we did not find it so return NULL */
return NULL;
}
@@ -1103,8 +1189,12 @@ Datum age_delete_global_graphs(PG_FUNCTION_ARGS)
{
char *graph_name = NULL;
- graph_name = agtv_temp->val.string.val;
+ graph_name = strndup(agtv_temp->val.string.val,
+ agtv_temp->val.string.len);
+
success = delete_specific_GRAPH_global_contexts(graph_name);
+
+ free(graph_name);
}
else
{
