On 21.05.24 20:48, Andres Freund wrote:
> Where I'd be more concerned about peformance is the added branch in
> READ_LOCATION_FIELD. There are a lot of calls to that, addding runtime
> branches to each, with external function calls inside, is somewhat likely to
> be measurable.
Ok, I have an improved plan. I'm wrapping all the code related to this
in #ifdef DEBUG_NODE_TESTS_ENABLED. This in turn is defined in
assert-enabled builds, or if you define it explicitly, or if you define
one of the legacy individual symbols. That way you get the run-time
settings in a normal development build, but there is no new run-time
overhead. This is the same scheme that we use for debug_discard_caches.
(An argument could be made to enable this code if and only if assertions
are enabled, since these tests are themselves kind of assertions. But I
think having a separate symbol documents the purpose of the various code
sections better.)
>> Another thought: Do we really need three separate settings?
>
> Maybe not three settings, but a single setting, with multiple values, like
> debug_io_direct?
Yeah, good idea. Let's get some more feedback on this before I code up
a complicated list parser.
Another approach might be levels. My testing showed that the overhead
of the copy_parse_plan_trees and raw_expression_coverage_tests flags is
hardly noticeable, but write_read_parse_plan_trees has some noticeable
impact. So you could do 0=off, 1=only the cheap ones, 2=all tests.
In fact, if we could make "only the cheap ones" the default for
assert-enabled builds, then most people won't even need to worry about
this setting: The only way to mess up the write_read_parse_plan_trees is
if you write custom node support, which is rare. But the raw expression
coverage still needs to be maintained by hand, so it's more often
valuable to have it checked automatically.