Thread: Review: Hot standby
<br />I think we should avoid the #define's like below which uses a local variable. I guess the same #define is used
elsewherein the code as well. If the code is rearranged or the variable name is changed, the code would break.<br /><br
/>Theuse of malloc should also be avoided (unless the memory subsystem is not up yet; I haven't checked).<br /><br
/><br/>***************<br />*** 706,713 ****<br /> (errcode(ERRCODE_OUT_OF_MEMORY),<br
/> errmsg("out of memory")));<br /> Assert(snapshot->subxip == NULL);<br />
snapshot->subxip= (TransactionId *)<br />! malloc(arrayP->maxProcs * PGPROC_MAX_CACHED_SUBXIDS *
sizeof(TransactionId));<br/> if (snapshot->subxip == NULL)<br /> ereport(ERROR,<br
/> (errcode(ERRCODE_OUT_OF_MEMORY),<br />--- 1003,1011 ----<br />
(errcode(ERRCODE_OUT_OF_MEMORY),<br/> errmsg("out of memory")));<br />
Assert(snapshot->subxip== NULL);<br />+ #define maxNumSubXids (arrayP->maxProcs * PGPROC_MAX_CACHED_SUBXIDS)<br
/> snapshot->subxip = (TransactionId *)<br />! malloc(maxNumSubXids * sizeof(TransactionId));<br />
if (snapshot->subxip == NULL)<br /> ereport(ERROR,<br />
(errcode(ERRCODE_OUT_OF_MEMORY),<br/><br /><br clear="all" />Thanks,<br />Pavan<br /><br />-- <br />Pavan Deolasee<br
/>EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />
<br />I wonder if there is corner case below where there are no WAL records to replay during standby recovery.
Specifically,that may cause IsRunningXactDataValid() to return false since latestObservedXid remains set to
InvalidTransactionIdand that prevents postmaster from serving read-only clients.<br /><br />I don't have a test case,
butI recall seeing the issue while experimenting. Though that could be because of the WALInsertLock issue reported
earlier.<br/><br /> /*<br /> * Can we signal Postmaster to enter consistent recovery mode?<br
/> *<br /> * There are two points in the log that we must pass. The first<br
/> * is minRecoveryPoint, which is the LSN at the time the<br /> * base backup was taken
thatwe are about to rollfoward from.<br /> * If recovery has ever crashed or was stopped there is
also<br/> * another point also: minSafeStartPoint, which we know the<br /> * latest LSN
thatrecovery could have reached prior to crash.<br /> *<br /> * We must also have
assembledsufficient information about<br /> * transaction state to allow valid snapshots to be
taken.<br/> */<br /> if (!reachedSafeStartPoint &&<br />
IsRunningXactDataValid()&&<br /> XLByteLE(ControlFile->minSafeStartPoint, EndRecPtr)
&&<br/> XLByteLE(ControlFile->minRecoveryPoint, EndRecPtr))<br /> {<br
/> reachedSafeStartPoint = true;<br /> if (InArchiveRecovery)<br
/> {<br /> ereport(LOG,<br /> (errmsg("database has
nowreached consistent state at %X/%X",<br /> EndRecPtr.xlogid, EndRecPtr.xrecoff)));<br
/> InitRecoveryTransactionEnvironment();<br />
StartCleanupDelayStats();<br/> if (IsUnderPostmaster)<br />
SendPostmasterSignal(PMSIGNAL_RECOVERY_START);<br/> }<br /> }<br /><br /><br
/>Thanks,<br/>Pavan<br clear="all" /><br />-- <br />Pavan Deolasee<br />EnterpriseDB <a
href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
<br />+ /*<br />+ * Release locks, if any.<br />+ */<br />+ RelationReleaseRecoveryLocks(xlrec->slotId);<br/><br /><br />If I am reading the patch correctly, AccessExclusiveLocksacquired by a transaction will be released when the transaction is committed or aborted. If the transactionerrors out with FATAL, the locks will be released when the next transaction occupying the same slot is committed/aborted.<brclear="all" /><br />I smell some sort of deadlock condition here. What if the following events happen:<br/><br />- transaction A (slot 1) starts and acquires AEL lock on relation<br />- transaction A errors out with FATAL error<br />- transaction B (slot 1) starts and requests AEL lock on the same relation<br /><br />Won't B deadlockwith A ? Since B hasn't yet committed/aborted, the lock held by A is not released and <br />relation_redo_lock()would indefinitely wait for the lock.<br /><br />Thanks,<br />Pavan<br /><br />-- <br />Pavan Deolasee<br/> EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />
On Fri, 2008-11-21 at 13:22 +0530, Pavan Deolasee wrote: > > I think we should avoid the #define's like below which uses a local > variable. I guess the same #define is used elsewhere in the code as > well. If the code is rearranged or the variable name is changed, the > code would break. I use a #define because same value is used elsewhere in code. > The use of malloc should also be avoided (unless the memory subsystem > is not up yet; I haven't checked). The malloc was part of the existing code, explained by comments. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
On Fri, 2008-11-21 at 15:21 +0530, Pavan Deolasee wrote: > > + /* > + * Release locks, if any. > + */ > + RelationReleaseRecoveryLocks(xlrec->slotId); > > > If I am reading the patch correctly, AccessExclusiveLocks acquired by > a transaction will be released when the transaction is committed or > aborted. If the transaction errors out with FATAL, the locks will be > released when the next transaction occupying the same slot is > committed/aborted. > > I smell some sort of deadlock condition here. What if the following > events happen: > > - transaction A (slot 1) starts and acquires AEL lock on relation > - transaction A errors out with FATAL error > - transaction B (slot 1) starts and requests AEL lock on the same > relation > > Won't B deadlock with A ? Since B hasn't yet committed/aborted, the > lock held by A is not released and > relation_redo_lock() would indefinitely wait for the lock. This won't happen because the lock is held by the startup process on behalf of slot 1. Explained in comments in inval.c code. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
<br /><br /><div class="gmail_quote">On Fri, Nov 21, 2008 at 6:59 PM, Simon Riggs <span dir="ltr"><<a href="mailto:simon@2ndquadrant.com">simon@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br /><div class="Ih2E3d"><br/></div>The malloc was part of the existing code, explained by comments.<br /><font color="#888888"><br/></font></blockquote></div><br />Oh I see. But I don't see any explanations for using malloc insteadof palloc. Not that the current patch is responsible for this, I am wondering why its done that way and if we arefreeing the malloced memory at all ?<br /><br />malloc is used at another place in a new code. Although it seems thatthe allocation happens just once, please check if its better to use palloc there.<br /><br />Thanks,<br />Pavan<br clear="all"/><br />-- <br />Pavan Deolasee<br /> EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
On Sat, 2008-11-22 at 15:14 +0530, Pavan Deolasee wrote: > > The malloc was part of the existing code, explained by > comments. > > > Oh I see. But I don't see any explanations for using malloc instead of > palloc. Not that the current patch is responsible for this, I am > wondering why its done that way and if we are freeing the malloced > memory at all ? > > malloc is used at another place in a new code. Although it seems that > the allocation happens just once, please check if its better to use > palloc there. Thanks for your comments. OK, here's my thoughts after checking. The malloc in the current code occurs within GetSnapshotData where it exists to optimise memory allocation. The memory is allocated once and never released, so malloc is appropriate. The patch adds another use of malloc in GetRunningTransactionData() which is similar to GetSnapshotData in many ways. It only ever runs within bgwriter, and again, once allocated the memory is never released. So malloc is appropriate there also. I will change the #define as you suggested earlier. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Pavan Deolasee escribió: > On Fri, Nov 21, 2008 at 6:59 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > > > The malloc was part of the existing code, explained by comments. > > Oh I see. But I don't see any explanations for using malloc instead of > palloc. Not that the current patch is responsible for this, I am wondering > why its done that way and if we are freeing the malloced memory at all ? It's an optimization. We don't ever free it -- we alloc it once (the first time the snapshot is taken) and then the allocated space is reused until the backend dies. The reason for not using palloc is that if you're not going to do any context-related management, what would be the point? We save the palloc overhead this way (admittedly not a lot). -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Fri, 2008-11-21 at 14:17 +0530, Pavan Deolasee wrote: > I wonder if there is corner case I remain on the lookout for these, so thanks for thinking of this. > below where there are no WAL records to replay during standby > recovery. Specifically, that may cause IsRunningXactDataValid() to > return false since latestObservedXid remains set to > InvalidTransactionId and that prevents postmaster from serving > read-only clients. I don't think so, because this section of code is only called if there is redo to perform. If no redo, we never get here. There certainly is no guarantee that the correct conditions will ever exist to allow read-only access. If snapshots overflow consistently over a long period then the correct state will never be reached. That is very unlikely, but possible. I could prevent the delay, but then the code would so seldom run that it would probably have bugs, so it didn't seem worth trying to plug the gap. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
<br /><br /><div class="gmail_quote">On Sun, Nov 23, 2008 at 3:12 AM, Simon Riggs <span dir="ltr"><<a href="mailto:simon@2ndquadrant.com">simon@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br /><div class="Ih2E3d"><br/><br /></div>I don't think so, because this section of code is only called if there<br /> is redo to perform.If no redo, we never get here.<br /><br /></blockquote></div><br />OK. But why not open up the database for read-onlyaccess when there is no redo action to be done ? Or am I missing something trivial ?<br /><br />Anyways, I willwait for your latest version to continue with the test/review. <br clear="all" /><br />Thanks,<br />Pavan<br /><br />--<br />Pavan Deolasee<br />EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />
<br />The following sequence of commands causes server crash.<br /><br />postgres=# BEGIN TRANSACTION READ WRITE ;<br />BEGIN<br/>postgres=# SELECT * FROM pg_class FOR UPDATE;<br />FATAL: cannot assign TransactionIds during recovery<br />STATEMENT: SELECT * FROM pg_class FOR UPDATE;<br />FATAL: cannot assign TransactionIds during recovery<br />server closedthe connection unexpectedly<br /> This probably means the server terminated abnormally<br /> before orwhile processing the request.<br /> The connection to the server was lost. Attempting reset: Succeeded.<br /><br /><br/>I think we should disallow starting a read-write transaction during recovery. The patch attempts to do that by settingtransaction mode to read-only, but not enough to prevent somebody to explicitly mark the transaction as read-write.<brclear="all" /><br />Thanks,<br />Pavan<br /><br />-- <br />Pavan Deolasee<br />EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
"Pavan Deolasee" <pavan.deolasee@gmail.com> writes:
> I think we should disallow starting a read-write transaction during
> recovery. The patch attempts to do that by setting transaction mode to
> read-only, but not enough to prevent somebody to explicitly mark the
> transaction as read-write.
Huh? The "read only" transaction mode is not hard read-only anyway,
so if that's the only step being taken, it's entirely useless.
regards, tom lane
<br /><br /><div class="gmail_quote">On Tue, Nov 25, 2008 at 6:55 PM, Tom Lane <span dir="ltr"><<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>></span>wrote:<br /><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br /><br/></div>Huh? The "read only" transaction mode is not hard read-only anyway,<br /> so if that's the only step beingtaken, it's entirely useless.<br /><br /></blockquote></div><br />I think there are explicit checks for some utilitystatements (like VACUUM), but I haven't checked if all necessary code paths are covered or not.<br /><br />Thanks,<br/>Pavan<br /><br />-- <br />Pavan Deolasee<br /> EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
On Tue, 2008-11-25 at 19:02 +0530, Pavan Deolasee wrote: > On Tue, Nov 25, 2008 at 6:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Huh? The "read only" transaction mode is not hard read-only > anyway, > so if that's the only step being taken, it's entirely useless. > > > I think there are explicit checks for some utility statements (like > VACUUM), but I haven't checked if all necessary code paths are covered > or not. The commands that need protecting have been explicitly identified in the notes and there are 7 files changed that were specifically identified with protective changes. You've identified a way of breaking part the first line of defence, but the command was caught by the second line of defence in the patch. Problem, yes. Major issue, no. Will fix. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
<br />ISTM that the redo conflict resolution is not working as intended. I did the following test and it throws some surprises.<br/><br />On standby:<br /><br />postgres=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE ;<br />BEGIN<br />postgres=# SELECT * from test;<br /> a | b<br />-----+---<br /> 102 |<br /> 103 |<br />(2 rows)<br /><br /><br />On primary:<br/><br />postgres=# SELECT * from test;<br /> a | b<br />-----+---<br /> 102 |<br /> 103 |<br />(2 rows)<br /><br/>postgres=#<br /> postgres=# UPDATE test SET a = a + 100;<br />UPDATE 2<br />postgres=# VACUUM test;<br />VACUUM<br/>postgres=# SELECT pg_switch_xlog();<br /> pg_switch_xlog<br />----------------<br /> 0/2D000288<br />(1 row)<br/><br /><br />On standby (server log):<br /><br />LOG: restored log file "00000001000000000000002D" from archive<br/>LOG: recovery cancels activity of virtual transaction 2/2 pid 10593 because it blocks exclusive locks (currentdelay now 5 secs)<br />CONTEXT: xlog redo exclusive relation lock: slot 99 db 11517 rel 24576<br /> LOG: recoverycancels activity of virtual transaction 2/2 pid 10593 because it blocks exclusive locks (current delay now 5 secs)<br/>CONTEXT: xlog redo exclusive relation lock: slot 99 db 11517 rel 24576<br />LOG: recovery cancels activity ofvirtual transaction 2/2 pid 10593 because it blocks exclusive locks (current delay now 5 secs)<br /> CONTEXT: xlog redoexclusive relation lock: slot 99 db 11517 rel 24576<br />LOG: recovery cancels activity of virtual transaction 2/2 pid10593 because it blocks exclusive locks (current delay now 5 secs)<br />CONTEXT: xlog redo exclusive relation lock: slot99 db 11517 rel 24576<br /> LOG: recovery cancels activity of virtual transaction 2/2 pid 10593 because it blocks exclusivelocks (current delay now 5 secs)<br />CONTEXT: xlog redo exclusive relation lock: slot 99 db 11517 rel 24576<br/>LOG: recovery cancels activity of virtual transaction 2/2 pid 10593 because it blocks exclusive locks (currentdelay now 5 secs)<br /> CONTEXT: xlog redo exclusive relation lock: slot 99 db 11517 rel 24576<br />LOG: recoverycancels activity of virtual transaction 2/2 pid 10593 because it blocks exclusive locks (current delay now 5 secs)<br/>CONTEXT: xlog redo exclusive relation lock: slot 99 db 11517 rel 24576<br /> <same message repeated><br/><br /><br />The open transaction (see above) on the standby is not still not aborted and if I query thetable in the same transaction, I get:<br /><br />(Note: the transaction is still open)<br />postgres=#<br /> postgres=#SELECT * from test;<br /> a | b<br />---+---<br />(0 rows)<br /><br /><br />I think whats happening is that ResolveRecoveryConflictWithVirtualXIDs()is failing to abort the open transaction and it keeps trying for that, everytimedoubling the sleep time, so the LOG messages come less frequently later, but they are never ending. Soon the sleepbecomes exponentially large.<br /><br />Even though the standby has a open transaction, its obvious that the cleanup_redohas also failed to abort the transaction. Thats why the tuples have disappeared from the standby (most likelybecause they are cleaned up by VACUUM).<br /><br /><br />Thanks,<br />Pavan<br /><br />-- <br />Pavan Deolasee<br />EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br />
<br /><br /><div class="gmail_quote">On Wed, Nov 26, 2008 at 3:52 PM, Pavan Deolasee <span dir="ltr"><<a href="mailto:pavan.deolasee@gmail.com">pavan.deolasee@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote"style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br /><br/><br />I think whats happening is that ResolveRecoveryConflictWithVirtualXIDs() is failing to abort the open transaction</blockquote></div><br clear="all" /><br />Btw, ISTM that SIGINT works only for statement cancellation. So ifthe transaction is in idle state, SIGINT has nothing to cancel and hence also fails to abort the transaction.<br /><br/>Thanks,<br />Pavan<br /><br />-- <br />Pavan Deolasee<br />EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br/>
On Wed, 2008-11-26 at 18:02 +0530, Pavan Deolasee wrote: > I think whats happening is that > ResolveRecoveryConflictWithVirtualXIDs() is failing to abort > the open transaction > > > Btw, ISTM that SIGINT works only for statement cancellation. So if the > transaction is in idle state, SIGINT has nothing to cancel and hence > also fails to abort the transaction. [If I read this correctly this second post is the cause of the first problem, so we have one problem, rather than two.] Understood; yes that seems to be a problem. I will propose a solution later today. (I've been tied up with a few things over last few days, but I'm free of that now). -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
On Thu, 2008-11-27 at 17:14 +0000, Simon Riggs wrote: > On Wed, 2008-11-26 at 18:02 +0530, Pavan Deolasee wrote: > > > I think whats happening is that > > ResolveRecoveryConflictWithVirtualXIDs() is failing to abort > > the open transaction > > > > > > Btw, ISTM that SIGINT works only for statement cancellation. So if the > > transaction is in idle state, SIGINT has nothing to cancel and hence > > also fails to abort the transaction. > > [If I read this correctly this second post is the cause of the first > problem, so we have one problem, rather than two.] > > Understood; yes that seems to be a problem. After some thought, the way I would handle this is by sending a slightly different kind of signal. We can send a shared invalidation message which means "end the transaction, whether or not you are currently running a statement". We would then send the backend a SIGUSR1 to ensure that it reads the shared invalidation message as soon as possible. This is easily possible with the new sinval processing for 8.4. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> After some thought, the way I would handle this is by sending a slightly
> different kind of signal.
> We can send a shared invalidation message which means "end the
> transaction, whether or not you are currently running a statement".
No, a thousand times no. The sinval queue is an *utterly* inappropriate
mechanism for such a thing.
regards, tom lane
On Fri, 2008-11-28 at 11:14 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > After some thought, the way I would handle this is by sending a slightly > > different kind of signal. > > > We can send a shared invalidation message which means "end the > > transaction, whether or not you are currently running a statement". > > No, a thousand times no. So you're against it? ;-) > The sinval queue is an *utterly* inappropriate > mechanism for such a thing. To be honest, it did seem quite a neat solution. Any particular direction of thought you'd like me to pursue instead? Asking the backend to kill itself is much cleaner than the other ways I imagined. So my other thoughts steer towards hijacking the SIGUSR1 signal somehow for my nefarious purposes. Would that way sound OK? -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2008-11-28 at 11:14 -0500, Tom Lane wrote:
>> The sinval queue is an *utterly* inappropriate
>> mechanism for such a thing.
> To be honest, it did seem quite a neat solution. Any particular
> direction of thought you'd like me to pursue instead?
I hadn't been following the discussion closely enough to know what the
problem is. But "cancel the current transaction" is far outside the
bounds of what sinval events are supposed to do. If you try to do that
we'll forever be fighting bugs in code that expected
AcceptInvalidationMessages to do no more than invalidate cache entries.
regards, tom lane
On Fri, 2008-11-28 at 11:44 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > On Fri, 2008-11-28 at 11:14 -0500, Tom Lane wrote: > >> The sinval queue is an *utterly* inappropriate > >> mechanism for such a thing. > > > To be honest, it did seem quite a neat solution. Any particular > > direction of thought you'd like me to pursue instead? > > I hadn't been following the discussion closely enough to know what the > problem is. When we replay an AccessExclusiveLock on the standby we need to kick off any current lock holders, after a configurable grace period. Current lock holders may include some read-only backends that are idle-in-transaction. SIGINT, which is what the current patch uses, is not sufficient to dislodge the idle backends. So we need to send a signal to the idle backends and then have them react. We could use a multi-meaning approach for SIGUSR1 as we do for pmsignal, or ... -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2008-11-28 at 11:44 -0500, Tom Lane wrote:
>> I hadn't been following the discussion closely enough to know what the
>> problem is.
> When we replay an AccessExclusiveLock on the standby we need to kick off
> any current lock holders, after a configurable grace period. Current
> lock holders may include some read-only backends that are
> idle-in-transaction. SIGINT, which is what the current patch uses, is
> not sufficient to dislodge the idle backends.
Hm. People have complained of that fact from time to time in normal
usage as well. Should we simply change SIGINT handling to allow it to
cancel an idle transaction?
regards, tom lane
On Fri, 2008-11-28 at 12:45 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > On Fri, 2008-11-28 at 11:44 -0500, Tom Lane wrote: > >> I hadn't been following the discussion closely enough to know what the > >> problem is. > > > When we replay an AccessExclusiveLock on the standby we need to kick off > > any current lock holders, after a configurable grace period. Current > > lock holders may include some read-only backends that are > > idle-in-transaction. SIGINT, which is what the current patch uses, is > > not sufficient to dislodge the idle backends. > > Hm. People have complained of that fact from time to time in normal > usage as well. Should we simply change SIGINT handling to allow it to > cancel an idle transaction? Yes, that is by far the best solution. ISTM many people will be happy. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
On Fri, Nov 28, 2008 at 6:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Hm. People have complained of that fact from time to time in normal > usage as well. Should we simply change SIGINT handling to allow it to > cancel an idle transaction? If this means that we would be able to able to easily rollback the current transaction of a backend stuck in "idle in transaction" status, a big +1. -- Guillaume
Guillaume Smet wrote: > On Fri, Nov 28, 2008 at 6:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Hm. People have complained of that fact from time to time in normal >> usage as well. Should we simply change SIGINT handling to allow it to >> cancel an idle transaction? >> > > If this means that we would be able to able to easily rollback the > current transaction of a backend stuck in "idle in transaction" > status, a big +1. > > Yes, I have had clients caught by this regularly (last time less than a week ago), it would be a significant enhancement. cheers andrew
On Fri, 2008-11-28 at 12:45 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > On Fri, 2008-11-28 at 11:44 -0500, Tom Lane wrote: > >> I hadn't been following the discussion closely enough to know what the > >> problem is. > > > When we replay an AccessExclusiveLock on the standby we need to kick off > > any current lock holders, after a configurable grace period. Current > > lock holders may include some read-only backends that are > > idle-in-transaction. SIGINT, which is what the current patch uses, is > > not sufficient to dislodge the idle backends. > > Hm. People have complained of that fact from time to time in normal > usage as well. Should we simply change SIGINT handling to allow it to > cancel an idle transaction? I'm looking at allowing SIGINT cancel an idle transaction. Just edit StatementCancelHandler() in postgres.c, so that it doesn't ignore a signal when DoingCommandRead at line 2577. This patch does actually do what I wanted, but it has some unintended consequences as well. These mask the fact that it does actually work, which is confusing and has taken me a while to understand. The backend accepts the signal and throws an error which then cancels the transaction. The ERROR appears in the log immediately. However, a psql client does not respond in any way when this occurs and only when a new request is sent do we then generate the ERROR message on the client. pg_stat_activity continues to show "<IDLE> in transaction", even after global xmin is higher than the xid of the cancelled backend. Then afterwards the client gets out of sync with the server and starts putting replies on the wrong messages. Wow. I'm not sure why recv() doesn't return with EINTR, but I guess I'm about to find out. Hopefully? -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support