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

Handling system tables errors #327

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 69 additions & 2 deletions bin/pg_repack.c
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ typedef struct repack_table

static bool is_superuser(void);
static void check_tablespace(void);
static bool check_systemtables(void);
static bool preliminary_checks(char *errbuf, size_t errsize);
static bool is_requested_relation_exists(char *errbuf, size_t errsize);
static void repack_all_databases(const char *order_by);
Expand Down Expand Up @@ -421,6 +422,47 @@ is_superuser(void)
return false;
}

bool
check_systemtables()

{
PGresult *query_result = NULL;
int num;
SimpleStringListCell *cell;
StringInfoData sql;
int iparam = 0;

num = simple_string_list_size(table_list);

initStringInfo(&sql);

appendStringInfoString(&sql, "select 1 from pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace where n.nspname IN ('pg_catalog','information_schema') AND c.oid IN (");

for (cell = table_list.head; cell; cell = cell->next)
{
appendStringInfo(&sql, "'%s'::regclass::oid", cell->val);
iparam++;
if (iparam < num)
appendStringInfoChar(&sql, ',');
}

appendStringInfoString(&sql,")");

query_result = execute_elevel(sql.data,0,NULL, DEBUG2);

if (PQresultStatus(query_result) == PGRES_TUPLES_OK)
{
if (PQntuples(query_result) >= 1)
{
return true;
}
}

CLEARPGRES(query_result);

return false;
}

/*
* Check if the tablespace requested exists.
*
Expand Down Expand Up @@ -487,6 +529,12 @@ preliminary_checks(char *errbuf, size_t errsize){
goto cleanup;
}

if (check_systemtables()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would make sense to make this check inside repack_one_table and repack_table_indexes only? That way this check won't interrupt pg_repack completely, and it will skip only system tables.

if (errbuf)
snprintf(errbuf, errsize, "For System Tables Use VACUUM FULL.");
goto cleanup;
}

/* Query the extension version. Exit if no match */
res = execute_elevel("select repack.version(), repack.version_sql()",
0, NULL, DEBUG2);
Expand Down Expand Up @@ -1276,7 +1324,7 @@ repack_one_table(repack_table *table, const char *orderby)
return;

/* push repack_cleanup_callback() on stack to clean temporary objects */
pgut_atexit_push(repack_cleanup_callback, &table->target_oid);
pgut_atexit_push(repack_cleanup_callback, table);

/*
* 1. Setup advisory lock and trigger on main table.
Expand Down Expand Up @@ -1966,7 +2014,8 @@ lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool sta
void
repack_cleanup_callback(bool fatal, void *userdata)
{
Oid target_table = *(Oid *) userdata;
repack_table *table = (repack_table *) userdata;
Oid target_table = table->target_oid;
const char *params[2];
char buffer[12];
char num_buff[12];
Expand All @@ -1981,7 +2030,16 @@ repack_cleanup_callback(bool fatal, void *userdata)
* so just use an unconditional reconnect().
*/
reconnect(ERROR);

command("BEGIN ISOLATION LEVEL READ COMMITTED", 0, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you included non-relevant changes. Can you remove them?

if (!(lock_exclusive(connection, params[0], table->lock_table, false)))
{
elog(ERROR, "lock_exclusive() failed in connection for %s during cleanup callback",
table->target_name);
}

command("SELECT repack.repack_drop($1, $2)", 2, params);
command("COMMIT", 0, NULL);
temp_obj_num = 0; /* reset temporary object counter after cleanup */
}
}
Expand Down Expand Up @@ -2011,7 +2069,16 @@ repack_cleanup(bool fatal, const repack_table *table)
/* do cleanup */
params[0] = utoa(table->target_oid, buffer);
params[1] = utoa(temp_obj_num, num_buff);

command("BEGIN ISOLATION LEVEL READ COMMITTED", 0, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. This looks like a non-relevant change.

if (!(lock_exclusive(connection, params[0], table->lock_table, false)))
{
elog(ERROR, "lock_exclusive() failed in connection for %s during cleanup",
table->target_name);
}

command("SELECT repack.repack_drop($1, $2)", 2, params);
command("COMMIT", 0, NULL);
temp_obj_num = 0; /* reset temporary object counter after cleanup */
}
}
Expand Down
2 changes: 1 addition & 1 deletion bin/pgut/pgut.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ extern const char *PROGRAM_ISSUES;
extern bool interrupted;
extern int pgut_log_level;
extern int pgut_abort_level;
extern bool pgut_echo;
extern bool pgut_echo;

extern void pgut_init(int argc, char **argv);
extern void pgut_atexit_push(pgut_atexit_callback callback, void *userdata);
Expand Down