Обсуждение: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

The following bug has been logged on the website:

Bug reference:      18147
Logged by:          Hans Buschmann
Email address:      buschmann@nidsa.net
PostgreSQL version: 16.0
Operating system:   Fedora 38 x86-64 64bit, also on Win64
Description:

We have recently moved our production cluster from pg15.4 to pg16.0

In a long lasting correct case (since about pg 9.6) an update statement now
fails with $subject.

I have simplified the case and the error remains (here shown on windows)

------------------
the query:

-- explain analyze -- explain analyze verbose -- explain -- select * from (
-- select count(*) from ( -- select length(sel) from (
with qp_netto as (
select 
77812::int                              as id_of        ,
1.000000::numeric(8,6)                  as fac_to_us    ,
6.9318647425014148::numeric(8,3)        as prfac_netto_1,
0.0::numeric(8,3)                       as prfac_netto_2,
1.000000::numeric(8,6)                  as our_to_us    ,
6.88795000000000000000::numeric(8,3)    as prour_netto_1,
0.0::numeric(8,3)                       as prour_netto_2
)
-- select * from qp_netto;
update  or_followup set
 of_pr1_fac_netto=coalesce(prfac_netto_1,0.0)
,of_pr1_fac_netusd=coalesce(prfac_netto_1*fac_to_us,0.0)
,of_pr2_fac_netto=coalesce(prfac_netto_2,0.0)
,of_pr2_fac_netusd=coalesce(prfac_netto_2*fac_to_us,0.0)
,of_pr1_our_netto=coalesce(prour_netto_1,0.0)
,of_pr1_our_netusd=coalesce(prour_netto_1*our_to_us,0.0)
,of_pr2_our_netto=coalesce(prour_netto_2,0.0)
,of_pr2_our_netusd=coalesce(prour_netto_2*our_to_us,0.0)
from qp_netto
where
or_followup.id_of=qp_netto.id_of
and or_followup.of_season=35
;
------------------------
result:


xxxdb=# select version ();
-[ RECORD 1 ]-------------------------------------------------------
version | PostgreSQL 16.0, compiled by Visual C++ build 1935, 64-bit


xxxdb=# explain analyze -- explain analyze verbose -- explain -- select *
from ( -- select count(*) from ( -- select length(sel) from (
xxxdb-# with qp_netto as (
xxxdb(# select
xxxdb(# 77812::int                              as id_of        ,
xxxdb(# 1.000000::numeric(8,6)                  as fac_to_us    ,
xxxdb(# 6.9318647425014148::numeric(8,3)        as prfac_netto_1,
xxxdb(# 0.0::numeric(8,3)                       as prfac_netto_2,
xxxdb(# 1.000000::numeric(8,6)                  as our_to_us    ,
xxxdb(# 6.88795000000000000000::numeric(8,3)    as prour_netto_1,
xxxdb(# 0.0::numeric(8,3)                       as prour_netto_2
xxxdb(# )
xxxdb-# -- select * from qp_netto;
xxxdb-# update  or_followup set
xxxdb-#  of_pr1_fac_netto=coalesce(prfac_netto_1,0.0)
xxxdb-# ,of_pr1_fac_netusd=coalesce(prfac_netto_1*fac_to_us,0.0)
xxxdb-# ,of_pr2_fac_netto=coalesce(prfac_netto_2,0.0)
xxxdb-# ,of_pr2_fac_netusd=coalesce(prfac_netto_2*fac_to_us,0.0)
xxxdb-# ,of_pr1_our_netto=coalesce(prour_netto_1,0.0)
xxxdb-# ,of_pr1_our_netusd=coalesce(prour_netto_1*our_to_us,0.0)
xxxdb-# ,of_pr2_our_netto=coalesce(prour_netto_2,0.0)
xxxdb-# ,of_pr2_our_netusd=coalesce(prour_netto_2*our_to_us,0.0)
xxxdb-# from qp_netto
xxxdb-# where
xxxdb-# or_followup.id_of=qp_netto.id_of
xxxdb-# and or_followup.of_season=35
xxxdb-# ;
FEHLER:  invalid perminfoindex 0 in RTE with relid 17034

I have found a relating discussion under

https://www.postgresql.org/message-id/flat/CANQ0oxfxBKKTReQgSh_KbL99DqdjfBZTastC0XT2ZZMBkAhTQw%40mail.gmail.com

but could not resolve the problem.

The query is quite simplified.. Perhaps it is good to now, that the table
or_followup has an inherited table or_followup_archiv (= relid 17034) which
is chosen by of_season and has not the same index definitions as
or_followup.

Thank you for looking!

Hans Buschmann

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> The simplified query is preferred so long as it produces the error - but
> you really should be providing the create table commands to make the entire
> script self-contained.

Yeah, it's impossible to reproduce or investigate this without
table definitions ... and I for one don't feel like guessing at
the table details.

            regards, tom lane

Hans Buschmann <buschmann@nidsa.net> writes:
> For your reference I include a simple dump of a test case database, which executes the queries but does NOT reproduce
theerror. 
> This case seems much more complicated then I thought on first view.

Hmm.  The most likely explanation for the error sometimes appearing
and sometimes not is that it depends on the shape of the selected
query plan, which in turn would depend on current pg_statistics
contents.

Perhaps you could devise some dummy data that would repro the issue
and you could share with us?

            regards, tom lane

Hans Buschmann <buschmann@nidsa.net> writes:
> I finally managed to reproduce the bug on a demo database as shown below.

Thanks!  Reproduced it as described on HEAD (although for me, repeating
the query repeats the error, there's no need to do anything fancy to
re-arm it).  Backtrace looks like

#0  errfinish (filename=0xaad704 "parse_relation.c", lineno=3907,
    funcname=0xaae690 <__func__.24632> "getRTEPermissionInfo") at elog.c:480
#1  0x00000000004b07fa in getRTEPermissionInfo (rteperminfos=<optimized out>,
    rte=0x1725ba8) at parse_relation.c:3906
#2  0x00000000006d6345 in GetResultRTEPermissionInfo (
    estate=estate@entry=0x1739120, relinfo=<optimized out>,
    relinfo=<optimized out>) at execUtils.c:1386
#3  0x00000000006d755c in ExecGetUpdatedCols (relinfo=relinfo@entry=0x17395e0,
    estate=estate@entry=0x1739120) at execUtils.c:1295
#4  0x00000000006c8292 in index_unchanged_by_update (indexRelation=0x1772770,
    indexInfo=0x17ea4a0, estate=0x1739120, resultRelInfo=0x17395e0)
    at execIndexing.c:985
#5  ExecInsertIndexTuples (resultRelInfo=resultRelInfo@entry=0x17395e0,
    slot=slot@entry=0x17ea140, estate=0x1739120, update=update@entry=true,
    noDupErr=noDupErr@entry=false, specConflict=specConflict@entry=0x0,
    arbiterIndexes=0x0, onlySummarizing=false) at execIndexing.c:427
#6  0x00000000006f560a in ExecUpdateEpilogue (
    resultRelInfo=resultRelInfo@entry=0x17395e0,
    tupleid=tupleid@entry=0x7ffd1fa2b10e, oldtuple=oldtuple@entry=0x0,
    slot=0x17ea140, updateCxt=<optimized out>, context=<optimized out>,
    context=<optimized out>) at nodeModifyTable.c:2130
#7  0x00000000006f6b24 in ExecUpdate (context=0x7ffd1fa2b140,
    resultRelInfo=0x17395e0, tupleid=0x7ffd1fa2b10e, oldtuple=0x0,
    slot=0x17ea140, canSetTag=<optimized out>) at nodeModifyTable.c:2478
#8  0x00000000006f834b in ExecModifyTable (pstate=<optimized out>)
    at nodeModifyTable.c:3824

In ExecGetUpdatedCols,

(gdb) p relinfo->ri_RootResultRelInfo
$2 = (struct ResultRelInfo *) 0x0
(gdb) p relinfo->ri_RangeTableIndex
$3 = 5

RTE 5 is the added-on RTE for the child table or_followup_archiv.
It looks like

      {RANGETBLENTRY
      :alias
         {ALIAS
         :aliasname or_followup
         :colnames ("id_of" "of_season" "of_pr1_fac_netto" "of_pr1_fac_netusd"
          "of_pr2_fac_netto" "of_pr2_fac_netusd" "of_pr1_our_netto" "of_pr1_ou
         r_netusd" "of_pr2_our_netto" "of_pr2_our_netusd")
         }
      :eref
         {ALIAS
         :aliasname or_followup
         :colnames ("id_of" "of_season" "of_pr1_fac_netto" "of_pr1_fac_netusd"
          "of_pr2_fac_netto" "of_pr2_fac_netusd" "of_pr1_our_netto" "of_pr1_ou
         r_netusd" "of_pr2_our_netto" "of_pr2_our_netusd")
         }
      :rtekind 0
      :relid 41352     or_followup_archiv
      :relkind r
      :rellockmode 3
      :tablesample <>
      :perminfoindex 0
      :lateral false
      :inh false
      :inFromCl false
      :securityQuals <>
      }

So the proximate problem is that we're trying to fetch an updatedCols
bitmapset for a child table that has no permissions checks to make
and thus doesn't need an RTEPermissionInfo, so its perminfoindex is
zero.  It might be that this relinfo should have ri_RootResultRelInfo
set, but it doesn't.  It's also possible that we erred by taking those
bitmapsets out of RTEs, although that'd be an unpleasant conclusion
with v16 already released.

However ... the need to fetch that data is ultimately coming from
index_unchanged_by_update, and I don't see how that is not buggy
as can be, independently of this.  How can it be okay to ignore
the effects of BEFORE triggers when deciding if an index is unchanged?
If this is because it's "only a hint" and doesn't have to be reliable,
okay, but the documentation around indexUnchanged utterly fails to
make that clear.  I fear some poor index AM writer is going to get
screwed big time when they assume this flag is good for more than
heuristic decisions about when to do noncritical maintenance.

The reason I'm on about that is that if it's okay for
index_unchanged_by_update to lie, another approach we could consider
is to return a default result if we're looking at a child table
for which we lack updatedCols data.  We might have to do that in
v16, since we don't have much wiggle room to adjust the
RTEPermissionInfo data in a released branch.

(I haven't looked into exactly why it's so hard to reach this
error.  Seems like any inherited UPDATE is at risk, so there
must be additional gating factors to explain why we've not
noticed this before.)

            regards, tom lane

On Mon, Oct 23, 2023 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> However ... the need to fetch that data is ultimately coming from
> index_unchanged_by_update, and I don't see how that is not buggy
> as can be, independently of this.  How can it be okay to ignore
> the effects of BEFORE triggers when deciding if an index is unchanged?
> If this is because it's "only a hint" and doesn't have to be reliable,
> okay, but the documentation around indexUnchanged utterly fails to
> make that clear.  I fear some poor index AM writer is going to get
> screwed big time when they assume this flag is good for more than
> heuristic decisions about when to do noncritical maintenance.

That's fair, though note that index_unchanged_by_update does at least
own the fact that it ignores the effects of BEFORE triggers in code
comments. It also doesn't care about predicates in partial indexes,
for reasons that are fairly specific to the way that the hint is
actually used on the nbtree side.

> The reason I'm on about that is that if it's okay for
> index_unchanged_by_update to lie, another approach we could consider
> is to return a default result if we're looking at a child table
> for which we lack updatedCols data.  We might have to do that in
> v16, since we don't have much wiggle room to adjust the
> RTEPermissionInfo data in a released branch.

Note that index_unchanged_by_update already lies on v14: it
unconditionally indicates that any non-HOT update has unchanged
indexes for all indexes, regardless of their individual status. This
was due to a pragmatic trade-off made when looking for a backpatchable
fix, but I don't think that there's actually much downside to it. In
general I suspect that index_unchanged_by_update is a little
overengineered.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Oct 23, 2023 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If this is because it's "only a hint" and doesn't have to be reliable,
>> okay, but the documentation around indexUnchanged utterly fails to
>> make that clear.  I fear some poor index AM writer is going to get
>> screwed big time when they assume this flag is good for more than
>> heuristic decisions about when to do noncritical maintenance.

> That's fair, though note that index_unchanged_by_update does at least
> own the fact that it ignores the effects of BEFORE triggers in code
> comments. It also doesn't care about predicates in partial indexes,
> for reasons that are fairly specific to the way that the hint is
> actually used on the nbtree side.

Yeah, there are comments within index_unchanged_by_update about those
things.  What I'm unhappy about is that indexam.sgml's discussion of
the indexUnchanged flag makes it sound far more trustworthy than it
actually is.  Somebody who just read that doco and didn't scour the
underlying code would be badly misled.

            regards, tom lane

On Mon, Oct 23, 2023 at 11:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, there are comments within index_unchanged_by_update about those
> things.  What I'm unhappy about is that indexam.sgml's discussion of
> the indexUnchanged flag makes it sound far more trustworthy than it
> actually is.  Somebody who just read that doco and didn't scour the
> underlying code would be badly misled.

I understand. I'll come up with a doc patch for that later on today.

--
Peter Geoghegan

I wrote:
> So the proximate problem is that we're trying to fetch an updatedCols
> bitmapset for a child table that has no permissions checks to make
> and thus doesn't need an RTEPermissionInfo, so its perminfoindex is
> zero.  It might be that this relinfo should have ri_RootResultRelInfo
> set, but it doesn't.

That seems to be a fairly good guess.  I was able to reduce the
test case to this:


drop table if exists t1 cascade;

create table t1 (f1 int, f2 int, f3 int
  , check (f1 < 10) no inherit
);

create table t2 () inherits(t1);

insert into t2 select i, i+1, 0 from generate_series(1,1000) i;

create index on t2(f1,f2);

update t1 set f3 = 11 where f1 = 12 and f2 = 13;


You don't see the failure without the check constraint on t1.
What is happening is that with that, we remove t1 from the plan
entirely, causing us to think that t2 is the root target relation,
and then things blow up because the root should have an
RTEPermissionInfo and doesn't.  The place where things start
to go off the rails is in ExecInitModifyTable:

     * If it's a partitioned table, the root partition doesn't appear
     * elsewhere in the plan and its RT index is given explicitly in
     * node->rootRelation.  Otherwise (i.e. table inheritance) the target
     * relation is the first relation in the node->resultRelations list.

node->resultRelations contains only t2, so boom.

Maybe we could fix this by setting ModifyTable.rootRelation to
the parent relation in the plain-inheritance case not only the
partitioned case; but I've not investigated what else might be
expecting it to be set only for partitions.

According to this understanding, the root cause is quite old
and the RTEPermissionInfo failure just exposes it in an obvious
way.  I wonder whether we can find related cases that misbehave
even before v16.

> (I haven't looked into exactly why it's so hard to reach this
> error.  Seems like any inherited UPDATE is at risk, so there
> must be additional gating factors to explain why we've not
> noticed this before.)

I haven't tracked that down yet, but I did find that without the
CHECK constraint we don't reach index_unchanged_by_update at
all in repeat executions of the problem query.  This is because
ExecUpdateEpilogue sees updateCxt->updateIndexes == TU_None
and thinks it doesn't have to do anything.  This seems a bit
suspicious in its own right.  Maybe it's okay, because the
updates would be HOT in this test case, but the first one
should be HOT too.  It smells like something is being cached
across queries that probably should not be.

            regards, tom lane

On Mon, Oct 23, 2023 at 12:01 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Mon, Oct 23, 2023 at 11:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Yeah, there are comments within index_unchanged_by_update about those
> > things.  What I'm unhappy about is that indexam.sgml's discussion of
> > the indexUnchanged flag makes it sound far more trustworthy than it
> > actually is.  Somebody who just read that doco and didn't scour the
> > underlying code would be badly misled.
>
> I understand. I'll come up with a doc patch for that later on today.

What do you think of the attached?

If there are no objections I'll commit this soon, backpatching to v14.

Separately, I wonder if index_unchanged_by_update should actually just
always give the hint with a non-HOT update, regardless of the
specifics for each index/its columns -- just like on the v14 branch.

In general I'm more concerned about the danger of not giving the hint
when we should, rather than giving the hint too often. Most individual
btinsert() calls do nothing with the hint already (because there is
still enough free space for the incoming item on the page). The hint
is inherently nothing more than a signal that bottom-up index deletion
might be a good idea, iff we're just about out of better options for
affected leaf pages.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Oct 23, 2023 at 12:01 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> I understand. I'll come up with a doc patch for that later on today.

> What do you think of the attached?

WFM.

> Separately, I wonder if index_unchanged_by_update should actually just
> always give the hint with a non-HOT update, regardless of the
> specifics for each index/its columns -- just like on the v14 branch.

I'm confused.  Wouldn't that be the exact opposite of "unchanged"?

Maybe the real problem here is that the meaning of the hint is not
what you'd expect from its name?

            regards, tom lane

On Mon, Oct 23, 2023 at 5:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Separately, I wonder if index_unchanged_by_update should actually just
> > always give the hint with a non-HOT update, regardless of the
> > specifics for each index/its columns -- just like on the v14 branch.
>
> I'm confused.  Wouldn't that be the exact opposite of "unchanged"?

Well, in practice "indexUnchanged = true" means "do bottom-up deletion
if it's the only way to avoid a page split". The justification is that
the incoming tuple is "logically unchanged" (actually it's more
complicated than that, but that's our starting point). Maybe that
naming convention makes things more confusing than necessary. Naming
things is hard.

> Maybe the real problem here is that the meaning of the hint is not
> what you'd expect from its name?

Maybe. Perhaps I should have chosen a name that made it clearer that
there really is only one way to apply "indexUnchanged = true". Though
the docs are pretty clear about this already. I can't say I feel too
strongly about the name myself.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Oct 23, 2023 at 5:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Separately, I wonder if index_unchanged_by_update should actually just
>>> always give the hint with a non-HOT update, regardless of the
>>> specifics for each index/its columns -- just like on the v14 branch.

>> I'm confused.  Wouldn't that be the exact opposite of "unchanged"?

> Well, in practice "indexUnchanged = true" means "do bottom-up deletion
> if it's the only way to avoid a page split". The justification is that
> the incoming tuple is "logically unchanged" (actually it's more
> complicated than that, but that's our starting point).

But doesn't the need for a non-HOT update show that the tuple *was*
changed --- in index-relevant columns, even?  Maybe I'm still not
understanding exactly what condition we're detecting.

            regards, tom lane

On Mon, Oct 23, 2023 at 6:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Well, in practice "indexUnchanged = true" means "do bottom-up deletion
> > if it's the only way to avoid a page split". The justification is that
> > the incoming tuple is "logically unchanged" (actually it's more
> > complicated than that, but that's our starting point).
>
> But doesn't the need for a non-HOT update show that the tuple *was*
> changed --- in index-relevant columns, even?  Maybe I'm still not
> understanding exactly what condition we're detecting.

Not necessarily. For one thing you might need to do a non-HOT update
purely because there isn't enough free space to fit the successor
version on the original heap page. In general, even a 100% HOT-safe
UPDATE statement might not be able to perform HOT updates.

As I said earlier, the way that we deal with partial indexes doesn't
really make too much sense if you try to shoehorn it into an abstract
definition. It makes a lot more sense when seen in the context of a
workload with a partial index, and shown by a test case from Marko
Tiikkaja:


https://www.postgresql.org/message-id/flat/CAL9smLC%3DSxYiN7yZ4HDyk0RnZyXoP2vaHD-Vg1JskOEHyhMXug%40mail.gmail.com#e79eca5922789de828314e296fdcb82d

I freely admit that this general approach is non-modular, even ugly.

--
Peter Geoghegan

On Tue, Oct 24, 2023 at 5:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > So the proximate problem is that we're trying to fetch an updatedCols
> > bitmapset for a child table that has no permissions checks to make
> > and thus doesn't need an RTEPermissionInfo, so its perminfoindex is
> > zero.  It might be that this relinfo should have ri_RootResultRelInfo
> > set, but it doesn't.
>
> That seems to be a fairly good guess.  I was able to reduce the
> test case to this:
>
> drop table if exists t1 cascade;
>
> create table t1 (f1 int, f2 int, f3 int
>   , check (f1 < 10) no inherit
> );
>
> create table t2 () inherits(t1);
>
> insert into t2 select i, i+1, 0 from generate_series(1,1000) i;
>
> create index on t2(f1,f2);
>
> update t1 set f3 = 11 where f1 = 12 and f2 = 13;
>
>
> You don't see the failure without the check constraint on t1.
> What is happening is that with that, we remove t1 from the plan
> entirely, causing us to think that t2 is the root target relation,
> and then things blow up because the root should have an
> RTEPermissionInfo and doesn't.  The place where things start
> to go off the rails is in ExecInitModifyTable:
>
>          * If it's a partitioned table, the root partition doesn't appear
>          * elsewhere in the plan and its RT index is given explicitly in
>          * node->rootRelation.  Otherwise (i.e. table inheritance) the target
>          * relation is the first relation in the node->resultRelations list.
>
> node->resultRelations contains only t2, so boom.
>
> Maybe we could fix this by setting ModifyTable.rootRelation to
> the parent relation in the plain-inheritance case not only the
> partitioned case; but I've not investigated what else might be
> expecting it to be set only for partitions.

That appears to be the easiest fix to me (attached PoC makes the error
go away for me and passes check-world).

By setting rootRelation for plan-inheritance roots, there would be two
ResultRelInfos for the root relation (that is, if it's also present in
resultRelations, unlike in this case) -- one in
ModifyTableState.rootResultRelInfo and another in the 1st element of
ModifyTableState.resultRelInfo[].  That might be fine, because,
AFAICS, the partitioning-related code sites that look at the former
have checks to ensure that they don't accidentally try to access a
plain-inheritance relation as a partitioned one.  I might need to take
a closer look.

> According to this understanding, the root cause is quite old
> and the RTEPermissionInfo failure just exposes it in an obvious
> way.  I wonder whether we can find related cases that misbehave
> even before v16.

I decided to look at fire{BS|AS}Triggers() first, because we changed
them in f49b85d783f to look at ModifyTableState.rootResultRelInfo to
get the table whose triggers to fire.  They seem broken for this case:

create or replace function t1_stmt_trig_func() returns trigger as $$
begin raise notice 'updating t1'; return NULL; end; $$ language
plpgsql;
create trigger t1_stmt_trig before update on t1 execute function
t1_stmt_trig_func();

-- trigger should've been fired
update t1 set f3 = 11 where f1 = 12 and f2 = 13;
ERROR:  invalid perminfoindex 0 in RTE with relid 32799

-- partitioned roots seem fine
create table p (a int check (a = 0)) partition by list (a);
create table p1 partition of p default;
create trigger p_stmt_trig before update on p execute function
t1_stmt_trig_func();
update p set a = 1 where a = 1;
NOTICE:  updating t1
UPDATE 0

I recalled we fixed a similar case in ab5fcf2b04f.  I suspect we would
not have seen this report if we had also considered the
excluded-root-parent case in that commit, though I might be wrong.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Amit Langote <amitlangote09@gmail.com> writes:
> On Tue, Oct 24, 2023 at 5:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Maybe we could fix this by setting ModifyTable.rootRelation to
>> the parent relation in the plain-inheritance case not only the
>> partitioned case; but I've not investigated what else might be
>> expecting it to be set only for partitions.

> That appears to be the easiest fix to me (attached PoC makes the error
> go away for me and passes check-world).

> By setting rootRelation for plan-inheritance roots, there would be two
> ResultRelInfos for the root relation (that is, if it's also present in
> resultRelations, unlike in this case) -- one in
> ModifyTableState.rootResultRelInfo and another in the 1st element of
> ModifyTableState.resultRelInfo[].  That might be fine, because,
> AFAICS, the partitioning-related code sites that look at the former
> have checks to ensure that they don't accidentally try to access a
> plain-inheritance relation as a partitioned one.  I might need to take
> a closer look.

I suggest that it might be cleaner if we make rootRelation point
to the original appendrel (that is, the RTE entry with inh = true).
That would be exactly parallel to the partitioning situation, in
that that RTE is not scanned in the plan.  But it's for the same
table as what's normally the first result table, so it should have
the same permissions info.

BTW, I noted while looking at this example that there are two
RTEPermissionInfo structs with identical contents, one for the
appendrel RTE and one for the first child.  That seems kind of
unnecessary.

> I decided to look at fire{BS|AS}Triggers() first, because we changed
> them in f49b85d783f to look at ModifyTableState.rootResultRelInfo to
> get the table whose triggers to fire.  They seem broken for this case:

Yuck.  But either proposal above would fix that, right?

            regards, tom lane

Hans Buschmann <buschmann@nidsa.net> writes:
> 1st query:                                  ERROR
> 2nd query to or_followup_archiv: Succeeds
> then 1st query repeated:       SUCCEEDS!!

> With this direct update to or_followup_archiv (or an unclustered table) the error disappears.

Yeah.  After more study I think there's less there than meets the
eye.  If the row update manages to be HOT then we don't need to make
any new index entries so we never reach the troublesome code.
In your original reproducer, the CLUSTER step was important because
it left the target tuple in a fully-packed page with no room for a
HOT update on the same page.  When playing around with variants or
even re-executing the same query, it matters how much free space
there is on the page containing the target tuple, and that is
affected by all sorts of seemingly-irrelevant actions.

Anyway, we do now have a simple and reliable reproducer, so we can
work on fixing this.  Thanks again for the report!

            regards, tom lane

I wrote:
> I suggest that it might be cleaner if we make rootRelation point
> to the original appendrel (that is, the RTE entry with inh = true).
> That would be exactly parallel to the partitioning situation, in
> that that RTE is not scanned in the plan.  But it's for the same
> table as what's normally the first result table, so it should have
> the same permissions info.

After looking closer I see that that's exactly what's happening
in your patch, so it should all be good.  We can make the code in
grouping_planner be simpler rather than more complicated, though.

I went ahead and pushed that to v14 and up, with adjustments of
relevant comments and a test case.  There are still loose ends
here, though:

* The wrong-table-for-triggers bug occurs pre-v14, but the patch
doesn't come close to applying because both the planner and
executor look quite a bit different in this area.  We could
devise a separate patch maybe, but given the lack of field
complaints I'm not sure this is worth doing.  I'd be afraid
to put such a patch into v11 at this point anyway, given that
there will be no opportunity to fix any new bugs after November.

* I'm still seeing the extra RTEPermissionsInfo.  It looks like that's
a consequence of this kluge in expand_single_inheritance_child:

    /*
     * No permission checking for the child RTE unless it's the parent
     * relation in its child role, which only applies to traditional
     * inheritance.
     */
    if (childOID != parentOID)
        childrte->perminfoindex = 0;

I suspect that now this should just unconditionally clear
childrte->perminfoindex, but it's minor cleanup not a bug fix
so I didn't pursue that in the initial patch.

* It seems like ModifyTable.nominalRelation and
ModifyTable.rootRelation are pretty darn redundant.  Maybe we
should make an effort to get rid of one of them.  Or maybe
it's not worth the trouble.

            regards, tom lane

On Wed, Oct 25, 2023 at 4:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > I suggest that it might be cleaner if we make rootRelation point
> > to the original appendrel (that is, the RTE entry with inh = true).
> > That would be exactly parallel to the partitioning situation, in
> > that that RTE is not scanned in the plan.  But it's for the same
> > table as what's normally the first result table, so it should have
> > the same permissions info.
>
> After looking closer I see that that's exactly what's happening
> in your patch, so it should all be good.  We can make the code in
> grouping_planner be simpler rather than more complicated, though.
>
> I went ahead and pushed that to v14 and up, with adjustments of
> relevant comments and a test case.

Thanks.

>  There are still loose ends
> here, though:
>
> * The wrong-table-for-triggers bug occurs pre-v14, but the patch
> doesn't come close to applying because both the planner and
> executor look quite a bit different in this area.  We could
> devise a separate patch maybe, but given the lack of field
> complaints I'm not sure this is worth doing.  I'd be afraid
> to put such a patch into v11 at this point anyway, given that
> there will be no opportunity to fix any new bugs after November.

Yeah, it might not be worthwhile to try to back-patch this to 12 and 13.

> * I'm still seeing the extra RTEPermissionsInfo.  It looks like that's
> a consequence of this kluge in expand_single_inheritance_child:
>
>     /*
>      * No permission checking for the child RTE unless it's the parent
>      * relation in its child role, which only applies to traditional
>      * inheritance.
>      */
>     if (childOID != parentOID)
>         childrte->perminfoindex = 0;
>
> I suspect that now this should just unconditionally clear
> childrte->perminfoindex, but it's minor cleanup not a bug fix
> so I didn't pursue that in the initial patch.

Would you like me to apply something like the attached?

> * It seems like ModifyTable.nominalRelation and
> ModifyTable.rootRelation are pretty darn redundant.  Maybe we
> should make an effort to get rid of one of them.  Or maybe
> it's not worth the trouble.

We had a discussion on unifying the two before:
https://www.postgresql.org/message-id/12148.1538938507%40sss.pgh.pa.us

I'm fine with leaving that as-is.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Amit Langote <amitlangote09@gmail.com> writes:
> On Wed, Oct 25, 2023 at 4:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I suspect that now this should just unconditionally clear
>> childrte->perminfoindex, but it's minor cleanup not a bug fix
>> so I didn't pursue that in the initial patch.

> Would you like me to apply something like the attached?

Diff looks fine, but I'm not sure that it's appropriate to characterize
the existing code as an oversight.  It was a necessary hack while the
executor was behaving as it did (ie, using the first child as root).

>> * It seems like ModifyTable.nominalRelation and
>> ModifyTable.rootRelation are pretty darn redundant.  Maybe we
>> should make an effort to get rid of one of them.  Or maybe
>> it's not worth the trouble.

> We had a discussion on unifying the two before:
> https://www.postgresql.org/message-id/12148.1538938507%40sss.pgh.pa.us

Ah, so we did.  The "serve different masters" argument did re-occur
to me while I was looking at this yesterday, but I'm not sure how
strong it is really.  Anyway, I'm also content to leave it be.

            regards, tom lane

On Wed, Oct 25, 2023 at 10:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Wed, Oct 25, 2023 at 4:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I suspect that now this should just unconditionally clear
> >> childrte->perminfoindex, but it's minor cleanup not a bug fix
> >> so I didn't pursue that in the initial patch.
>
> > Would you like me to apply something like the attached?
>
> Diff looks fine, but I'm not sure that it's appropriate to characterize
> the existing code as an oversight.  It was a necessary hack while the
> executor was behaving as it did (ie, using the first child as root).

Ah, you're right.  I've updated the commit message.  Will push later today.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Amit Langote <amitlangote09@gmail.com> writes:
> On Wed, Oct 25, 2023 at 10:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Diff looks fine, but I'm not sure that it's appropriate to characterize
>> the existing code as an oversight.  It was a necessary hack while the
>> executor was behaving as it did (ie, using the first child as root).

> Ah, you're right.  I've updated the commit message.  Will push later today.

I'd suggest specifying *first* child RTE in the commit message.
LGTM otherwise.

            regards, tom lane

On Thu, Oct 26, 2023 at 11:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Wed, Oct 25, 2023 at 10:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Diff looks fine, but I'm not sure that it's appropriate to characterize
> >> the existing code as an oversight.  It was a necessary hack while the
> >> executor was behaving as it did (ie, using the first child as root).
>
> > Ah, you're right.  I've updated the commit message.  Will push later today.
>
> I'd suggest specifying *first* child RTE in the commit message.
> LGTM otherwise.

Thanks for checking.  Pushed after changing the commit message like that.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com