Re: table_delete and table_update don't document snapshot parameter

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