Re: Different results between PostgreSQL and Oracle for "for update" statement - Mailing list pgsql-hackers

From Andy Fan
Subject Re: Different results between PostgreSQL and Oracle for "for update" statement
Date
Msg-id CAKU4AWrN4HRgxN_VXw0Uxx0Wq6M37_yHaAweNFuKBKsoU=0CDg@mail.gmail.com
Whole thread Raw
In response to Re: Different results between PostgreSQL and Oracle for "for update" statement  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: Different results between PostgreSQL and Oracle for "for update" statement  (Andreas Karlsson <andreas@proxel.se>)
List pgsql-hackers


On Thu, Nov 19, 2020 at 11:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andy Fan <zhihui.fan1213@gmail.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

pgsql-hackers by date:

Previous
From: Greg Nancarrow
Date:
Subject: Re: Parallel INSERT (INTO ... SELECT ...)
Next
From: "Drouvot, Bertrand"
Date:
Subject: Re: Add Information during standby recovery conflicts