Обсуждение: Obsolete comments in ResultRelInfo struct

Hi,

While working on commit 62a1211d3, I noticed $SUBJECT:

    /*
     * Information needed by tuple routing target relations
     *
     * RootResultRelInfo gives the target relation mentioned in the query, if
     * it's a partitioned table. It is not set if the target relation
     * mentioned in the query is an inherited table, nor when tuple routing is
     * not needed.
     *
     * PartitionTupleSlot is non-NULL if RootToChild conversion is needed and
     * the relation is a partition.
     */
    struct ResultRelInfo *ri_RootResultRelInfo;
    TupleTableSlot *ri_PartitionTupleSlot;

I think the comment about ri_RootResultRelInfo is correct for pre-14
versions, but not for later versions because it is set also when the
target relation is an inherited table (see ExecInitModifyTable()), in
which case it is used for transition capture, not for tuple routing.
So I would like to propose to update that comment like this:

    /*
     * Other information needed by child result relations
     *
     * RootResultRelInfo gives the target relation mentioned in the query.
     * Used as the root for tuple routing and/or transition capture.
     *
     * PartitionTupleSlot is non-NULL if the relation is a partition to route
     * tuples into and RootToChild conversion is needed.
     */

I slightly modified the top and bottom comments as well.  (In the top
comment, I added "Other" because we have the definitions of members
such as ri_ChildToRootMap and ri_RootToChildMap above.)

Comments welcome!

Best regards,
Etsuro Fujita

On Mon, 11 Aug 2025 at 16:25, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> Hi,
>
> While working on commit 62a1211d3, I noticed $SUBJECT:
>
>     /*
>      * Information needed by tuple routing target relations
>      *
>      * RootResultRelInfo gives the target relation mentioned in the query, if
>      * it's a partitioned table. It is not set if the target relation
>      * mentioned in the query is an inherited table, nor when tuple routing is
>      * not needed.
>      *
>      * PartitionTupleSlot is non-NULL if RootToChild conversion is needed and
>      * the relation is a partition.
>      */
>     struct ResultRelInfo *ri_RootResultRelInfo;
>     TupleTableSlot *ri_PartitionTupleSlot;
>
> I think the comment about ri_RootResultRelInfo is correct for pre-14
> versions, but not for later versions because it is set also when the
> target relation is an inherited table (see ExecInitModifyTable()), in
> which case it is used for transition capture, not for tuple routing.
> So I would like to propose to update that comment like this:
>
>     /*
>      * Other information needed by child result relations
>      *
>      * RootResultRelInfo gives the target relation mentioned in the query.
>      * Used as the root for tuple routing and/or transition capture.
>      *
>      * PartitionTupleSlot is non-NULL if the relation is a partition to route
>      * tuples into and RootToChild conversion is needed.
>      */
>
> I slightly modified the top and bottom comments as well.  (In the top
> comment, I added "Other" because we have the definitions of members
> such as ri_ChildToRootMap and ri_RootToChildMap above.)
>
> Comments welcome!
>
> Best regards,
> Etsuro Fujita
>
>

Hi! Looks like you forgot to actually attach a patch file?

-- 
Best regards,
Kirill Reshke

On Mon, 11 Aug 2025 at 12:25, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
>     /*
>      * Other information needed by child result relations
>      *
>      * RootResultRelInfo gives the target relation mentioned in the query.
>      * Used as the root for tuple routing and/or transition capture.
>      *
>      * PartitionTupleSlot is non-NULL if the relation is a partition to route
>      * tuples into and RootToChild conversion is needed.
>      */

That seems reasonable. I think it's also worth adding the "ri_" prefix
to the field names in those comments.

Regards,
Dean

Hi,

On Tue, Aug 12, 2025 at 2:03 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Mon, 11 Aug 2025 at 12:25, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> >
> >     /*
> >      * Other information needed by child result relations
> >      *
> >      * RootResultRelInfo gives the target relation mentioned in the query.
> >      * Used as the root for tuple routing and/or transition capture.
> >      *
> >      * PartitionTupleSlot is non-NULL if the relation is a partition to route
> >      * tuples into and RootToChild conversion is needed.
> >      */
>
> That seems reasonable.

Cool!

> I think it's also worth adding the "ri_" prefix
> to the field names in those comments.

+1, so I created a patch incorporating your proposal, which I am attaching.

Thanks for the comment!

Best regards,
Etsuro Fujita

Hi,

On Mon, Aug 11, 2025 at 8:53 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
> Hi! Looks like you forgot to actually attach a patch file?

I didn't attach a patch intentionally because this is a
documentation-only change, but you can find a patch for the updated
version of this in [1].

Thanks!

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/CAPmGK140581qQRWA97rb%2BmW7orTBJ%2BDMgw5rzS%3DqgdSCkoX6XA%40mail.gmail.com

On Tue, Aug 12, 2025 at 7:21 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Tue, Aug 12, 2025 at 2:03 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> > On Mon, 11 Aug 2025 at 12:25, Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > >
> > >     /*
> > >      * Other information needed by child result relations
> > >      *
> > >      * RootResultRelInfo gives the target relation mentioned in the query.
> > >      * Used as the root for tuple routing and/or transition capture.
> > >      *
> > >      * PartitionTupleSlot is non-NULL if the relation is a partition to route
> > >      * tuples into and RootToChild conversion is needed.
> > >      */
> >
> > That seems reasonable.
>
> Cool!
>
> > I think it's also worth adding the "ri_" prefix
> > to the field names in those comments.
>
> +1, so I created a patch incorporating your proposal, which I am attaching.

Pushed and backpatched down to v14.

Best regards,
Etsuro Fujita