Re: Assert() failures during RI checks - Mailing list pgsql-hackers

Hi,

On 2020-03-23 13:54:31 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-03-22 18:30:04 -0400, Tom Lane wrote:
> >> Yeah, I was wondering about giving that a new result code, too.
> >> It would be a little bit invasive and not at all back-patchable,
> >> but (say) TM_SerializationViolation seems like a cleaner output ---
> >> and we could define it to not return any TID info, as TM_Delete
> >> doesn't.  But we also need a fix that *can* be back-patched.
> 
> > I wonder if returning TM_Invisible in the backbranches would be the
> > right answer. "The affected tuple wasn't visible to the relevant
> > snapshot" is not the worst description for a crosscheck snapshot
> > violation.
> 
> I'm not for that.  Abusing an existing result code is what got us
> into this mess in the first place; abusing a different result code
> than what we did in prior minor releases can only make things worse
> not better.

Well, I think TM_Invisible is much less error prone - callers wouldn't
try to chase ctid chains or such. There's really not much they can do
except to error out. It also seems semantically be a better match than
TM_Updated.


> Of course, if there's no external callers of these
> functions then we could get away with changing it ... but I'm not
> prepared to assume that, are you?)

I'm certain we can't assume that.


> I think the right thing in the back branches is to keep on returning
> the "Updated" result code, but adjust the crosscheck code path so that
> the TID that's sent back is always the tuple's own TID.

If we do that, and set xmax to InvalidTransactionId, I think we'd likely
prevent any "wrong" chaining from outside heapam, since that already
needed to check xmax to be correct.


> >> The stanza right after the crosscheck one has always assumed that
> >> it is dealing with an outdated-by-update tuple, so that chasing up
> >> to the next TID is a sane thing to do.> 
> > Do you just mean the
> >         tmfd->ctid = tp.t_data->t_ctid;
> > and
> >         tmfd->xmax = HeapTupleHeaderGetUpdateXid(tp.t_data);
> > ? I would not have described that as chasing, which would explain my
> > difficulty following.
> 
> The bit about returning t_ctid rather than the tuple's own TID seems
> like chasing a next-tid link to me.  The Assert about xmax being valid
> is certainly intended to verify that that's what is happening.  I can't
> speak to what the code thinks it's returning in tmfd->xmax, because
> that functionality wasn't there the last time I studied this.

We could probably get rid of tmfd->xmax. It kind of was introduced as
part of

commit 6868ed7491b7ea7f0af6133bb66566a2f5fe5a75
Author: Kevin Grittner <kgrittn@postgresql.org>
Date:   2012-10-26 14:55:36 -0500

    Throw error if expiring tuple is again updated or deleted.

It did previously exist as an explicit heap_delete/heap_update
parameter, however - and has for a long time:

commit f57e3f4cf36f3fdd89cae8d566479ad747809b2f
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   2005-08-20 00:40:32 +0000

    Repair problems with VACUUM destroying t_ctid chains too soon, and with
    insufficient paranoia in code that follows t_ctid links.  (We must do both


Since Kevin's commit the relevant logic has since been pushed down into
the AM layer as part of tableam. While still used in one place
internally inside heap AM (basically where the check from Kevin's commit
commit has been moved), but that looks trivial to solve differently.


> > I am wondering if the correct HEAD answer would be to somehow lift the
> > crosscheck logic out of heapam.c. ISTM that heapam.c is kind of too low
> > level for that type of concern. But it seems hard to do that without
> > performing unnecessary updates that immediately are thrown away by
> > throwing an error at a higher level.
> 
> Yeah, I'd be the first to agree that the crosscheck stuff is messy.
> But it's hard to see how to do better.

Obviously not a short-term solution at all: I wonder if some of this
stems from implementing RI triggers partially in SQL, even though we
require semantics that aren't efficiently doable in SQL. With the
consequences of having RI specific code in various parts of the executor
and AMs, and of needing a lot more memory etc.

Greetings,

Andres Freund