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

On Thu, Mar 8, 2018 at 12:52 PM, Pavan Deolasee
<pavan(dot)deolasee(at)gmail(dot)com> wrote:
>
> 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.
>

Right, I feel we need some tests to prove it, I think as per code I
can see we need checks in few more places (like the ones mentioned in
the previous email) apart from where this patch has added.

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

Amul, can you please look into the scenario being discussed and see if
you can write a test to see the behavior.

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

Sure, I guess in that case, we need to update in comments why it would
be okay after abort.

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

I think even if we want to uplift this restriction, storing ctid link
of another partition appears to be a major change somebody would like
to do for this feature. We had some discussion on this matter earlier
where Robert, Greg seems to have said something like that as well.
See [1][2]. I think one way could be if updates/deletes, encounter
InvalidBlkID, they can use metadata of partition table to refind the
row. We already had a discussion on this point in the original thread
"UPDATE of partition key" and agreed to throw an error as the better
way to deal with it.

[1] - https://www.postgresql.org/message-id/CAM-w4HPis7rbnwi%2BoXjnouqMSRAC5DeVcMdxEXTMfDos1kaYPQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CA%2BTgmoY1W-jaS0vH8f%3D5xKQB3EWj5L0XcBf6P7WB7JqbKB3tSQ%40mail.gmail.com

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