On 05/12/2025 17: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.
An alternative is to just delete that code. (No patch attached, but you
can imagine it.)
#if 0
Oid subtype = PG_GETARG_OID(3);
#endif
is yet another option. It keeps the indentation, although you won't get
the compiler checking.
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?
I'm guessing that the commented out code detoasts the arguments.
- 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.
Wow, that jsonb_set_lax() function is difficult to follow. Especially
the "jsonb_delete_path(fcinfo)" call seems pretty accidental to work,
because jsonb_delete_path() just happens to have the same two arguments.
- 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.
Hmm, yeah, the right thing to do would be to actually document the
'subtype'. I don't remember what it is off the top of my head.
- Heikki
