Commit 71404af2 authored by Tom Lane's avatar Tom Lane

Fix EvalPlanQual bug when query contains both locked and not-locked rels.

In commit afb9249d, we (probably I) made ExecLockRows assign
null test tuples to all relations of the query while setting up to do an
EvalPlanQual recheck for a newly-updated locked row.  This was sheerest
brain fade: we should only set test tuples for relations that are lockable
by the LockRows node, and in particular empty test tuples are only sensible
for inheritance child relations that weren't the source of the current
tuple from their inheritance tree.  Setting a null test tuple for an
unrelated table causes it to return NULLs when it should not, as exhibited
in bug #14034 from Bronislav Houdek.  To add insult to injury, doing it the
wrong way required two loops where one would suffice; so the corrected code
is even a bit shorter and faster.

Add a regression test case based on his example, and back-patch to 9.5
where the bug was introduced.
parent f6bd0da6
...@@ -262,29 +262,15 @@ lnext: ...@@ -262,29 +262,15 @@ lnext:
*/ */
if (epq_needed) if (epq_needed)
{ {
int i;
/* Initialize EPQ machinery */ /* Initialize EPQ machinery */
EvalPlanQualBegin(&node->lr_epqstate, estate); EvalPlanQualBegin(&node->lr_epqstate, estate);
/* /*
* Transfer already-fetched tuples into the EPQ state, and make sure * Transfer any already-fetched tuples into the EPQ state, and fetch a
* its test tuples for other tables are reset to NULL. * copy of any rows that were successfully locked without any update
*/ * having occurred. (We do this in a separate pass so as to avoid
for (i = 0; i < node->lr_ntables; i++) * overhead in the common case where there are no concurrent updates.)
{ * Make sure any inactive child rels have NULL test tuples in EPQ.
EvalPlanQualSetTuple(&node->lr_epqstate,
i + 1,
node->lr_curtuples[i]);
/* freeing this tuple is now the responsibility of EPQ */
node->lr_curtuples[i] = NULL;
}
/*
* Next, fetch a copy of any rows that were successfully locked
* without any update having occurred. (We do this in a separate pass
* so as to avoid overhead in the common case where there are no
* concurrent updates.)
*/ */
foreach(lc, node->lr_arowMarks) foreach(lc, node->lr_arowMarks)
{ {
...@@ -293,15 +279,25 @@ lnext: ...@@ -293,15 +279,25 @@ lnext:
HeapTupleData tuple; HeapTupleData tuple;
Buffer buffer; Buffer buffer;
/* ignore non-active child tables */ /* skip non-active child tables, but clear their test tuples */
if (!erm->ermActive) if (!erm->ermActive)
{ {
Assert(erm->rti != erm->prti); /* check it's child table */ Assert(erm->rti != erm->prti); /* check it's child table */
EvalPlanQualSetTuple(&node->lr_epqstate, erm->rti, NULL);
continue; continue;
} }
if (EvalPlanQualGetTuple(&node->lr_epqstate, erm->rti) != NULL) /* was tuple updated and fetched above? */
continue; /* it was updated and fetched above */ if (node->lr_curtuples[erm->rti - 1] != NULL)
{
/* yes, so set it as the EPQ test tuple for this rel */
EvalPlanQualSetTuple(&node->lr_epqstate,
erm->rti,
node->lr_curtuples[erm->rti - 1]);
/* freeing this tuple is now the responsibility of EPQ */
node->lr_curtuples[erm->rti - 1] = NULL;
continue;
}
/* foreign tables should have been fetched above */ /* foreign tables should have been fetched above */
Assert(erm->relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE); Assert(erm->relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE);
......
...@@ -144,3 +144,22 @@ accountid balance ...@@ -144,3 +144,22 @@ accountid balance
checking 1050 checking 1050
savings 600 savings 600
starting permutation: updateforss readforss c1 c2
step updateforss:
UPDATE table_a SET value = 'newTableAValue' WHERE id = 1;
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
step readforss:
SELECT ta.id AS ta_id, ta.value AS ta_value,
(SELECT ROW(tb.id, tb.value)
FROM table_b tb WHERE ta.id = tb.id) AS tb_row
FROM table_a ta
WHERE ta.id = 1 FOR UPDATE OF ta;
<waiting ...>
step c1: COMMIT;
step readforss: <... completed>
ta_id ta_value tb_row
1 newTableAValue (1,tableBValue)
step c2: COMMIT;
...@@ -16,12 +16,18 @@ setup ...@@ -16,12 +16,18 @@ setup
INSERT INTO c1 SELECT 0, a / 3, a % 3 FROM generate_series(0, 9) a; INSERT INTO c1 SELECT 0, a / 3, a % 3 FROM generate_series(0, 9) a;
INSERT INTO c2 SELECT 1, a / 3, a % 3 FROM generate_series(0, 9) a; INSERT INTO c2 SELECT 1, a / 3, a % 3 FROM generate_series(0, 9) a;
INSERT INTO c3 SELECT 2, a / 3, a % 3 FROM generate_series(0, 9) a; INSERT INTO c3 SELECT 2, a / 3, a % 3 FROM generate_series(0, 9) a;
CREATE TABLE table_a (id integer, value text);
CREATE TABLE table_b (id integer, value text);
INSERT INTO table_a VALUES (1, 'tableAValue');
INSERT INTO table_b VALUES (1, 'tableBValue');
} }
teardown teardown
{ {
DROP TABLE accounts; DROP TABLE accounts;
DROP TABLE p CASCADE; DROP TABLE p CASCADE;
DROP TABLE table_a, table_b;
} }
session "s1" session "s1"
...@@ -64,6 +70,15 @@ step "lockwithvalues" { ...@@ -64,6 +70,15 @@ step "lockwithvalues" {
FOR UPDATE OF a1; FOR UPDATE OF a1;
} }
# these tests exercise EvalPlanQual with a SubLink sub-select (which should be
# unaffected by any EPQ recheck behavior in the outer query); cf bug #14034
step "updateforss" {
UPDATE table_a SET value = 'newTableAValue' WHERE id = 1;
UPDATE table_b SET value = 'newTableBValue' WHERE id = 1;
}
session "s2" session "s2"
setup { BEGIN ISOLATION LEVEL READ COMMITTED; } setup { BEGIN ISOLATION LEVEL READ COMMITTED; }
step "wx2" { UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking'; } step "wx2" { UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking'; }
...@@ -81,6 +96,13 @@ step "returningp1" { ...@@ -81,6 +96,13 @@ step "returningp1" {
WITH u AS ( UPDATE p SET b = b WHERE a > 0 RETURNING * ) WITH u AS ( UPDATE p SET b = b WHERE a > 0 RETURNING * )
SELECT * FROM u; SELECT * FROM u;
} }
step "readforss" {
SELECT ta.id AS ta_id, ta.value AS ta_value,
(SELECT ROW(tb.id, tb.value)
FROM table_b tb WHERE ta.id = tb.id) AS tb_row
FROM table_a ta
WHERE ta.id = 1 FOR UPDATE OF ta;
}
step "c2" { COMMIT; } step "c2" { COMMIT; }
session "s3" session "s3"
...@@ -95,3 +117,4 @@ permutation "readp1" "writep1" "readp2" "c1" "c2" ...@@ -95,3 +117,4 @@ permutation "readp1" "writep1" "readp2" "c1" "c2"
permutation "writep2" "returningp1" "c1" "c2" permutation "writep2" "returningp1" "c1" "c2"
permutation "wx2" "partiallock" "c2" "c1" "read" permutation "wx2" "partiallock" "c2" "c1" "read"
permutation "wx2" "lockwithvalues" "c2" "c1" "read" permutation "wx2" "lockwithvalues" "c2" "c1" "read"
permutation "updateforss" "readforss" "c1" "c2"
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment