Small Bug in GetConflictingVirtualXIDs

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

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

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

Alvaro Herrera <alvherre(at)commandprompt(dot)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

On Mon, 2009-12-21 at 10:38 -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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

On Monday 21 December 2009 16:38:07 Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)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

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

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

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

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

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

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

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

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