Thread: [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) ->
IndexScan 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
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
(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.7070605@lab.ntt.co.jp
Thanks,
Best regards,
Etsuro Fujita
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
(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
(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
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
(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
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
(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
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
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
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
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
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. +