Odd behaviour of SELECT ... ORDER BY ... FOR UPDATE
Hi,
While working on the foreign-join-pushdown issue, I noticed that in READ
COMMITTED isolation level it's possible that the result of SELECT ...
ORDER BY ... FOR UPDATE is not sorted correctly due to concurrent
updates that replaced the sort key columns with new values as shown in
the below example. That seems odd to me. So, I'd like to propose
raising an error rather than returning a possibly-incorrect result for
cases where the sorted tuples to be locked were modified by concurrent
updates. Patch attached. Is it OK to add this to the current CF?
Create an environment:
postgres=# create table test (a int);
CREATE TABLE
postgres=# insert into test values (1);
INSERT 0 1
postgres=# insert into test values (2);
INSERT 0 1
Run an example:
[Terminal 1]
postgres=# begin;
BEGIN
postgres=# update test set a = 3 where a = 1;
UPDATE 1
[Terminal 2]
postgres=# select * from test order by a for update;
[Terminal 1]
postgres=# commit;
COMMIT
[Terminal 2]
(The following result will be shown after the commit in Terminal 1.
Note that the output ordering is not correct.)
a
---
3
2
(2 rows)
Best regards,
Etsuro Fujita
Attachments:
select-orderby-forupdate-v1.patchtext/x-patch; name=select-orderby-forupdate-v1.patchDownload
*** a/src/backend/executor/nodeLockRows.c
--- b/src/backend/executor/nodeLockRows.c
***************
*** 39,44 **** TupleTableSlot * /* return: a tuple or NULL */
--- 39,45 ----
ExecLockRows(LockRowsState *node)
{
TupleTableSlot *slot;
+ Plan *plan;
EState *estate;
PlanState *outerPlan;
bool epq_needed;
***************
*** 47,52 **** ExecLockRows(LockRowsState *node)
--- 48,54 ----
/*
* get information from the node
*/
+ plan = node->ps.plan;
estate = node->ps.state;
outerPlan = outerPlanState(node);
***************
*** 214,219 **** lnext:
--- 216,225 ----
ereport(ERROR,
(errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
errmsg("could not serialize access due to concurrent update")));
+ if (((LockRows *) plan)->ordering)
+ ereport(ERROR,
+ (errcode(ERRCODE_TRANSACTION_ROLLBACK),
+ errmsg("tuple to be locked was changed due to concurrent update")));
if (ItemPointerEquals(&hufd.ctid, &tuple.t_self))
{
/* Tuple was deleted, so don't return it */
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 967,972 **** _copyLockRows(const LockRows *from)
--- 967,973 ----
/*
* copy remainder of node
*/
+ COPY_SCALAR_FIELD(ordering);
COPY_NODE_FIELD(rowMarks);
COPY_SCALAR_FIELD(epqParam);
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 842,847 **** _outLockRows(StringInfo str, const LockRows *node)
--- 842,848 ----
_outPlanInfo(str, (const Plan *) node);
+ WRITE_BOOL_FIELD(ordering);
WRITE_NODE_FIELD(rowMarks);
WRITE_INT_FIELD(epqParam);
}
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***************
*** 4801,4807 **** make_setop(SetOpCmd cmd, SetOpStrategy strategy, Plan *lefttree,
* Build a LockRows plan node
*/
LockRows *
! make_lockrows(Plan *lefttree, List *rowMarks, int epqParam)
{
LockRows *node = makeNode(LockRows);
Plan *plan = &node->plan;
--- 4801,4807 ----
* Build a LockRows plan node
*/
LockRows *
! make_lockrows(Plan *lefttree, bool ordering, List *rowMarks, int epqParam)
{
LockRows *node = makeNode(LockRows);
Plan *plan = &node->plan;
***************
*** 4816,4821 **** make_lockrows(Plan *lefttree, List *rowMarks, int epqParam)
--- 4816,4822 ----
plan->lefttree = lefttree;
plan->righttree = NULL;
+ node->ordering = ordering;
node->rowMarks = rowMarks;
node->epqParam = epqParam;
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 2302,2308 **** grouping_planner(PlannerInfo *root, double tuple_fraction)
--- 2302,2311 ----
*/
if (parse->rowMarks)
{
+ bool ordering = parse->sortClause ? true : false;
+
result_plan = (Plan *) make_lockrows(result_plan,
+ ordering,
root->rowMarks,
SS_assign_special_param(root));
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
***************
*** 809,814 **** typedef struct SetOp
--- 809,815 ----
typedef struct LockRows
{
Plan plan;
+ bool ordering; /* do we need to preserve the ordering? */
List *rowMarks; /* a list of PlanRowMark's */
int epqParam; /* ID of Param for EvalPlanQual re-eval */
} LockRows;
*** a/src/include/optimizer/planmain.h
--- b/src/include/optimizer/planmain.h
***************
*** 74,80 **** extern Group *make_group(PlannerInfo *root, List *tlist, List *qual,
Plan *lefttree);
extern Plan *materialize_finished_plan(Plan *subplan);
extern Unique *make_unique(Plan *lefttree, List *distinctList);
! extern LockRows *make_lockrows(Plan *lefttree, List *rowMarks, int epqParam);
extern Limit *make_limit(Plan *lefttree, Node *limitOffset, Node *limitCount,
int64 offset_est, int64 count_est);
extern SetOp *make_setop(SetOpCmd cmd, SetOpStrategy strategy, Plan *lefttree,
--- 74,80 ----
Plan *lefttree);
extern Plan *materialize_finished_plan(Plan *subplan);
extern Unique *make_unique(Plan *lefttree, List *distinctList);
! extern LockRows *make_lockrows(Plan *lefttree, bool ordering, List *rowMarks, int epqParam);
extern Limit *make_limit(Plan *lefttree, Node *limitOffset, Node *limitCount,
int64 offset_est, int64 count_est);
extern SetOp *make_setop(SetOpCmd cmd, SetOpStrategy strategy, Plan *lefttree,
On 7/2/15 9:15 AM, Etsuro Fujita wrote:
While working on the foreign-join-pushdown issue, I noticed that in READ
COMMITTED isolation level it's possible that the result of SELECT ...
ORDER BY ... FOR UPDATE is not sorted correctly due to concurrent
updates that replaced the sort key columns with new values as shown in
the below example. That seems odd to me. So, I'd like to propose
raising an error rather than returning a possibly-incorrect result for
cases where the sorted tuples to be locked were modified by concurrent
updates.
I don't like the idea of READ COMMITTED suddenly throwing errors due to
concurrency problems. Using FOR UPDATE correctly is really tricky, and
this is just one example. And a documented one, at that, too.
.m
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Marko,
On 2015/07/02 16:27, Marko Tiikkaja wrote:
On 7/2/15 9:15 AM, Etsuro Fujita wrote:
While working on the foreign-join-pushdown issue, I noticed that in READ
COMMITTED isolation level it's possible that the result of SELECT ...
ORDER BY ... FOR UPDATE is not sorted correctly due to concurrent
updates that replaced the sort key columns with new values as shown in
the below example. That seems odd to me. So, I'd like to propose
raising an error rather than returning a possibly-incorrect result for
cases where the sorted tuples to be locked were modified by concurrent
updates.
I don't like the idea of READ COMMITTED suddenly throwing errors due to
concurrency problems. Using FOR UPDATE correctly is really tricky, and
this is just one example. And a documented one, at that, too.
Ah, you are right. I'll withdraw this. Sorry for the noise.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jul 2, 2015 at 3:59 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
Hi Marko,
On 2015/07/02 16:27, Marko Tiikkaja wrote:
On 7/2/15 9:15 AM, Etsuro Fujita wrote:
While working on the foreign-join-pushdown issue, I noticed that in READ
COMMITTED isolation level it's possible that the result of SELECT ...
ORDER BY ... FOR UPDATE is not sorted correctly due to concurrent
updates that replaced the sort key columns with new values as shown in
the below example. That seems odd to me. So, I'd like to propose
raising an error rather than returning a possibly-incorrect result for
cases where the sorted tuples to be locked were modified by concurrent
updates.I don't like the idea of READ COMMITTED suddenly throwing errors due to
concurrency problems. Using FOR UPDATE correctly is really tricky, and
this is just one example. And a documented one, at that, too.Ah, you are right. I'll withdraw this. Sorry for the noise.
Does 385f337c9f39b21dca96ca4770552a10a6d5af24 make any difference to
the issue described here?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers