On Tue, Feb 13, 2018 at 12:41 PM, amul sul <sula...@gmail.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. 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         }
2313         ctid = tp.t_data->t_ctid;

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

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

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? 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. May be it was discussed somewhere else and ruled
out. I happened to notice this patch because of the bug I encountered.


 Pavan Deolasee                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Reply via email to