From 6374261b470d71f96265dd5bc9c8a6a7de2d7136 Mon Sep 17 00:00:00 2001 From: Artur Zakirov Date: Sun, 6 Jul 2025 23:57:54 +0200 Subject: [PATCH 1/2] Issue #457: Move SAVEPOINT outside the retry loop and call RELEASE SAVEPOINT lock_access_share() and lock_exclusive() doesn't call RELEASE SAVEPOINT on every retry which pollutes the cache and degrades the performance. This commit calls RELEASE SAVEPOINT when created savepoint is not necessary anymore. Additionally it moves SAVEPOINT outside of the retry loop since it isn't necessary to create new savepoint on every retry at all. lock_exclusive() also moves start of transaction outside of the retry loop and creates a savepoint regardless of the value start_xact. --- bin/pg_repack.c | 45 ++++++++++++++++++++-------------- regress/expected/nosuper.out | 6 +---- regress/expected/nosuper_1.out | 6 +---- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/bin/pg_repack.c b/bin/pg_repack.c index b5db6959..4fdb733d 100644 --- a/bin/pg_repack.c +++ b/bin/pg_repack.c @@ -1799,6 +1799,8 @@ lock_access_share(PGconn *conn, Oid relid, const char *target_name) int i; bool ret = true; + pgut_command(conn, "SAVEPOINT repack_sp1", 0, NULL); + initStringInfo(&sql); for (i = 1; ; i++) @@ -1807,8 +1809,6 @@ lock_access_share(PGconn *conn, Oid relid, const char *target_name) PGresult *res; int wait_msec; - pgut_command(conn, "SAVEPOINT repack_sp1", 0, NULL); - duration = time(NULL) - start; /* Cancel queries unconditionally, i.e. don't bother waiting @@ -1856,6 +1856,13 @@ lock_access_share(PGconn *conn, Oid relid, const char *target_name) } termStringInfo(&sql); + + /* + * Make sure that the current transaction isn't in failed state before + * releasing the savepoint + */ + if (PQtransactionStatus(conn) == PQTRANS_INTRANS) + pgut_command(conn, "RELEASE SAVEPOINT repack_sp1", 0, NULL); pgut_command(conn, "RESET lock_timeout", 0, NULL); return ret; } @@ -1904,9 +1911,10 @@ static bool advisory_lock(PGconn *conn, const char *relid) * conn: connection to use * relid: OID of relation * lock_query: LOCK TABLE ... IN ACCESS EXCLUSIVE query to be executed - * start_xact: whether we will issue a BEGIN ourselves. If not, we will - * use a SAVEPOINT and ROLLBACK TO SAVEPOINT if our query - * times out, to avoid leaving the transaction in error state. + * start_xact: whether we will issue a BEGIN ourselves. Regardless of the + * value we will use a SAVEPOINT and ROLLBACK TO SAVEPOINT if our + * query times out, to avoid leaving the transaction in error + * state. */ static bool lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool start_xact) @@ -1915,6 +1923,10 @@ lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool sta int i; bool ret = true; + if (start_xact) + pgut_command(conn, "BEGIN ISOLATION LEVEL READ COMMITTED", 0, NULL); + pgut_command(conn, "SAVEPOINT repack_sp1", 0, NULL); + for (i = 1; ; i++) { time_t duration; @@ -1922,11 +1934,6 @@ lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool sta PGresult *res; int wait_msec; - if (start_xact) - pgut_command(conn, "BEGIN ISOLATION LEVEL READ COMMITTED", 0, NULL); - else - pgut_command(conn, "SAVEPOINT repack_sp1", 0, NULL); - duration = time(NULL) - start; if (duration > wait_timeout) { @@ -1936,10 +1943,7 @@ lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool sta ret = false; /* Before exit the loop reset the transaction */ - if (start_xact) - pgut_rollback(conn); - else - pgut_command(conn, "ROLLBACK TO SAVEPOINT repack_sp1", 0, NULL); + pgut_command(conn, "ROLLBACK TO SAVEPOINT repack_sp1", 0, NULL); break; } else @@ -1982,10 +1986,7 @@ lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool sta { /* retry if lock conflicted */ CLEARPGRES(res); - if (start_xact) - pgut_rollback(conn); - else - pgut_command(conn, "ROLLBACK TO SAVEPOINT repack_sp1", 0, NULL); + pgut_command(conn, "ROLLBACK TO SAVEPOINT repack_sp1", 0, NULL); continue; } else @@ -1998,6 +1999,14 @@ lock_exclusive(PGconn *conn, const char *relid, const char *lock_query, bool sta } } + /* + * Make sure that the current transaction isn't in failed state before + * releasing the savepoint + */ + if (PQtransactionStatus(conn) == PQTRANS_INTRANS) + pgut_command(conn, "RELEASE SAVEPOINT repack_sp1", 0, NULL); + if (!ret && start_xact) + pgut_rollback(conn); pgut_command(conn, "RESET lock_timeout", 0, NULL); return ret; } diff --git a/regress/expected/nosuper.out b/regress/expected/nosuper.out index 8d0a94ec..588af232 100644 --- a/regress/expected/nosuper.out +++ b/regress/expected/nosuper.out @@ -21,11 +21,7 @@ GRANT USAGE ON SCHEMA repack TO nosuper; -- => ERROR \! pg_repack --dbname=contrib_regression --table=tbl_cluster --username=nosuper --no-superuser-check INFO: repacking table "public.tbl_cluster" -ERROR: query failed: ERROR: current transaction is aborted, commands ignored until end of transaction block -DETAIL: query was: RESET lock_timeout -ERROR: query failed: ERROR: current transaction is aborted, commands ignored until end of transaction block -DETAIL: query was: RESET lock_timeout -ERROR: permission denied for table tbl_cluster +WARNING: lock_exclusive() failed for public.tbl_cluster ERROR: permission denied for table tbl_cluster REVOKE ALL ON ALL TABLES IN SCHEMA repack FROM nosuper; REVOKE USAGE ON SCHEMA repack FROM nosuper; diff --git a/regress/expected/nosuper_1.out b/regress/expected/nosuper_1.out index 3eda71f2..f922549b 100644 --- a/regress/expected/nosuper_1.out +++ b/regress/expected/nosuper_1.out @@ -21,11 +21,7 @@ GRANT USAGE ON SCHEMA repack TO nosuper; -- => ERROR \! pg_repack --dbname=contrib_regression --table=tbl_cluster --username=nosuper --no-superuser-check INFO: repacking table "public.tbl_cluster" -ERROR: query failed: ERROR: current transaction is aborted, commands ignored until end of transaction block -DETAIL: query was: RESET lock_timeout -ERROR: query failed: ERROR: current transaction is aborted, commands ignored until end of transaction block -DETAIL: query was: RESET lock_timeout -ERROR: permission denied for relation tbl_cluster +WARNING: lock_exclusive() failed for public.tbl_cluster ERROR: permission denied for relation tbl_cluster REVOKE ALL ON ALL TABLES IN SCHEMA repack FROM nosuper; REVOKE USAGE ON SCHEMA repack FROM nosuper; From b407e31d4c89c9188927bc69317ffee2ec3b53d9 Mon Sep 17 00:00:00 2001 From: Artur Zakirov Date: Tue, 8 Jul 2025 14:40:49 +0200 Subject: [PATCH 2/2] PGDG 18+ packages are built with the --with-libnuma option To be able to build pg_repack with Postgres 18 we need to install libnuma-dev: https://github.com/postgres/postgres/commit/65c298f61fc70f2f960437c05649f71b862e2c48 --- .github/workflows/regression.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/regression.yml b/.github/workflows/regression.yml index 1ec99103..b6ff888f 100644 --- a/.github/workflows/regression.yml +++ b/.github/workflows/regression.yml @@ -32,7 +32,7 @@ jobs: - name: Install additional build-dependencies for 18+ if: ${{ matrix.pg >= 18 }} - run: apt-get install -y libcurl4-openssl-dev + run: apt-get install -y libcurl4-openssl-dev libnuma-dev - name: Check out the repo uses: actions/checkout@v3