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
