Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key

On Thu, Apr 5, 2018 at 7:14 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> I've attached a noticeably editorialized patch:
>
> - I'm uncomfortable with the "moved" information not being crash-safe /
> replicated. Thus I added a new flag to preserve it, and removed the
> masking of the moved bit in the ctid from heap_mask().
>
> - renamed macros to not mention valid / invalid block numbers, but
> rather
> HeapTupleHeaderSetMovedPartitions / HeapTupleHeaderIndicatesMovedPartitions
> and
> ItemPointerSetMovedPartitions / ItemPointerIndicatesMovedPartitions
>
> I'm not wedded to these names, but I'l be adamant they they're not
> talking about invalid block numbers. Makes code harder to understand
> imo.
>

The new names for macros make the code easier to understand.

> - removed new assertion from heap_get_latest_tid(), it's wrong for the
> case where all row versions are invisible.
>

Why? tid is both an input and output parameter. The input tid is
valid and is verified at the top of the function, now if no row
version is visible, then it should have the same value as passed Tid.
I am not telling that it was super important to have that assertion,
but if it is valid then it can catch a case where we might have missed
checking the tuple which has invalid block number (essentialy the case
introduced by the patch).

I assume you are talking about below assertion:

+
+ /* Make sure that the return value has a valid block number */
+ Assert(ItemPointerValidBlockNumber(tid));

>
> Questions:
>
> - I'm not perfectly happy with
> "tuple to be locked was already moved to another partition due to concurrent update"
> as the error message. If somebody has a better suggestions.
>

I don't have any better suggestion, but I have noticed a small
inconsistency in the message. In case of delete, the message is
"tuple to be updated was ...". I think here it should be "tuple to be
deleted was ...".

> - should heap_get_latest_tid() error out when the chain ends in a moved
> tuple?

Won't the same question applies to the similar usage in
EvalPlanQualFetch and heap_lock_updated_tuple_rec. In
EvalPlanQualFetch, we consider such a tuple to be deleted and will
silently miss/skip it which seems contradictory to the places where we
have detected such a situation and raised an error. In
heap_lock_updated_tuple_rec, we will skip locking the versions of a
tuple after we encounter a tuple version that is moved to another
partition.

> - I'm not that happy with the number of added spec test files with
> number postfixes. Can't we combine them into a single file?

+1 for doing so.

> - as remarked elsewhere on this thread, I think the used errcode should
> be a serialization failure
>

No problem. I was telling up thread that the used error code has some
precedent in the code for similar usage, but we have precedent for the
serialization failure error code as well, so it should be okay to use
it.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com