Horton

Thread

Commits

  1. Fix invalid boolean if-test

  1. [BUG?] missing array index may result in a wrong constraint name (pg_dump, bin-upgrade, >=18)

    George Tarasov <george.v.tarasov@gmail.com> — 2026-03-07T00:28:14Z

         Greetings!

I performed static code analysis and got an error: ALWAYS FALSE SUBEXPR!
Let's look at a little snippet (pg_dump.c:10203).

     /*
      * In binary upgrade of inheritance child tables, must have a
      * constraint name that we can UPDATE later; same if there's a
      * comment on the constraint.
      */
     if ((dopt->binary_upgrade &&
          !tbinfo->ispartition &&
          !tbinfo->notnull_islocal) ||
          ^^^^^^^^^^^^^^^^^^^^^^^^            ALWAYS FALSE SUBEXPR!
         !PQgetisnull(res, r, i_notnull_comment))
     {

It seems like index "[j]" is missing for this subexpression to make any 
sense.

"notnull_islocal" is an array of bools.  It is _always_ created before 
this snippet
in function "getTableAttrs()".  So, the value of tbinfo->notnull_islocal 
is _always_ TRUE
(it is a valid memory pointer), and therefore the entire subexpression 
is _always_ FALSE.
And so, in binary upgrade mode this condition may not be evaluated as 
expected
(when there's NO comment on the constraint!).

To be honest, I don't fully understand the overall logic behind these 
preparations
for further processing (in pg_dump), but it seems like "[j]" index is 
definitely missing.

That is how it should be.

      /*
      * In binary upgrade of inheritance child tables, must have a
      * constraint name that we can UPDATE later; same if there's a
      * comment on the constraint.
      */
     if ((dopt->binary_upgrade &&
          !tbinfo->ispartition &&
          !tbinfo->notnull_islocal[j]) ||
                                  ^^^                  MISSING ARRAY INDEX!
         !PQgetisnull(res, r, i_notnull_comment))
     {

Dear, Hackers! Please review.
Is there a really mistake here or am I wrong to raise this issue?

Thanks!
George Tarasov

  2. Re: [BUG?] missing array index may result in a wrong constraint name (pg_dump, bin-upgrade, >=18)

    Álvaro Herrera <alvherre@kurilemu.de> — 2026-03-07T13:32:18Z

    On 2026-Mar-07, George Tarasov wrote:

> I performed static code analysis and got an error: ALWAYS FALSE SUBEXPR!
> Let's look at a little snippet (pg_dump.c:10203).
> 
>     /*
>      * In binary upgrade of inheritance child tables, must have a
>      * constraint name that we can UPDATE later; same if there's a
>      * comment on the constraint.
>      */
>     if ((dopt->binary_upgrade &&
>          !tbinfo->ispartition &&
>          !tbinfo->notnull_islocal) ||
>          ^^^^^^^^^^^^^^^^^^^^^^^^            ALWAYS FALSE SUBEXPR!
>         !PQgetisnull(res, r, i_notnull_comment))
>     {
> 
> It seems like index "[j]" is missing for this subexpression to make any
> sense.

Yeah, that's incorrect.  It had been reported before:
https://postgr.es/m/CAEudQAo7ah=4TDheuEjtb0dsv6bHoK7uBNqv53Tsub2h-xBSJw@mail.gmail.com
However, it's pretty innocuous as far as I can understand.  I pushed the
suggested fix now anyway, with this commit message:

commit eb2c867b0aab5fd6bbea1d8dbfea23d9d6fa1a94
Author:     Álvaro Herrera <alvherre@kurilemu.de> [Álvaro Herrera <alvherre@kurilemu.de>]
AuthorDate: Sat Mar 7 14:28:16 2026 +0100
CommitDate: Sat Mar 7 14:28:16 2026 +0100

    Fix invalid boolean if-test
    
    We were testing the truth value of the array of booleans (which is
    always true) instead of the boolean element specific to the affected
    table column.
    
    This causes a binary-upgrade dump fail to omit the name of a constraint;
    that is, the correct constraint name is always printed, even when it's
    not needed.  The affected case is a binary-upgrade dump of a not-null
    constraint in an inherited column, which must in addition have no
    comment.
    
    Another point is that in order for this to make a difference, the
    constraint must have the default name in the child table.  That is, the
    constraint must have been created _in the parent table_ with the name
    that it would have in the child table, like so:
      CREATE TABLE parent (a int CONSTRAINT child_a_not_null NOT NULL);
      CREATE TABLE child () INHERITS (parent);
    Otherwise, the correct name must be printed by binary-upgrade pg_dump
    anyway, since it wouldn't match the name produced at the parent.
    
    Moreover, when it does hit, the pre-18-compatibility code (which has to
    work with a constraint that has no name) gets involved and uses the
    UPDATE on pg_constraint using the conkey instead of column name ... and
    so everything ends up working correctly AFAICS.
    
    I think it might cause a problem if the table and column names are
    overly long, but I didn't want to spend time investigating further.
    
    Still, it's wrong code, and static analyzers have twice complained about
    it, so fix it by adding the array index accessor that was obviously
    meant.
    
    Reported-by: Ranier Vilela <ranier.vf@gmail.com>
    Reported-by: George Tarasov <george.v.tarasov@gmail.com>
    Backpatch-through: 18
    Discussion: https://postgr.es/m/CAEudQAo7ah=4TDheuEjtb0dsv6bHoK7uBNqv53Tsub2h-xBSJw@mail.gmail.com
    Discussion: https://postgr.es/m/f3029f25-acc9-4cb9-a74f-fe93bcfb3a27@gmail.com

Thanks for reporting it!

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/