[Bug] Inconsistent result for inheritance and FOR UPDATE.
Hello, this is about the second issue.
SELECT FROM <inheritance parent> WHERE <cond> FOR UPDATE may
return results which does not match the <cond>. The following
steps will reproduce the problematic behavior (A and B are
individual sessions) on master and back to 9.0 but 8.4 gives
correct result. I haven't checked on 8.3.
- Reproducing the symptom
A=# SET enable_seqscan TO false;
A=# SET enable_bitmapscan TO false;
A=# DROP TABLE IF EXISTS p CASCADE;
A=# CREATE TABLE p (id text, a text, b text, c text);
A=# CREATE INDEX p_i1 ON p (a, b, c) WHERE b IN ('0', '1') AND c = '0';
A=# CREATE TABLE c1 (LIKE p INCLUDING INDEXES) INHERITS (p);
A=# CREATE TABLE c2 (LIKE p INCLUDING INDEXES) INHERITS (p);
A=# CREATE TABLE c3 (LIKE p INCLUDING INDEXES) INHERITS (p);
A=# INSERT INTO c1 (SELECT 1 + a, 0, a % 4, 0 FROM generate_series(0, 7) a);
A=# INSERT INTO c2 (SELECT 11 + a, 1, a % 4, 0 FROM generate_series(0, 7) a);
A=# INSERT INTO c3 (SELECT 21 + a, 2, a % 4, 0 FROM generate_series(0, 7) a);
A=# ANALYZE;
A=# BEGIN;
A=# CREATE TEMP TABLE tt1 AS
A=# SELECT id FROM p WHERE b IN ('0', '1') AND c = '0' ORDER BY id LIMIT 1 FOR UPDATE;
A=# UPDATE p SET b = -1 WHERE id IN (SELECT id FROM tt1) RETURNING id;
A=# DROP TABLE tt1;
A=# SET enable_seqscan TO false;
A=# SET enable_bitmapscan TO false;
B=# SELECT tableoid, ctid, * FROM p WHERE b IN ('0', '1') AND c = '0' ORDER BY id LIMIT 1 FOR UPDATE;
A=# COMMIT;
On session B.
| tableoid | ctid | id | a | b | c
| ----------+-------+----+---+----+---
| 34316 | (0,9) | 1 | 0 | -1 | 0
b = -1 apparently contradicts the WHERE clause.
The plan for the query is as following. The part "b IN ('0',
'1')" in the WHERE clause is omitted even though required by EPQ
recheck.
Limit
-> LockRows
-> Sort
Sort Key: p.id
-> Result
-> Append
-> Index Scan using p_i1 on p
Index Cond: (c = '0'::text)
-> Index Scan using c1_a_b_c_idx on c1
Index Cond: (c = '0'::text)
-> Index Scan using c2_a_b_c_idx on c2
Index Cond: (c = '0'::text)
-> Index Scan using c3_a_b_c_idx on c3
Index Cond: (c = '0'::text)
- Analysys and solution
This is caused by that IndexRecheck examines the test tuple with
a qual "c = '0'" without "b IN ('0', '1')". The part has been
removed in create_indexscan_plan. It decieds whether to remove a
qual or not using get_parse_rowmark(root->parse(->rowMarks)) and
predicate_implied_by(). But the former always says no (NULL) for
child relations even if the parent has rowMarks.
On the other hand, rowmarks on children is already distributed at
the time by expand_inherited_rtentry() into root->rowMarks.
So I replaced the get_parse_rowmark() with get_plan_rowmark() as
the attached patch and the problem disappeared.
By the way, get_plan_rowmark() has the comment like this,
* This probably ought to be elsewhere, but there's no very good place
I haven't moved it anywhere but createplan.c might be the good plance.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Fix-bogus-tuples-for-inhertance-and-FOR-UPDATE.patchtext/x-patch; charset=us-asciiDownload
>From a0e26063bd412ce06df7dc252b8881e51af10f35 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 11 Dec 2014 18:20:37 +0900
Subject: [PATCH] Fix bogus tuples for inhertance and FOR UPDATE
---
src/backend/optimizer/plan/createplan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index bf8dbe0..30df3ce 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -29,6 +29,7 @@
#include "optimizer/clauses.h"
#include "optimizer/cost.h"
#include "optimizer/paths.h"
+#include "optimizer/prep.h"
#include "optimizer/placeholder.h"
#include "optimizer/plancat.h"
#include "optimizer/planmain.h"
@@ -1231,7 +1232,7 @@ create_indexscan_plan(PlannerInfo *root,
if (best_path->indexinfo->indpred)
{
if (baserelid != root->parse->resultRelation &&
- get_parse_rowmark(root->parse, baserelid) == NULL)
+ get_plan_rowmark(root->rowMarks, baserelid) == NULL)
if (predicate_implied_by(clausel,
best_path->indexinfo->indpred))
continue; /* implied by index predicate */
--
2.1.0.GIT
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
This is caused by that IndexRecheck examines the test tuple with
a qual "c = '0'" without "b IN ('0', '1')". The part has been
removed in create_indexscan_plan. It decieds whether to remove a
qual or not using get_parse_rowmark(root->parse(->rowMarks)) and
predicate_implied_by(). But the former always says no (NULL) for
child relations even if the parent has rowMarks.
On the other hand, rowmarks on children is already distributed at
the time by expand_inherited_rtentry() into root->rowMarks.
So I replaced the get_parse_rowmark() with get_plan_rowmark() as
the attached patch and the problem disappeared.
Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong. Will
fix that too.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(2014/12/12 10:37), Tom Lane wrote:
Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong. Will
fix that too.
I don't think we need to fix that too. In order to support that, I'm
proposing to modify postgresGetForeignPlan() in the following way [1]/messages/by-id/5487BB9D.7070605@lab.ntt.co.jp
(see fdw-inh-5.patch).
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 824,834 **** postgresGetForeignPlan(PlannerInfo *root,
{
RowMarkClause *rc = get_parse_rowmark(root->parse, baserel->relid); if (rc)
{
/*
! * Relation is specified as a FOR UPDATE/SHARE target, so handle
! * that.
*
* For now, just ignore any [NO] KEY specification, since (a) it's
* not clear what that means for a remote table that we don't have
--- 824,847 ----
{
RowMarkClause *rc = get_parse_rowmark(root->parse, baserel->relid);+ /*
+ * It's possible that relation is contained in an inheritance set and
+ * that parent relation is selected FOR UPDATE/SHARE. If so, get the
+ * RowMarkClause for parent relation.
+ */
+ if (rc == NULL)
+ {
+ PlanRowMark *prm = get_plan_rowmark(root->rowMarks, baserel->relid);
+
+ if (prm && prm->rti != prm->prti)
+ rc = get_parse_rowmark(root->parse, prm->prti);
+ }
+
if (rc)
{
/*
! * Relation or parent relation is specified as a FOR UPDATE/SHARE
! * target, so handle that.
*
* For now, just ignore any [NO] KEY specification, since (a) it's
* not clear what that means for a remote table that we don't have[1]: /messages/by-id/5487BB9D.7070605@lab.ntt.co.jp
Thanks,
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
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
(2014/12/12 10:37), Tom Lane wrote:
Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong. Will
fix that too.
I don't think we need to fix that too. In order to support that, I'm
proposing to modify postgresGetForeignPlan() in the following way [1]
(see fdw-inh-5.patch).
My goodness, that's ugly. And it's still wrong, because this is planner
code so it shouldn't be using get_parse_rowmark at all. The whole point
here is that the rowmark info has been transformed into something
appropriate for the planner to use. While that transformation is
relatively trivial today, it might not always be so.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(2014/12/12 11:19), Tom Lane wrote:
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
(2014/12/12 10:37), Tom Lane wrote:
Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong. Will
fix that too.I don't think we need to fix that too. In order to support that, I'm
proposing to modify postgresGetForeignPlan() in the following way [1]
(see fdw-inh-5.patch).My goodness, that's ugly. And it's still wrong, because this is planner
code so it shouldn't be using get_parse_rowmark at all. The whole point
here is that the rowmark info has been transformed into something
appropriate for the planner to use. While that transformation is
relatively trivial today, it might not always be so.
OK, I'll update the inheritance patch on top of the revison you'll make.
Thanks,
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
(2014/12/12 11:33), Etsuro Fujita wrote:
(2014/12/12 11:19), Tom Lane wrote:
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
(2014/12/12 10:37), Tom Lane wrote:
Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong. Will
fix that too.I don't think we need to fix that too. In order to support that, I'm
proposing to modify postgresGetForeignPlan() in the following way [1]
(see fdw-inh-5.patch).My goodness, that's ugly. And it's still wrong, because this is planner
code so it shouldn't be using get_parse_rowmark at all. The whole point
here is that the rowmark info has been transformed into something
appropriate for the planner to use. While that transformation is
relatively trivial today, it might not always be so.OK, I'll update the inheritance patch on top of the revison you'll make.
Thanks for your speedy work.
While updating the inheritance patch, I noticed that the fix for
postgresGetForeignPlan() is not right. Since PlanRowMarks for foreign
tables get the ROW_MARK_COPY markType during preprocess_rowmarks(), so
we can't get the locking strength from the PlanRowMarks, IIUC. In order
to get the locking strength, I think we need to see the RowMarkClauses
and thus still need to use get_parse_rowmark() in
postgresGetForeignPlan(), though I agree with you that that is ugly.
Thanks,
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
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
(2014/12/12 10:37), Tom Lane wrote:
Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong. Will
fix that too.
While updating the inheritance patch, I noticed that the fix for
postgresGetForeignPlan() is not right. Since PlanRowMarks for foreign
tables get the ROW_MARK_COPY markType during preprocess_rowmarks(), so
we can't get the locking strength from the PlanRowMarks, IIUC.
Ugh, you're right.
In order
to get the locking strength, I think we need to see the RowMarkClauses
and thus still need to use get_parse_rowmark() in
postgresGetForeignPlan(), though I agree with you that that is ugly.
I think this needs more thought; I'm still convinced that having the FDW
look at the parse rowmarks is the Wrong Thing. However, we don't need
to solve it in existing branches. With 9.4 release so close, the right
thing is to revert that change for now and consider a HEAD-only patch
later. (One idea is to go ahead and make a ROW_MARK_COPY item, but
add a field to PlanRowMark to record the original value. We should
probably also think about allowing FDWs to change these settings if
they want to. The real source of trouble here is that planner.c
has a one-size-fits-all approach to row locking for FDWs; and we're
now seeing that that one size doesn't fit postgres_fdw.)
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(2014/12/13 1:17), Tom Lane wrote:
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
(2014/12/12 10:37), Tom Lane wrote:
Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong.
In order
to get the locking strength, I think we need to see the RowMarkClauses
and thus still need to use get_parse_rowmark() in
postgresGetForeignPlan(), though I agree with you that that is ugly.
I think this needs more thought; I'm still convinced that having the FDW
look at the parse rowmarks is the Wrong Thing. However, we don't need
to solve it in existing branches. With 9.4 release so close, the right
thing is to revert that change for now and consider a HEAD-only patch
later.
OK
(One idea is to go ahead and make a ROW_MARK_COPY item, but
add a field to PlanRowMark to record the original value.
+1
We should
probably also think about allowing FDWs to change these settings if
they want to.
This is not clear to me. Maybe I'm missing something, but I think that
the FDW only needs to look at the original locking strength in
GetForeignPlan(). Please explain that in a little more detail.
Thanks,
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
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
(2014/12/13 1:17), Tom Lane wrote:
We should
probably also think about allowing FDWs to change these settings if
they want to.
This is not clear to me. Maybe I'm missing something, but I think that
the FDW only needs to look at the original locking strength in
GetForeignPlan(). Please explain that in a little more detail.
Well, the point is that for postgres_fdw, we could consider using the
same locked-row-identification methods as for local tables, ie CTID.
Now admittedly this might be the only such case, but I'm not entirely
convinced of that --- you could imagine using FDWs for many of the same
use-cases that KaiGai-san has been proposing custom scans for, and
in some of those cases CTIDs would be useful for row IDs.
We'd originally dismissed this on the argument that ROWMARK_REFERENCE
is a cheaper implementation than CTID-based implementations for any
FDW (since it avoids the possible need to fetch a remote row twice).
I'm not sure I still believe that though. Fetching all columns of all
retrieved rows in order to avoid what might be zero or a small number of
re-fetches is not obviously a win, especially not for FDWs that
represent not-actually-remote resources.
So as long as we're revisiting this area, it might be worth thinking
about letting an FDW have some say in which ROWMARK method is selected
for its tables.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(2014/12/16 2:59), Tom Lane wrote:
Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
(2014/12/13 1:17), Tom Lane wrote:
We should
probably also think about allowing FDWs to change these settings if
they want to.This is not clear to me. Maybe I'm missing something, but I think that
the FDW only needs to look at the original locking strength in
GetForeignPlan(). Please explain that in a little more detail.Well, the point is that for postgres_fdw, we could consider using the
same locked-row-identification methods as for local tables, ie CTID.
Now admittedly this might be the only such case, but I'm not entirely
convinced of that --- you could imagine using FDWs for many of the same
use-cases that KaiGai-san has been proposing custom scans for, and
in some of those cases CTIDs would be useful for row IDs.We'd originally dismissed this on the argument that ROWMARK_REFERENCE
is a cheaper implementation than CTID-based implementations for any
FDW (since it avoids the possible need to fetch a remote row twice).
I'm not sure I still believe that though. Fetching all columns of all
retrieved rows in order to avoid what might be zero or a small number of
re-fetches is not obviously a win, especially not for FDWs that
represent not-actually-remote resources.So as long as we're revisiting this area, it might be worth thinking
about letting an FDW have some say in which ROWMARK method is selected
for its tables.
Understood. So, to what extext should we consider such things in the
FDW improvement? We've already started an independent infrastructure
for such things, ie, custom scans, IIUC.
Thank you for the explanation!
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
Hello, I have a favor for you committers.
I confirmed that this issue the another have been fixed in the
repository, thank you.
Then, could you please give me when the next release of 9.2.10 is
to come?
The bugs are found in some system under developing, which is to
make use of PG9.2 and it wants to adopt the new release.
regards,
At Thu, 11 Dec 2014 20:37:34 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <17534.1418348254@sss.pgh.pa.us>
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
So I replaced the get_parse_rowmark() with get_plan_rowmark() as
the attached patch and the problem disappeared.Yeah, this is clearly a thinko: really, nothing in the planner should
be using get_parse_rowmark(). I looked around for other errors of the
same type and found that postgresGetForeignPlan() is also using
get_parse_rowmark(). While that's harmless at the moment because we
don't support foreign tables as children, it's still wrong. Will
fix that too.
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Dec 16, 2014 at 12:21 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
Hello, I have a favor for you committers.
I confirmed that this issue the another have been fixed in the
repository, thank you.Then, could you please give me when the next release of 9.2.10 is
to come?The bugs are found in some system under developing, which is to
make use of PG9.2 and it wants to adopt the new release.
We seem not to have had a new release of 9.2 since July, which is an
awfully long time ago. So, hopefully soon?
--
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
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Dec 16, 2014 at 12:21 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:Then, could you please give me when the next release of 9.2.10 is
to come?
We seem not to have had a new release of 9.2 since July, which is an
awfully long time ago. So, hopefully soon?
Nothing's likely to happen during the holidays, so probably mid-January
is the earliest feasible target.
I agree we're a bit overdue.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thank you for the answer. That sounds reasonable from the
situation.
On Tue, Dec 16, 2014 at 12:21 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:Then, could you please give me when the next release of 9.2.10 is
to come?We seem not to have had a new release of 9.2 since July, which is an
awfully long time ago. So, hopefully soon?Nothing's likely to happen during the holidays, so probably mid-January
is the earliest feasible target.I agree we're a bit overdue.
regards, tom lane
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Dec 16, 2014 at 11:57:54AM -0500, Tom Lane wrote:
We seem not to have had a new release of 9.2 since July, which is an
awfully long time ago. So, hopefully soon?Nothing's likely to happen during the holidays, so probably mid-January
is the earliest feasible target.I agree we're a bit overdue.
In our defense, we had the need for too many 9.3.X releases too closely,
so in some sense I am glad we didn't feel a dire need to release another
9.3.X recently. I agree with Tom that mid-January is likely.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers