Re: [PATCH] Fix minor race in commit_ts SLRU truncation vs lookups

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