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

On 2018-04-05 10:17:59 +0530, Amit Kapila wrote:
> On Thu, Apr 5, 2018 at 7:14 AM, Andres Freund <andres@anarazel.de> wrote:
> 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).

You're right. It's bonkers that the output parameter isn't set to an
invalid value if the tuple isn't found. Makes the whole function
entirely useless.

> > - 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 ...".

Yea, I noticed that too. Note that the message a few lines up is
similarly wrong:
ereport(ERROR,
		(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
		 errmsg("tuple to be updated was already modified by an operation triggered by the current command"),
		 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));


> > - 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.

I don't think so?


> 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.

if (ItemPointerIndicatesMovedPartitions(&hufd.ctid))
	ereport(ERROR,
			(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
			 errmsg("tuple to be locked was already moved to another partition due to concurrent update")));


> 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 don't think that's true? We'll not lock *any* tuple in that case, but
return HeapTupleUpdated. Which callers then interpret in whatever way
they need to?

Greetings,

Andres Freund