Обсуждение: Getting ERROR with FOR UPDATE/SHARE for partitioned table.

On Fri, May 22, 2020 at 9:09 PM amul sul <sulamul@gmail.com> wrote:
> I tried similar things on inherit partitioning as follow and that looks fine:
>
> DROP TABLE tbl;
> CREATE TABLE tbl (c1 INT,c2 TEXT);
> CREATE TABLE tbl_null(check (c1 is NULL)) INHERITS (tbl);
> CREATE TABLE tbl_1 (check (c1 > 0 and c1 < 4)) INHERITS (tbl);
> INSERT INTO tbl_1 VALUES(generate_series(1,3));
>
> postgres=# SELECT func(10);
>  func
> ------
>    10
> (1 row)
>
> On looking further for declarative partition, I found that issue happens only if
> the partitioning pruning enabled, see this:
>
> -- Execute on original set of test case.
> postgres=# ALTER FUNCTION func SET enable_partition_pruning to off;
> ALTER FUNCTION
>
> postgres=# SELECT func(10);
>  func
> ------
>    10
> (1 row)
>
> I think we need some indication in execCurrentOf() to skip error if the relation
> is pruned.  Something like that we already doing for inheriting partitioning,
> see following comment execCurrentOf():
>
>         /*
>          * This table didn't produce the cursor's current row; some other
>          * inheritance child of the same parent must have.  Signal caller to
>          * do nothing on this table.
>          */

Actually, if you declare the cursor without FOR SHARE/UPDATE, the case
would fail even with traditional inheritance:

drop table if exists p cascade;
create table p (a int);
create table c (check (a = 2)) inherits (p);
insert into p values (1);
insert into c values (2);
begin;
declare c cursor for select * from p where a = 1;
fetch c;
update p set a = a where current of c;
ERROR:  cursor "c" is not a simply updatable scan of table "c"
ROLLBACK

When there are no RowMarks to use because no FOR SHARE/UPDATE clause
was specified when declaring the cursor, execCurrentOf() tries to find
the cursor's current table by looking up its Scan node in the plan
tree but will not find it if it was excluded in the cursor's query.

With FOR SHARE/UPDATE, it seems to work because the planner delivers
the RowMarks of all the children irrespective of whether or not they
are present in the plan tree itself (something I had complained about
in past [1]).  execCurrentOf() doesn't complain as long as there is a
RowMark present even if it's never used.  For partitioning, the
planner doesn't make RowMarks for pruned partitions, so
execCurrentOf() can't find one if it's passed a pruned partition's
oid.

I don't see a way to avoid these errors.  How does execCurrentOf()
distinguish a table that could *never* be present in a cursor from a
table that could be had it not been pruned/excluded?  If it can do
that, then give an error for the former and return false for the
latter.

I guess the workaround is to declare the cursor such that no
partitions/children are pruned/excluded.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/468c85d9-540e-66a2-1dde-fec2b741e688%40lab.ntt.co.jp

On Wed, May 27, 2020 at 12:53 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Fri, May 22, 2020 at 9:09 PM amul sul <sulamul@gmail.com> wrote:
> > I tried similar things on inherit partitioning as follow and that looks fine:
> >
> > DROP TABLE tbl;
> > CREATE TABLE tbl (c1 INT,c2 TEXT);
> > CREATE TABLE tbl_null(check (c1 is NULL)) INHERITS (tbl);
> > CREATE TABLE tbl_1 (check (c1 > 0 and c1 < 4)) INHERITS (tbl);
> > INSERT INTO tbl_1 VALUES(generate_series(1,3));
> >
> > postgres=# SELECT func(10);
> >  func
> > ------
> >    10
> > (1 row)
> >
> > On looking further for declarative partition, I found that issue happens only if
> > the partitioning pruning enabled, see this:
> >
> > -- Execute on original set of test case.
> > postgres=# ALTER FUNCTION func SET enable_partition_pruning to off;
> > ALTER FUNCTION
> >
> > postgres=# SELECT func(10);
> >  func
> > ------
> >    10
> > (1 row)
> >
> > I think we need some indication in execCurrentOf() to skip error if the relation
> > is pruned.  Something like that we already doing for inheriting partitioning,
> > see following comment execCurrentOf():
> >
> >         /*
> >          * This table didn't produce the cursor's current row; some other
> >          * inheritance child of the same parent must have.  Signal caller to
> >          * do nothing on this table.
> >          */
>
> Actually, if you declare the cursor without FOR SHARE/UPDATE, the case
> would fail even with traditional inheritance:
>
> drop table if exists p cascade;
> create table p (a int);
> create table c (check (a = 2)) inherits (p);
> insert into p values (1);
> insert into c values (2);
> begin;
> declare c cursor for select * from p where a = 1;
> fetch c;
> update p set a = a where current of c;
> ERROR:  cursor "c" is not a simply updatable scan of table "c"
> ROLLBACK
>
> When there are no RowMarks to use because no FOR SHARE/UPDATE clause
> was specified when declaring the cursor, execCurrentOf() tries to find
> the cursor's current table by looking up its Scan node in the plan
> tree but will not find it if it was excluded in the cursor's query.
>
> With FOR SHARE/UPDATE, it seems to work because the planner delivers
> the RowMarks of all the children irrespective of whether or not they
> are present in the plan tree itself (something I had complained about
> in past [1]).  execCurrentOf() doesn't complain as long as there is a
> RowMark present even if it's never used.  For partitioning, the
> planner doesn't make RowMarks for pruned partitions, so
> execCurrentOf() can't find one if it's passed a pruned partition's
> oid.

I am missing something in this explanation. WHERE CURRENT OF works on
the row that was last fetched from a cursor. How could a pruned
partition's row be fetched and thus cause this error.
-- 
Best Wishes,
Ashutosh Bapat

On Wed, May 27, 2020 at 9:11 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Wed, May 27, 2020 at 12:53 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, May 22, 2020 at 9:09 PM amul sul <sulamul@gmail.com> wrote:
> > > I tried similar things on inherit partitioning as follow and that looks fine:
> > >
> > > DROP TABLE tbl;
> > > CREATE TABLE tbl (c1 INT,c2 TEXT);
> > > CREATE TABLE tbl_null(check (c1 is NULL)) INHERITS (tbl);
> > > CREATE TABLE tbl_1 (check (c1 > 0 and c1 < 4)) INHERITS (tbl);
> > > INSERT INTO tbl_1 VALUES(generate_series(1,3));
> > >
> > > postgres=# SELECT func(10);
> > >  func
> > > ------
> > >    10
> > > (1 row)
> > >
> > > On looking further for declarative partition, I found that issue happens only if
> > > the partitioning pruning enabled, see this:
> > >
> > > -- Execute on original set of test case.
> > > postgres=# ALTER FUNCTION func SET enable_partition_pruning to off;
> > > ALTER FUNCTION
> > >
> > > postgres=# SELECT func(10);
> > >  func
> > > ------
> > >    10
> > > (1 row)
> > >
> > > I think we need some indication in execCurrentOf() to skip error if the relation
> > > is pruned.  Something like that we already doing for inheriting partitioning,
> > > see following comment execCurrentOf():
> > >
> > >         /*
> > >          * This table didn't produce the cursor's current row; some other
> > >          * inheritance child of the same parent must have.  Signal caller to
> > >          * do nothing on this table.
> > >          */
> >
> > Actually, if you declare the cursor without FOR SHARE/UPDATE, the case
> > would fail even with traditional inheritance:
> >
> > drop table if exists p cascade;
> > create table p (a int);
> > create table c (check (a = 2)) inherits (p);
> > insert into p values (1);
> > insert into c values (2);
> > begin;
> > declare c cursor for select * from p where a = 1;
> > fetch c;
> > update p set a = a where current of c;
> > ERROR:  cursor "c" is not a simply updatable scan of table "c"
> > ROLLBACK
> >
> > When there are no RowMarks to use because no FOR SHARE/UPDATE clause
> > was specified when declaring the cursor, execCurrentOf() tries to find
> > the cursor's current table by looking up its Scan node in the plan
> > tree but will not find it if it was excluded in the cursor's query.
> >
> > With FOR SHARE/UPDATE, it seems to work because the planner delivers
> > the RowMarks of all the children irrespective of whether or not they
> > are present in the plan tree itself (something I had complained about
> > in past [1]).  execCurrentOf() doesn't complain as long as there is a
> > RowMark present even if it's never used.  For partitioning, the
> > planner doesn't make RowMarks for pruned partitions, so
> > execCurrentOf() can't find one if it's passed a pruned partition's
> > oid.
>
> I am missing something in this explanation. WHERE CURRENT OF works on
> the row that was last fetched from a cursor. How could a pruned
> partition's row be fetched and thus cause this error.

So in Rajkumar's example, the cursor is declared as:

CURSOR IS SELECT * FROM tbl WHERE c1< 5 FOR UPDATE;

and the WHERE CURRENT OF query is this:

 UPDATE tbl SET c2='aa' WHERE CURRENT OF cur;

You can see that the UPDATE will scan all partitions, whereas the
cursor's query does not.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 28, 2020 at 1:36 PM amul sul <sulamul@gmail.com> wrote:
> On Wed, May 27, 2020 at 12:53 PM Amit Langote <amitlangote09@gmail.com> wrote:
>> Actually, if you declare the cursor without FOR SHARE/UPDATE, the case
>> would fail even with traditional inheritance:
>>
>> drop table if exists p cascade;
>> create table p (a int);
>> create table c (check (a = 2)) inherits (p);
>> insert into p values (1);
>> insert into c values (2);
>> begin;
>> declare c cursor for select * from p where a = 1;
>> fetch c;
>> update p set a = a where current of c;
>> ERROR:  cursor "c" is not a simply updatable scan of table "c"
>> ROLLBACK
>>
>
> I am not sure I understood the point, you'll see the same error with declarative
> partitioning as well.

My point is that if a table is not present in the cursor's plan, there
is no way for CURRENT OF to access it.  Giving an error in that case
seems justified.

OTOH, when the CURRENT OF implementation has RowMarks to look at, it
avoids the error for traditional inheritance children due their
inactive RowMarks being present in the cursor's PlannedStmt.  I think
that's only by accident though.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On Thu, May 28, 2020 at 1:36 PM amul sul <sulamul@gmail.com> wrote:
> On Wed, May 27, 2020 at 12:53 PM Amit Langote <amitlangote09@gmail.com> wrote:
>> I guess the workaround is to declare the cursor such that no
>> partitions/children are pruned/excluded.
>
> Disabling pruning as well -- at-least for the statement or function.

Now *that* is actually a workaround to tell a customer. :)

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 27, 2020 at 6:51 PM Amit Langote <amitlangote09@gmail.com> wrote:

>
> So in Rajkumar's example, the cursor is declared as:
>
> CURSOR IS SELECT * FROM tbl WHERE c1< 5 FOR UPDATE;
>
> and the WHERE CURRENT OF query is this:
>
>  UPDATE tbl SET c2='aa' WHERE CURRENT OF cur;

Thanks for the clarification. So it looks like we expand UPDATE on
partitioned table to UPDATE on each partition (inheritance_planner for
DML) and then execute each of those. If CURRENT OF were to save the
table oid or something we could run the UPDATE only on that partition.
I am possibly shooting in dark, but this puzzles me. And it looks like
we can cause wrong rows to be updated in non-partition inheritance
where the ctids match?

-- 
Best Wishes,
Ashutosh Bapat

On Thu, May 28, 2020 at 11:08 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Wed, May 27, 2020 at 6:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > So in Rajkumar's example, the cursor is declared as:
> >
> > CURSOR IS SELECT * FROM tbl WHERE c1< 5 FOR UPDATE;
> >
> > and the WHERE CURRENT OF query is this:
> >
> >  UPDATE tbl SET c2='aa' WHERE CURRENT OF cur;
>
> Thanks for the clarification. So it looks like we expand UPDATE on
> partitioned table to UPDATE on each partition (inheritance_planner for
> DML) and then execute each of those. If CURRENT OF were to save the
> table oid or something we could run the UPDATE only on that partition.

Are you saying that the planner should take into account the state of
the cursor specified in WHERE CURRENT OF to determine which of the
tables to scan for the UPDATE?  Note that neither partition pruning
nor constraint exclusion know that CurrentOfExpr can possibly allow to
exclude children of the UPDATE target.

> I am possibly shooting in dark, but this puzzles me. And it looks like
> we can cause wrong rows to be updated in non-partition inheritance
> where the ctids match?

I don't think that hazard exists, because the table OID is matched
before the TID.  Consider this example:

drop table if exists p cascade;
create table p (a int);
create table c (check (a = 2)) inherits (p);
insert into p values (1);
insert into c values (2);
begin;
declare c cursor for select * from p;
fetch c;
update p set a = a where current of c;
                         QUERY PLAN
------------------------------------------------------------
 Update on p  (cost=0.00..8.02 rows=2 width=10)
   Update on p
   Update on c p_1
   ->  Tid Scan on p  (cost=0.00..4.01 rows=1 width=10)
         TID Cond: CURRENT OF c
   ->  Tid Scan on c p_1  (cost=0.00..4.01 rows=1 width=10)
         TID Cond: CURRENT OF c
(7 rows)

Whenever the TID scan evaluates the CURRENT OF qual, it passes the
table being scanned to execCurrentOf().  execCurrentOf() then fetches
the ExecRowMark or the ScanState for *that* table from the cursor's
("c") PlanState via its portal.  Only if it confirms that such a
ExecRowMark or a ScanState exists and is valid/active that it returns
the TID found therein as the cursor's current TID.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com

On 2020-Jun-03, Amit Langote wrote:

> Are you saying that the planner should take into account the state of
> the cursor specified in WHERE CURRENT OF to determine which of the
> tables to scan for the UPDATE?  Note that neither partition pruning
> nor constraint exclusion know that CurrentOfExpr can possibly allow to
> exclude children of the UPDATE target.

I think from a user POV this is pretty obvious.  The user doesn't really
care that there are partitions that were pruned, because obviously
UPDATE WHERE CURRENT OF cannot refer to a tuple in those partitions.

> > I am possibly shooting in dark, but this puzzles me. And it looks like
> > we can cause wrong rows to be updated in non-partition inheritance
> > where the ctids match?
> 
> I don't think that hazard exists, because the table OID is matched
> before the TID.

It sounds like CURRENT OF should somehow inform pruning that the
partition OID is to be matched as well.  I don't know offhand if this is
easily implementable, though.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jun 3, 2020 at 12:44 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, May 28, 2020 at 11:08 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> > On Wed, May 27, 2020 at 6:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > So in Rajkumar's example, the cursor is declared as:
> > >
> > > CURSOR IS SELECT * FROM tbl WHERE c1< 5 FOR UPDATE;
> > >
> > > and the WHERE CURRENT OF query is this:
> > >
> > >  UPDATE tbl SET c2='aa' WHERE CURRENT OF cur;
> >
> > Thanks for the clarification. So it looks like we expand UPDATE on
> > partitioned table to UPDATE on each partition (inheritance_planner for
> > DML) and then execute each of those. If CURRENT OF were to save the
> > table oid or something we could run the UPDATE only on that partition.
>
> Are you saying that the planner should take into account the state of
> the cursor specified in WHERE CURRENT OF to determine which of the
> tables to scan for the UPDATE?  Note that neither partition pruning
> nor constraint exclusion know that CurrentOfExpr can possibly allow to
> exclude children of the UPDATE target.

Yes. But it may not be possible to know the value of current of at the
time of planning since that need not be a plan time constant. This
pruning has to happen at run time. But as Alvaro has mentioned in his
reply for a user this is a surprising behaviour and should be fixed.

-- 
Best Wishes,
Ashutosh Bapat

On Fri, Jun 12, 2020 at 9:22 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Wed, Jun 3, 2020 at 12:44 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > Are you saying that the planner should take into account the state of
> > the cursor specified in WHERE CURRENT OF to determine which of the
> > tables to scan for the UPDATE?  Note that neither partition pruning
> > nor constraint exclusion know that CurrentOfExpr can possibly allow to
> > exclude children of the UPDATE target.
>
> Yes. But it may not be possible to know the value of current of at the
> time of planning since that need not be a plan time constant. This
> pruning has to happen at run time.

Good point about not doing anything at planning time.

I wonder if it wouldn't be okay to simply make execCurrentOf() return
false if it can't find either a row mark or a Scan node in the cursor
matching the table being updated/deleted from, instead of giving an
error message?  I mean what do we gain by erroring out here instead of
simply not doing anything?  Now, it would be nicer if we could do so
only if the table being updated/deleted from is a child table, but it
seems pretty inconvenient to tell that from the bottom of a plan tree
from where execCurrentOf() is called.

The other option would be to have some bespoke "pruning" logic in,
say, ExecInitModifyTable() that fetches the current active table from
the cursor and processes only the matching child result relation.  Or
maybe wait until we have run-time pruning for ModifyTable, because the
result relation code restructuring required for that will also be
something we'd need for this.

-- 
Amit Langote
EnterpriseDB: http://www.enterprisedb.com