Horton
Menu

Thread

  1. Small Bug in GetConflictingVirtualXIDs

    Andres Freund <andres@anarazel.de> — 2009-12-21T03:02:37Z

    Hi Simon, Hi all,


HS currently does the following in GetConflictingVirtualXIDs


TransactionId pxmin = proc->xmin;

/*
 * We ignore an invalid pxmin because this means that backend
 * has no snapshot and cannot get another one while we hold exclusive lock.
 */
if (TransactionIdIsValid(pxmin) && !TransactionIdFollows(pxmin, limitXmin))
{
	VirtualTransactionId vxid;

	GET_VXID_FROM_PGPROC(vxid, *proc);
	if (VirtualTransactionIdIsValid(vxid))
		vxids[count++] = vxid;
}


The logic behind this seems fine except in the case of dropping a database. 
There you very well might have a open connection without an open snapshot.

This leads to nice errors when youre connected to a database on the slave 
without having a in progress transaction while dropping the database on the 
primary:


CET FATAL:  terminating connection due to administrator command
CET LOG:  shutting down
CET LOG:  restartpoint starting: shutdown immediate
CET FATAL:  could not open file "base/16604/1259": No such file or directory
CET CONTEXT:  writing block 5 of relation base/16604/1259
CET WARNING:  could not write block 5 of base/16604/1259
CET DETAIL:  Multiple failures --- write error might be permanent.
CET CONTEXT:  writing block 5 of relation base/16604/1259

Should easily be solvable through an extra parameter for 
GetConflictingVirtualXIDs. Want me to prepare a patch?

Andres

  2. Re: Small Bug in GetConflictingVirtualXIDs

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-21T07:46:39Z

    On Mon, 2009-12-21 at 04:02 +0100, Andres Freund wrote:

> The logic behind this seems fine except in the case of dropping a database. 
> There you very well might have a open connection without an open snapshot.

Yes, you're right, thanks for the report. 

I re-arranged the logic there recently to reduce the number of backends
that would conflict, so it looks like I broke that case when I did
that. 

> Should easily be solvable through an extra parameter for 
> GetConflictingVirtualXIDs. Want me to prepare a patch?

Much appreciated, thanks.

-- 
 Simon Riggs           www.2ndQuadrant.com

  3. Re: Small Bug in GetConflictingVirtualXIDs

    Alvaro Herrera <alvherre@commandprompt.com> — 2009-12-21T15:25:34Z

    Andres Freund wrote:

> The logic behind this seems fine except in the case of dropping a database. 
> There you very well might have a open connection without an open snapshot.

Perhaps the simplest fix is to ensure that drop database gets a snapshot?

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  4. Re: Small Bug in GetConflictingVirtualXIDs

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-21T15:38:07Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
> Andres Freund wrote:
>> The logic behind this seems fine except in the case of dropping a database. 
>> There you very well might have a open connection without an open snapshot.

> Perhaps the simplest fix is to ensure that drop database gets a snapshot?

I confess to not having followed the thread closely, but why is DROP
DATABASE special in this regard?  Wouldn't we soon find ourselves
needing every utility command to take a snapshot?

			regards, tom lane

  5. Re: Small Bug in GetConflictingVirtualXIDs

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-21T15:48:52Z

    On Mon, 2009-12-21 at 10:38 -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Andres Freund wrote:
> >> The logic behind this seems fine except in the case of dropping a database. 
> >> There you very well might have a open connection without an open snapshot.
> 
> > Perhaps the simplest fix is to ensure that drop database gets a snapshot?
> 
> I confess to not having followed the thread closely, but why is DROP
> DATABASE special in this regard?  Wouldn't we soon find ourselves
> needing every utility command to take a snapshot?

Andres has worded this a little imprecisely, causing a confusion. In
cases regarding HS we need to be clear whether the interacting sessions
are on the master or on the standby to understand the reasons for poor
interactions.

What he means is that you can be connected to the standby without an
open snapshot (from the standby) at the point we replay a drop database
command that had been run on the master. That case currently causes the
bug, created by my recent change to GetConflictingVirtualXids().

Giving the drop database a snapshot is not the answer. I expect Andres
to be able to fix this with a simple patch that would not effect the
case of normal running.

-- 
 Simon Riggs           www.2ndQuadrant.com

  6. Re: Small Bug in GetConflictingVirtualXIDs

    Andres Freund <af@cybertec.de> — 2009-12-21T16:04:58Z

    On Monday 21 December 2009 16:38:07 Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Andres Freund wrote:
> >> The logic behind this seems fine except in the case of dropping a
> >> database. There you very well might have a open connection without an
> >> open snapshot.
> > Perhaps the simplest fix is to ensure that drop database gets a snapshot?
> I confess to not having followed the thread closely, but why is DROP
> DATABASE special in this regard?  Wouldn't we soon find ourselves
> needing every utility command to take a snapshot?
Because most other "entities" are locked when you access them. So on the 
standby the AccessExlusive (generated on the master) will conflict with 
whatever lock you currently have on that entity (on the slave).
There are no locks for an idle session though.

Andres

  7. Re: Small Bug in GetConflictingVirtualXIDs

    Andres Freund <af@cybertec.de> — 2009-12-22T02:19:09Z

    On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> Giving the drop database a snapshot is not the answer. I expect Andres
> to be able to fix this with a simple patch that would not effect the
> case of normal running.
Actually its less simply than I had thought at first - I don't think the code 
ever handled that correctly.
I might be wrong there, my knowledge of the involved code is a bit sparse...
The whole conflict resolution builds on the concept of waiting for an VXid, but 
an idle backend does not have a valid vxid. Thats correct, right?
Sure, the code should be modifyable to handle that code mostly transparently 
(simply ignoring a invalid localTransactionId when the database id is valid), 
but ...

I am inclined to just unconditionally kill the users of the database. Its not 
like that would be an issue in production. I cant see a case where its 
important to run a session to its end on a database which was dropped on the 
master.
Opinions on that?

Andres

  8. Re: Small Bug in GetConflictingVirtualXIDs

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-22T10:42:30Z

    On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > Giving the drop database a snapshot is not the answer. I expect Andres
> > to be able to fix this with a simple patch that would not effect the
> > case of normal running.
> Actually its less simply than I had thought at first - I don't think the code 
> ever handled that correctly.
> I might be wrong there, my knowledge of the involved code is a bit sparse...
> The whole conflict resolution builds on the concept of waiting for an VXid, but 
> an idle backend does not have a valid vxid. Thats correct, right?

Yes, that's correct. I'll take this one back then.

> Sure, the code should be modifyable to handle that code mostly transparently 
> (simply ignoring a invalid localTransactionId when the database id is valid), 
> but ...
> 
> I am inclined to just unconditionally kill the users of the database. Its not 
> like that would be an issue in production. I cant see a case where its 
> important to run a session to its end on a database which was dropped on the 
> master.
> Opinions on that?

I don't see any mileage in making Startup process wait for an idle
session, so no real reason to wait for others either.

-- 
 Simon Riggs           www.2ndQuadrant.com

  9. Re: Small Bug in GetConflictingVirtualXIDs

    Andres Freund <af@cybertec.de> — 2009-12-22T18:03:20Z

    On Tuesday 22 December 2009 11:42:30 Simon Riggs wrote:
> On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> > On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > > Giving the drop database a snapshot is not the answer. I expect Andres
> > > to be able to fix this with a simple patch that would not effect the
> > > case of normal running.
> >
> > Actually its less simply than I had thought at first - I don't think the
> > code ever handled that correctly.
> > I might be wrong there, my knowledge of the involved code is a bit
> > sparse... The whole conflict resolution builds on the concept of waiting
> > for an VXid, but an idle backend does not have a valid vxid. Thats
> > correct, right?
> 
> Yes, that's correct. I'll take this one back then.
So youre writing a fix or shall I?

Andres

  10. Re: Small Bug in GetConflictingVirtualXIDs

    Andres Freund <af@cybertec.de> — 2009-12-27T19:11:26Z

    On Tuesday 22 December 2009 11:42:30 Simon Riggs wrote:
> On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> > On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > > Giving the drop database a snapshot is not the answer. I expect Andres
> > > to be able to fix this with a simple patch that would not effect the
> > > case of normal running.
> >
> > Actually its less simply than I had thought at first - I don't think the
> > code ever handled that correctly.
> > I might be wrong there, my knowledge of the involved code is a bit
> > sparse... The whole conflict resolution builds on the concept of waiting
> > for an VXid, but an idle backend does not have a valid vxid. Thats
> > correct, right?
> I don't see any mileage in making Startup process wait for an idle
> session, so no real reason to wait for others either.
So here is a small patch implementing that behaviour.

Andres

  11. Re: Small Bug in GetConflictingVirtualXIDs

    Simon Riggs <simon@2ndquadrant.com> — 2009-12-27T20:04:43Z

    On Sun, 2009-12-27 at 20:11 +0100, Andres Freund wrote:
> On Tuesday 22 December 2009 11:42:30 Simon Riggs wrote:
> > On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> > > On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > > > Giving the drop database a snapshot is not the answer. I expect Andres
> > > > to be able to fix this with a simple patch that would not effect the
> > > > case of normal running.
> > >
> > > Actually its less simply than I had thought at first - I don't think the
> > > code ever handled that correctly.
> > > I might be wrong there, my knowledge of the involved code is a bit
> > > sparse... The whole conflict resolution builds on the concept of waiting
> > > for an VXid, but an idle backend does not have a valid vxid. Thats
> > > correct, right?
> > I don't see any mileage in making Startup process wait for an idle
> > session, so no real reason to wait for others either.
> So here is a small patch implementing that behaviour.

OK, thanks. I have also written something, as mentioned. Will review.

-- 
 Simon Riggs           www.2ndQuadrant.com

  12. Re: Small Bug in GetConflictingVirtualXIDs

    Andres Freund <af@cybertec.de> — 2009-12-27T20:20:01Z

    On Sunday 27 December 2009 21:04:43 Simon Riggs wrote:
> On Sun, 2009-12-27 at 20:11 +0100, Andres Freund wrote:
> > On Tuesday 22 December 2009 11:42:30 Simon Riggs wrote:
> > > On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> > > > On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > > > > Giving the drop database a snapshot is not the answer. I expect
> > > > > Andres to be able to fix this with a simple patch that would not
> > > > > effect the case of normal running.
> > > >
> > > > Actually its less simply than I had thought at first - I don't think
> > > > the code ever handled that correctly.
> > > > I might be wrong there, my knowledge of the involved code is a bit
> > > > sparse... The whole conflict resolution builds on the concept of
> > > > waiting for an VXid, but an idle backend does not have a valid vxid.
> > > > Thats correct, right?
> > >
> > > I don't see any mileage in making Startup process wait for an idle
> > > session, so no real reason to wait for others either.
> >
> > So here is a small patch implementing that behaviour.
> 
> OK, thanks. I have also written something, as mentioned. Will review.
Thats why I had asked in another mail ;-) But I have learned a bit more about 
pg while writing that patch so its fine for me at least ;-)

Andres

  13. Re: Small Bug in GetConflictingVirtualXIDs

    Simon Riggs <simon@2ndquadrant.com> — 2010-01-10T15:49:00Z

    On Sun, 2009-12-27 at 20:11 +0100, Andres Freund wrote:
> On Tuesday 22 December 2009 11:42:30 Simon Riggs wrote:
> > On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> > > On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > > > Giving the drop database a snapshot is not the answer. I expect Andres
> > > > to be able to fix this with a simple patch that would not effect the
> > > > case of normal running.
> > >
> > > Actually its less simply than I had thought at first - I don't think the
> > > code ever handled that correctly.
> > > I might be wrong there, my knowledge of the involved code is a bit
> > > sparse... The whole conflict resolution builds on the concept of waiting
> > > for an VXid, but an idle backend does not have a valid vxid. Thats
> > > correct, right?
> > I don't see any mileage in making Startup process wait for an idle
> > session, so no real reason to wait for others either.
> So here is a small patch implementing that behaviour.

I've committed a slightly modified form of this patch.

It was an outstanding bug, so delaying fix at this stage not worth it. I
had in mind a slightly grander fix, but it's hardly a priority to polish
the chrome on this one.

Thanks for the bug report and fix.

-- 
 Simon Riggs           www.2ndQuadrant.com

  14. Re: Small Bug in GetConflictingVirtualXIDs

    Simon Riggs <simon@2ndquadrant.com> — 2010-01-14T11:27:42Z

    On Sun, 2009-12-27 at 20:11 +0100, Andres Freund wrote:
> On Tuesday 22 December 2009 11:42:30 Simon Riggs wrote:
> > On Tue, 2009-12-22 at 03:19 +0100, Andres Freund wrote:
> > > On Monday 21 December 2009 16:48:52 Simon Riggs wrote:
> > > > Giving the drop database a snapshot is not the answer. I expect Andres
> > > > to be able to fix this with a simple patch that would not effect the
> > > > case of normal running.
> > >
> > > Actually its less simply than I had thought at first - I don't think the
> > > code ever handled that correctly.
> > > I might be wrong there, my knowledge of the involved code is a bit
> > > sparse... The whole conflict resolution builds on the concept of waiting
> > > for an VXid, but an idle backend does not have a valid vxid. Thats
> > > correct, right?
> > I don't see any mileage in making Startup process wait for an idle
> > session, so no real reason to wait for others either.
> So here is a small patch implementing that behaviour.

On further testing, I received a re-connection from an automatic session
retry. That shouldn't have happened, but it indicates the need for
locking around the conflict handler. I had understood that to be placed
elsewhere but that seems wrong now.

This is a low priority item, so looking for a quick fix to allow time on
other areas.

Any objections?

-- 
 Simon Riggs           www.2ndQuadrant.com