Thread: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vs lookups
Hi all There's a minor race between commit_ts SLRU truncation and concurrent commit_ts lookups, where a lookup can check the lower valid bound xid without knowing it's already been truncated away. This would result in a SLRU lookup error. It's pretty low-harm since it's hard to trigger and the only problem is an error being emitted when we should otherwise return null/zero. Most notably you have to pass an xid that used to be within the datrozenxid but due to a concurrent vacuum has just moved outside it. This can't happen if you're passing the xmin of a tuple that still exists so it only matters for callers passing arbitrary XIDs in via pg_xact_commit_timestamp(...). The race window is bigger on standby because there we don't find out about the advance of the lower commit ts bound until the next checkpoint. But you still have to be looking at very old xids that don't exist on the heap anymore. We might as well fix it in HEAD, but it's totally pointless to back-patch, and the only part of the race that can be realistically hit is on standby, where we can't backpatch a fix w/o changing the xlog format. Nope. We could narrow the scope by limiting commit_ts slru truncation to just before a checkpoint, but given how hard this is to hit... I don't care. (This came up as part of the investigation I've been doing on the txid_status thread, where Robert pointed out a similar problem that can arise where txid_status races with clog truncation. I noticed the issue with standby while looking into that.) -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 28/12/16 15:01, Craig Ringer wrote: > Hi all > > There's a minor race between commit_ts SLRU truncation and concurrent > commit_ts lookups, where a lookup can check the lower valid bound xid > without knowing it's already been truncated away. This would result in > a SLRU lookup error. > > It's pretty low-harm since it's hard to trigger and the only problem > is an error being emitted when we should otherwise return null/zero. > Most notably you have to pass an xid that used to be within the > datrozenxid but due to a concurrent vacuum has just moved outside it. > This can't happen if you're passing the xmin of a tuple that still > exists so it only matters for callers passing arbitrary XIDs in via > pg_xact_commit_timestamp(...). > > The race window is bigger on standby because there we don't find out > about the advance of the lower commit ts bound until the next > checkpoint. But you still have to be looking at very old xids that > don't exist on the heap anymore. > > We might as well fix it in HEAD, but it's totally pointless to > back-patch, and the only part of the race that can be realistically > hit is on standby, where we can't backpatch a fix w/o changing the > xlog format. Nope. We could narrow the scope by limiting commit_ts > slru truncation to just before a checkpoint, but given how hard this > is to hit... I don't care. > > (This came up as part of the investigation I've been doing on the > txid_status thread, where Robert pointed out a similar problem that > can arise where txid_status races with clog truncation. I noticed the > issue with standby while looking into that.) > Hi, I remember thinking this might affect committs when I was reading that thread but didn't have time to investigate it yet myself. Thanks for doing the all the work yourself. About the patch, it looks good to me for master with the minor exception that: > + appendStringInfo(buf, "pageno %d, xid %u", > + trunc.pageno, trunc.oldestXid); This should probably say oldestXid instead of xid in the text description. About back-patching, I wonder if standby could be modified to move oldestXid based on pageno reverse calculation, but it's going to be slightly ugly. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vs lookups
From
Craig Ringer
Date:
On 28 December 2016 at 22:16, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > About the patch, it looks good to me for master with the minor exception > that: >> + appendStringInfo(buf, "pageno %d, xid %u", >> + trunc.pageno, trunc.oldestXid); > > This should probably say oldestXid instead of xid in the text description. Agreed. > About back-patching, I wonder if standby could be modified to move > oldestXid based on pageno reverse calculation, but it's going to be > slightly ugly. Not worth it IMO. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vs lookups
From
Craig Ringer
Date:
On 29 December 2016 at 16:51, Craig Ringer <craig@2ndquadrant.com> wrote: > On 28 December 2016 at 22:16, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > >> About the patch, it looks good to me for master with the minor exception >> that: >>> + appendStringInfo(buf, "pageno %d, xid %u", >>> + trunc.pageno, trunc.oldestXid); >> >> This should probably say oldestXid instead of xid in the text description. > > Agreed. Slightly amended attached. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Re: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vslookups
From
Peter Eisentraut
Date:
On 12/29/16 4:28 AM, Craig Ringer wrote: > On 29 December 2016 at 16:51, Craig Ringer <craig@2ndquadrant.com> wrote: >> On 28 December 2016 at 22:16, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: >> >>> About the patch, it looks good to me for master with the minor exception >>> that: >>>> + appendStringInfo(buf, "pageno %d, xid %u", >>>> + trunc.pageno, trunc.oldestXid); >>> >>> This should probably say oldestXid instead of xid in the text description. >> >> Agreed. > > Slightly amended attached. I've looked over this. It looks correct to me in principle. The commit message does not actually explain how the race on the standby is fixed by the patch. Also, I wonder whether we should not in vacuum.c change the order of the calls of SetTransactionIdLimit() and SetMultiXactIdLimit() as well, just to keep everything consistent. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vslookups
From
Alvaro Herrera
Date:
Peter Eisentraut wrote: > Also, I wonder whether we should not in vacuum.c change the order of the > calls of SetTransactionIdLimit() and SetMultiXactIdLimit() as well, just > to keep everything consistent. I am wary of doing that. The current coding is well battle-tested by now, but doing things in the opposite order, not at all. Pending some testing to show that there is no problem with a change, I would leave things as they are. Probably said testing is too onerous for the benefit (which is just a little consistency). What I fear is: what happens if a concurrent checkpoint reads the values between the two operations, and a crash occurs? I think that the checkpoint might save the updated values, so after crash recovery the truncate would not be executed, possibly leaving files around. Leaving files around might be dangerous for multixacts at least (it's probably harmless for xids). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jan 19, 2017 at 10:06 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Peter Eisentraut wrote: > >> Also, I wonder whether we should not in vacuum.c change the order of the >> calls of SetTransactionIdLimit() and SetMultiXactIdLimit() as well, just >> to keep everything consistent. > > I am wary of doing that. The current coding is well battle-tested by > now, but doing things in the opposite order, not at all. Pending some > testing to show that there is no problem with a change, I would leave > things as they are. Probably said testing is too onerous for the > benefit (which is just a little consistency). What I fear is: what > happens if a concurrent checkpoint reads the values between the two > operations, and a crash occurs? I think that the checkpoint might save > the updated values, so after crash recovery the truncate would not be > executed, possibly leaving files around. Leaving files around might be > dangerous for multixacts at least (it's probably harmless for xids). Don't both SLRUs eventually wrap? If so, leaving file around seems dangerous for either in equal measure. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vslookups
From
Alvaro Herrera
Date:
Robert Haas wrote: > On Thu, Jan 19, 2017 at 10:06 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Peter Eisentraut wrote: > > > >> Also, I wonder whether we should not in vacuum.c change the order of the > >> calls of SetTransactionIdLimit() and SetMultiXactIdLimit() as well, just > >> to keep everything consistent. > > > > I am wary of doing that. The current coding is well battle-tested by > > now, but doing things in the opposite order, not at all. Pending some > > testing to show that there is no problem with a change, I would leave > > things as they are. Probably said testing is too onerous for the > > benefit (which is just a little consistency). What I fear is: what > > happens if a concurrent checkpoint reads the values between the two > > operations, and a crash occurs? I think that the checkpoint might save > > the updated values, so after crash recovery the truncate would not be > > executed, possibly leaving files around. Leaving files around might be > > dangerous for multixacts at least (it's probably harmless for xids). > > Don't both SLRUs eventually wrap? If so, leaving file around seems > dangerous for either in equal measure. Well, multixact uses the whole 2^32 space as valid, while xid only uses half of it. I think you would have to crash on every checkpoint just between those two points for two billion xacts for there to be a problem for xids. Anyway this just reinforces my argument that we shouldn't move those calls -- moving only the commit_ts one is good. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vslookups
From
Alvaro Herrera
Date:
Peter Eisentraut wrote: > On 12/29/16 4:28 AM, Craig Ringer wrote: > > On 29 December 2016 at 16:51, Craig Ringer <craig@2ndquadrant.com> wrote: > >> On 28 December 2016 at 22:16, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote: > >> > >>> About the patch, it looks good to me for master with the minor exception > >>> that: > >>>> + appendStringInfo(buf, "pageno %d, xid %u", > >>>> + trunc.pageno, trunc.oldestXid); > >>> > >>> This should probably say oldestXid instead of xid in the text description. > >> > >> Agreed. > > > > Slightly amended attached. > > I've looked over this. It looks correct to me in principle. Thanks, pushed. I added a comment in vacuum.c, as well as removing the memcpy()s of the xlog record, which were unnecessary now that there's an intermediate struct. WAL version bumped. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services