Обсуждение: comments around heap_lock_tuple confus{ing,ed} around deleted tuples

Hi,

While looking at resolving [1] I re-read heap_lock_tuple() and
subsidiary routines and got thoroughly confused for a while.

One reason was that function names and comments talk about updated, when
they also actually deal with deletes.  heap_lock_updated_tuple()
specifically is called on tuples that have not been updated, but have
been deleted.

/*
 * heap_lock_updated_tuple
 *        Follow update chain when locking an updated tuple, acquiring locks (row
 *        marks) on the updated versions.
 *
 * The initial tuple is assumed to be already locked.

So

a) The function name is wrong, we're not necessarily dealing with an
   updated tuple.
b) The initial tuple is actually not generally locked when the function
   is called. See the call below the
   /* if there are updates, follow the update chain */
   comment.

   Or is that supposed to mean that the initial tuple has already been
   locked with the heavyweight lock?  But that can't be true either,
   because afaics the heap_lock_updated_tuple() call for
   LockTupleKeyShare doesn't even do that?


It's also fairly weird that heap_lock_updated_tuple() returns
    /* nothing to lock */
    return HeapTupleMayBeUpdated;
when the tuple has been deleted (and thus
ItemPointerEquals(&tuple->t_self, ctid)). That'll not get returned by
heap_lock_tuple() itself, but seems thoroughly confusing.


There's some argument to be made for not changing this because "it seems
to work", but the wrong comments and function names are not unlikely to
cause future bugs...

Greetings,

Andres Freund

[1] http://archives.postgresql.org/message-id/CAAJ_b95PkwojoYfz0bzXU8OokcTVGzN6vYGCNVUukeUDrnF3dw%40mail.gmail.com

Andres Freund wrote:

> While looking at resolving [1] I re-read heap_lock_tuple() and
> subsidiary routines and got thoroughly confused for a while.
> 
> One reason was that function names and comments talk about updated, when
> they also actually deal with deletes.  heap_lock_updated_tuple()
> specifically is called on tuples that have not been updated, but have
> been deleted.

No objection to renaming the function.  I am certain that when I first
wrote it, it was going to be used for updated tuples; I never considered
deletes.  After it was repurposed, I never thought about renaming it.

> b) The initial tuple is actually not generally locked when the function
>    is called. See the call below the
>    /* if there are updates, follow the update chain */
>    comment.

Hmm, OK, I don't remember this.  But no, it's not about the heavyweight
lock -- it's about the xmax-level tuple lock.

> It's also fairly weird that heap_lock_updated_tuple() returns
>     /* nothing to lock */
>     return HeapTupleMayBeUpdated;
> when the tuple has been deleted (and thus
> ItemPointerEquals(&tuple->t_self, ctid)). That'll not get returned by
> heap_lock_tuple() itself, but seems thoroughly confusing.

Yeah, what MayBeUpdated is supposed to mean in this case is "there is no
error, we were able to do the thing we were asked to do", rather than
exactly "yes, you may update the tuple".  I guess you could argue that
reusing HTSU result values for it was wrong.  It was certainly
convenient.

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

Hi,

On 2018-04-04 18:34:26 -0300, Alvaro Herrera wrote:
> > It's also fairly weird that heap_lock_updated_tuple() returns
> >     /* nothing to lock */
> >     return HeapTupleMayBeUpdated;
> > when the tuple has been deleted (and thus
> > ItemPointerEquals(&tuple->t_self, ctid)). That'll not get returned by
> > heap_lock_tuple() itself, but seems thoroughly confusing.
> 
> Yeah, what MayBeUpdated is supposed to mean in this case is "there is no
> error, we were able to do the thing we were asked to do", rather than
> exactly "yes, you may update the tuple".  I guess you could argue that
> reusing HTSU result values for it was wrong.  It was certainly
> convenient.

I think just adding a comment along those lines should be good enough...

Greetings,

Andres Freund