Re: [HACKERS] Hot Standby and query cancel

2010-01-14 Thread Simon Riggs 
On Wed, 2010-01-13 at 19:23 +, Simon Riggs wrote:
 On Wed, 2010-01-13 at 19:58 +0100, Andres Freund wrote:
 
   I am still testing patch, so should be confident to commit tomorrow
   barring issues.
  I have only looked at briefly because right now I dont have the time (going 
  to 
  eat at a friends place...) but I think I spotted an issue:
  The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt is 
  not 
  correct right now because that returns true for TBLOCK_SUBABORT as well.
  Wouldnt that mess with the case where were in a failed subxact and then 
  rollback only that subxact?
 
 Well spotted, yes. 

Latest version of same patch, but uses conflict reasons passed-thru
directly from recovery to backend.

Please review, no commit before tomorrow.

-- 
 Simon Riggs   www.2ndQuadrant.com
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
***
*** 313,320  IsTransactionState(void)
  /*
   *	IsAbortedTransactionBlockState
   *
!  *	This returns true if we are currently running a query
!  *	within an aborted transaction block.
   */
  bool
  IsAbortedTransactionBlockState(void)
--- 313,319 
  /*
   *	IsAbortedTransactionBlockState
   *
!  *	This returns true if we are within an aborted transaction block.
   */
  bool
  IsAbortedTransactionBlockState(void)
*** a/src/backend/storage/ipc/procarray.c
--- b/src/backend/storage/ipc/procarray.c
***
*** 324,329  ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
--- 324,330 
  		/* must be cleared with xid/xmin: */
  		proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
  		proc-inCommit = false; /* be sure this is cleared in abort */
+ 		proc-recoveryConflictPending = false;
  
  		/* Clear the subtransaction-XID cache too while holding the lock */
  		proc-subxids.nxids = 0;
***
*** 350,355  ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
--- 351,357 
  		/* must be cleared with xid/xmin: */
  		proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
  		proc-inCommit = false; /* be sure this is cleared in abort */
+ 		proc-recoveryConflictPending = false;
  
  		Assert(proc-subxids.nxids == 0);
  		Assert(proc-subxids.overflowed == false);
***
*** 377,383  ProcArrayClearTransaction(PGPROC *proc)
  	proc-xid = InvalidTransactionId;
  	proc-lxid = InvalidLocalTransactionId;
  	proc-xmin = InvalidTransactionId;
! 	proc-recoveryConflictMode = 0;
  
  	/* redundant, but just in case */
  	proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
--- 379,385 
  	proc-xid = InvalidTransactionId;
  	proc-lxid = InvalidLocalTransactionId;
  	proc-xmin = InvalidTransactionId;
! 	proc-recoveryConflictPending = false;
  
  	/* redundant, but just in case */
  	proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
***
*** 1665,1671  GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid,
  		if (proc-pid == 0)
  			continue;
  
! 		if (skipExistingConflicts  proc-recoveryConflictMode  0)
  			continue;
  
  		if (!OidIsValid(dbOid) ||
--- 1667,1673 
  		if (proc-pid == 0)
  			continue;
  
! 		if (skipExistingConflicts  proc-recoveryConflictPending)
  			continue;
  
  		if (!OidIsValid(dbOid) ||
***
*** 1704,1710  GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid,
   * Returns pid of the process signaled, or 0 if not found.
   */
  pid_t
! CancelVirtualTransaction(VirtualTransactionId vxid, int cancel_mode)
  {
  	ProcArrayStruct *arrayP = procArray;
  	int			index;
--- 1706,1712 
   * Returns pid of the process signaled, or 0 if not found.
   */
  pid_t
! CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
  {
  	ProcArrayStruct *arrayP = procArray;
  	int			index;
***
*** 1722,1749  CancelVirtualTransaction(VirtualTransactionId vxid, int cancel_mode)
  		if (procvxid.backendId == vxid.backendId 
  			procvxid.localTransactionId == vxid.localTransactionId)
  		{
! 			/*
! 			 * Issue orders for the proc to read next time it receives SIGINT
! 			 */
! 			if (proc-recoveryConflictMode  cancel_mode)
! proc-recoveryConflictMode = cancel_mode;
! 
  			pid = proc-pid;
  			break;
  		}
  	}
  
  	LWLockRelease(ProcArrayLock);
  
- 	if (pid != 0)
- 	{
- 		/*
- 		 * Kill the pid if it's still here. If not, that's what we wanted
- 		 * so ignore any errors.
- 		 */
- 		kill(pid, SIGINT);
- 	}
- 
  	return pid;
  }
  
--- 1724,1745 
  		if (procvxid.backendId == vxid.backendId 
  			procvxid.localTransactionId == vxid.localTransactionId)
  		{
! 			proc-recoveryConflictPending = true;
  			pid = proc-pid;
+ 			if (pid != 0)
+ 			{
+ /*
+  * Kill the pid if it's still here. If not, that's what we wanted
+  * so ignore any errors.
+  */
+ (void) SendProcSignal(pid, sigmode, vxid.backendId);
+ 			}
  			break;
  		}
  	}
  
  	LWLockRelease(ProcArrayLock);
  
  	return pid;
  }
  
***
*** 1834,1839  CancelDBBackends(Oid

Re: [HACKERS] Hot Standby and query cancel

2010-01-14 Thread Andres Freund 
On Thursday 14 January 2010 13:21:07 Simon Riggs wrote:
 On Wed, 2010-01-13 at 19:23 +, Simon Riggs wrote:
  On Wed, 2010-01-13 at 19:58 +0100, Andres Freund wrote:
I am still testing patch, so should be confident to commit tomorrow
barring issues.
   
   I have only looked at briefly because right now I dont have the time
   (going to eat at a friends place...) but I think I spotted an issue:
   The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt
   is not correct right now because that returns true for TBLOCK_SUBABORT
   as well. Wouldnt that mess with the case where were in a failed
   subxact and then rollback only that subxact?
  
  Well spotted, yes.
 
 Latest version of same patch, but uses conflict reasons passed-thru
 directly from recovery to backend.
 
 Please review, no commit before tomorrow.
I only noted a tiny thing (which was present earlier on):

snprintf(waitactivitymsg, sizeof(waitactivitymsg),
 waiting for max_standby_delay (%u ms),
 MaxStandbyDelay);
in ResolveRecoveryConflictWithVirtualXIDs.

Shouldnt that be seconds? Otherwise the check in WaitExceedsMaxStandbyDelay is 
wrong...


Andres

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Hot Standby and query cancel

2010-01-13 Thread Andres Freund 
Hi Simon,

On Wednesday 13 January 2010 19:24:22 Simon Riggs wrote:
 We've been chewing around query cancel on Hot Standby and I think things
 have got fairly confusing, hence a new thread.
Good idea.

 I enclose a patch that includes all the things that we all agree on so
 far, in my understanding
cool.

 * Recovery conflict processing uses SIGUSR1 rather than shmem per Tom,
 while holding ProcArrayLock per Andres
 
 * CONFLICT_MODE_ERROR throws ERROR when in a transaction, not idle and
 not in subtransaction, otherwise becomes CONFLICT_MODE_FATAL per Tom and
 other discussion
 
 * Recovery abort message has additional detail, per Heikki
 
 It doesn't include anything still under discussion, though is intended
 as a base upon which further patches can progress independently.

 I am still testing patch, so should be confident to commit tomorrow
 barring issues.
I have only looked at briefly because right now I dont have the time (going to 
eat at a friends place...) but I think I spotted an issue:
The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt is not 
correct right now because that returns true for TBLOCK_SUBABORT as well.
Wouldnt that mess with the case where were in a failed subxact and then 
rollback only that subxact?


Andres

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Hot Standby and query cancel

2010-01-13 Thread Simon Riggs 
On Wed, 2010-01-13 at 19:58 +0100, Andres Freund wrote:

  I am still testing patch, so should be confident to commit tomorrow
  barring issues.
 I have only looked at briefly because right now I dont have the time (going 
 to 
 eat at a friends place...) but I think I spotted an issue:
 The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt is 
 not 
 correct right now because that returns true for TBLOCK_SUBABORT as well.
 Wouldnt that mess with the case where were in a failed subxact and then 
 rollback only that subxact?

Well spotted, yes. 

-- 
 Simon Riggs   www.2ndQuadrant.com
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
***
*** 313,320  IsTransactionState(void)
  /*
   *	IsAbortedTransactionBlockState
   *
!  *	This returns true if we are currently running a query
!  *	within an aborted transaction block.
   */
  bool
  IsAbortedTransactionBlockState(void)
--- 313,319 
  /*
   *	IsAbortedTransactionBlockState
   *
!  *	This returns true if we are within an aborted transaction block.
   */
  bool
  IsAbortedTransactionBlockState(void)
*** a/src/backend/storage/ipc/procarray.c
--- b/src/backend/storage/ipc/procarray.c
***
*** 324,329  ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
--- 324,330 
  		/* must be cleared with xid/xmin: */
  		proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
  		proc-inCommit = false; /* be sure this is cleared in abort */
+ 		proc-recoveryConflictPending = false;
  
  		/* Clear the subtransaction-XID cache too while holding the lock */
  		proc-subxids.nxids = 0;
***
*** 350,355  ProcArrayEndTransaction(PGPROC *proc, TransactionId latestXid)
--- 351,357 
  		/* must be cleared with xid/xmin: */
  		proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
  		proc-inCommit = false; /* be sure this is cleared in abort */
+ 		proc-recoveryConflictPending = false;
  
  		Assert(proc-subxids.nxids == 0);
  		Assert(proc-subxids.overflowed == false);
***
*** 377,383  ProcArrayClearTransaction(PGPROC *proc)
  	proc-xid = InvalidTransactionId;
  	proc-lxid = InvalidLocalTransactionId;
  	proc-xmin = InvalidTransactionId;
! 	proc-recoveryConflictMode = 0;
  
  	/* redundant, but just in case */
  	proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
--- 379,385 
  	proc-xid = InvalidTransactionId;
  	proc-lxid = InvalidLocalTransactionId;
  	proc-xmin = InvalidTransactionId;
! 	proc-recoveryConflictPending = false;
  
  	/* redundant, but just in case */
  	proc-vacuumFlags = ~PROC_VACUUM_STATE_MASK;
***
*** 1665,1671  GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid,
  		if (proc-pid == 0)
  			continue;
  
! 		if (skipExistingConflicts  proc-recoveryConflictMode  0)
  			continue;
  
  		if (!OidIsValid(dbOid) ||
--- 1667,1673 
  		if (proc-pid == 0)
  			continue;
  
! 		if (skipExistingConflicts  proc-recoveryConflictPending)
  			continue;
  
  		if (!OidIsValid(dbOid) ||
***
*** 1704,1710  GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid,
   * Returns pid of the process signaled, or 0 if not found.
   */
  pid_t
! CancelVirtualTransaction(VirtualTransactionId vxid, int cancel_mode)
  {
  	ProcArrayStruct *arrayP = procArray;
  	int			index;
--- 1706,1712 
   * Returns pid of the process signaled, or 0 if not found.
   */
  pid_t
! CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
  {
  	ProcArrayStruct *arrayP = procArray;
  	int			index;
***
*** 1722,1749  CancelVirtualTransaction(VirtualTransactionId vxid, int cancel_mode)
  		if (procvxid.backendId == vxid.backendId 
  			procvxid.localTransactionId == vxid.localTransactionId)
  		{
! 			/*
! 			 * Issue orders for the proc to read next time it receives SIGINT
! 			 */
! 			if (proc-recoveryConflictMode  cancel_mode)
! proc-recoveryConflictMode = cancel_mode;
! 
  			pid = proc-pid;
  			break;
  		}
  	}
  
  	LWLockRelease(ProcArrayLock);
  
- 	if (pid != 0)
- 	{
- 		/*
- 		 * Kill the pid if it's still here. If not, that's what we wanted
- 		 * so ignore any errors.
- 		 */
- 		kill(pid, SIGINT);
- 	}
- 
  	return pid;
  }
  
--- 1724,1745 
  		if (procvxid.backendId == vxid.backendId 
  			procvxid.localTransactionId == vxid.localTransactionId)
  		{
! 			proc-recoveryConflictPending = true;
  			pid = proc-pid;
+ 			if (pid != 0)
+ 			{
+ /*
+  * Kill the pid if it's still here. If not, that's what we wanted
+  * so ignore any errors.
+  */
+ (void) SendProcSignal(pid, sigmode, vxid.backendId);
+ 			}
  			break;
  		}
  	}
  
  	LWLockRelease(ProcArrayLock);
  
  	return pid;
  }
  
***
*** 1834,1839  CancelDBBackends(Oid databaseid)
--- 1830,1836 
  {
  	ProcArrayStruct *arrayP = procArray;
  	int			index;
+ 	pid_t		pid = 0;
  
  	/* tell all backends to die */
  	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
***
***