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

On Thu, Mar 8, 2018 at 12:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Mar 8, 2018 at 11:04 AM, Pavan Deolasee
> <pavan.deolasee@gmail.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