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:
>>
[.....]
>
>> 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
>

I tried the following test to hit this code and found that the situation is not
that much unpleasant.

heap_get_latest_tid() will follow the chain and return latest tid iff the
current tuple satisfies visibility check (via HeapTupleSatisfiesVisibility), in
our case it doesn't and we are safe here, but I agree with Amit -- it is better
to add invalid block id check.
In EvalPlanQualFetch() invalid block id check already there before
ItemPointerEquals call.

=== TEST ==
create table foo (a int2, b text) partition by list (a);
create table foo1 partition of foo for values IN (1);
create table foo2 partition of foo for values IN (2);
insert into foo values(1, 'Initial record');
update foo set b= b || ' -> update1' where a=1;
update foo set b= b || ' -> update2' where a=1;

postgres=# select tableoid::regclass, ctid, * from foo;
tableoid | ctid | a | b
----------+-------+---+--------------------------------------
foo1 | (0,3) | 1 | Initial record -> update1 -> update2
(1 row)

postgres=# select currtid2('foo1','(0,1)');
currtid2
----------
(0,3)
(1 row)

postgres=# select tableoid::regclass, ctid, * from foo;
tableoid | ctid | a | b
----------+-------+---+----------------------------------------------
foo2 | (0,1) | 2 | Initial record -> update1 -> update2-> moved
(1 row)

postgres=# select currtid2('foo1','(0,1)');
currtid2
----------
(0,1)
(1 row)

=== END ===

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

Will look into the places carefully where ItemPointerEquals() call
made for heap tuple.

Regards,
Amul