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
