Skip to content

Commit db4ec04

Browse files
adunstanadunstan
committed
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.
1 parent 9f77a2f commit db4ec04

File tree

2 files changed

+57
-49
lines changed

2 files changed

+57
-49
lines changed

‎bin/pg_repack.c

Lines changed: 55 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ static void check_tablespace(void);
195195
static bool preliminary_checks(char *errbuf, size_t errsize);
196196
static void repack_all_databases(const char *order_by);
197197
static bool repack_one_database(const char *order_by, char *errbuf, size_t errsize);
198-
static void repack_one_table(const repack_table *table, const char *order_by);
198+
static void repack_one_table(repack_table *table, const char *order_by);
199199
static bool repack_table_indexes(PGresult *index_details);
200200
static bool repack_all_indexes(char *errbuf, size_t errsize);
201201
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)
669669
const char *create_table_2;
670670
const char *tablespace;
671671
const char *ckey;
672-
PGresult *indexres = NULL;
673-
const char *indexparams[2];
674-
char buffer[12];
675-
int j;
676672
int c = 0;
677673

678674
table.target_name = getstr(res, i, c++);
@@ -741,41 +737,6 @@ repack_one_database(const char *orderby, char *errbuf, size_t errsize)
741737
table.create_table = sql.data;
742738
}
743739

744-
indexparams[0] = utoa(table.target_oid, buffer);
745-
indexparams[1] = moveidx ? tablespace : NULL;
746-
747-
/* First, just display a warning message for any invalid indexes
748-
* which may be on the table (mostly to match the behavior of 1.1.8).
749-
*/
750-
indexres = execute(
751-
"SELECT pg_get_indexdef(indexrelid)"
752-
" FROM pg_index WHERE indrelid = $1 AND NOT indisvalid",
753-
1, indexparams);
754-
755-
for (j = 0; j < PQntuples(indexres); j++)
756-
{
757-
const char *indexdef;
758-
indexdef = getstr(indexres, j, 0);
759-
elog(WARNING, "skipping invalid index: %s", indexdef);
760-
}
761-
762-
indexres = execute(
763-
"SELECT indexrelid,"
764-
" repack.repack_indexdef(indexrelid, indrelid, $2, FALSE) "
765-
" FROM pg_index WHERE indrelid = $1 AND indisvalid",
766-
2, indexparams);
767-
768-
table.n_indexes = PQntuples(indexres);
769-
table.indexes = pgut_malloc(table.n_indexes * sizeof(repack_index));
770-
771-
for (j = 0; j < table.n_indexes; j++)
772-
{
773-
table.indexes[j].target_oid = getoid(indexres, j, 0);
774-
table.indexes[j].create_index = getstr(indexres, j, 1);
775-
table.indexes[j].status = UNPROCESSED;
776-
table.indexes[j].worker_idx = -1; /* Unassigned */
777-
}
778-
779740
repack_one_table(&table, orderby);
780741
}
781742
ret = true;
@@ -1021,17 +982,20 @@ rebuild_indexes(const repack_table *table)
1021982
* Re-organize one table.
1022983
*/
1023984
static void
1024-
repack_one_table(const repack_table *table, const char *orderby)
985+
repack_one_table(repack_table *table, const char *orderby)
1025986
{
1026987
PGresult *res = NULL;
1027988
const char *params[2];
1028989
int num;
1029990
int num_waiting = 0;
1030-
int i;
1031991
char *vxid = NULL;
1032992
char buffer[12];
1033993
StringInfoData sql;
1034994
bool ret = false;
995+
PGresult *indexres = NULL;
996+
const char *indexparams[2];
997+
char indexbuffer[12];
998+
int j;
1035999

10361000
/* Keep track of whether we have gotten through setup to install
10371001
* 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)
10651029
elog(DEBUG2, "sql_delete : %s", table->sql_delete);
10661030
elog(DEBUG2, "sql_update : %s", table->sql_update);
10671031
elog(DEBUG2, "sql_pop : %s", table->sql_pop);
1068-
for (i = 0; i < table->n_indexes; i++)
1069-
{
1070-
elog(DEBUG2, "index[%d].target_oid : %u", i, table->indexes[i].target_oid);
1071-
elog(DEBUG2, "index[%d].create_index : %s", i, table->indexes[i].create_index);
1072-
}
10731032

10741033
if (dryrun)
10751034
return;
@@ -1090,6 +1049,53 @@ repack_one_table(const repack_table *table, const char *orderby)
10901049
goto cleanup;
10911050
}
10921051

1052+
/*
1053+
* pg_get_indexdef requires an access share lock, so do those calls while
1054+
* we have an access exclusive lock anyway, so we know they won't block.
1055+
*/
1056+
1057+
indexparams[0] = utoa(table->target_oid, indexbuffer);
1058+
indexparams[1] = moveidx ? tablespace : NULL;
1059+
1060+
/* First, just display a warning message for any invalid indexes
1061+
* which may be on the table (mostly to match the behavior of 1.1.8).
1062+
*/
1063+
indexres = execute(
1064+
"SELECT pg_get_indexdef(indexrelid)"
1065+
" FROM pg_index WHERE indrelid = $1 AND NOT indisvalid",
1066+
1, indexparams);
1067+
1068+
for (j = 0; j < PQntuples(indexres); j++)
1069+
{
1070+
const char *indexdef;
1071+
indexdef = getstr(indexres, j, 0);
1072+
elog(WARNING, "skipping invalid index: %s", indexdef);
1073+
}
1074+
1075+
indexres = execute(
1076+
"SELECT indexrelid,"
1077+
" repack.repack_indexdef(indexrelid, indrelid, $2, FALSE) "
1078+
" FROM pg_index WHERE indrelid = $1 AND indisvalid",
1079+
2, indexparams);
1080+
1081+
table->n_indexes = PQntuples(indexres);
1082+
table->indexes = pgut_malloc(table->n_indexes * sizeof(repack_index));
1083+
1084+
for (j = 0; j < table->n_indexes; j++)
1085+
{
1086+
table->indexes[j].target_oid = getoid(indexres, j, 0);
1087+
table->indexes[j].create_index = getstr(indexres, j, 1);
1088+
table->indexes[j].status = UNPROCESSED;
1089+
table->indexes[j].worker_idx = -1; /* Unassigned */
1090+
}
1091+
1092+
for (j = 0; j < table->n_indexes; j++)
1093+
{
1094+
elog(DEBUG2, "index[%d].target_oid : %u", j, table->indexes[j].target_oid);
1095+
elog(DEBUG2, "index[%d].create_index : %s", j, table->indexes[j].create_index);
1096+
}
1097+
1098+
10931099
/*
10941100
* Check z_repack_trigger is the trigger executed last so that
10951101
* other before triggers cannot modify triggered tuples.
@@ -1284,6 +1290,8 @@ repack_one_table(const repack_table *table, const char *orderby)
12841290
if (!rebuild_indexes(table))
12851291
goto cleanup;
12861292

1293+
/* don't clear indexres until after rebuild_indexes or bad things happen */
1294+
CLEARPGRES(indexres);
12871295
CLEARPGRES(res);
12881296

12891297
/*

‎regress/expected/repack.out

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,16 +117,16 @@ SELECT * FROM tbl_with_dropped_toast;
117117
\! pg_repack --dbname=contrib_regression --table=tbl_cluster
118118
INFO: repacking table "tbl_cluster"
119119
\! pg_repack --dbname=contrib_regression --table=tbl_badindex
120-
WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n)
121120
INFO: repacking table "tbl_badindex"
121+
WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n)
122122
\! pg_repack --dbname=contrib_regression
123123
INFO: repacking table "tbl_cluster"
124124
INFO: repacking table "tbl_only_pkey"
125125
INFO: repacking table "tbl_gistkey"
126126
INFO: repacking table "tbl_with_dropped_column"
127127
INFO: repacking table "tbl_with_dropped_toast"
128-
WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n)
129128
INFO: repacking table "tbl_badindex"
129+
WARNING: skipping invalid index: CREATE UNIQUE INDEX idx_badindex_n ON tbl_badindex USING btree (n)
130130
INFO: repacking table "tbl_idxopts"
131131
--
132132
-- after

0 commit comments

Comments
 (0)