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
Attachments:
update-comments-in-ResultRelInfo-struct.patchapplication/octet-stream; name=update-comments-in-ResultRelInfo-struct.patchDownload
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index e107d6e5f81..de782014b2d 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -609,15 +609,13 @@ typedef struct ResultRelInfo
bool ri_RootToChildMapValid;
/*
- * Information needed by tuple routing target relations
+ * Other information needed by child result 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.
+ * ri_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 RootToChild conversion is needed and
- * the relation is a partition.
+ * ri_PartitionTupleSlot is non-NULL if the relation is a partition to
+ * route tuples into and ri_RootToChildMap conversion is needed.
*/
struct ResultRelInfo *ri_RootResultRelInfo;
TupleTableSlot *ri_PartitionTupleSlot;
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]/messages/by-id/CAPmGK140581qQRWA97rb+mW7orTBJ+DMgw5rzS=qgdSCkoX6XA@mail.gmail.com.
Thanks!
Best regards,
Etsuro Fujita
[1]: /messages/by-id/CAPmGK140581qQRWA97rb+mW7orTBJ+DMgw5rzS=qgdSCkoX6XA@mail.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