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