VACUUM (INTERRUPTIBLE)?
Hi,
Jeff Janes in [1]/messages/by-id/20200827213506.4baaanygq62q7pcj@alap3.anarazel.de found that I ([2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d72f6c750) broke autovacuum cancellations. It
obviously would be good to add a test for this, but it seems hard to
have that be reliable given how long it can take for autovacuum actually
get around to vacuum a specific table.
That made me wonder if it'd be worthwhile to add an option that'd make
user invoked VACUUM be interruptible by conflicting lock requests, just
like autovacuum is.
That's actually something I've wanted, and see other people want, a
couple times. Sometimes one wants to vacuum a table, but not block out
more important things like DDL. Which isn't really possible right now.
So how about adding an INTERRUPTIBLE option to VACUUM and ANALYZE?
Implementation wise it seems like the best way to implement this would
be to replace PROC_VACUUM_FOR_WRAPAROUND with
PROC_INTERRUPTIBLE_VACANALYZE or such (i.e. inverting the meaning).
One question I'm a bit split on is whether we'd want to continue
restricting the signalling in ProcSleep() to commands running VACUUM or
ANALYZE.
We could have a generic PROC_INTERRUPTIBLE_COMMAND or such, and have
deadlock.c / proc.c trigger whether to kill based on just that, without
looking at PROC_IS_AUTOVACUUM / PROC_IN_VACUUM.
Alternatively, if we do want to restrict it to VACUUM and ANALYZE, we'd
have to re-introduce PROC_IN_ANALYZE ;). After 12 years of not being
used and removed just weeks ago...
Greetings,
Andres Freund
[1]: /messages/by-id/20200827213506.4baaanygq62q7pcj@alap3.anarazel.de
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d72f6c750
On Tue, Sep 8, 2020 at 8:49 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
Jeff Janes in [1] found that I ([2]) broke autovacuum cancellations. It
obviously would be good to add a test for this, but it seems hard to
have that be reliable given how long it can take for autovacuum actually
get around to vacuum a specific table.That made me wonder if it'd be worthwhile to add an option that'd make
user invoked VACUUM be interruptible by conflicting lock requests, just
like autovacuum is.That's actually something I've wanted, and see other people want, a
couple times. Sometimes one wants to vacuum a table, but not block out
more important things like DDL. Which isn't really possible right now.So how about adding an INTERRUPTIBLE option to VACUUM and ANALYZE?
Implementation wise it seems like the best way to implement this would
be to replace PROC_VACUUM_FOR_WRAPAROUND with
PROC_INTERRUPTIBLE_VACANALYZE or such (i.e. inverting the meaning).One question I'm a bit split on is whether we'd want to continue
restricting the signalling in ProcSleep() to commands running VACUUM or
ANALYZE.We could have a generic PROC_INTERRUPTIBLE_COMMAND or such, and have
deadlock.c / proc.c trigger whether to kill based on just that, without
looking at PROC_IS_AUTOVACUUM / PROC_IN_VACUUM.Alternatively, if we do want to restrict it to VACUUM and ANALYZE, we'd
have to re-introduce PROC_IN_ANALYZE ;). After 12 years of not being
used and removed just weeks ago...
One thing I've been wanting many times but never properly got around to
investigating how much work it would be to make happen, was to be able to
trigger an autovacuum manually (yeah, strange choice of terms). That is,
instead of the above, you'd have something like "VACUUM BACKGROUND" which
would trigger an autovacuum worker to do the work, and then release your
session. The points then being both (1) the ability to interrupt it, and
(2) that it'd run in the backgorund and thus the foreground session could
disconnect.
I think both would probably solve your problem, and being able to trigger a
background one would add some extra value? But we'd have to figure out and
be clear about what to do if all workers are busy for example - queue or
error?
Worth considering, or am I missing something?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
On 2020-Sep-08, Andres Freund wrote:
That made me wonder if it'd be worthwhile to add an option that'd make
user invoked VACUUM be interruptible by conflicting lock requests, just
like autovacuum is.
Yeah, I recall a request for this in the past, too.
So how about adding an INTERRUPTIBLE option to VACUUM and ANALYZE?
+1 on adding it to VACUUM. I'm not sure about ANALYZE ... most of the
time it is not as bad as VACUUM in terms of blocking other things, and
things get ugly if that ANALYZE is part of a transaction. I think I'd
leave that out, since we don't have to cover all DDL that could take
ShareUpdateExclusive lock (CIC etc). VACUUM is especially problematic,
ISTM, which is we would do it for that.
Alternatively, if we do want to restrict it to VACUUM and ANALYZE, we'd
have to re-introduce PROC_IN_ANALYZE ;). After 12 years of not being
used and removed just weeks ago...
Hah :-)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On 2020-09-08 17:36:00 -0300, Alvaro Herrera wrote:
On 2020-Sep-08, Andres Freund wrote:
That made me wonder if it'd be worthwhile to add an option that'd make
user invoked VACUUM be interruptible by conflicting lock requests, just
like autovacuum is.Yeah, I recall a request for this in the past, too.
So how about adding an INTERRUPTIBLE option to VACUUM and ANALYZE?
+1 on adding it to VACUUM. I'm not sure about ANALYZE ... most of the
time it is not as bad as VACUUM in terms of blocking other things, and
things get ugly if that ANALYZE is part of a transaction. I think I'd
leave that out, since we don't have to cover all DDL that could take
ShareUpdateExclusive lock (CIC etc). VACUUM is especially problematic,
ISTM, which is we would do it for that.
ANALYZE sometimes is worse than VACUUM in some ways, it'll often take
longer on large tables (more random IO, no freeze map equivalent). And
there's still TRUNCATE / DROP TABLE etc that you want to be able to
interrupt (auto-)analyze.
I'm not sure it's really necessary to distinguish transaction /
non-transactional for ANALYZE. We continue to wait for the lock, right?
So signalling the transaction is fine, it'll error out. Even in the
case of ANALYZE in a subtransaction, we'll not do something incorrect,
we might just wait, if the top level transaction also has a lock, and
doesn't abort immediately.
So it doesn't seem that useful to not make manual analyze interruptible?
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
So it doesn't seem that useful to not make manual analyze interruptible?
I find the main attraction of this idea is that instead of saying
"autovacuum/autoanalyze have these magic behaviors", we can say
"autovacuum/autoanalyze use option FOO". So whatever behavior
autoanalyze has today, we should make available to manual analyze,
without quibbling too hard over how much of a use-case there is.
regards, tom lane
Hi,
On 2020-09-08 22:30:40 +0200, Magnus Hagander wrote:
One thing I've been wanting many times but never properly got around to
investigating how much work it would be to make happen, was to be able to
trigger an autovacuum manually (yeah, strange choice of terms). That is,
instead of the above, you'd have something like "VACUUM BACKGROUND" which
would trigger an autovacuum worker to do the work, and then release your
session. The points then being both (1) the ability to interrupt it, and
(2) that it'd run in the backgorund and thus the foreground session could
disconnect.I think both would probably solve your problem, and being able to trigger a
background one would add some extra value? But we'd have to figure out and
be clear about what to do if all workers are busy for example - queue or
error?Worth considering, or am I missing something?
It seems like it could be useful in general. Not that much for my case
however. It'd be much easier to test whether vacuum was successfully
cancelled if we can see the cancellation, which we can't in the
autovacuum case. And there'd likely be some fairly ugly logic around
needing to wait until the "autovacuum request" is processed etc,
including the case that all workers are currently busy.
So my INTERRUPTIBLE idea seems to be a better fit for the tests, and
independently quite useful. E.g. wanting to know whether VACUUM errored
out is useful for scripts that want their VACUUMs to be interruptible,
and that doesn't work well with the "backgrounding" idea of yours.
Having said that, your idea does seem like it could be helpful. The
difficulty seems to depend a bit on the exact desired
semantics. E.g. would the "queue" command block until vacuum started or
not? The latter would obviously be much easier...
Greetings,
Andres Freund
Hi,
On 2020-09-08 18:37:05 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
So it doesn't seem that useful to not make manual analyze interruptible?
I find the main attraction of this idea is that instead of saying
"autovacuum/autoanalyze have these magic behaviors", we can say
"autovacuum/autoanalyze use option FOO". So whatever behavior
autoanalyze has today, we should make available to manual analyze,
without quibbling too hard over how much of a use-case there is.
Yea, I think that's a fair point. It's basically more work to
distinguish the two anyway.
It was fairly straight forward to implement the basics of
INTERRUPTIBLE. However it seems like it might not actually address my
main concern, i.e. making this reliably testable with isolation
tester. At least not the way I envisioned it...
My idea for the test was to have one isolationtester session start a
seqscan, which would then reliably block a concurrent VACUUM (FREEZE,
INTERRUPTIBLE). That'd then give a stable point to switch back to the
first session, which would interrupt the VACUUM by doing a LOCK.
But because there's no known waiting-for pid for a buffer pin wait, we
currently don't detect that we're blocked :(.
Now, it'd not be too hard to make pg_isolation_test_session_is_blocked
also report being blocked when we're waiting for a buffer pin (ignoring
interesting_pids), similar to the safe snapshot test. However I'm
worried that that could lead to "false" reports of blocking? But maybe
that's a small enough risk because there's few unconditional cleanup
lock acquisitions?
Hacking such a wait condition test into
pg_isolation_test_session_is_blocked() successfully allows my test to
work for VACUUM.
Not yet sure about what a suitable way to test this for analyze would
be, unless we'd also accept VacuumDelay as a wait condition :(. I guess
one fairly easy way would be to include an expression index, and have
the expression index acquire an unavailable lock. Which'd allow to
switch to another session.
Greetings,
Andres Freund
Hi,
On 2020-09-08 19:30:40 -0700, Andres Freund wrote:
It was fairly straight forward to implement the basics of
INTERRUPTIBLE. However it seems like it might not actually address my
main concern, i.e. making this reliably testable with isolation
tester. At least not the way I envisioned it...My idea for the test was to have one isolationtester session start a
seqscan, which would then reliably block a concurrent VACUUM (FREEZE,
INTERRUPTIBLE). That'd then give a stable point to switch back to the
first session, which would interrupt the VACUUM by doing a LOCK.But because there's no known waiting-for pid for a buffer pin wait, we
currently don't detect that we're blocked :(.Now, it'd not be too hard to make pg_isolation_test_session_is_blocked
also report being blocked when we're waiting for a buffer pin (ignoring
interesting_pids), similar to the safe snapshot test. However I'm
worried that that could lead to "false" reports of blocking? But maybe
that's a small enough risk because there's few unconditional cleanup
lock acquisitions?Hacking such a wait condition test into
pg_isolation_test_session_is_blocked() successfully allows my test to
work for VACUUM.
Here's a patch series implementing that:
0001: Adds INTERRUPTIBLE to VACUUM ANALYZE
There's quite a few XXX's inside. And it needs some none
isolationtester test.
0002: Treat BufferPin as a waiting condition in isolationtest.
That's the aforementioned workaround.
0003: A test, finally.
But it only tests VACUUM, not yet ANALYZE. Perhaps also a test for
not allowing interruptions, somehow?
Clearly WIP, but good enough for some comments, I hope?
A few comments:
- Right now there can only be one such blocking task, because
PROC_IS_INTERRUPTIBLE is only set by vacuum / analyze, and the lock
levels are self exclusive. But it's generically named now, so it seems
just a matter of time until somebody adds that to other commands. I
think it's ok to not add support for ProcSleep() killing multiple
other processes?
- It's a bit annoying that normal user backends just see a generic
"ERROR: canceling statement due to user request". Do we need something
better?
- The order in which VACUUM parameters are documented & implemented
seems fairly random. Perhaps it'd make sense to reorder
alphabetically?
Not yet sure about what a suitable way to test this for analyze would
be, unless we'd also accept VacuumDelay as a wait condition :(. I guess
one fairly easy way would be to include an expression index, and have
the expression index acquire an unavailable lock. Which'd allow to
switch to another session.
But here I've not yet done anything. That just seems too ugly & fragile.
Greetings,
Andres Freund
Attachments:
v1-0001-WIP-Add-INTERRUPTIBLE-option-to-VACUUM-ANALYZE.patchtext/x-diff; charset=us-asciiDownload
From 29d4c1bdf536222c1e86c7502bbbf3fd49751a90 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 8 Sep 2020 20:47:38 -0700
Subject: [PATCH v1 1/3] WIP: Add INTERRUPTIBLE option to VACUUM & ANALYZE.
Partially because that's practically useful, partially because it's
useful for testing.
There's a few XXXs in the code, and there's a comment or two I have
intentionally not yet rewrapped.
---
src/include/commands/vacuum.h | 1 +
src/include/storage/lock.h | 6 ++--
src/include/storage/proc.h | 5 +--
src/backend/commands/vacuum.c | 14 +++++---
src/backend/postmaster/autovacuum.c | 2 ++
src/backend/storage/lmgr/deadlock.c | 54 ++++++++++++++++-------------
src/backend/storage/lmgr/proc.c | 24 +++++++------
doc/src/sgml/ref/analyze.sgml | 16 +++++++++
doc/src/sgml/ref/vacuum.sgml | 17 +++++++++
9 files changed, 94 insertions(+), 45 deletions(-)
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index d9475c99890..7e064ef45e9 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -215,6 +215,7 @@ typedef struct VacuumParams
int multixact_freeze_table_age; /* multixact age at which to scan
* whole table */
bool is_wraparound; /* force a for-wraparound vacuum */
+ bool is_interruptible; /* allow cancelling on lock conflict */
int log_min_duration; /* minimum execution threshold in ms at
* which verbose logs are activated, -1
* to use default */
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index 1c3e9c1999f..f21f9f3f974 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -499,8 +499,8 @@ typedef enum
DS_NO_DEADLOCK, /* no deadlock detected */
DS_SOFT_DEADLOCK, /* deadlock avoided by queue rearrangement */
DS_HARD_DEADLOCK, /* deadlock, no way out but ERROR */
- DS_BLOCKED_BY_AUTOVACUUM /* no deadlock; queue blocked by autovacuum
- * worker */
+ DS_BLOCKED_BY_INTERRUPTIBLE /* no deadlock; queue blocked by interruptible
+ * task (e.g. autovacuum worker) */
} DeadLockState;
/*
@@ -588,7 +588,7 @@ extern void lock_twophase_standby_recover(TransactionId xid, uint16 info,
void *recdata, uint32 len);
extern DeadLockState DeadLockCheck(PGPROC *proc);
-extern PGPROC *GetBlockingAutoVacuumPgproc(void);
+extern PGPROC *GetBlockingInterruptiblePgproc(void);
extern void DeadLockReport(void) pg_attribute_noreturn();
extern void RememberSimpleDeadLock(PGPROC *proc1,
LOCKMODE lockmode,
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae457..e660972a010 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -53,13 +53,14 @@ struct XidCache
*/
#define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */
#define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */
-#define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */
+#define PROC_IS_INTERRUPTIBLE 0x08 /* vacuum / analyze may be interrupted
+ * when locks conflict */
#define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical
* decoding outside xact */
/* flags reset at EOXact */
#define PROC_VACUUM_STATE_MASK \
- (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND)
+ (PROC_IN_VACUUM | PROC_IS_INTERRUPTIBLE)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index ddeec870d81..df717d2951a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -123,6 +123,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "skip_locked") == 0)
skip_locked = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "interruptible") == 0)
+ params.is_interruptible = defGetBoolean(opt);
else if (!vacstmt->is_vacuumcmd)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1754,19 +1756,21 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
* contents of other tables is arguably broken, but we won't break it
* here by violating transaction semantics.)
*
- * We also set the VACUUM_FOR_WRAPAROUND flag, which is passed down by
- * autovacuum; it's used to avoid canceling a vacuum that was invoked
- * in an emergency.
+ * We also set the INTERRUPTIBLE flag when user indicated or when
+ * started by autovacuum (except for anti-wraparound autovacuum, which
+ * we do not want to cancel), that governs whether the process may be
+ * cancelled upon a lock conflict.
*
* Note: these flags remain set until CommitTransaction or
* AbortTransaction. We don't want to clear them until we reset
* MyProc->xid/xmin, otherwise GetOldestNonRemovableTransactionId()
* might appear to go backwards, which is probably Not Good.
*/
+ Assert(!params->is_wraparound || !params->is_interruptible);
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
MyProc->vacuumFlags |= PROC_IN_VACUUM;
- if (params->is_wraparound)
- MyProc->vacuumFlags |= PROC_VACUUM_FOR_WRAPAROUND;
+ if (params->is_interruptible)
+ MyProc->vacuumFlags |= PROC_IS_INTERRUPTIBLE;
ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
LWLockRelease(ProcArrayLock);
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 1b8cd7bacd4..9fdc3f8ade1 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2899,6 +2899,8 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.multixact_freeze_min_age = multixact_freeze_min_age;
tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age;
tab->at_params.is_wraparound = wraparound;
+ /* Allow interrupting autovacuum unless it's an emergency one */
+ tab->at_params.is_interruptible = !wraparound;
tab->at_params.log_min_duration = log_min_duration;
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c
index e1246b8a4da..770b871f761 100644
--- a/src/backend/storage/lmgr/deadlock.c
+++ b/src/backend/storage/lmgr/deadlock.c
@@ -124,8 +124,8 @@ static int maxPossibleConstraints;
static DEADLOCK_INFO *deadlockDetails;
static int nDeadlockDetails;
-/* PGPROC pointer of any blocking autovacuum worker found */
-static PGPROC *blocking_autovacuum_proc = NULL;
+/* PGPROC pointer of any blocking but interruptible process found */
+static PGPROC *blocking_interruptible_proc = NULL;
/*
@@ -224,8 +224,8 @@ DeadLockCheck(PGPROC *proc)
nPossibleConstraints = 0;
nWaitOrders = 0;
- /* Initialize to not blocked by an autovacuum worker */
- blocking_autovacuum_proc = NULL;
+ /* Initialize to not blocked by an interruptible process */
+ blocking_interruptible_proc = NULL;
/* Search for deadlocks and possible fixes */
if (DeadLockCheckRecurse(proc))
@@ -278,24 +278,24 @@ DeadLockCheck(PGPROC *proc)
/* Return code tells caller if we had to escape a deadlock or not */
if (nWaitOrders > 0)
return DS_SOFT_DEADLOCK;
- else if (blocking_autovacuum_proc != NULL)
- return DS_BLOCKED_BY_AUTOVACUUM;
+ else if (blocking_interruptible_proc != NULL)
+ return DS_BLOCKED_BY_INTERRUPTIBLE;
else
return DS_NO_DEADLOCK;
}
/*
- * Return the PGPROC of the autovacuum that's blocking a process.
+ * Return the PGPROC of an interruptible process that's blocking a process.
*
* We reset the saved pointer as soon as we pass it back.
*/
PGPROC *
-GetBlockingAutoVacuumPgproc(void)
+GetBlockingInterruptiblePgproc(void)
{
PGPROC *ptr;
- ptr = blocking_autovacuum_proc;
- blocking_autovacuum_proc = NULL;
+ ptr = blocking_interruptible_proc;
+ blocking_interruptible_proc = NULL;
return ptr;
}
@@ -606,30 +606,36 @@ FindLockCycleRecurseMember(PGPROC *checkProc,
}
/*
- * No deadlock here, but see if this proc is an autovacuum
+ * No deadlock here, but see if this proc is an interruptible process
* that is directly hard-blocking our own proc. If so,
* report it so that the caller can send a cancel signal
* to it, if appropriate. If there's more than one such
* proc, it's indeterminate which one will be reported.
*
- * We don't touch autovacuums that are indirectly blocking
+ * We don't touch processes that are indirectly blocking
* us; it's up to the direct blockee to take action. This
* rule simplifies understanding the behavior and ensures
- * that an autovacuum won't be canceled with less than
- * deadlock_timeout grace period.
+ * such a process won't be canceled with a grace period of
+ * less than deadlock_timeout.
*
- * Note we read vacuumFlags without any locking. This is
- * OK only for checking the PROC_IS_AUTOVACUUM flag,
- * because that flag is set at process start and never
- * reset. There is logic elsewhere to avoid canceling an
- * autovacuum that is working to prevent XID wraparound
- * problems (which needs to read a different vacuumFlag
- * bit), but we don't do that here to avoid grabbing
- * ProcArrayLock.
+ * Note we read vacuumFlags without any locking. That is
+ * ok because 32bit reads are atomic, because surrounding
+ * operations provide strong enough memory barriers, and
+ * because we re-check whether IS_INTERRUPTIBLE is still
+ * set before actually cancelling the backend.
*/
if (checkProc == MyProc &&
- proc->vacuumFlags & PROC_IS_AUTOVACUUM)
- blocking_autovacuum_proc = proc;
+ proc->vacuumFlags & PROC_IS_INTERRUPTIBLE)
+ {
+ /*
+ * XXX: At some point there could be more than one
+ * proc blocking us here. Currently that's not
+ * possible because of the lock levels used by VACUUM
+ * / ANALYZE, which are the only ones to set this flag
+ * currently, but ...
+ */
+ blocking_interruptible_proc = proc;
+ }
/* We're done looking at this proclock */
break;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 19a9f939492..0b288ac6f2d 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1064,7 +1064,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
PROC_QUEUE *waitQueue = &(lock->waitProcs);
LOCKMASK myHeldLocks = MyProc->heldLocks;
bool early_deadlock = false;
- bool allow_autovacuum_cancel = true;
+ bool allow_interruptible_cancel = true;
ProcWaitStatus myWaitStatus;
PGPROC *proc;
PGPROC *leader = MyProc->lockGroupLeader;
@@ -1304,23 +1304,21 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
myWaitStatus = *((volatile ProcWaitStatus *) &MyProc->waitStatus);
/*
- * If we are not deadlocked, but are waiting on an autovacuum-induced
- * task, send a signal to interrupt it.
+ * If we are not deadlocked, but are waiting on an interruptible
+ * (e.g. autovacuum-induced) task, send a signal to interrupt it.
*/
- if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM && allow_autovacuum_cancel)
+ if (deadlock_state == DS_BLOCKED_BY_INTERRUPTIBLE && allow_interruptible_cancel)
{
- PGPROC *autovac = GetBlockingAutoVacuumPgproc();
+ PGPROC *autovac = GetBlockingInterruptiblePgproc();
uint8 vacuumFlags;
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
/*
- * Only do it if the worker is not working to protect against Xid
- * wraparound.
+ * Only do it if the process is still interruptible.
*/
vacuumFlags = ProcGlobal->vacuumFlags[autovac->pgxactoff];
- if ((vacuumFlags & PROC_IS_AUTOVACUUM) &&
- !(vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))
+ if (vacuumFlags & PROC_IS_INTERRUPTIBLE)
{
int pid = autovac->pid;
StringInfoData locktagbuf;
@@ -1340,8 +1338,12 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
LWLockRelease(ProcArrayLock);
/* send the autovacuum worker Back to Old Kent Road */
+ /*
+ * FIXME: do we want to continue to identify autovacuum here?
+ * Could do so based on PROC_IS_AUTOVACUUM.
+ */
ereport(DEBUG1,
- (errmsg("sending cancel to blocking autovacuum PID %d",
+ (errmsg("sending cancel to blocking autovacuum|XXX PID %d",
pid),
errdetail_log("%s", logbuf.data)));
@@ -1370,7 +1372,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
LWLockRelease(ProcArrayLock);
/* prevent signal from being resent more than once */
- allow_autovacuum_cancel = false;
+ allow_interruptible_cancel = false;
}
/*
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 5ac3ba83219..c7f3f58c6da 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -28,6 +28,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
SKIP_LOCKED [ <replaceable class="parameter">boolean</replaceable> ]
+ INTERRUPTIBLE [ <replaceable class="parameter">boolean</replaceable> ]
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -95,6 +96,21 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INTERRUPTIBLE</literal></term>
+ <listitem>
+ <para>
+ Specifies whether <command>ANALYZE</command> may be cancelled when
+ another connection needs a lock conflicting with this.
+ <command>ANALYZE</command>. That can be useful to reduce the impact of
+ running <command>ANALYZE</command> in a busy system, by e.g. allowing
+ DDL commands to run before <command>ANALYZE</command> has finished. If
+ the option is not specified <command>ANALYZE</command> is not
+ interruptible.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index a48f75ad7ba..1d69041566e 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -35,6 +35,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
INDEX_CLEANUP [ <replaceable class="parameter">boolean</replaceable> ]
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
+ INTERRUPTIBLE [ <replaceable class="parameter">boolean</replaceable> ]
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -255,6 +256,22 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INTERRUPTIBLE</literal></term>
+ <listitem>
+ <para>
+ Specifies whether <command>VACUUM</command> may be cancelled when
+ another connection needs a lock conflicting with this.
+ <command>VACUUM</command>. That can be useful to reduce the impact of
+ running <command>VACUUM</command> in a busy system, by e.g. allowing DDL
+ commands to run before <command>VACUUM</command> has finished. This
+ option is currently ignored if the <literal>FULL</literal> option is
+ used. If the option is not specified <command>VACUUM</command> is not
+ interruptible.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
--
2.25.0.114.g5b0ca878e0
v1-0002-WIP-Treat-BufferPin-as-a-waiting-condition-in-iso.patchtext/x-diff; charset=us-asciiDownload
From 6613850df90f60a6e9daa0677bd81c64d5eb3107 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 8 Sep 2020 20:49:37 -0700
Subject: [PATCH v1 2/3] WIP: Treat BufferPin as a waiting condition in
isolationtest.
---
src/include/storage/procarray.h | 1 +
src/backend/storage/ipc/procarray.c | 24 ++++++++++++++++++++++++
src/backend/utils/adt/lockfuncs.c | 25 +++++++++++++++++++++++++
3 files changed, 50 insertions(+)
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index ea8a876ca45..62ee6833787 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -66,6 +66,7 @@ extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
extern int BackendXidGetPid(TransactionId xid);
extern bool IsBackendPid(int pid);
+extern uint32 BackendPidGetWaitEvent(int pid);
extern VirtualTransactionId *GetCurrentVirtualXIDs(TransactionId limitXmin,
bool excludeXmin0, bool allDbs, int excludeVacuum,
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 1c0cd6b2487..5fb67e0e217 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3023,6 +3023,30 @@ BackendPidGetProcWithLock(int pid)
return result;
}
+/*
+ * BackendPidGetWaitEvent -- return wait event of a backend
+ *
+ * This return 0 both if there is no backend with the passed PID and if the
+ * backend exists but is not currently waiting.
+ */
+uint32
+BackendPidGetWaitEvent(int pid)
+{
+ PGPROC *proc;
+ uint32 raw_wait_event = 0;
+
+ LWLockAcquire(ProcArrayLock, LW_SHARED);
+
+ proc = BackendPidGetProcWithLock(pid);
+
+ if (proc)
+ raw_wait_event = UINT32_ACCESS_ONCE(proc->wait_event_info);
+
+ LWLockRelease(ProcArrayLock);
+
+ return raw_wait_event;
+}
+
/*
* BackendXidGetPid -- get a backend's pid given its XID
*
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index f592292d067..1f41a85d496 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -17,7 +17,9 @@
#include "catalog/pg_type.h"
#include "funcapi.h"
#include "miscadmin.h"
+#include "pgstat.h"
#include "storage/predicate_internals.h"
+#include "storage/procarray.h"
#include "utils/array.h"
#include "utils/builtins.h"
@@ -601,6 +603,7 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
int num_interesting_pids;
int num_blocking_pids;
int dummy;
+ uint32 raw_wait_event;
int i,
j;
@@ -653,6 +656,28 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
if (GetSafeSnapshotBlockingPids(blocked_pid, &dummy, 1) > 0)
PG_RETURN_BOOL(true);
+ raw_wait_event = BackendPidGetWaitEvent(blocked_pid);
+
+ if (raw_wait_event != 0)
+ {
+ const char* wait_event_type;
+ const char* wait_event;
+
+ /*
+ * FIXME: probably better to match on the integer value itself. But
+ * currently the class / event mask is not exposed outside pgstat.c...
+ */
+ wait_event_type = pgstat_get_wait_event_type(raw_wait_event);
+ wait_event = pgstat_get_wait_event(raw_wait_event);
+
+ if (wait_event_type != NULL && wait_event_type != NULL &&
+ strcmp(wait_event_type, "BufferPin") == 0 &&
+ strcmp(wait_event, "BufferPin") == 0)
+ {
+ PG_RETURN_BOOL(true);
+ }
+ }
+
PG_RETURN_BOOL(false);
}
--
2.25.0.114.g5b0ca878e0
v1-0003-WIP-Test-for-VACUUM-INTERRUPTIBLE-cancellation-wo.patchtext/x-diff; charset=us-asciiDownload
From c5061503e34fe4676388b17297a4c2f02354bcd5 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Tue, 8 Sep 2020 20:51:09 -0700
Subject: [PATCH v1 3/3] WIP: Test for VACUUM (INTERRUPTIBLE) cancellation
working.
Needs ANALYZE support. Perhaps also a better approach for switching
sessions once the VACUUM has started (see previous commit).
Author:
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
src/test/isolation/expected/vacuum-cancel.out | 45 +++++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/vacuum-cancel.spec | 37 +++++++++++++++
3 files changed, 83 insertions(+)
create mode 100644 src/test/isolation/expected/vacuum-cancel.out
create mode 100644 src/test/isolation/specs/vacuum-cancel.spec
diff --git a/src/test/isolation/expected/vacuum-cancel.out b/src/test/isolation/expected/vacuum-cancel.out
new file mode 100644
index 00000000000..2532fb4c7c7
--- /dev/null
+++ b/src/test/isolation/expected/vacuum-cancel.out
@@ -0,0 +1,45 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_begin s1_pin s2_delete s2_vacuum_interruptible s1_commit
+step s1_begin: BEGIN;
+step s1_pin:
+ DECLARE pin_page CURSOR WITHOUT HOLD FOR SELECT * FROM test_vacuum_cancel;
+ FETCH NEXT FROM pin_page;
+
+data
+
+somedata
+step s2_delete: DELETE FROM test_vacuum_cancel;
+step s2_vacuum_interruptible: VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; <waiting ...>
+step s1_commit: COMMIT;
+step s2_vacuum_interruptible: <... completed>
+
+starting permutation: s1_begin s1_pin s2_delete s2_analyze_interruptible s1_commit
+step s1_begin: BEGIN;
+step s1_pin:
+ DECLARE pin_page CURSOR WITHOUT HOLD FOR SELECT * FROM test_vacuum_cancel;
+ FETCH NEXT FROM pin_page;
+
+data
+
+somedata
+step s2_delete: DELETE FROM test_vacuum_cancel;
+step s2_analyze_interruptible: ANALYZE (INTERRUPTIBLE) test_vacuum_cancel;
+step s1_commit: COMMIT;
+
+starting permutation: s1_begin s1_pin s2_delete s2_vacuum_interruptible s1_lock s1_commit
+step s1_begin: BEGIN;
+step s1_pin:
+ DECLARE pin_page CURSOR WITHOUT HOLD FOR SELECT * FROM test_vacuum_cancel;
+ FETCH NEXT FROM pin_page;
+
+data
+
+somedata
+step s2_delete: DELETE FROM test_vacuum_cancel;
+step s2_vacuum_interruptible: VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; <waiting ...>
+step s1_lock: LOCK test_vacuum_cancel; <waiting ...>
+step s1_lock: <... completed>
+step s2_vacuum_interruptible: <... completed>
+error in steps s1_lock s2_vacuum_interruptible: ERROR: canceling statement due to user request
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 6acbb695ece..c02be96fb8c 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -78,6 +78,7 @@ test: timeouts
test: vacuum-concurrent-drop
test: vacuum-conflict
test: vacuum-skip-locked
+test: vacuum-cancel
test: predicate-hash
test: predicate-gist
test: predicate-gin
diff --git a/src/test/isolation/specs/vacuum-cancel.spec b/src/test/isolation/specs/vacuum-cancel.spec
new file mode 100644
index 00000000000..e41877dce42
--- /dev/null
+++ b/src/test/isolation/specs/vacuum-cancel.spec
@@ -0,0 +1,37 @@
+setup
+{
+ CREATE TABLE test_vacuum_cancel(data text);
+ /* don't want autovacuum clean up to-be-cleaned data concurrently */
+ ALTER TABLE test_vacuum_cancel SET (AUTOVACUUM_ENABLED = false);
+ /* insert some data */
+ INSERT INTO test_vacuum_cancel VALUES('somedata');
+ INSERT INTO test_vacuum_cancel VALUES('otherdata');
+}
+
+teardown
+{
+ DROP TABLE test_vacuum_cancel;
+}
+
+session "s1"
+step "s1_begin" { BEGIN; }
+step "s1_pin" {
+ DECLARE pin_page CURSOR WITHOUT HOLD FOR SELECT * FROM test_vacuum_cancel;
+ FETCH NEXT FROM pin_page;
+}
+step "s1_lock" { LOCK test_vacuum_cancel; }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+step "s2_delete" { DELETE FROM test_vacuum_cancel; }
+step "s2_vacuum_interruptible" { VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; }
+step "s2_analyze_interruptible" { ANALYZE (INTERRUPTIBLE) test_vacuum_cancel; }
+
+# First test pin release resolves issues
+permutation "s1_begin" "s1_pin" "s2_delete" "s2_vacuum_interruptible" "s1_commit"
+# XXX: This doesn't actually wait on the pin, need alternative wait trick
+permutation "s1_begin" "s1_pin" "s2_delete" "s2_analyze_interruptible" "s1_commit"
+
+# Then track that concurrent TRUNCATE interrupts VACUUM / ANALYZE
+permutation "s1_begin" "s1_pin" "s2_delete" "s2_vacuum_interruptible" "s1_lock" "s1_commit"
+#permutation "s1_begin" "s1_pin" "s2_delete" "s2_analyze_interruptible" "s3_truncate"
--
2.25.0.114.g5b0ca878e0
On Wed, Sep 9, 2020 at 12:58 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2020-09-08 22:30:40 +0200, Magnus Hagander wrote:
One thing I've been wanting many times but never properly got around to
investigating how much work it would be to make happen, was to be able to
trigger an autovacuum manually (yeah, strange choice of terms). That is,
instead of the above, you'd have something like "VACUUM BACKGROUND" which
would trigger an autovacuum worker to do the work, and then release your
session. The points then being both (1) the ability to interrupt it, and
(2) that it'd run in the backgorund and thus the foreground session could
disconnect.I think both would probably solve your problem, and being able to
trigger a
background one would add some extra value? But we'd have to figure out
and
be clear about what to do if all workers are busy for example - queue or
error?Worth considering, or am I missing something?
It seems like it could be useful in general. Not that much for my case
however. It'd be much easier to test whether vacuum was successfully
cancelled if we can see the cancellation, which we can't in the
autovacuum case. And there'd likely be some fairly ugly logic around
That does bring up the other thing that I even put together some hacky
patch for at one point (long since lost or badly-rebased-into-nothingness)
which is to have the stats collector track on a per-relation basis the
number of autovacuum interruptions that have happened on a specific table :)
But yes, with that it would still not be *as easy* to use, definitely.
needing to wait until the "autovacuum request" is processed etc,
including the case that all workers are currently busy.
So my INTERRUPTIBLE idea seems to be a better fit for the tests, and
independently quite useful. E.g. wanting to know whether VACUUM errored
out is useful for scripts that want their VACUUMs to be interruptible,
and that doesn't work well with the "backgrounding" idea of yours.Having said that, your idea does seem like it could be helpful. The
difficulty seems to depend a bit on the exact desired
semantics. E.g. would the "queue" command block until vacuum started or
not? The latter would obviously be much easier...
Yeah, that's where I stalled in my own thoughts about it I think. OTOH, why
wait for it to start, if you're not waiting for it to finish... But also,
if there is a max size of the queue, what do you do if you hit that one?
--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
Hi,
On 2020-09-08 21:11:47 -0700, Andres Freund wrote:
Not yet sure about what a suitable way to test this for analyze would
be, unless we'd also accept VacuumDelay as a wait condition :(. I guess
one fairly easy way would be to include an expression index, and have
the expression index acquire an unavailable lock. Which'd allow to
switch to another session.But here I've not yet done anything. That just seems too ugly & fragile.
I found a better way after a bunch of tinkering. Having an
isolationtester session lock pg_class in SHARE mode works, because both
VACUUM and ANALYZE update the relation's row. I *think* this is
reliable, due to the report_multiple_error_messages() logic.
This version also adds setting of PROC_INTERRUPTIBLE for ANALYZE
(INTERUPTIBLE), which the previous version didn't yet contain. As
currently implemented this means that autovacuum started ANALYZEs are
interruptible, which they were not before. That's dead trivial to
change, but to me it makes sense to leave it this way. But I also see an
argument that it'd be better to do so separately?
- It's a bit annoying that normal user backends just see a generic
"ERROR: canceling statement due to user request". Do we need something
better?
I think this is ok for this iteration, and instead at some point solve
this in a bit more generic manner. This isn't the only place where
carrying a bit more information about why and by whom a query is
cancelled would be very useful.
There's one XXX left in here, which is:
@@ -1340,8 +1338,12 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
LWLockRelease(ProcArrayLock);
/* send the autovacuum worker Back to Old Kent Road */
+ /*
+ * FIXME: do we want to continue to identify autovacuum here?
+ * Could do so based on PROC_IS_AUTOVACUUM.
+ */
ereport(DEBUG1,
- (errmsg("sending cancel to blocking autovacuum PID %d",
+ (errmsg("sending cancel to blocking autovacuum|XXX PID %d",
pid),
errdetail_log("%s", logbuf.data)));given that this is a DEBUG1 I'm inclined to just replace this with
"... blocking interruptible process with PID %d"
or such?
Comments?
Greetings,
Andres Freund
Attachments:
v2-0001-Add-INTERRUPTIBLE-option-to-VACUUM-ANALYZE-test-i.patchtext/x-diff; charset=us-asciiDownload
From bd3f6fdc187f92e8e98018e9c9cfd9cac8753dc7 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Wed, 30 Sep 2020 20:24:38 -0700
Subject: [PATCH v2] Add INTERRUPTIBLE option to VACUUM & ANALYZE, test
interruption.
Partially because that's practically useful, partially because it's
useful for testing (see 5871f09c985).
To implement add the new PROC_IS_INTERRUPTIBLE flag, controlling
whether a process should be interrupted if it holds a required
lock. As PROC_VACUUM_FOR_WRAPAROUND was only used to signal that a
PROC_IS_AUTOVACUUM proc should not be interrupted, I have removed
that.
Now that it's easier to trigger interruptible vacuums, add a test
doing so.
FIXME: change commit message based on whether we keep the behaviour of
autovacuum cancelling analyze or not.
Discussion: https://www.postgresql.org/message-id/20200909041147.amcitkxmlexgjfty%40alap3.anarazel.de
---
src/include/commands/vacuum.h | 1 +
src/include/storage/lock.h | 6 +-
src/include/storage/proc.h | 5 +-
src/backend/commands/analyze.c | 22 +++++++
src/backend/commands/vacuum.c | 14 +++--
src/backend/postmaster/autovacuum.c | 2 +
src/backend/storage/lmgr/deadlock.c | 63 ++++++++++---------
src/backend/storage/lmgr/proc.c | 24 +++----
doc/src/sgml/ref/analyze.sgml | 16 +++++
doc/src/sgml/ref/vacuum.sgml | 17 +++++
src/test/isolation/expected/vacuum-cancel.out | 41 ++++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/vacuum-cancel.spec | 55 ++++++++++++++++
13 files changed, 218 insertions(+), 49 deletions(-)
create mode 100644 src/test/isolation/expected/vacuum-cancel.out
create mode 100644 src/test/isolation/specs/vacuum-cancel.spec
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index d9475c99890..7e064ef45e9 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -215,6 +215,7 @@ typedef struct VacuumParams
int multixact_freeze_table_age; /* multixact age at which to scan
* whole table */
bool is_wraparound; /* force a for-wraparound vacuum */
+ bool is_interruptible; /* allow cancelling on lock conflict */
int log_min_duration; /* minimum execution threshold in ms at
* which verbose logs are activated, -1
* to use default */
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index 1c3e9c1999f..f21f9f3f974 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -499,8 +499,8 @@ typedef enum
DS_NO_DEADLOCK, /* no deadlock detected */
DS_SOFT_DEADLOCK, /* deadlock avoided by queue rearrangement */
DS_HARD_DEADLOCK, /* deadlock, no way out but ERROR */
- DS_BLOCKED_BY_AUTOVACUUM /* no deadlock; queue blocked by autovacuum
- * worker */
+ DS_BLOCKED_BY_INTERRUPTIBLE /* no deadlock; queue blocked by interruptible
+ * task (e.g. autovacuum worker) */
} DeadLockState;
/*
@@ -588,7 +588,7 @@ extern void lock_twophase_standby_recover(TransactionId xid, uint16 info,
void *recdata, uint32 len);
extern DeadLockState DeadLockCheck(PGPROC *proc);
-extern PGPROC *GetBlockingAutoVacuumPgproc(void);
+extern PGPROC *GetBlockingInterruptiblePgproc(void);
extern void DeadLockReport(void) pg_attribute_noreturn();
extern void RememberSimpleDeadLock(PGPROC *proc1,
LOCKMODE lockmode,
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9c9a50ae457..e660972a010 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -53,13 +53,14 @@ struct XidCache
*/
#define PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */
#define PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */
-#define PROC_VACUUM_FOR_WRAPAROUND 0x08 /* set by autovac only */
+#define PROC_IS_INTERRUPTIBLE 0x08 /* vacuum / analyze may be interrupted
+ * when locks conflict */
#define PROC_IN_LOGICAL_DECODING 0x10 /* currently doing logical
* decoding outside xact */
/* flags reset at EOXact */
#define PROC_VACUUM_STATE_MASK \
- (PROC_IN_VACUUM | PROC_VACUUM_FOR_WRAPAROUND)
+ (PROC_IN_VACUUM | PROC_IS_INTERRUPTIBLE)
/*
* We allow a small number of "weak" relation locks (AccessShareLock,
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 8af12b5c6b2..9bafd07e758 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -252,6 +252,15 @@ analyze_rel(Oid relid, RangeVar *relation,
pgstat_progress_start_command(PROGRESS_COMMAND_ANALYZE,
RelationGetRelid(onerel));
+ /* mark us as interruptible if appropriate */
+ if (params->is_interruptible)
+ {
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->vacuumFlags |= PROC_IS_INTERRUPTIBLE;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* Do the normal non-recursive ANALYZE. We can skip this for partitioned
* tables, which don't contain any rows.
@@ -276,6 +285,19 @@ analyze_rel(Oid relid, RangeVar *relation,
relation_close(onerel, NoLock);
pgstat_progress_end_command();
+
+ /*
+ * Reset flags if we set them. Need to that (in contrast to VACUUM)
+ * because analyze can be run within a transaction, so we can't rely on
+ * the end-of-xact code doing this for us.
+ */
+ if (params->is_interruptible)
+ {
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->vacuumFlags &= ~PROC_IS_INTERRUPTIBLE;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
}
/*
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index ddeec870d81..df717d2951a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -123,6 +123,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "skip_locked") == 0)
skip_locked = defGetBoolean(opt);
+ else if (strcmp(opt->defname, "interruptible") == 0)
+ params.is_interruptible = defGetBoolean(opt);
else if (!vacstmt->is_vacuumcmd)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -1754,19 +1756,21 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
* contents of other tables is arguably broken, but we won't break it
* here by violating transaction semantics.)
*
- * We also set the VACUUM_FOR_WRAPAROUND flag, which is passed down by
- * autovacuum; it's used to avoid canceling a vacuum that was invoked
- * in an emergency.
+ * We also set the INTERRUPTIBLE flag when user indicated or when
+ * started by autovacuum (except for anti-wraparound autovacuum, which
+ * we do not want to cancel), that governs whether the process may be
+ * cancelled upon a lock conflict.
*
* Note: these flags remain set until CommitTransaction or
* AbortTransaction. We don't want to clear them until we reset
* MyProc->xid/xmin, otherwise GetOldestNonRemovableTransactionId()
* might appear to go backwards, which is probably Not Good.
*/
+ Assert(!params->is_wraparound || !params->is_interruptible);
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
MyProc->vacuumFlags |= PROC_IN_VACUUM;
- if (params->is_wraparound)
- MyProc->vacuumFlags |= PROC_VACUUM_FOR_WRAPAROUND;
+ if (params->is_interruptible)
+ MyProc->vacuumFlags |= PROC_IS_INTERRUPTIBLE;
ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
LWLockRelease(ProcArrayLock);
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2cef56f115f..630e068649e 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2915,6 +2915,8 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.multixact_freeze_min_age = multixact_freeze_min_age;
tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age;
tab->at_params.is_wraparound = wraparound;
+ /* Allow interrupting autovacuum unless it's an emergency one */
+ tab->at_params.is_interruptible = !wraparound;
tab->at_params.log_min_duration = log_min_duration;
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c
index e1246b8a4da..425a81194f7 100644
--- a/src/backend/storage/lmgr/deadlock.c
+++ b/src/backend/storage/lmgr/deadlock.c
@@ -124,8 +124,8 @@ static int maxPossibleConstraints;
static DEADLOCK_INFO *deadlockDetails;
static int nDeadlockDetails;
-/* PGPROC pointer of any blocking autovacuum worker found */
-static PGPROC *blocking_autovacuum_proc = NULL;
+/* PGPROC pointer of any blocking but interruptible process found */
+static PGPROC *blocking_interruptible_proc = NULL;
/*
@@ -224,8 +224,8 @@ DeadLockCheck(PGPROC *proc)
nPossibleConstraints = 0;
nWaitOrders = 0;
- /* Initialize to not blocked by an autovacuum worker */
- blocking_autovacuum_proc = NULL;
+ /* Initialize to not blocked by an interruptible process */
+ blocking_interruptible_proc = NULL;
/* Search for deadlocks and possible fixes */
if (DeadLockCheckRecurse(proc))
@@ -278,24 +278,24 @@ DeadLockCheck(PGPROC *proc)
/* Return code tells caller if we had to escape a deadlock or not */
if (nWaitOrders > 0)
return DS_SOFT_DEADLOCK;
- else if (blocking_autovacuum_proc != NULL)
- return DS_BLOCKED_BY_AUTOVACUUM;
+ else if (blocking_interruptible_proc != NULL)
+ return DS_BLOCKED_BY_INTERRUPTIBLE;
else
return DS_NO_DEADLOCK;
}
/*
- * Return the PGPROC of the autovacuum that's blocking a process.
+ * Return the PGPROC of an interruptible process that's blocking a process.
*
* We reset the saved pointer as soon as we pass it back.
*/
PGPROC *
-GetBlockingAutoVacuumPgproc(void)
+GetBlockingInterruptiblePgproc(void)
{
PGPROC *ptr;
- ptr = blocking_autovacuum_proc;
- blocking_autovacuum_proc = NULL;
+ ptr = blocking_interruptible_proc;
+ blocking_interruptible_proc = NULL;
return ptr;
}
@@ -606,30 +606,37 @@ FindLockCycleRecurseMember(PGPROC *checkProc,
}
/*
- * No deadlock here, but see if this proc is an autovacuum
- * that is directly hard-blocking our own proc. If so,
- * report it so that the caller can send a cancel signal
- * to it, if appropriate. If there's more than one such
- * proc, it's indeterminate which one will be reported.
+ * No deadlock here, but see if this proc is an
+ * interruptible process that is directly hard-blocking
+ * our own proc. If so, report it so that the caller can
+ * send a cancel signal to it, if appropriate. If there's
+ * more than one such proc, it's indeterminate which one
+ * will be reported.
*
- * We don't touch autovacuums that are indirectly blocking
+ * We don't touch processes that are indirectly blocking
* us; it's up to the direct blockee to take action. This
* rule simplifies understanding the behavior and ensures
- * that an autovacuum won't be canceled with less than
- * deadlock_timeout grace period.
+ * such a process won't be canceled with a grace period of
+ * less than deadlock_timeout.
*
- * Note we read vacuumFlags without any locking. This is
- * OK only for checking the PROC_IS_AUTOVACUUM flag,
- * because that flag is set at process start and never
- * reset. There is logic elsewhere to avoid canceling an
- * autovacuum that is working to prevent XID wraparound
- * problems (which needs to read a different vacuumFlag
- * bit), but we don't do that here to avoid grabbing
- * ProcArrayLock.
+ * Note we read vacuumFlags without any locking. That is
+ * ok because 32bit reads are atomic, because surrounding
+ * operations provide strong enough memory barriers, and
+ * because we re-check whether IS_INTERRUPTIBLE is still
+ * set before actually cancelling the backend.
*/
if (checkProc == MyProc &&
- proc->vacuumFlags & PROC_IS_AUTOVACUUM)
- blocking_autovacuum_proc = proc;
+ proc->vacuumFlags & PROC_IS_INTERRUPTIBLE)
+ {
+ /*
+ * XXX: At some point there could be more than one
+ * proc blocking us here. Currently that's not
+ * possible because of the lock levels used by VACUUM
+ * / ANALYZE, which are the only ones to set this flag
+ * currently, but ...
+ */
+ blocking_interruptible_proc = proc;
+ }
/* We're done looking at this proclock */
break;
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 88566bd9fab..f95eb549e3c 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1064,7 +1064,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
PROC_QUEUE *waitQueue = &(lock->waitProcs);
LOCKMASK myHeldLocks = MyProc->heldLocks;
bool early_deadlock = false;
- bool allow_autovacuum_cancel = true;
+ bool allow_interruptible_cancel = true;
ProcWaitStatus myWaitStatus;
PGPROC *proc;
PGPROC *leader = MyProc->lockGroupLeader;
@@ -1304,23 +1304,21 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
myWaitStatus = *((volatile ProcWaitStatus *) &MyProc->waitStatus);
/*
- * If we are not deadlocked, but are waiting on an autovacuum-induced
- * task, send a signal to interrupt it.
+ * If we are not deadlocked, but are waiting on an interruptible
+ * (e.g. autovacuum-induced) task, send a signal to interrupt it.
*/
- if (deadlock_state == DS_BLOCKED_BY_AUTOVACUUM && allow_autovacuum_cancel)
+ if (deadlock_state == DS_BLOCKED_BY_INTERRUPTIBLE && allow_interruptible_cancel)
{
- PGPROC *autovac = GetBlockingAutoVacuumPgproc();
+ PGPROC *autovac = GetBlockingInterruptiblePgproc();
uint8 vacuumFlags;
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
/*
- * Only do it if the worker is not working to protect against Xid
- * wraparound.
+ * Only do it if the process is still interruptible.
*/
vacuumFlags = ProcGlobal->vacuumFlags[autovac->pgxactoff];
- if ((vacuumFlags & PROC_IS_AUTOVACUUM) &&
- !(vacuumFlags & PROC_VACUUM_FOR_WRAPAROUND))
+ if (vacuumFlags & PROC_IS_INTERRUPTIBLE)
{
int pid = autovac->pid;
StringInfoData locktagbuf;
@@ -1340,8 +1338,12 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
LWLockRelease(ProcArrayLock);
/* send the autovacuum worker Back to Old Kent Road */
+ /*
+ * FIXME: do we want to continue to identify autovacuum here?
+ * Could do so based on PROC_IS_AUTOVACUUM.
+ */
ereport(DEBUG1,
- (errmsg("sending cancel to blocking autovacuum PID %d",
+ (errmsg("sending cancel to blocking autovacuum|XXX PID %d",
pid),
errdetail_log("%s", logbuf.data)));
@@ -1370,7 +1372,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
LWLockRelease(ProcArrayLock);
/* prevent signal from being sent again more than once */
- allow_autovacuum_cancel = false;
+ allow_interruptible_cancel = false;
}
/*
diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 5ac3ba83219..c7f3f58c6da 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -28,6 +28,7 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
SKIP_LOCKED [ <replaceable class="parameter">boolean</replaceable> ]
+ INTERRUPTIBLE [ <replaceable class="parameter">boolean</replaceable> ]
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -95,6 +96,21 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INTERRUPTIBLE</literal></term>
+ <listitem>
+ <para>
+ Specifies whether <command>ANALYZE</command> may be cancelled when
+ another connection needs a lock conflicting with this.
+ <command>ANALYZE</command>. That can be useful to reduce the impact of
+ running <command>ANALYZE</command> in a busy system, by e.g. allowing
+ DDL commands to run before <command>ANALYZE</command> has finished. If
+ the option is not specified <command>ANALYZE</command> is not
+ interruptible.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 26ede69bb31..b3e31423f17 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -35,6 +35,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
INDEX_CLEANUP [ <replaceable class="parameter">boolean</replaceable> ]
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
+ INTERRUPTIBLE [ <replaceable class="parameter">boolean</replaceable> ]
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -255,6 +256,22 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INTERRUPTIBLE</literal></term>
+ <listitem>
+ <para>
+ Specifies whether <command>VACUUM</command> may be cancelled when
+ another connection needs a lock conflicting with this.
+ <command>VACUUM</command>. That can be useful to reduce the impact of
+ running <command>VACUUM</command> in a busy system, by e.g. allowing DDL
+ commands to run before <command>VACUUM</command> has finished. This
+ option is currently ignored if the <literal>FULL</literal> option is
+ used. If the option is not specified <command>VACUUM</command> is not
+ interruptible.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
diff --git a/src/test/isolation/expected/vacuum-cancel.out b/src/test/isolation/expected/vacuum-cancel.out
new file mode 100644
index 00000000000..7914df3f8a2
--- /dev/null
+++ b/src/test/isolation/expected/vacuum-cancel.out
@@ -0,0 +1,41 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1_begin s1_block_vacuum s2_delete s2_vacuum_interruptible s1_commit
+step s1_begin: BEGIN;
+step s1_block_vacuum: LOCK pg_class IN SHARE MODE;
+step s2_delete: DELETE FROM test_vacuum_cancel;
+step s2_vacuum_interruptible: VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; <waiting ...>
+step s1_commit: COMMIT;
+step s2_vacuum_interruptible: <... completed>
+
+starting permutation: s1_begin s1_block_vacuum s2_delete s2_analyze_interruptible s1_commit
+step s1_begin: BEGIN;
+step s1_block_vacuum: LOCK pg_class IN SHARE MODE;
+step s2_delete: DELETE FROM test_vacuum_cancel;
+step s2_analyze_interruptible: ANALYZE (INTERRUPTIBLE) test_vacuum_cancel; <waiting ...>
+step s1_commit: COMMIT;
+step s2_analyze_interruptible: <... completed>
+
+starting permutation: s1_begin s1_block_vacuum s2_delete s2_vacuum_interruptible s3_lock s3_commit s1_commit
+step s1_begin: BEGIN;
+step s1_block_vacuum: LOCK pg_class IN SHARE MODE;
+step s2_delete: DELETE FROM test_vacuum_cancel;
+step s2_vacuum_interruptible: VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; <waiting ...>
+step s3_lock: BEGIN; LOCK test_vacuum_cancel; <waiting ...>
+step s3_lock: <... completed>
+step s2_vacuum_interruptible: <... completed>
+error in steps s3_lock s2_vacuum_interruptible: ERROR: canceling statement due to user request
+step s3_commit: COMMIT;
+step s1_commit: COMMIT;
+
+starting permutation: s1_begin s1_block_vacuum s2_delete s2_analyze_interruptible s3_lock s3_commit s1_commit
+step s1_begin: BEGIN;
+step s1_block_vacuum: LOCK pg_class IN SHARE MODE;
+step s2_delete: DELETE FROM test_vacuum_cancel;
+step s2_analyze_interruptible: ANALYZE (INTERRUPTIBLE) test_vacuum_cancel; <waiting ...>
+step s3_lock: BEGIN; LOCK test_vacuum_cancel; <waiting ...>
+step s3_lock: <... completed>
+step s2_analyze_interruptible: <... completed>
+error in steps s3_lock s2_analyze_interruptible: ERROR: canceling statement due to user request
+step s3_commit: COMMIT;
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index aa386ab1a25..ff437fcbdd3 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -78,6 +78,7 @@ test: timeouts
test: vacuum-concurrent-drop
test: vacuum-conflict
test: vacuum-skip-locked
+test: vacuum-cancel
test: predicate-hash
test: predicate-gist
test: predicate-gin
diff --git a/src/test/isolation/specs/vacuum-cancel.spec b/src/test/isolation/specs/vacuum-cancel.spec
new file mode 100644
index 00000000000..f8e502bb99f
--- /dev/null
+++ b/src/test/isolation/specs/vacuum-cancel.spec
@@ -0,0 +1,55 @@
+setup
+{
+ CREATE TABLE test_vacuum_cancel(data text);
+ /* don't want autovacuum clean up to-be-cleaned data concurrently */
+ ALTER TABLE test_vacuum_cancel SET (AUTOVACUUM_ENABLED = false);
+ /* insert some data */
+ INSERT INTO test_vacuum_cancel VALUES('somedata'),('otherdata');
+}
+
+teardown
+{
+ DROP TABLE test_vacuum_cancel;
+}
+
+session "s1"
+step "s1_begin" { BEGIN; }
+step "s1_block_vacuum" { LOCK pg_class IN SHARE MODE; }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+step "s2_delete" { DELETE FROM test_vacuum_cancel; }
+step "s2_vacuum_interruptible" { VACUUM (FREEZE, INTERRUPTIBLE) test_vacuum_cancel; }
+step "s2_analyze_interruptible" { ANALYZE (INTERRUPTIBLE) test_vacuum_cancel; }
+
+session "s3"
+step "s3_lock" { BEGIN; LOCK test_vacuum_cancel; }
+step "s3_commit" { COMMIT; }
+
+# First test if releasing the lock on pg_class allows to continue
+permutation
+ # block vacuum
+ "s1_begin" "s1_block_vacuum" "s2_delete"
+ # vacuum, which will be blocked by the above
+ "s2_vacuum_interruptible"
+ # and allow to continue
+ "s1_commit"
+permutation "s1_begin" "s1_block_vacuum" "s2_delete" "s2_analyze_interruptible" "s1_commit"
+
+# Then track that concurrent LOCK interrupts VACUUM / ANALYZE
+permutation
+ # block vacuum
+ "s1_begin" "s1_block_vacuum" "s2_delete"
+ # vacuum, which will be blocked by the above
+ "s2_vacuum_interruptible"
+ # issue lock request conflicting with vacuum
+ "s3_lock"
+ # and release the blocking lock request again. Done separately, to
+ # ensure isolationtester schedules predictably
+ "s3_commit" "s1_commit"
+
+permutation
+ "s1_begin" "s1_block_vacuum" "s2_delete"
+ "s2_analyze_interruptible"
+ "s3_lock"
+ "s3_commit" "s1_commit"
--
2.28.0.651.g306ee63a70