On Thu, 26 Mar 2026 at 12:14, Melanie Plageman
<[email protected]> wrote:
> Attached v46 addresses your feedback and has a bit of assorted cleanup in it.
(I've not had a chance to process this thread, so apologies if I
missed discussion on certain things I'm going to say)
I was looking at v46-0001. With:
+++ b/src/include/nodes/plannodes.h
@@ -112,6 +112,12 @@ typedef struct PlannedStmt
*/
Bitmapset *unprunableRelids;
+ /*
+ * RT indexes of relations modified by the query through
+ * UPDATE/DELETE/INSERT/MERGE or targeted by SELECT FOR UPDATE/SHARE.
+ */
+ Bitmapset *modifiedRelids;
+
This doesn't really mention anything about leaf partitions not being
mentioned for INSERT queries. You did mention it in standard_planner()
here:
+ * modification. Conversely, leaf partitions whose result relations are
+ * created at the time of insert are not included here.
I think if someone is going to use this field, they're going to look
at where the field is defined to find out what it is, not where it
gets populated.
I'm also wondering about having this combined field. If you were to
have a Bitmapset field that mirrors "List *resultRelations;", then
have another:
/* a list of PlanRowMark's */
List *rowMarks;
+ /* Relids which have rowMarks */
+ Bitmapset *rowMarkRelids;
I think they're more likely to be useful for other purposes, and I
think the only pain that it causes you is that you have to call
bms_is_member() twice in ScanRelIsReadOnly().
Then, as a follow-up, maybe we could consider removing
PlannedStmt.resultRelations. (The deprecated)
ExecRelationIsTargetRelation() could use the new Bitmapset, which
would be more efficient. OverExplain does do:
if (es->format != EXPLAIN_FORMAT_TEXT ||
plannedstmt->resultRelations != NIL)
overexplain_intlist("Result RTIs", plannedstmt->resultRelations, es);
but maybe Robert is ok with those coming out in ascending numerical
order rather than list order. overexplain_bitmapset() would do that.
In [1], I didn't see any code actually using the field. Just a couple
of projects that have duplicated the copyObject() code.
I did quickly look over the remaining patches. I wondered if you might
want to add a new ScanOption SO_NONE = 0, or SO_EMPTY_FLAGS. It might
make the places where you're passing zero directly easier to read?
David
[1] https://codesearch.debian.net/search?q=resultRelations&literal=1
