Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Prevent duplicate RTEPermissionInfo for plain-inheritance parents

  2. Fix problems when a plain-inheritance parent table is excluded.

  3. Doc: indexUnchanged is strictly a hint.

  1. BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    PG Bug reporting form <noreply@postgresql.org> — 2023-10-05T14:01:06Z

    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

  2. Re: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    David G. Johnston <david.g.johnston@gmail.com> — 2023-10-05T15:12:47Z

    On Thu, Oct 5, 2023 at 8:05 AM PG Bug reporting form <noreply@postgresql.org>
wrote:

> 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:
>
> 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.
>
>
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.

David J.

  3. Re: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-05T17:07:06Z

    "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

  4. AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Hans Buschmann <buschmann@nidsa.net> — 2023-10-08T13:16:52Z

    Hello,


For your reference I include a simple dump of a test case database, which executes the queries but does NOT reproduce the error.

This case seems much more complicated then I thought on first view.

The problem arose on the production database after it has been dumped/restore from pg15.4 to pg16.0 and was observed on failing queries from the application.

Many tables in production have an inherited table called xxxtable_archiv, which contains elder data and are not often updated by the application. So the error is seldom.

The normal access is only through the main table like:

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
;

xxxdb-# ;
FEHLER:  invalid perminfoindex 0 in RTE with relid 17034

(relid 17034=or_followup_archiv)

which failed repeatedly on production and a local copy (pg_dump/restore).


When you try to access the xxx_archiv table directly like :

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_archiv 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_archiv.id_of=qp_netto.id_of
and or_followup_archiv.of_season=35
;

this query succeeds and shows the execution plan!

BUT:

Once this query of the archiv table is run (and updated 1 record) the original query through the main table (without archiv) also succeeds!
So when one update is run successfully, the error is not reproducable any more!

This behavior is preserved through pg_dump/pg_restore of the whole databsase for both succes and failure case.

I have no clue what difference in the dump file would trigger this: On comparison of an sql dump only the updated row is moved at the end of the copy, nothing else changed.


Unfortunatly the provided errdb_noerr does not show the error (due to manually creation steps perhaps).

Hans Buschmann

  5. Re: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-08T15:24:38Z

    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 the error.
> 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

  6. AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Hans Buschmann <buschmann@nidsa.net> — 2023-10-23T17:05:03Z

    Hello all,

I finally managed to reproduce the bug on a demo database as shown below.

The file err_demo.sql shows all the steps to reproduce. The table creation is done with errdb_noerr_231008.sql (unchanged from last mail).

The clue is:

- you have an inherited table
- on the archiv part, you have only (one or more) brin indexes
- to reorder the tuples before moving to a new major version (with pg_dumpo/restore) the archiv part is clustered.
 this requires to create a btree index, which is removed immediately after the cluster statement.

when all this has been done, a write access to the archiv-part of the table hierarchy causes the error.

 A direct update access to or_followup_archiv clears the error (as shown in the final comment part of err_demo.sql).

To make the error reappear, you have to redo the 3 recluster steps.

PS: please apply the steps of err_demo.sql manually because it was not tested for full automation.

Hope this enables you to investigate the root cause of the error.

Hans Buschmann
________________________________

  7. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-23T18:30:50Z

    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

  8. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Peter Geoghegan <pg@bowt.ie> — 2023-10-23T18:49:20Z

    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

  9. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-23T18:55:08Z

    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

  10. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Peter Geoghegan <pg@bowt.ie> — 2023-10-23T19:01:37Z

    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

  11. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-23T20:59:25Z

    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

  12. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Peter Geoghegan <pg@bowt.ie> — 2023-10-23T22:15:03Z

    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

  13. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-24T00:28:33Z

    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

  14. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Peter Geoghegan <pg@bowt.ie> — 2023-10-24T00:45:39Z

    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

  15. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-24T01:06:01Z

    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

  16. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Peter Geoghegan <pg@bowt.ie> — 2023-10-24T01:23:51Z

    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

  17. AW: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Hans Buschmann <buschmann@nidsa.net> — 2023-10-24T06:16:13Z

    

________________________________

>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

To clear the error (as it is now on production system) you have to execute the update against or_followup_archiv, directly (bypassing the parent table) as shown in

err_demo=#
err_demo=# explain analyze -- explain analyze verbose -- explain -- select * from ( -- select count(*) from ( -- select length(sel) from (
err_demo-# with qp_netto as (
err_demo(# select
err_demo(# 72812::int                              as id_of        ,
err_demo(# 1.000000::numeric(8,6)                  as fac_to_us    ,
err_demo(# 6.9318647425014148::numeric(8,3)        as prfac_netto_1,
err_demo(# 0.0::numeric(8,3)                       as prfac_netto_2,
err_demo(# 1.000000::numeric(8,6)                  as our_to_us    ,
err_demo(# 6.88795000000000000000::numeric(8,3)    as prour_netto_1,
err_demo(# 0.0::numeric(8,3)                       as prour_netto_2
err_demo(# )
err_demo-# -- select * from qp_netto;
err_demo-# update  or_followup set
err_demo-#  of_pr1_fac_netto=coalesce(prfac_netto_1,0.0)
err_demo-# ,of_pr1_fac_netusd=coalesce(prfac_netto_1*fac_to_us,0.0)
err_demo-# ,of_pr2_fac_netto=coalesce(prfac_netto_2,0.0)
err_demo-# ,of_pr2_fac_netusd=coalesce(prfac_netto_2*fac_to_us,0.0)
err_demo-# ,of_pr1_our_netto=coalesce(prour_netto_1,0.0)
err_demo-# ,of_pr1_our_netusd=coalesce(prour_netto_1*our_to_us,0.0)
err_demo-# ,of_pr2_our_netto=coalesce(prour_netto_2,0.0)
err_demo-# ,of_pr2_our_netusd=coalesce(prour_netto_2*our_to_us,0.0)
err_demo-# from qp_netto
err_demo-# where
err_demo-# or_followup.id_of=qp_netto.id_of
err_demo-# and or_followup.of_season=35
err_demo-# ;
FEHLER:  invalid perminfoindex 0 in RTE with relid 30512
err_demo=# explain analyze -- explain analyze verbose -- explain -- select * from ( -- select count(*) from ( -- select length(sel) from (
err_demo-# with qp_netto as (
err_demo(# select
err_demo(# 72812::int                              as id_of        ,
err_demo(# 1.000000::numeric(8,6)                  as fac_to_us    ,
err_demo(# 6.9318647425014148::numeric(8,3)        as prfac_netto_1,
err_demo(# 0.0::numeric(8,3)                       as prfac_netto_2,
err_demo(# 1.000000::numeric(8,6)                  as our_to_us    ,
err_demo(# 6.88795000000000000000::numeric(8,3)    as prour_netto_1,
err_demo(# 0.0::numeric(8,3)                       as prour_netto_2
err_demo(# )
err_demo-# -- select * from qp_netto;
err_demo-# update  or_followup_archiv set
err_demo-#  of_pr1_fac_netto=coalesce(prfac_netto_1,0.0)
err_demo-# ,of_pr1_fac_netusd=coalesce(prfac_netto_1*fac_to_us,0.0)
err_demo-# ,of_pr2_fac_netto=coalesce(prfac_netto_2,0.0)
err_demo-# ,of_pr2_fac_netusd=coalesce(prfac_netto_2*fac_to_us,0.0)
err_demo-# ,of_pr1_our_netto=coalesce(prour_netto_1,0.0)
err_demo-# ,of_pr1_our_netusd=coalesce(prour_netto_1*our_to_us,0.0)
err_demo-# ,of_pr2_our_netto=coalesce(prour_netto_2,0.0)
err_demo-# ,of_pr2_our_netusd=coalesce(prour_netto_2*our_to_us,0.0)
err_demo-# from qp_netto
err_demo-# where
err_demo-# or_followup_archiv.id_of=qp_netto.id_of
err_demo-# and or_followup_archiv.of_season=35
err_demo-# ;
                                                                      QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------
 Update on or_followup_archiv  (cost=3.00..373.13 rows=0 width=0) (actual time=1.680..1.680 rows=0 loops=1)
   ->  Bitmap Heap Scan on or_followup_archiv  (cost=3.00..373.13 rows=1 width=118) (actual time=0.152..0.161 rows=1 loops=1)
         Recheck Cond: ((of_season = 35) AND (id_of = 72812))
         Rows Removed by Index Recheck: 543
         Heap Blocks: lossy=4
         ->  Bitmap Index Scan on brin_or_followup_archiv_season_id_of  (cost=0.00..3.00 rows=542 width=0) (actual time=0.083..0.083 rows=40 loops=1)
               Index Cond: ((of_season = 35) AND (id_of = 72812))
 Planning Time: 1.456 ms
 Execution Time: 1.733 ms
(9 Zeilen)


err_demo=#
err_demo=# explain analyze -- explain analyze verbose -- explain -- select * from ( -- select count(*) from ( -- select length(sel) from (
err_demo-# with qp_netto as (
err_demo(# select
err_demo(# 72812::int                              as id_of        ,
err_demo(# 1.000000::numeric(8,6)                  as fac_to_us    ,
err_demo(# 6.9318647425014148::numeric(8,3)        as prfac_netto_1,
err_demo(# 0.0::numeric(8,3)                       as prfac_netto_2,
err_demo(# 1.000000::numeric(8,6)                  as our_to_us    ,
err_demo(# 6.88795000000000000000::numeric(8,3)    as prour_netto_1,
err_demo(# 0.0::numeric(8,3)                       as prour_netto_2
err_demo(# )
err_demo-# -- select * from qp_netto;
err_demo-# update  or_followup set
err_demo-#  of_pr1_fac_netto=coalesce(prfac_netto_1,0.0)
err_demo-# ,of_pr1_fac_netusd=coalesce(prfac_netto_1*fac_to_us,0.0)
err_demo-# ,of_pr2_fac_netto=coalesce(prfac_netto_2,0.0)
err_demo-# ,of_pr2_fac_netusd=coalesce(prfac_netto_2*fac_to_us,0.0)
err_demo-# ,of_pr1_our_netto=coalesce(prour_netto_1,0.0)
err_demo-# ,of_pr1_our_netusd=coalesce(prour_netto_1*our_to_us,0.0)
err_demo-# ,of_pr2_our_netto=coalesce(prour_netto_2,0.0)
err_demo-# ,of_pr2_our_netusd=coalesce(prour_netto_2*our_to_us,0.0)
err_demo-# from qp_netto
err_demo-# where
err_demo-# or_followup.id_of=qp_netto.id_of
err_demo-# and or_followup.of_season=35
err_demo-# ;
                                                                         QUERY PLAN
------------------------------------------------------------------------------------------------------------------------------------------------------------
 Update on or_followup  (cost=3.00..373.14 rows=0 width=0) (actual time=0.169..0.169 rows=0 loops=1)
   Update on or_followup_archiv or_followup_1
   ->  Result  (cost=3.00..373.14 rows=1 width=122) (actual time=0.147..0.148 rows=1 loops=1)
         ->  Bitmap Heap Scan on or_followup_archiv or_followup_1  (cost=3.00..373.13 rows=1 width=10) (actual time=0.147..0.147 rows=1 loops=1)
               Recheck Cond: ((of_season = 35) AND (id_of = 72812))
               Rows Removed by Index Recheck: 831
               Heap Blocks: lossy=7
               ->  Bitmap Index Scan on brin_or_followup_archiv_season_id_of  (cost=0.00..3.00 rows=542 width=0) (actual time=0.042..0.042 rows=70 loops=1)
                     Index Cond: ((of_season = 35) AND (id_of = 72812))
 Planning Time: 0.671 ms
 Execution Time: 0.201 ms
(11 Zeilen)


err_demo=#

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.

Hans Buschmann

  18. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    amit <amitlangote09@gmail.com> — 2023-10-24T09:48:06Z

    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

  19. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-24T14:48:52Z

    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

  20. Re: AW: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-24T15:56:17Z

    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

  21. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-24T19:07:44Z

    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

  22. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    amit <amitlangote09@gmail.com> — 2023-10-25T08:16:22Z

    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

  23. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-25T13:51:31Z

    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

  24. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    amit <amitlangote09@gmail.com> — 2023-10-26T02:27:00Z

    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

  25. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-26T02:31:49Z

    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

  26. Re: AW: AW: BUG #18147: ERROR: invalid perminfoindex 0 in RTE with relid xxxxx

    amit <amitlangote09@gmail.com> — 2023-10-26T03:01:59Z

    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