Обсуждение: New compiler warning

I am seeing a new gcc 12.2.0 compiler warning from
src/backend/commands/sequence.c:

    sequence.c: In function ‘DefineSequence’:
    sequence.c:196:35: warning: ‘coldef’ may be used uninitialized [-Wmaybe-uninitialized]
      196 |                 stmt->tableElts = lappend(stmt->tableElts, coldef);
          |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    sequence.c:175:29: note: ‘coldef’ was declared here
      175 |                 ColumnDef  *coldef;
          |                             ^~~~~~

The code is:

    for (i = SEQ_COL_FIRSTCOL; i <= SEQ_COL_LASTCOL; i++)
    {
-->        ColumnDef  *coldef;

        switch (i)
        {
        case SEQ_COL_LASTVAL:
            coldef = makeColumnDef("last_value", INT8OID, -1, InvalidOid);
            value[i - 1] = Int64GetDatumFast(seqdataform.last_value);
            break;
        case SEQ_COL_LOG:
            coldef = makeColumnDef("log_cnt", INT8OID, -1, InvalidOid);
            value[i - 1] = Int64GetDatum((int64) 0);
            break;
        case SEQ_COL_CALLED:
            coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
            value[i - 1] = BoolGetDatum(false);
            break;
        }

        coldef->is_not_null = true;
        null[i - 1] = false;

-->        stmt->tableElts = lappend(stmt->tableElts, coldef);
    }

and I think it is caused by this commit:

    commit 1fa9241bdd
    Author: Peter Eisentraut <peter@eisentraut.org>
    Date:   Tue Aug 29 08:41:04 2023 +0200
    
        Make more use of makeColumnDef()
    
        Since we already have it, we might as well make full use of it,
        instead of assembling ColumnDef by hand in several places.
    
        Reviewed-by: Alvaro Herrera <alvherre@alvh.no-ip.org>
        Discussion: https://www.postgresql.org/message-id/flat/52a125e4-ff9a-95f5-9f61-b87cf447e4da@eisentraut.org

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

Hi,

> I am seeing a new gcc 12.2.0 compiler warning from
> src/backend/commands/sequence.c:

Yep, the compiler is just not smart enough to derive that this
actually is not going to happen.

Here is a proposed fix.

-- 
Best regards,
Aleksander Alekseev

On 8/30/23 08:10, Aleksander Alekseev wrote:
> 
>> I am seeing a new gcc 12.2.0 compiler warning from
>> src/backend/commands/sequence.c:
> 
> Yep, the compiler is just not smart enough to derive that this
> actually is not going to happen.
> 
> Here is a proposed fix.

Here's an alternate way to deal with this which is a bit more efficient 
(code not tested):

-        case SEQ_COL_CALLED:
-            coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
-            value[i - 1] = BoolGetDatum(false);
-            break;
+        default:
+                   Assert(i == SEQ_COL_CALLED);
+            coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
+            value[i - 1] = BoolGetDatum(false);
+            break;

The downside is that any garbage in i will lead to processing as 
SEQ_COL_CALLED. But things are already pretty bad in that case, ISTM, 
even with the proposed patch (or the original code for that matter).

Regards,
-David

Peter Eisentraut has applied a patch to fix this.

---------------------------------------------------------------------------

On Wed, Aug 30, 2023 at 10:07:24AM -0400, David Steele wrote:
> On 8/30/23 08:10, Aleksander Alekseev wrote:
> > 
> > > I am seeing a new gcc 12.2.0 compiler warning from
> > > src/backend/commands/sequence.c:
> > 
> > Yep, the compiler is just not smart enough to derive that this
> > actually is not going to happen.
> > 
> > Here is a proposed fix.
> 
> Here's an alternate way to deal with this which is a bit more efficient
> (code not tested):
> 
> -        case SEQ_COL_CALLED:
> -            coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
> -            value[i - 1] = BoolGetDatum(false);
> -            break;
> +        default:
> +                   Assert(i == SEQ_COL_CALLED);
> +            coldef = makeColumnDef("is_called", BOOLOID, -1, InvalidOid);
> +            value[i - 1] = BoolGetDatum(false);
> +            break;
> 
> The downside is that any garbage in i will lead to processing as
> SEQ_COL_CALLED. But things are already pretty bad in that case, ISTM, even
> with the proposed patch (or the original code for that matter).
> 
> Regards,
> -David

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.