Re: Remove HeapTuple and Buffer dependency for predicate locking functions

On Sun, Nov 10, 2019 at 8:21 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:

> I pushed the first two,

Thank You!

but on another read-through of the main patch
> I didn't like the comments for CheckForSerializableConflictOut() or
> the fact that it checks SerializationNeededForRead() again, after I
> thought a bit about what the contract for this API is now. Here's a
> version with small fixup that I'd like to squash into the patch.
> Please let me know what you think,

The thought or reasoning behind having SerializationNeededForRead()
inside CheckForSerializableConflictOut() is to keep that API clean and
complete by itself. Only if AM like heap needs to perform some AM
specific checking only for serialization needed case can do so but is
not forced. So, if AM for example Zedstore doesn't need to do any
specific checking, then it can directly call
CheckForSerializableConflictOut(). With the modified fixup patch, the
responsibility is unnecessarily forced to caller even if
CheckForSerializableConflictOut() can do it. I understand the intent
is to avoid duplicate check for heap.

>
> or if you see how to improve it
> further.
>

I had proposed as alternative way in initial email and also later,
didn't receive comment on that, so re-posting.

Alternative way to refactor. CheckForSerializableConflictOut() can
take callback function and (void *) callback argument for AM specific
check. So, the flow would be AM calling
CheckForSerializableConflictOut() as today and only if
SerializationNeededForRead() will invoke the callback to check with AM
if more work should be performed or not. Essentially
HeapCheckForSerializableConflictOut() will become callback function
instead. So, roughly would look like....

typedef bool (*AMCheckForSerializableConflictOutCallback) (void *arg);

void CheckForSerializableConflictOut(Relation relation, TransactionId xid,
Snapshot snapshot, AMCheckForSerializableConflictOutCallback callback, void
*callback_arg)
{
if (!SerializationNeededForRead(relation, snapshot))
return;
if (callback != NULL && !callback(callback_args))
return;
........
.....
}

With this AMs which don't have any specific checks to perform can pass
callback as NULL. So, function call is involved only if
SerializationNeededForRead() and only for AMs which need it.

Just due to void* callback argument aspect I didn't prefer that
solution and felt AM performing checks and calling
CheckForSerializableConflictOut() seems better. Please let me know
how you feel about this.