Re: patch: Use pg_assume in jsonb_util.c to fix GCC 15 warnings

Dmitry Mityugov <d.mityugov@postgrespro.ru> writes:
> When compiled with Assert() macro disabled, GCC 15 produces warnings
> about possibly uninitialized variables in
> src/backend/utils/adt/jsonb_util.c module. This problem was discussed in
> detail in this thread, in April 2025:
> https://www.postgresql.org/message-id/988bf1bc-3f1f-99f3-bf98-222f1cd9dc5e@xs4all.nl
> .

> Recently introduced pg_assume() macro let fix such problems easily. The
> attached patch fixes them in jsonb_util.c module. I verified that
> PostgreSQL compiles clearly with this patch and GCC 15.1.1 on an x86
> 64-bit machine (with and without --enable-cassert), and with GCC 14.2.1
> on a 64-bit ARM machine. `make check` also passes.

I don't care for this patch: replacing an Assert with pg_assume just
seems like a very bad idea.  If the assertion condition ever failed,
which doesn't seem all that impossible in logic as complicated as
this, then instead of an identifiable assertion trap we'd get
unspecified and impossible-to-debug behavior.

We could conceivably do

+#ifdef USE_ASSERT_CHECKING
            Assert(ra != WJB_END_ARRAY && ra != WJB_END_OBJECT);
            Assert(rb != WJB_END_ARRAY && rb != WJB_END_OBJECT);
+#else
+           pg_assume(ra != WJB_END_ARRAY && ra != WJB_END_OBJECT);
+           pg_assume(rb != WJB_END_ARRAY && rb != WJB_END_OBJECT);
+#endif

but that seems ugly.  In any case, it's just doubling down on the
assumption that a compiler capable of detecting the "may be used
uninitialized" issue will conclude that rejecting WJB_END_ARRAY
and WJB_END_OBJECT (but not WJB_DONE) eliminates the possibility of
va.type/vb.type not being set.

What I think we should do about this is what I mentioned in the
other thread: adjust the code to guarantee that va.type/vb.type
are defined in all cases.  There are a couple of ways we could
do that, but after reflection I think the best way is to modify
JsonbIteratorNext to make that guarantee.  I've checked that
the attached silences the warning on gcc 15.1.1 (current
Fedora 42).

            regards, tom lane

diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index c8b6c15e059..136952861e1 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -277,9 +277,6 @@ compareJsonbContainers(JsonbContainer *a, JsonbContainer *b)
         else
         {
             /*
-             * It's safe to assume that the types differed, and that the va
-             * and vb values passed were set.
-             *
              * If the two values were of the same container type, then there'd
              * have been a chance to observe the variation in the number of
              * elements/pairs (when processing WJB_BEGIN_OBJECT, say). They're
@@ -852,15 +849,20 @@ JsonbIteratorInit(JsonbContainer *container)
  * It is our job to expand the jbvBinary representation without bothering them
  * with it.  However, clients should not take it upon themselves to touch array
  * or Object element/pair buffers, since their element/pair pointers are
- * garbage.  Also, *val will not be set when returning WJB_END_ARRAY or
- * WJB_END_OBJECT, on the assumption that it's only useful to access values
- * when recursing in.
+ * garbage.
+ *
+ * *val is not meaningful when the result is WJB_DONE, WJB_END_ARRAY or
+ * WJB_END_OBJECT.  However, we set val->type = jbvNull in those cases,
+ * so that callers may assume that val->type is always well-defined.
  */
 JsonbIteratorToken
 JsonbIteratorNext(JsonbIterator **it, JsonbValue *val, bool skipNested)
 {
     if (*it == NULL)
+    {
+        val->type = jbvNull;
         return WJB_DONE;
+    }

     /*
      * When stepping into a nested container, we jump back here to start
@@ -898,6 +900,7 @@ recurse:
                  * nesting).
                  */
                 *it = freeAndGetParent(*it);
+                val->type = jbvNull;
                 return WJB_END_ARRAY;
             }

@@ -951,6 +954,7 @@ recurse:
                  * of nesting).
                  */
                 *it = freeAndGetParent(*it);
+                val->type = jbvNull;
                 return WJB_END_OBJECT;
             }
             else
@@ -995,8 +999,10 @@ recurse:
                 return WJB_VALUE;
     }

-    elog(ERROR, "invalid iterator state");
-    return -1;
+    elog(ERROR, "invalid jsonb iterator state");
+    /* satisfy compilers that don't know that elog(ERROR) doesn't return */
+    val->type = jbvNull;
+    return WJB_DONE;
 }

 /*