Skip to content

Commit bebe6ff

Browse files
schmiddyschmiddy
committed
Factor out advisory locking code into advisory_lock()
Also, some further small fixes of error messages, comments, marking dubious kludges (e.g. strchr() to determine schema name).
1 parent 673eac7 commit bebe6ff

File tree

1 file changed

+53
-45
lines changed

1 file changed

+53
-45
lines changed

‎bin/pg_repack.c

Lines changed: 53 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,7 @@ static bool rebuild_indexes(const repack_table *table);
184184

185185
static char *getstr(PGresult *res, int row, int col);
186186
static Oid getoid(PGresult *res, int row, int col);
187+
static bool advisory_lock(PGconn *conn, const char *relid);
187188
static bool lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool start_xact);
188189
static bool kill_ddl(PGconn *conn, Oid relid, bool terminate);
189190
static bool lock_access_share(PGconn *conn, Oid relid, const char *target_name);
@@ -1005,26 +1006,12 @@ repack_one_table(const repack_table *table, const char *orderby)
10051006
*/
10061007
elog(DEBUG2, "---- setup ----");
10071008

1008-
/* Obtain an advisory lock on the table's OID, to make sure no other
1009-
* pg_repack is working on the table. (Not a real concern with only
1010-
* full-table repacks, but mainly for index-only repacks.)
1011-
*/
10121009
params[0] = utoa(table->target_oid, buffer);
1013-
res = pgut_execute(connection, "SELECT pg_try_advisory_lock($1::bigint)",
1014-
1, params);
1015-
if (PQresultStatus(res) != PGRES_TUPLES_OK)
1016-
{
1017-
elog(ERROR, "%s", PQerrorMessage(connection));
1018-
goto cleanup;
1019-
}
1020-
else if (strcmp(getstr(res, 0, 0), "t") != 0)
1021-
{
1022-
elog(WARNING, "Another pg_repack command may be running on the table. Please try again later.");
1010+
1011+
if (!advisory_lock(connection, buffer))
10231012
goto cleanup;
1024-
}
1025-
CLEARPGRES(res);
10261013

1027-
if (!(lock_exclusive(connection, utoa(table->target_oid, buffer), table->lock_table, TRUE)))
1014+
if (!(lock_exclusive(connection, buffer, table->lock_table, TRUE)))
10281015
{
10291016
elog(WARNING, "lock_exclusive() failed for %s", table->target_name);
10301017
goto cleanup;
@@ -1465,6 +1452,40 @@ lock_access_share(PGconn *conn, Oid relid, const char *target_name)
14651452
}
14661453

14671454

1455+
/* XXX: Make sure that repack_one_table() also obtains an advisory
1456+
* lock on the table, so that we can't have a table-wide repack running
1457+
* along with an indexes-only repack. Also, since advisory locks are
1458+
* 8 bytes wide and OIDs are only 4 bytes, consider using our own prefix
1459+
* rather than just the table OID, to avoid inadvertent conflict with
1460+
* other applications using advisory locks.
1461+
*/
1462+
1463+
/* Obtain an advisory lock on the table's OID, to make sure no other
1464+
* pg_repack is working on the table. This is not so much a concern with
1465+
* full-table repacks, but mainly so that index-only repacks don't interfere
1466+
* with each other or a full-table repack.
1467+
*/
1468+
static bool advisory_lock(PGconn *conn, const char *relid)
1469+
{
1470+
PGresult *res = NULL;
1471+
bool ret = false;
1472+
1473+
res = pgut_execute(conn, "SELECT pg_try_advisory_lock($1::bigint)",
1474+
1, &relid);
1475+
1476+
if (PQresultStatus(res) != PGRES_TUPLES_OK) {
1477+
elog(ERROR, "%s", PQerrorMessage(connection));
1478+
}
1479+
else if (strcmp(getstr(res, 0, 0), "t") != 0) {
1480+
elog(WARNING, "Another pg_repack command may be running on the table. Please try again later.");
1481+
}
1482+
else {
1483+
ret = true;
1484+
}
1485+
CLEARPGRES(res);
1486+
return ret;
1487+
}
1488+
14681489
/*
14691490
* Try acquire an ACCESS EXCLUSIVE table lock, avoiding deadlocks and long
14701491
* waits by killing off other sessions.
@@ -1668,12 +1689,14 @@ repack_one_index(Oid table, const char *table_name, Oid index, const char *schem
16681689
* Call repack_one_index for each of the indexes
16691690
*/
16701691
static bool
1671-
repack_all_indexes(char *errbuf, size_t errsize){
1692+
repack_all_indexes(char *errbuf, size_t errsize)
1693+
{
16721694
bool ret = false;
1673-
PGresult *res = NULL, *res2 = NULL;
1695+
PGresult *res = NULL;
16741696
int i;
16751697
int num;
16761698
StringInfoData sql;
1699+
char buffer[12];
16771700
const char *params[1];
16781701
const char *table_name = NULL;
16791702
const char *schema_name = NULL;
@@ -1710,12 +1733,12 @@ repack_all_indexes(char *errbuf, size_t errsize){
17101733
{
17111734
ereport(ERROR,
17121735
(errcode(EINVAL),
1713-
errmsg("index \"%s\" doesnot exist.\n", r_index)));
1736+
errmsg("index \"%s\" does not exist.\n", r_index)));
17141737
goto cleanup;
17151738
}
17161739
}
17171740

1718-
// seperate schema name and index name
1741+
// separate schema name and index name. FIXME: kludge
17191742
pos = strchr(params[0], '.');
17201743
if (pos)
17211744
{
@@ -1751,7 +1774,7 @@ repack_all_indexes(char *errbuf, size_t errsize){
17511774
}
17521775
}
17531776

1754-
// seperate schema name and table name
1777+
// separate schema name and table name. FIXME: kludge
17551778
pos = strchr(params[0], '.');
17561779
if (pos)
17571780
{
@@ -1763,30 +1786,14 @@ repack_all_indexes(char *errbuf, size_t errsize){
17631786
table_name = params[0];
17641787
}
17651788

1766-
/* XXX: Make sure that repack_one_table() also obtains an advisory
1767-
* lock on the table, so that we can't have a table-wide repack running
1768-
* along with an indexes-only repack. Also, since advisory locks are
1769-
* 8 bytes wide and OIDs are only 4 bytes, consider using our own prefix
1770-
* rather than just the table OID, to avoid inadvertent conflict with
1771-
* other applications using advisory locks.
1789+
/* Check if any concurrent pg_repack command is being run on the same
1790+
* table.
17721791
*/
1773-
1774-
/* Check if any concurrent pg_repack command is being run on the same table */
1775-
initStringInfo(&sql);
1776-
appendStringInfo(&sql, "SELECT pg_try_advisory_lock(%u)", getoid(res, 0, 3));
1777-
1778-
res2 = execute_elevel(sql.data, 0, NULL, DEBUG2);
1779-
if (PQresultStatus(res2) != PGRES_TUPLES_OK)
1780-
{
1781-
elog(ERROR, "%s", PQerrorMessage(connection));
1792+
if (!advisory_lock(connection, utoa(getoid(res, 0, 3), buffer))) {
1793+
snprintf(errbuf, errsize, "Unable to obtain advisory lock on %s",
1794+
table_name);
17821795
goto cleanup;
17831796
}
1784-
else if (strcmp(getstr(res2, 0, 0), "t") != 0)
1785-
{
1786-
snprintf(errbuf, errsize, "Another pg_repack command may be running on the table. Please try again later.");
1787-
goto cleanup;
1788-
}
1789-
17901797
for (i = 0; i < num; i++)
17911798
{
17921799
char *isvalid = getstr(res, i, 2);
@@ -1797,8 +1804,10 @@ repack_all_indexes(char *errbuf, size_t errsize){
17971804
else
17981805
elog(INFO, "repacking index \"%s\"", getstr(res, i, 0));
17991806

1800-
if (!(repack_one_index(getoid(res, i, 3), table_name, getoid(res, i, 1), schema_name)))
1807+
if (!(repack_one_index(getoid(res, i, 3), table_name, getoid(res, i, 1), schema_name))) {
1808+
/* FIXME: fill in errbuf here */
18011809
goto cleanup;
1810+
}
18021811
}
18031812
else
18041813
if (schema_name)
@@ -1810,7 +1819,6 @@ repack_all_indexes(char *errbuf, size_t errsize){
18101819

18111820
cleanup:
18121821
CLEARPGRES(res);
1813-
CLEARPGRES(res2);
18141822
disconnect();
18151823
termStringInfo(&sql);
18161824
return ret;

0 commit comments

Comments
 (0)