Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)

On Thu, Mar 26, 2026 at 7:10 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> I was looking at v46-0001. With:

Thanks for taking a look!

> +++ b/src/include/nodes/plannodes.h
> + 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:
>
> 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().

Yea, outside of the insert into leaf partitions case, I thought of
another, perhaps even more compelling reason the combined field might
be a bit confusing:
Take a table t1 and a table t2. If you do
SELECT * FROM t1 JOIN t2 ON t1.id = t2.id FOR UPDATE of t1;
t1 will get a ROW_MARK_EXCLUSIVE and t2 would get a ROW_MARK_REFERENCE
(that's just how preprocess_rowmarks() works).
That means modifiedRelids would contain t2, even though t2 is not
being locked for update. For the purposes of setting the VM, it's
totally fine that we are more conservative than we need to be and
don't consider setting it when scanning t2. But for the purposes of
modifiedRelids, it's a bit confusing that t2 is in there.

But we can't just exclude ROW_MARK_REFERENCE from modifiedRelids
because we rely on ROW_MARK_REFERENCE to avoid setting the VM for a
table we are updating or deleting from when it is mentioned more than
once in the query (e.g. UPDATE foo SET x = 1 FROM foo f2 WHERE foo.id
= f2.id).

So, for that reason and because of the missing leaf partitions for
inserts, I think making quick reference bitmapsets would be better.
I've done this in attached v47.

I've also removed the asserts in ExecInsert/Update/Delete because they
are a bit tautological now.

My one remaining question is whether the two new bitmapsets
(rowMarkRelids and resultRelationRelids) should move from the
PlannedStmt to the EState. They are determined at plan time and never
modified during execution. However, I do notice there are other EState
members that seem like just a copy of info from the PlannedStmt that
isn't modified during execution (e.g. es_rteperminfos/permInfos).
However, putting them in the EState increases the work required to get
them to parallel workers and to the child estate for EPQs. I would
prefer to keep it in the PlannedStmt but am worried that breaks
convention.

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

Yea, I like this and think it makes sense. Done in v47.

> 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?

That makes sense to me. Done in v47.

- Melanie