Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Mark commented out code as unused

  2. Remove incorrect commented out code

  1. commented out code

    Peter Eisentraut <peter@eisentraut.org> — 2025-12-05T15:33:22Z

    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.)

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.

- 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.

Thoughts?

  2. Re: commented out code

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-05T16:38:03Z

    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

  3. Re: commented out code

    David Geier <geidav.pg@gmail.com> — 2025-12-07T19:10:24Z

    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

  4. Re: commented out code

    Chao Li <li.evan.chao@gmail.com> — 2025-12-09T08:43:25Z

    
> On Dec 5, 2025, at 23:33, Peter Eisentraut <peter@eisentraut.org> 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.)
> 
> 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.
> 
> - 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.
> 
> Thoughts?
> <0001-Mark-commented-out-code-as-unused.patch>


Looking at the definition of pg_attribute_unused:
```
/* only GCC supports the unused attribute */
#ifdef __GNUC__
#define pg_attribute_unused() __attribute__((unused))
#else
#define pg_attribute_unused()
#endif
```

Only GCC really supports it. Even with gcc, based on my understanding, for example:

+	pg_attribute_unused() text *query = PG_GETARG_TEXT_PP(2);

The assignment to “query” will still happen, “__attribute__((unused))" only hides “unused variable” compile warning. So this patch is not a pure refactoring but having some runtime impacts. From this perspective, I am actually keen on #if 0 as Heikki suggested. If we go along with #if 0, then the 3 curious issues would not happen.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/