Обсуждение: Re: [HACKERS] [PATCH] Fix minor race in commit_ts SLRU truncation vslookups

On 1/19/17 10:06 AM, Alvaro Herrera 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.

I appreciate this hesitation.

The issue the patch under discussion is addressing is that we have a
user-space interface to read information about commit timestamps.  So if
we truncate away old information before we update the mark where the
good information starts, then we get the race.  We don't have a
user-space interface reading, say, the clog, but it doesn't seem
implausible for that to exist at some point.  How would this code have
to be structured then?

> 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).

Why is leaving files around dangerous?  If this is a problem, why is it
not a problem for commit timestamps?  I don't understand why these
different SLRU uses are different.

Yeah, we could go ahead with this patch as is and it might be fine, but
I feel like we don't understand the broader issue completely yet.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut wrote:
> On 1/19/17 10:06 AM, Alvaro Herrera 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.
> 
> I appreciate this hesitation.
> 
> The issue the patch under discussion is addressing is that we have a
> user-space interface to read information about commit timestamps.  So if
> we truncate away old information before we update the mark where the
> good information starts, then we get the race.

Correct.

> We don't have a user-space interface reading, say, the clog, but it
> doesn't seem implausible for that to exist at some point.  How would
> this code have to be structured then?

One option would be to add another limit Xid which advances before the
truncation but which is not used for other decisions other than limiting
what can users consult.  Another option is not to implement direct reads
from the clog.  Yet another option is that before we add such interface
somebody produces proof that the problem does not, in fact, exist.  I
think producing proof is onerous enough that nobody will step up to it
until there is some real need for user-invoked clog lookups.  (We've
gone so long without them, perhaps we will never need them.)

> > 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).
> 
> Why is leaving files around dangerous?

Leftover files could confuse the truncation algorithm.  We already had
this problem for multixacts, and it took a lot of sweat and blood to get
it fixed.  The algorithm has since been fixed, but the mechanism is
delicate enough that I am afraid that tinkering with it may break it.

> If this is a problem, why is it not a problem for commit timestamps?
> I don't understand why these different SLRU uses are different.

commit_ts heavily depends on the clog cycle, so my thinking is that if
clog is protected enough, then so is commit_ts.  Given a strong clog, I
expect commit_ts not to break.  If I make clog brittle, will that break
commit_ts too?  Perhaps.  Maybe not, but again that would require more
study.

I have looked at clog a bit more this morning and I think perhaps it is
safe to make it work in the same way as commit_ts -- but somebody would
have to go to the trouble of verifying that it is.

> Yeah, we could go ahead with this patch as is and it might be fine, but
> I feel like we don't understand the broader issue completely yet.

ISTM the more complicated uses of slru.c have turned out to have a lot
of emergent properties that weren't completely understood, so yeah I
agree that we don't (or at least I don't, and most people don't either).
Someday, somebody will rewrite slru.c and/or remove multixacts, and
these problems will all go away.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 20 January 2017 at 05:32, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 1/19/17 10:06 AM, Alvaro Herrera 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.
>
> I appreciate this hesitation.
>
> The issue the patch under discussion is addressing is that we have a
> user-space interface to read information about commit timestamps.  So if
> we truncate away old information before we update the mark where the
> good information starts, then we get the race.  We don't have a
> user-space interface reading, say, the clog, but it doesn't seem
> implausible for that to exist at some point.  How would this code have
> to be structured then?

I have a patch in the current commitfest that exposes just such a user
interface, txid_status() .

See https://commitfest.postgresql.org/12/730/ .

Robert was about to commit when he identified this race in commit
status lookup, which led me to identify the same race addressed here
for commit timestamps.

>> 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).
>
> Why is leaving files around dangerous?

As far as I can tell, leaving the files around as such isn't dangerous.

The problem arises if we wrap around and start treating old SLRU
contents as new again without first truncating them away.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 20 January 2017 at 21:40, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> One option would be to add another limit Xid which advances before the
> truncation but which is not used for other decisions other than limiting
> what can users consult.

This could be useful for other things, but it's probably heavier than needed.

What I've done in the latest revision of the txid_status() patch is
simply to advance OldestXid _before_ truncating the clog. The rest of
the xid info is advanced after. Currently this is incorporated into
the txid_status patch, but can be separated if desired.

Relevant commit message portion:
   There was previously no way to look up an arbitrary xid without   running the risk of having clog truncated out from
underyou. This   hasn't been a problem because anything looking up xids in clog knows   they're protected by datminxid,
butthat's not the case for arbitrary   user-supplied XIDs. clog was truncated before we advance oldestXid so   taking
XidGenLockwas insufficient. There's no way to look up a   SLRU with soft-failure. To address this, increase oldestXid
underXidGenLock   before we trunate clog rather than after, so concurrent access is safe.
 

Note that while oldestXid is advanced before clog truncation, the xid
limits are advanced _after_ it. If we advanced the xid limits before
truncation too, we'd theoretically run the risk of allocating an xid
from the clog section we're about to truncate, which would be no fun.
(In practice it can't really happen since we only use 1/2 the
available space at a time).

Moving the lower bound up, truncating, and moving the upper bound up
is the way to go IMO.

>  Another option is not to implement direct reads
> from the clog.

I think there's a pretty decent argument for having clog lookups;
txid_status(...) serves as a useful halfway position between accepting
indeterminate commit status on connection loss and using full 2PC.

> Yet another option is that before we add such interface
> somebody produces proof that the problem does not, in fact, exist.

It does exist.

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 23 January 2017 at 12:34, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 20 January 2017 at 21:40, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
>> One option would be to add another limit Xid which advances before the
>> truncation but which is not used for other decisions other than limiting
>> what can users consult.
>
> This could be useful for other things, but it's probably heavier than needed.
>
> What I've done in the latest revision of the txid_status() patch is
> simply to advance OldestXid _before_ truncating the clog. The rest of
> the xid info is advanced after. Currently this is incorporated into
> the txid_status patch, but can be separated if desired.
>
> Relevant commit message portion:
>
>     There was previously no way to look up an arbitrary xid without
>     running the risk of having clog truncated out from under you. This
>     hasn't been a problem because anything looking up xids in clog knows
>     they're protected by datminxid, but that's not the case for arbitrary
>     user-supplied XIDs. clog was truncated before we advance oldestXid so
>     taking XidGenLock was insufficient. There's no way to look up a
>     SLRU with soft-failure. To address this, increase oldestXid under XidGenLock
>     before we trunate clog rather than after, so concurrent access is safe.
>
> Note that while oldestXid is advanced before clog truncation, the xid
> limits are advanced _after_ it. If we advanced the xid limits before
> truncation too, we'd theoretically run the risk of allocating an xid
> from the clog section we're about to truncate, which would be no fun.
> (In practice it can't really happen since we only use 1/2 the
> available space at a time).
>
> Moving the lower bound up, truncating, and moving the upper bound up
> is the way to go IMO.

Patch with explanation in commit msg:
https://www.postgresql.org/message-id/CAMsr+YHODEdUUA5vw1_rjPS_ASSvEMeJN_34rqd3pzHf5OFdJg@mail.gmail.com

-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services