On 05.12.2025 16:33, Peter Eisentraut wrote: > There are many PG_GETARG_* calls, mostly around gin, gist, spgist code, > that are commented out, presumably to indicate that the argument is > unused and to indicate that it wasn't forgotten or miscounted. Example: > > ... > StrategyNumber strategy = (StrategyNumber) PG_GETARG_UINT16(2); > > /* Oid subtype = PG_GETARG_OID(3); */ > bool *recheck = (bool *) PG_GETARG_POINTER(4); > ... > > But keeping commented-out code updated with refactorings and style > changes is annoying. (Also note that pgindent forces the blank line.) > > One way to address this is to de-comment that code but instead mark the > variables unused. That way the compiler can check the code, and the > purpose is clear to a reader. Example: > > pg_attribute_unused() Oid subtype = PG_GETARG_OID(3); > > (This is the correct placement of the attribute under forward-looking > C23 alignment.) > > I have attached a patch for that.
But that doesn't guarantee that the code is actually optimized away. The compiler might keep, for example, PG_GETARG_*() code that uses PG_DETOAST_DATUM_*(), if it cannot to prove that the code is side effect free. Did you check if the compiler actually removes all of the code marked pg_attribute_unused(), especially e.g. the call to PG_GETARG_TEXT_PP()? How do we avoid regressing when some of the PG_GETARG_*() code is changed? If we cannot be sure the compiler will actually remove the code, we could provide PG_GETARG_*_UNUSED() macros that truly won't do anything. > > An alternative is to just delete that code. (No patch attached, but you > can imagine it.) > Some particular curious things to check in the patch: > > - In contrib/hstore/hstore_gin.c, if I activate the commented out code, > it causes test failures in the hstore test. So the commented out code > is somehow wrong, which seems bad. Also, maybe there is more wrong code > like that, but which doesn't trigger test failures right now? > > - In src/backend/utils/adt/jsonfuncs.c, those calls, if activated, are > happening before null checks, so they are not correct. Also, the "in" > variable is shadowed later. So here, deleting the incorrect code is > probably the best solution in any case. +1 > > - In doc/src/sgml/gist.sgml, this is the source of the pattern, it > actually documents that you should write your functions with the > commented out code. We should think about an alternative way to > document this. I don't see the "subtype" argument documented anywhere > in the vicinity of this, so I don't know what the best advice would be. > Just silently skipping an argument number might be confusing here. -- David Geier
