Skip to content

Commit

Permalink
Fix issue 1878 - Regression test failure with delete global graphs (#…
Browse files Browse the repository at this point in the history 
…1881) (#2034)

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.
  • Loading branch information
@jrgemignani
jrgemignani authored Aug 9, 2024
1 parent e481556 commit 2417d27
Showing 1 changed file with 115 additions and 25 deletions.
140 changes: 115 additions & 25 deletions src/backend/utils/adt/age_global_graph.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,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 */
Expand Down Expand Up @@ -80,12 +82,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);
Expand Down Expand Up @@ -596,14 +608,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 */
Expand Down Expand Up @@ -632,8 +644,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);
Expand Down Expand Up @@ -662,13 +677,18 @@ static void free_specific_GRAPH_global_context(GRAPH_global_context *ggctx)
/* free the context */
pfree(ggctx);
ggctx = NULL;

return true;
}

/*
* Helper function to manage the GRAPH global contexts. It will create the
* 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)
Expand All @@ -692,32 +712,47 @@ 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;

/* 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
{
prev_ggctx->next = curr_ggctx->next;
}

/* 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
{
Expand All @@ -729,14 +764,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;
Expand All @@ -745,17 +783,17 @@ 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
{
new_ggctx->next = NULL;
}

/* 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);
Expand All @@ -774,6 +812,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);

Expand All @@ -783,31 +824,50 @@ 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;

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;
}
Expand All @@ -830,8 +890,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)
Expand All @@ -840,22 +903,33 @@ 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
* 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
{
prev_ggctx->next = curr_ggctx->next;
}

/* 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;
Expand All @@ -866,6 +940,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;
}
Expand Down Expand Up @@ -909,21 +986,30 @@ 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;
}

/* advance to the next context */
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;
}
Expand Down Expand Up @@ -1018,8 +1104,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
{
Expand Down

0 comments on commit 2417d27

Please sign in to comment.