Обсуждение: table_delete and table_update don't document snapshot parameter

Hi,

table_update and table_delete both have comments that document all of
their input parameters *except* for snapshot.  This seems like an
oversight, especially because:

 *      crosscheck - if not InvalidSnapshot, also check tuple against this

Without a comment about snapshot, what's the "also" about?

Suspiciously, the heap implementations of these functions completely
ignore the snapshot parameter and have no comments explaining the
reasons why they do so.  In fact, the only comment in
heapam_tuple_delete is this one, and it seems both misplaced (since it
seems to be a general comment about table AMs, not something
heap-specific) and in need of editing:

        /*
         * Currently Deleting of index tuples are handled at vacuum, in case if
         * the storage itself is cleaning the dead tuples by itself, it is the
         * time to call the index tuple deletion also.
         */

One particular thing I'm curious whether it's ever OK to pass the
snapshot as InvalidSnapshot, or whether it's expected a valid snapshot
should always be supplied.  If the latter, I think it would be a good
idea to add an Assert() to table_update and table_delete() to avoid
coding mistakes.

Thanks,

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2019-05-17 11:34:25 -0400, Robert Haas wrote:
> table_update and table_delete both have comments that document all of
> their input parameters *except* for snapshot.  This seems like an
> oversight

Hm, yea, it ought to be documented.


> , especially because:
> 
>  *      crosscheck - if not InvalidSnapshot, also check tuple against this
> 
> Without a comment about snapshot, what's the "also" about?

I don't think I've materially changed anything around that. It's just
the < 12 comment. The also refers to cid.


> Suspiciously, the heap implementations of these functions completely
> ignore the snapshot parameter and have no comments explaining the
> reasons why they do so.

I don't think there's a case where heap needs them - but it's different
for e.g. zheap (c.f. ZHeapTupleSatisfiesUpdate needing it the version
I'm looking at rn). IMO it's reasonable for an AM needing it to
disambiguate versions (although there's the complication that in the EPQ
case versions *newer* than the snapshot might need to be deleted).


> One particular thing I'm curious whether it's ever OK to pass the
> snapshot as InvalidSnapshot, or whether it's expected a valid snapshot
> should always be supplied.

I can't see any case where it would be OK to not supply it.


> If the latter, I think it would be a good
> idea to add an Assert() to table_update and table_delete() to avoid
> coding mistakes.

Yea, probably a good idea.

Greetings,

Andres Freund