Thread: BUG #9398: DELETE refering to a materialized view produces "cannot lock rows in materialized view" error

The following bug has been logged on the website:

Bug reference:      9398
Logged by:          nakag
Email address:      no-email@example.com
PostgreSQL version: 9.3.3
Operating system:   Linux
Description:

CREATE TABLE base ( id int primary key );
CREATE MATERIALIZED VIEW mv AS SELECT * FROM base;
CREATE TABLE d ( id int primary key );
DELETE FROM d WHERE EXISTS ( SELECT * FROM mv WHERE mv.id = d.id );

The above code produces an ERROR "cannot lock rows in materialized view."

On Sat, Mar 1, 2014 at 6:51 PM,  <no-email@example.com> wrote:
> The following bug has been logged on the website:
>
> Bug reference:      9398
> Logged by:          nakag
> Email address:      no-email@example.com
> PostgreSQL version: 9.3.3
> Operating system:   Linux
> Description:
>
> CREATE TABLE base ( id int primary key );
> CREATE MATERIALIZED VIEW mv AS SELECT * FROM base;
> CREATE TABLE d ( id int primary key );
> DELETE FROM d WHERE EXISTS ( SELECT * FROM mv WHERE mv.id = d.id );
>
> The above code produces an ERROR "cannot lock rows in materialized view."
This smells like a limitation to matviews and not a bug... This error
message refers to CheckValidRowMarkRel:execMain.c:
        case RELKIND_MATVIEW:
            /* Should not get here */
            ereport(ERROR,
                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                     errmsg("cannot lock rows in materialized view \"%s\"",
                            RelationGetRelationName(rel))));
            break;
Even if it is clearly written that this code path should not be
taken... Well it is actually taken.

Note that doing a similar operation on a foreign table or a view works:
=# create table aa (a int);
CREATE TABLE
=# create materialized view bb as select * from aa;
SELECT 0
=# delete from aa using bb where aa.a = bb.a;
ERROR:  42809: cannot lock rows in materialized view "bb"
LOCATION:  CheckValidRowMarkRel, execMain.c:1109
Time: 0.929 ms
=# create view cc as select * from aa;
CREATE VIEW
Time: 10.108 ms
=# delete from aa using cc where aa.a = cc.a;
DELETE 0
-- Create FDW server, etc...
=# CREATE FOREIGN TABLE aa_foreign (a int) SERVER postgres_server
OPTIONS (table_name 'aa');
CREATE FOREIGN TABLE
Time: 2.290 ms
=# delete from aa using aa_foreign where aa.a = aa_foreign.a;
DELETE 0

For views, planner expands the view to the parent relations to not
face this error. But this is not doable for a matview because I do not
think we can take locks on its rows without support for incremental
updates. Am I right? Shouldn't the error message be more explicit
here?
Regards,
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Sat, Mar 1, 2014 at 6:51 PM,  <no-email@example.com> wrote:
>> CREATE TABLE base ( id int primary key );
>> CREATE MATERIALIZED VIEW mv AS SELECT * FROM base;
>> CREATE TABLE d ( id int primary key );
>> DELETE FROM d WHERE EXISTS ( SELECT * FROM mv WHERE mv.id = d.id );
>>
>> The above code produces an ERROR "cannot lock rows in materialized view."

> This smells like a limitation to matviews and not a bug...

Oh, it's a bug all right.  There is no reason this command should be
rejected.

There are two possible fixes:

1. We could teach the planner (planner.c, around line 2210 in HEAD)
that rows coming from materialized views need to be processed via
ROW_MARK_COPY instead of ROW_MARK_REFERENCE.

2. We could remove the error complaint in CheckValidRowMarkRel(),
allowing a matview row to be marked the same as a regular-table row.

Since matview rows do in fact have TIDs and the same
visibility/vacuumability rules as regular-table rows (no?), I see no
reason that #2 wouldn't work, though I admit I've not actually tried it.
(There might be similar checks on relkind further down that would also
have to be adjusted, for one thing.)  CheckValidRowMarkRel is not really
about locking; the requirement is only that it be possible to fetch back a
previously-read row value using the TID, and be sure that we get the same
tuple value we'd seen earlier in the same query.

Assuming that it does work, I think #2 is a preferable fix to #1,
because #1 implies making a usually-unnecessary copy of each row
selected from the matview.

Comments, objections?

            regards, tom lane

On Thu, Mar 6, 2014 at 8:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Sat, Mar 1, 2014 at 6:51 PM,  <no-email@example.com> wrote:
>>> CREATE TABLE base ( id int primary key );
>>> CREATE MATERIALIZED VIEW mv AS SELECT * FROM base;
>>> CREATE TABLE d ( id int primary key );
>>> DELETE FROM d WHERE EXISTS ( SELECT * FROM mv WHERE mv.id = d.id );
>>>
>>> The above code produces an ERROR "cannot lock rows in materialized view."
>
>> This smells like a limitation to matviews and not a bug...
>
> Oh, it's a bug all right.  There is no reason this command should be
> rejected.
OK. So I was obviously wrong :)

> 1. We could teach the planner (planner.c, around line 2210 in HEAD)
> that rows coming from materialized views need to be processed via
> ROW_MARK_COPY instead of ROW_MARK_REFERENCE.
>
> 2. We could remove the error complaint in CheckValidRowMarkRel(),
> allowing a matview row to be marked the same as a regular-table row.
>
> Since matview rows do in fact have TIDs and the same
> visibility/vacuumability rules as regular-table rows (no?), I see no
> reason that #2 wouldn't work, though I admit I've not actually tried it.
> (There might be similar checks on relkind further down that would also
> have to be adjusted, for one thing.)  CheckValidRowMarkRel is not really
> about locking; the requirement is only that it be possible to fetch back a
> previously-read row value using the TID, and be sure that we get the same
> tuple value we'd seen earlier in the same query.
>
> Assuming that it does work, I think #2 is a preferable fix to #1,
> because #1 implies making a usually-unnecessary copy of each row
> selected from the matview.
>
> Comments, objections?
I am not much a fan of #1, because this need some extra checks to
prevent locking clauses like FOR SHARE to run on matviews. And by
looking for example at commit 88c5566, this is not allowed.

After digging into #2, I finished with the attached patch that passes
regression tests. The idea is simply to allow a matview to use
ROW_MARK_REFERENCE and ROW_MARK_COPY when its rows are referenced. I
added a regression test as well in the patch.
Regards,
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Mar 6, 2014 at 8:46 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 2. We could remove the error complaint in CheckValidRowMarkRel(),
>> allowing a matview row to be marked the same as a regular-table row.
>>
>> Since matview rows do in fact have TIDs and the same
>> visibility/vacuumability rules as regular-table rows (no?), I see no
>> reason that #2 wouldn't work, though I admit I've not actually tried it.

> After digging into #2, I finished with the attached patch that passes
> regression tests. The idea is simply to allow a matview to use
> ROW_MARK_REFERENCE and ROW_MARK_COPY when its rows are referenced. I
> added a regression test as well in the patch.

The ROW_MARK_COPY test is unnecessary since control will never reach this
function for that case.  (If it did, most of the other case branches would
need to be checking it; checking only here is inconsistent.)

Also, after comparing this to the code in CheckValidResultRel, I wonder
why we aren't allowing SELECT FOR UPDATE in maintenance mode; that is,
shouldn't the test go like this:

      if (markType != ROW_MARK_REFERENCE &&
          !MatViewIncrementalMaintenanceIsEnabled())

However, that smells less like a bug fix and more like a feature addition,
so I'll leave it to Kevin to decide whether that's appropriate.  In the
meantime, I committed the clear bug fix with some cosmetic adjustments.

            regards, tom lane