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

On Thu, Mar 8, 2018 at 11:04 AM, Pavan Deolasee
<pavan(dot)deolasee(at)gmail(dot)com> wrote:
>
> On Tue, Feb 13, 2018 at 12:41 PM, amul sul <sulamul(at)gmail(dot)com> wrote:
>>
>> Thanks for the confirmation, updated patch attached.
>>
>
> I am actually very surprised that 0001-Invalidate-ip_blkid-v5.patch does not
> do anything to deal with the fact that t_ctid may no longer point to itself
> to mark end of the chain. I just can't see how that would work.
>

I think it is not that patch doesn't care about the end of the chain.
For example, ctid pointing to itself is used to ensure that for
deleted rows, nothing more needs to be done like below check in the
ExecUpdate/ExecDelete code path.
if (!ItemPointerEquals(tupleid, &hufd.ctid))
{
..
}
..

It will deal with such cases by checking invalidblockid before these
checks. So, we should be fine in such cases.

> But if it
> does, it needs good amount of comments explaining why and most likely
> updating comments at other places where chain following is done. For
> example, how's this code in heap_get_latest_tid() is still valid? Aren't we
> setting "ctid" to some invalid value here?
>
> 2302 /*
> 2303 * If there's a valid t_ctid link, follow it, else we're done.
> 2304 */
> 2305 if ((tp.t_data->t_infomask & HEAP_XMAX_INVALID) ||
> 2306 HeapTupleHeaderIsOnlyLocked(tp.t_data) ||
> 2307 ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid))
> 2308 {
> 2309 UnlockReleaseBuffer(buffer);
> 2310 break;
> 2311 }
> 2312
> 2313 ctid = tp.t_data->t_ctid;
>

I have not tested, but it seems this could be problematic, but I feel
we could deal with such cases by checking invalid block id in the
above if check. Another one such case is in EvalPlanQualFetch

> This is just one example. I am almost certain there are many such cases that
> will require careful attention.
>

Right, I think we should be able to detect and fix such cases.

> What happens if a partition key update deletes a row, but the operation is
> aborted? Do we need any special handling for that case?
>

If the transaction is aborted than future updates would update the
ctid to a new row, do you see any problem with it?

> I am actually worried that we're tinkering with ip_blkid to handle one
> corner case of detecting partition key update. This is going to change
> on-disk format and probably need more careful attention. Are we certain that
> we would never require update-chain following when partition keys are
> updated?
>

I think we should never need update-chain following when the row is
moved from one partition to another partition, otherwise, we don't
change anything on the tuple.

> If so, can we think about some other mechanism which actually even
> leaves behind <new_partition, new_ctid>? I am not saying we should do that,
> but it warrants a thought.
>

Oh, this would much bigger disk-format change and need much more
thoughts, where will we store new partition information.

> May be it was discussed somewhere else and ruled
> out.

There were a couple of other options discussed in the original thread
"UPDATE of partition key". One of them was to have an additional bit
on the tuple, but we found reusing ctid a better approach.

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