Re: Changing WAL Header to reduce contention during ReserveXLogInsertLocation()

On 03/29/2018 08:02 PM, Robert Haas wrote:
> On Thu, Mar 29, 2018 at 1:13 PM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> On 03/29/2018 06:42 PM, Tom Lane wrote:
>>> The long and the short of it is that this is a very dangerous-looking
>>> proposal, we are at the tail end of a development cycle, and there are
>>> ~100 other patches remaining in the commitfest that also have claims
>>> on our attention in the short time that's left. If you're expecting
>>> people to spend more time thinking about this now, I feel you're being
>>> unreasonable.
>>
>> I agree.
>
> +1.
>
>>> Also, I will say it once more: this change DOES decrease robustness.
>>> It's like blockchain without the chain aspect, or git commits without
>>> a parent pointer. We are not only interested in whether individual
>>> WAL records are valid, but whether they form a consistent series.
>>> Cross-checking xl_prev provides some measure of confidence about that;
>>> xl_curr offers none.
>>
>> Not sure.
>>
>> If each WAL record has xl_curr, then we know to which position the
>> record belongs (after verifying the checksum). And we do know the size
>> of each WAL record, so we should be able to deduce if two records are
>> immediately after each other. Which I think is enough to rebuild the
>> chain of WAL records.
>>
>> To defeat this, this would need to happen:
>>
>> a) the WAL record gets written to a different location
>> b) the xl_curr gets corrupted in sync with (a)
>> c) the WAL checksum gets corrupted in sync with (b)
>> d) the record overwrites existing record (same size/boundaries)
>>
>> That seems very much like xl_prev.
>
> I don't think so, because this ignores, for example, timeline
> switches, or multiple clusters accidentally sharing an archive
> directory.
>

I'm curious? How does xl_prev prevents either of those issues (in a way
that xl_curr would not)?

> TBH, I'm not entirely sure how likely corruption would be under this
> proposal in practice, but I think Tom's got the right idea on a
> theoretical level. As he says, there's a reason why things like
> block chains and git commits include something about the previous
> record in what gets hashed to create the identifier for the new
> record. It ties those things together in a way that doesn't happen
> otherwise. If somebody proposed changing git so that the SHA
> identifier for a commit didn't hash the commit ID for the previous
> commit, that would break the security of the system in all kinds of
> ways: for example, an adversary could maliciously edit an old commit
> by just changing its SHA identifier and that of its immediate
> descendent. No other commit IDs would change and integrity checks
> would not fail -- there would be no easy way to notice that something
> bad had happened.
>

That seems like a rather poor analogy, because we're not protected
against such adversarial modifications with xl_prev either. I can damage
the previous WAL record, update its checksum and you won't notice anything.

My understanding is that xl_curr and xl_prev give us pretty much the
same amount of information, except that xl_prev provides it explicitly
while with xl_curr it's implicit and we need to deduce it.

> Now, in our case, the chances of problems are clearly a lot more
> remote because of the way that WAL records are packed into files.
> Every WAL record has a place where it's supposed to appear in the WAL
> stream, and positions in the WAL stream are never intentionally
> recycled (except in PITR scenarios, I guess). Because of that, the
> proposed xl_curr check provides a pretty strong cross-check on the
> validity of a record even without xl_prev. I think there is an
> argument to be made - as Simon is doing - that this check is in fact
> strong enough that it's good enough for government work. He might be

Is he? I think the claims in this thread were pretty much that xl_curr
and xl_prev provide the same level of protection.

> right. But he might also be wrong, because I think Tom is
> unquestionably correct when he says that this is weakening the check.
> What I'm not sure about is whether it's being weakened enough that
> it's actually going to cause problems in practice. Given where we are
> in the release cycle, I'd prefer to defer that question to next cycle.
>

Perhaps. I guess the ultimate argument would be an example where xl_prev
does provides protection that xl_curr does not. There have been a couple
of examples presented in this thread, but I don't think either of them
actually shows the xl_prev superiority (or I missed that).

regards

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