Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
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 br...@momjian.ushttp://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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
On Tue, Dec 16, 2014 at 12:21 AM, Kyotaro HORIGUCHI horiguchi.kyot...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
Robert Haas robertmh...@gmail.com writes: On Tue, Dec 16, 2014 at 12:21 AM, Kyotaro HORIGUCHI horiguchi.kyot...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
Thank you for the answer. That sounds reasonable from the situation. On Tue, Dec 16, 2014 at 12:21 AM, Kyotaro HORIGUCHI horiguchi.kyot...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
Etsuro Fujita fujita.ets...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/16 2:59), Tom Lane wrote: Etsuro Fujita fujita.ets...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
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 t...@sss.pgh.pa.us wrote in 17534.1418348...@sss.pgh.pa.us Kyotaro HORIGUCHI horiguchi.kyot...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/12 11:33), Etsuro Fujita wrote: (2014/12/12 11:19), Tom Lane wrote: Etsuro Fujita fujita.ets...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
Etsuro Fujita fujita.ets...@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
[HACKERS] [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
From a0e26063bd412ce06df7dc252b8881e51af10f35 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi horiguchi.kyot...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
Kyotaro HORIGUCHI horiguchi.kyot...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(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).
*** 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] http://www.postgresql.org/message-id/5487bb9d.7070...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
Etsuro Fujita fujita.ets...@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
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/12 11:19), Tom Lane wrote: Etsuro Fujita fujita.ets...@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
