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

On Thu, Mar 6, 2014 at 8:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> On Sat, Mar 1, 2014 at 6:51 PM, <no-email(at)example(dot)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