Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Jan-09, Fabien COELHO wrote:
>> - if (unlikely(__pg_log_level <= PG_LOG_DEBUG))
>> + if (pg_log_debug_level)
>> {
> Umm ... I find the original exceedingly ugly, but the new line is
> totally impenetrable.
So, I had not been paying any attention to this thread, but that
snippet is already enough to set off alarm bells.
1. (problem with already-committed code, evidently) The C standard is
quite clear that
-- All identifiers that begin with an underscore and
either an uppercase letter or another underscore are
always reserved for any use.
-- All identifiers that begin with an underscore are
always reserved for use as identifiers with file scope
in both the ordinary and tag name spaces.
"Reserved" in this context appears to mean "reserved for use by
system headers and/or compiler special behaviors".
Declaring our own global variables with double-underscore prefixes is not
just asking for trouble, it's waving a red flag in front of a bull.
2. (problem with proposed patch) I share Alvaro's allergy for replacing
uses of a common variable with a bunch of macros, especially macros that
don't look like macros. That's not reducing the reader's cognitive
burden. I'd even say it's actively misleading the reader, because what
the new code *looks* like it's doing is referencing several independent
global variables. We don't need our code to qualify as an entry for
the Obfuscated C Contest.
The notational confusion could be solved perhaps by writing the macros
with function-like parentheses, but it still doesn't seem like an
improvement. In particular, the whole point here is to have a common
idiom for logging, but I'm unconvinced that every frontend program
should be using unlikely() in this particular way. Maybe it's unlikely
for pgbench's usage that verbose logging would be turned on, but why
should we build in an assumption that that's universally the case?
TBH, my recommendation would be to drop *all* of these likely()
and unlikely() calls. What evidence have you got that those are
meaningfully improving the quality of the generated code? And if
they're buried inside macros, they certainly aren't doing anything
useful in terms of documenting the code.
regards, tom lane
