Re: Different results between PostgreSQL and Oracle for "for update" statement

On Thu, Nov 19, 2020 at 11:49 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> writes:
> > create table su (a int, b int);
> > insert into su values(1, 1);
>
> > - session 1:
> > begin;
> > update su set b = 2 where b = 1;
>
> > - sess 2:
> > select * from su where a in (select a from su where b = 1) for update;
>
> This'd probably work the way you expect if there were "for update"
> in the sub-select as well. As is, the sub-select will happily
> return "1".
>
> regards, tom lane
>

Hi Tom:

Thank you for your attention. Your suggestion would fix the issue. However
The difference will cause some risks when users move their application from
Oracle
to PostgreSQL. So I'd like to think which behavior is more reasonable.

In our current pg strategy, we would get

select * from su where a in (select a from su where b = 1) for update;

a | b
---+---
1 | 2
(1 row)

The data is obtained from 4 steps:

1. In the subquery, it gets su(a=1, b=1).
2. in the outer query, it is blocked.
3. session 1 is committed. sess 2 can continue.
4. outer query gets su(a=1, b=2).

By comparing the result in step 1 & step 4 in the same query, it
looks like we are using 2 different snapshots for the same query.
I think it is a bit strange.

If we think it is an issue, I think we can fix it with something like this:

+/*
+ * We should use the same RowMarkType for the RangeTblEntry
+ * if the underline relation is the same. Doesn't handle SubQuery for now.
+ */
+static RowMarkType
+select_rowmark_type_for_rangetable(RangeTblEntry *rte,
+ List
*rowmarks,
+
LockClauseStrength strength)
+{
+ ListCell *lc;
+ foreach(lc, rowmarks)
+ {
+ PlanRowMark *rc = lfirst_node(PlanRowMark, lc);
+ if (rc->relid == rte->relid)
+ return rc->markType;
+ }
+ return select_rowmark_type(rte, strength);
+}
+
/*
* preprocess_rowmarks - set up PlanRowMarks if needed
*/
@@ -2722,6 +2743,7 @@ preprocess_rowmarks(PlannerInfo *root)
newrc->strength = rc->strength;
newrc->waitPolicy = rc->waitPolicy;
newrc->isParent = false;
+ newrc->relid = rte->relid;

prowmarks = lappend(prowmarks, newrc);
}
@@ -2742,18 +2764,18 @@ preprocess_rowmarks(PlannerInfo *root)
newrc = makeNode(PlanRowMark);
newrc->rti = newrc->prti = i;
newrc->rowmarkId = ++(root->glob->lastRowMarkId);
- newrc->markType = select_rowmark_type(rte, LCS_NONE);
+ newrc->markType = select_rowmark_type_for_rangetable(rte,
prowmarks, LCS_NONE);
newrc->allMarkTypes = (1 << newrc->markType);
newrc->strength = LCS_NONE;
newrc->waitPolicy = LockWaitBlock; /* doesn't matter */
newrc->isParent = false;
-
+ newrc->relid = rte->relid;
prowmarks = lappend(prowmarks, newrc);
}
-
root->rowMarks = prowmarks;
}

+
/*
* Select RowMarkType to use for a given table
*/
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index ac5685da64..926086a69a 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -1103,6 +1103,7 @@ typedef struct PlanRowMark
LockClauseStrength strength; /* LockingClause's strength, or
LCS_NONE */
LockWaitPolicy waitPolicy; /* NOWAIT and SKIP LOCKED options */
bool isParent; /* true if this is a
"dummy" parent entry */
+ Oid relid; /* relation oid */
} PlanRowMark;

Do you think it is a bug and the above method is the right way to go?

--
Best Regards
Andy Fan