Re: BUG #17633: Define rule on views which do insert to another relation trigger cache lookup failed error.

The following bug has been logged on the website:

Bug reference: 17633
Logged by: leafji's git home
Email address: jiye_sw(at)126(dot)com
PostgreSQL version: 14.5
Operating system: Centos
Description:

Just execute as follow sql:

drop view v1;
drop table t1;
drop table t2;
create table t1(i int, j int);
create table t2(a int, b int, c int, d int);
create or replace rule t1_r as on insert to t1 do also insert into t2(c,d)
values (new.i, new.j);
insert into t1 values (1,default);
insert into t1 values (1,default),(2, default);
-- create rule on t1 directly no issue
drop rule t1_r on t1;
create view v1 as select * from t1;
— create rule on view query t1 will trigger this issue
create or replace rule v1_r as on insert to v1 do also insert into t2(c,d)
values (new.i, new.j);
insert into v1 values (1,default);
-- must multi values.
insert into v1 values (1,default),(2, default);
=> it will trigger cache lookup failed for type.

On Tue, Oct 11, 2022 at 5:48 PM PG Bug reporting form <
noreply(at)postgresql(dot)org> wrote:

> Just execute as follow sql:
>
> drop view v1;
> drop table t1;
> drop table t2;
> create table t1(i int, j int);
> create table t2(a int, b int, c int, d int);
> create or replace rule t1_r as on insert to t1 do also insert into t2(c,d)
> values (new.i, new.j);
> insert into t1 values (1,default);
> insert into t1 values (1,default),(2, default);
> -- create rule on t1 directly no issue
> drop rule t1_r on t1;
> create view v1 as select * from t1;
> — create rule on view query t1 will trigger this issue
> create or replace rule v1_r as on insert to v1 do also insert into t2(c,d)
> values (new.i, new.j);
> insert into v1 values (1,default);
> -- must multi values.
> insert into v1 values (1,default),(2, default);
> => it will trigger cache lookup failed for type.

Thanks for the report! I can reproduce this issue.

Apparently there is something wrong when we process the DEFAULT marker
in rewriteValuesRTE, because the contents inside att_tup are invalid.

p /x att_tup->atttypid
$13 = 0x7f7f7f7f

Thanks
Richard

On Tue, Oct 11, 2022 at 6:05 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

>
> On Tue, Oct 11, 2022 at 5:48 PM PG Bug reporting form <
> noreply(at)postgresql(dot)org> wrote:
>
>> -- must multi values.
>> insert into v1 values (1,default),(2, default);
>> => it will trigger cache lookup failed for type.
>
>
> Thanks for the report! I can reproduce this issue.
>
> Apparently there is something wrong when we process the DEFAULT marker
> in rewriteValuesRTE, because the contents inside att_tup are invalid.
>
> p /x att_tup->atttypid
> $13 = 0x7f7f7f7f
>

I think the problem exists for auto-updatable view, as we leave the
DEFAULT items untouched because we expect to apply the underlying base
rel's default.

In this case there is a rewrite rule on the view. Applying the rule
we'd get a product query whose target entries referring to the VALUES
RTE have attribute 3 and 4 while the relation has only two attributes.
Then we proceed to replacing the remaining DEFAULT items with NULLs.
And when we try to access the relation's 3rd and 4th attributes, we are
accessing illegal memory areas.

Thanks
Richard

On Tue, 11 Oct 2022 at 20:09, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> I think the problem exists for auto-updatable view, as we leave the
> DEFAULT items untouched because we expect to apply the underlying base
> rel's default.
>
> In this case there is a rewrite rule on the view. Applying the rule
> we'd get a product query whose target entries referring to the VALUES
> RTE have attribute 3 and 4 while the relation has only two attributes.
> Then we proceed to replacing the remaining DEFAULT items with NULLs.
> And when we try to access the relation's 3rd and 4th attributes, we are
> accessing illegal memory areas.
>

Yeah, I also notice this, attch a patch to fix it.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Tue, Oct 11, 2022 at 8:29 PM Japin Li <japinli(at)hotmail(dot)com> wrote:

>
> On Tue, 11 Oct 2022 at 20:09, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> >
> > I think the problem exists for auto-updatable view, as we leave the
> > DEFAULT items untouched because we expect to apply the underlying base
> > rel's default.
> >
> > In this case there is a rewrite rule on the view. Applying the rule
> > we'd get a product query whose target entries referring to the VALUES
> > RTE have attribute 3 and 4 while the relation has only two attributes.
> > Then we proceed to replacing the remaining DEFAULT items with NULLs.
> > And when we try to access the relation's 3rd and 4th attributes, we are
> > accessing illegal memory areas.
> >
>
> Yeah, I also notice this, attch a patch to fix it.

+1 for the idea. We need to identify the right target relation for each
product query and rt_entry_relation is not the right one.

A minor comment is can we know the product query is not CMD_SELECT?
If so I suggest we add an assertion before fetching the target relation,
something like:

Assert(pt->resultRelation != 0);

Thanks
Richard

On Tue, 11 Oct 2022 at 21:16, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Tue, Oct 11, 2022 at 8:29 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
>>
>> On Tue, 11 Oct 2022 at 20:09, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>>
>> Yeah, I also notice this, attch a patch to fix it.
>
>
> +1 for the idea. We need to identify the right target relation for each
> product query and rt_entry_relation is not the right one.
>

After some more thinking, I find the previous cannot work correctly.
For example:

CREATE OR REPLACE v1_r AS ON INSERT TO t1 DO ALSO SELECT * FROM t2;

> A minor comment is can we know the product query is not CMD_SELECT?
> If so I suggest we add an assertion before fetching the target relation,
> something like:
>
> Assert(pt->resultRelation != 0);
>

Oh, I think this might not be true. The product query comes from rules,
which might be a SELECT query, IIUC. See above example.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli(at)hotmail(dot)com> writes:
> On Tue, 11 Oct 2022 at 21:16, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>> +1 for the idea. We need to identify the right target relation for each
>> product query and rt_entry_relation is not the right one.

> After some more thinking, I find the previous cannot work correctly.

Yeah. The product query might not be an INSERT at all, yet
rewriteValuesRTE is assuming it is --- that whole business of
scanning the targetlist for referencing Vars and then saving
their resnos doesn't make sense otherwise.

I think the basic problem is that the two calls of rewriteValuesRTE
are really dealing with fundamentally different cases, and we should
probably not have tried to make the same function do both. I'm going
to try splitting it into two functions, one for the force_nulls case
and one for the !force_nulls case. The force_nulls case shouldn't
really need a target_relation parameter in the first place (since it
has no business assuming that the query is an INSERT). I think it
should just replace each SetToDefault with a NULL of the same type
as the SetToDefault, and call it good.

regards, tom lane

I wrote:
> I think the basic problem is that the two calls of rewriteValuesRTE
> are really dealing with fundamentally different cases, and we should
> probably not have tried to make the same function do both. I'm going
> to try splitting it into two functions, one for the force_nulls case
> and one for the !force_nulls case.

As attached. This seems *way* cleaner to me, even if it means we need
two copies of the loops-over-VALUES-list-entries. I didn't write a
regression test yet, but this fixes the submitted bug and passes
check-world.

regards, tom lane

On Tue, 11 Oct 2022 at 23:37, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> I think the basic problem is that the two calls of rewriteValuesRTE
>> are really dealing with fundamentally different cases, and we should
>> probably not have tried to make the same function do both. I'm going
>> to try splitting it into two functions, one for the force_nulls case
>> and one for the !force_nulls case.
>
> As attached. This seems *way* cleaner to me, even if it means we need
> two copies of the loops-over-VALUES-list-entries. I didn't write a
> regression test yet, but this fixes the submitted bug and passes
> check-world.
>

Great. LGTM. Should we add the test-case for this?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.