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

On Thu, Mar 8, 2018 at 12:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

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

Yeah, but it only looks for places where it needs to detect deleted tuples
and thus wants to throw an error. I am worried about other places where it
is assumed that the ctid points to a valid looking tid, self or otherwise.
I see no such places being either updated or commented.

Now may be there is no danger because of other protections in place, but it
looks hazardous.

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

>
> > 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 don't know. May be there is none. But it needs to explained why it's not
a problem.

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

I am not sure I follow. I understand that it's probably a tough problem to
follow update chain from one partition to another. But why do you think we
would never need that? What if someone wants to improve on the restriction
this patch is imposing and actually implement partition key UPDATEs the way
we do for regular tables i.e. instead of throwing error, we actually
update/delete the row in the new partition?

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

Yeah, but the disk format will change probably change just once. Or may be
this can be done local to a partition table without requiring any disk
format changes? Like adding a nullable hidden column in each partition to
store the forward pointer?

Thanks,
Pavan