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

Hi,

I took a look at the patch series, mostly because passing flags to
beginscan would be useful for the explain patches in the index prefetch
patch series. Right now there's no convenient way for the executor to
tell the TAM to collect instrumentation data (and so it's collected
always, but it'd be nice to make that optional).

And I think from this point of view it's fine. I have only a couple
rather superficial comments:

0001

- no comments

0002

- Don't we usually keep "flags" as the last parameter? It seems a bit
weird that it's added in between relation and snapshot.

- Do we really want to pass two sets of flags to table_beginscan_common?
I realize it's done to ensure "users" don't use internal flags, but
then maybe it'd be better to do that check in the places calling the
_common? Someone adding a new caller can break this in various ways
anyway, e.g. by setting bits in the internal flags, no?

If we want to have these checks, should we be more thorough? Should we
check the internal flags only set internal flags?

0003

- Half the "beginscan" calls use a ternary operator directly, half sets
a variable first (and then uses that). Often mixed in the same file.
Shouldn't it be a bit consistent?

0004, 0005

- no comment

--
Tomas Vondra