On Tue, Feb 13, 2018 at 12:41 PM, amul sul <sulamul@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) ||
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.
