Re: Convert node test compile-time settings into run-time parameters

Hi,

On 2024-05-20 09:28:39 +0200, Peter Eisentraut wrote:
> - Performance? Looking for example at pg_parse_query() and its siblings,
> they also check for other debugging settings like log_parser_stats in the
> main code path, so it doesn't seem to be a concern.

I don't think we can conclude that. Just because we've not been that careful
about performance in a few spots doesn't mean we shouldn't be careful in other
areas. And I think something like log_parser_stats is a lot more generally
useful than debug_copy_parse_plan_trees.

The branch itself isn't necessarily the issue, the branch predictor can handle
that to a good degree. The reduction in code density is a bigger concern - and
also very hard to measure, because the cost is very incremental and
distributed.

At the very least I'd add unlikely() to all of the branches, so the debug code
can be placed separately from the "normal" portions.

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.

> - Access control? I have these settings as PGC_USERSET for now. Maybe they
> should be PGC_SUSET?

That probably would be right.

> Another thought: Do we really need three separate settings?

Maybe not three settings, but a single setting, with multiple values, like
debug_io_direct?

Greetings,

Andres Freund