Re: Fix missing EvalPlanQual recheck for TID scans

> On Sep 14, 2025, at 06:12, Sophie Alpert <pg@sophiebits.com> wrote:
> 
> 
> And indeed, like I mentioned in my previous message, my isolation test `permutation tid1 tidsucceed2 c1 c2 read` from eval-plan-qual.spec in my patch will fail if Recheck were to return false in this case. Though somewhat contrived, you can imagine this happening with multiple sessions driven by the same application:
> 
> setup: two rows exist with ctid=(0,1) and (0,2)
> S1: BEGIN;
> S2: BEGIN;
> S1: UPDATE WHERE ctid=(0,1) RETURNING ctid;
> -- returns (0,3), which the application uses in the next query from another session:
> S2: UPDATE WHERE ctid=(0,1) OR ctid=(0,3); -- statement snapshot sees (0,1); recheck will see (0,3) and should pass
> S1: COMMIT;
> S2: COMMIT;
> 
> I would not defend this as good code from an application developer but the behavior is observable. So I understand it would be best to match the enable_tidscan = off behavior, which my existing strategy more verifiably does. Of course if the team disagrees with me then I will defer to everyone's better judgement.


This is a wrong example and (0,3) should NOT be updated. According to the definition of “read committed”:

https://www.postgresql.org/docs/current/transaction-iso.html#XACT-READ-COMMITTED
13.2. Transaction Isolation
postgresql.org

“A query sees only data committed before the query began”.

In this example, the update is s1 (0,3) is committed after s2’s update, so s2’s update should not see (0,3).

This behavior can be easily proved with a simple example with master branch:

S1:
```
evantest=# select * from t;
 id | a | b
----+---+----
  1 | 5 | 20
(1 row)
evantest=# begin;
BEGIN
evantest=*# update t set b=30 where id = 1;
UPDATE 1
evantest=*# insert into t values (2, 3, 4);       # this simulate (0,3) in your example
INSERT 0 1
```

S2:
```
evantest=# select * from t;
 id | a | b
----+---+----
  1 | 5 | 20
(1 row)
evantest=# begin;
BEGIN
evantest=*# update t set b = 30;  # block here
```

S1:
```
evantest=*# commit;
COMMIT
```

S2:
```
UPDATE 1
evantest=*# select * from t;
 id | a | b
----+---+----
  2 | 3 |  4     <=== the newly inserted tuple by s1 is NOT updated
  1 | 5 | 30    <=== only the old tuple is updated 
(2 rows)
```

This example also proves that your solution of TidListEval() is wrong, it may lead to unexpected update: (0,3) in your example.

It also proves the my proposal of checking visibility should work, because (0,3) is invisible to s2. And my proposal also works for your second example of doing “select for update” in s2, in that case, (0,1). After s1 committed, (0,1) is dead, so select of s2 should return nothing. The behavior can also be easily proved with a non-ctid example:

S1:
```
evantest=# begin;
BEGIN
evantest=*# update t set id=2 where id = 1;
UPDATE 1
```

S2:
```
evantest=# select * from t;
 id | a | b
----+---+----
  1 | 5 | 30
(1 row)

evantest=# select * from t where id = 1 for update; <=== block here
```

S1:
```
evantest=*# commit;
COMMIT
```

S2:
```
 id | a | b
----+---+---
(0 rows)         <=== s2 returned nothing, because id=1 is no long valid
```

And this example also proves my solution of checking visibility works.

And from this two examples, always returning FALSE seems to also work. But I am still not 100% sure if there are other use case that returning FALSE may not work. So, feels like checking visibility is a safe solution.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/