> On Apr 1, 2021, at 9:56 AM, Robert Haas <[email protected]> wrote:
>
> On Thu, Apr 1, 2021 at 12:32 PM Mark Dilger
> <[email protected]> wrote:
>>> - If xmax is a multi but seems to be garbled, I changed it to return
>>> true rather than false. The inserter is known to have committed by
>>> that point, so I think it's OK to try to deform the tuple. We just
>>> shouldn't try to check TOAST.
>>
>> It is hard to know what to do when at least one tuple header field is
>> corrupt. You don't necesarily know which one it is. For example, if
>> HEAP_XMAX_IS_MULTI is set, we try to interpret the xmax as a mxid, and if it
>> is out of bounds, we report it as corrupt. But was the xmax corrupt? Or
>> was the HEAP_XMAX_IS_MULTI bit corrupt? It's not clear. I took the view
>> that if either xmin or xmax appear to be corrupt when interpreted in light
>> of the various tuple header bits, all we really know is that the set of
>> fields/bits don't make sense as a whole, so we report corruption, don't
>> trust any of them, and abort further checking of the tuple. You have be
>> burden of proof the other way around. If the xmin appears fine, and xmax
>> appears corrupt, then we only know that xmax is corrupt, so the tuple is
>> checkable because according to the xmin it committed.
>
> I agree that it's hard to be sure what's gone once we start finding
> corrupted data, but deciding that maybe xmin didn't really commit
> because we see that there's something wrong with xmax seems excessive
> to me. I thought about a related case: if xmax is a bad multi but is
> also hinted invalid, should we try to follow TOAST pointers? I think
> that's hard to say, because we don't know whether (1) the invalid
> marking is in error, (2) it's wrong to consider it a multi rather than
> an XID, (3) the stored multi got overwritten with a garbage value, or
> (4) the stored multi got removed before the tuple was frozen. Not
> knowing which of those is the case, how are we supposed to decide
> whether the TOAST tuples might have been (or be about to get) pruned?
>
> But, in the case we're talking about here, I don't think it's a
> particularly close decision. All we need to say is that if xmax or the
> infomask bits pertaining to it are corrupted, we're still going to
> suppose that xmin and the infomask bits pertaining to it, which are
> all different bytes and bits, are OK. To me, the contrary decision,
> namely that a bogus xmax means xmin was probably lying about the
> transaction having been committed in the first place, seems like a
> serious overreaction. As you say:
>
>> I don't think how you have it causes undue problems, since deforming the
>> tuple when you shouldn't merely risks a bunch of extra not-so-helpful
>> corruption messages. And hey, maybe they're helpful to somebody clever
>> enough to diagnose why that particular bit of noise was generated.
>
> I agree. The biggest risk here is that we might omit >0 complaints
> when only 0 are justified. That will panic users. The possibility that
> we might omit >x complaints when only x are justified, for some x>0,
> is also a risk, but it's not nearly as bad, because there's definitely
> something wrong, and it's just a question of what it is exactly. So we
> have to be really conservative about saying that X is corruption if
> there's any possibility that it might be fine. But once we've
> complained about one thing, we can take a more balanced approach about
> whether to risk issuing more complaints. The possibility that
> suppressing the additional complaints might complicate resolution of
> the issue also needs to be considered.
This all seems fine to me. The main thing is that we don't go on to check the
toast, which we don't.
>> * If xmin_status happens to be XID_IN_PROGRESS, then in theory
>>
>> Did you mean to say XID_IS_CURRENT_XID here?
>
> Yes, I did, thanks.
Ouch. You've got a typo: s/XID_IN_CURRENT_XID/XID_IS_CURRENT_XID/
>> /* xmax is an MXID, not an MXID. Sanity check it. */
>>
>> Is it an MXID or isn't it?
>
> Good catch.
>
> New patch attached.
Seems fine other than the typo.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
