From db4ec04cf25eb7cf77138fb256f318834963d0d0 Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 30 Dec 2014 16:48:06 -0500 Subject: [PATCH] Do get_indexdef calls while the table is already locked. These calls can require an access share lock on the table, which might conflict with an existing or later acquires lock. So perform these calls while we already have an exclusive lock on the table. This unfortuantely means that we ave to remove the constness of the table parameter to repack_one_table, as it is not modifying the table object to set up the indexes. --- bin/pg_repack.c | 102 +++++++++++++++++++----------------- regress/expected/repack.out | 4 +- 2 files changed, 57 insertions(+), 49 deletions(-) diff --git a/bin/pg_repack.c b/bin/pg_repack.c index d91d338..1d24840 100644 --- a/bin/pg_repack.c +++ b/bin/pg_repack.c @@ -195,7 +195,7 @@ static void check_tablespace(void); static bool preliminary_checks(char *errbuf, size_t errsize); static void repack_all_databases(const char *order_by); static bool repack_one_database(const char *order_by, char *errbuf, size_t errsize); -static void repack_one_table(const repack_table *table, const char *order_by); +static void repack_one_table(repack_table *table, const char *order_by); static bool repack_table_indexes(PGresult *index_details); static bool repack_all_indexes(char *errbuf, size_t errsize); static void repack_cleanup(bool fatal, const repack_table *table); @@ -669,10 +669,6 @@ repack_one_database(const char *orderby, char *errbuf, size_t errsize) const char *create_table_2; const char *tablespace; const char *ckey; - PGresult *indexres = NULL; - const char *indexparams[2]; - char buffer[12]; - int j; int c = 0; table.target_name = getstr(res, i, c++); @@ -741,41 +737,6 @@ repack_one_database(const char *orderby, char *errbuf, size_t errsize) table.create_table = sql.data; } - indexparams[0] = utoa(table.target_oid, buffer); - indexparams[1] = moveidx ? tablespace : NULL; - - /* First, just display a warning message for any invalid indexes - * which may be on the table (mostly to match the behavior of 1.1.8). - */ - indexres = execute( - "SELECT pg_get_indexdef(indexrelid)" - " FROM pg_index WHERE indrelid = $1 AND NOT indisvalid", - 1, indexparams); - - for (j = 0; j < PQntuples(indexres); j++) - { - const char *indexdef; - indexdef = getstr(indexres, j, 0); - elog(WARNING, "skipping invalid index: %s", indexdef); - } - - indexres = execute( - "SELECT indexrelid," - " repack.repack_indexdef(indexrelid, indrelid, $2, FALSE) " - " FROM pg_index WHERE indrelid = $1 AND indisvalid", - 2, indexparams); - - table.n_indexes = PQntuples(indexres); - table.indexes = pgut_malloc(table.n_indexes * sizeof(repack_index)); - - for (j = 0; j < table.n_indexes; j++) - { - table.indexes[j].target_oid = getoid(indexres, j, 0); - table.indexes[j].create_index = getstr(indexres, j, 1); - table.indexes[j].status = UNPROCESSED; - table.indexes[j].worker_idx = -1; /* Unassigned */ - } - repack_one_table(&table, orderby); } ret = true; @@ -1021,17 +982,20 @@ cleanup: * Re-organize one table. */ static void -repack_one_table(const repack_table *table, const char *orderby) +repack_one_table(repack_table *table, const char *orderby) { PGresult *res = NULL; const char *params[2]; int num; int num_waiting = 0; - int i; char *vxid = NULL; char buffer[12]; StringInfoData sql; bool ret = false; + PGresult *indexres = NULL; + const char *indexparams[2]; + char indexbuffer[12]; + int j; /* Keep track of whether we have gotten through setup to install * the z_repack_trigger, log table, etc. ourselves. We don't want to @@ -1065,11 +1029,6 @@ repack_one_table(const repack_table *table, const char *orderby) elog(DEBUG2, "sql_delete : %s", table->sql_delete); elog(DEBUG2, "sql_update : %s", table->sql_update); elog(DEBUG2, "sql_pop : %s", table->sql_pop); - for (i = 0; i < table->n_indexes; i++) - { - elog(DEBUG2, "index[%d].target_oid : %u", i, table->indexes[i].target_oid); - elog(DEBUG2, "index[%d].create_index : %s", i, table->indexes[i].create_index); - } if (dryrun) return; @@ -1090,6 +1049,53 @@ repack_one_table(const repack_table *table, const char *orderby) goto cleanup; } + /* + * pg_get_indexdef requires an access share lock, so do those calls while + * we have an access exclusive lock anyway, so we know they won't block. + */ + + indexparams[0] = utoa(table->target_oid, indexbuffer); + indexparams[1] = moveidx ? tablespace : NULL; + + /* First, just display a warning message for any invalid indexes + * which may be on the table (mostly to match the behavior of 1.1.8). + */ + indexres = execute( + "SELECT pg_get_indexdef(indexrelid)" + " FROM pg_index WHERE indrelid = $1 AND NOT indisvalid", + 1, indexparams); + + for (j = 0; j < PQntuples(indexres); j++) + { + const char *indexdef; + indexdef = getstr(indexres, j, 0); + elog(WARNING, "skipping invalid index: %s", indexdef); + } + + indexres = execute( + "SELECT indexrelid," + " repack.repack_indexdef(indexrelid, indrelid, $2, FALSE) " + " FROM pg_index WHERE indrelid = $1 AND indisvalid", + 2, indexparams); + + table->n_indexes = PQntuples(indexres); + table->indexes = pgut_malloc(table->n_indexes * sizeof(repack_index)); + + for (j = 0; j < table->n_indexes; j++) + { + table->indexes[j].target_oid = getoid(indexres, j, 0); + table->indexes[j].create_index = getstr(indexres, j, 1); + table->indexes[j].status = UNPROCESSED; + table->indexes[j].worker_idx = -1; /* Unassigned */ + } + + for (j = 0; j < table->n_indexes; j++) + { + elog(DEBUG2, "index[%d].target_oid : %u", j, table->indexes[j].target_oid); + elog(DEBUG2, "index[%d].create_index : %s", j, table->indexes[j].create_index); + } + + /* * Check z_repack_trigger is the trigger executed last so that * other before triggers cannot modify triggered tuples. @@ -1284,6 +1290,8 @@ repack_one_table(const repack_table *table, const char *orderby) if (!rebuild_indexes(table)) goto cleanup; + /* don't clear indexres until after rebuild_indexes or bad things happen */ + CLEARPGRES(indexres); CLEARPGRES(res); /* diff --git a/regress/expected/repack.out b/regress/expected/repack.out index 1e1e9be..017d5a4 100644 --- a/regress/expected/repack.out +++ b/regress/expected/repack.out @@ -117,16 +117,16 @@ SELECT * FROM tbl_with_dropped_toast; \! pg_repack --dbname=contrib_regression --table=tbl_cluster INFO: repacking table "tbl_cluster" \! pg_repack --dbname=contrib_regression --table=tbl_badindex -WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n) INFO: repacking table "tbl_badindex" +WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n) \! pg_repack --dbname=contrib_regression INFO: repacking table "tbl_cluster" INFO: repacking table "tbl_only_pkey" INFO: repacking table "tbl_gistkey" INFO: repacking table "tbl_with_dropped_column" INFO: repacking table "tbl_with_dropped_toast" -WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n) INFO: repacking table "tbl_badindex" +WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n) INFO: repacking table "tbl_idxopts" -- -- after