Horton
Menu

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. Fix invalid boolean if-test

  1. Fix array access (src/bin/pg_dump/pg_dump.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2024-11-12T17:05:40Z

    Hi.

Per Coverity.

The function *determineNotNullFlags* has a little oversight.
The struct field *notnull_islocal* is an array.

I think this is a simple typo.
Fix using array notation access.

Trivial patch attached.

best regards,
Ranier Vilela

  2. Re: Fix array access (src/bin/pg_dump/pg_dump.c)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-11-12T19:11:07Z

    On 2024-Nov-12, Ranier Vilela wrote:

> Per Coverity.
> 
> The function *determineNotNullFlags* has a little oversight.
> The struct field *notnull_islocal* is an array.
> 
> I think this is a simple typo.
> Fix using array notation access.

Yeah, thanks, I had been made aware of this bug.  Before fixing I'd like
to construct a test case that tickles that code, because it's currently
uncovered  *shudder*

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Find a bug in a program, and fix it, and the program will work today.
Show the program how to find and fix a bug, and the program
will work forever" (Oliver Silfridge)

  3. Re: Fix array access (src/bin/pg_dump/pg_dump.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2024-11-12T22:13:35Z

    Em ter., 12 de nov. de 2024 às 16:11, Alvaro Herrera <
alvherre@alvh.no-ip.org> escreveu:

> On 2024-Nov-12, Ranier Vilela wrote:
>
> > Per Coverity.
> >
> > The function *determineNotNullFlags* has a little oversight.
> > The struct field *notnull_islocal* is an array.
> >
> > I think this is a simple typo.
> > Fix using array notation access.
>
> Yeah, thanks, I had been made aware of this bug.  Before fixing I'd like
> to construct a test case that tickles that code, because it's currently
> uncovered  *shudder*
>
Thanks for taking care of this.

best regards,
Ranier Vilela

  4. Re: Fix array access (src/bin/pg_dump/pg_dump.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2025-10-10T16:41:48Z

    Hi.

Em ter., 12 de nov. de 2024 às 19:13, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

> Em ter., 12 de nov. de 2024 às 16:11, Alvaro Herrera <
> alvherre@alvh.no-ip.org> escreveu:
>
>> On 2024-Nov-12, Ranier Vilela wrote:
>>
>> > Per Coverity.
>> >
>> > The function *determineNotNullFlags* has a little oversight.
>> > The struct field *notnull_islocal* is an array.
>> >
>> > I think this is a simple typo.
>> > Fix using array notation access.
>>
>> Yeah, thanks, I had been made aware of this bug.  Before fixing I'd like
>> to construct a test case that tickles that code, because it's currently
>> uncovered  *shudder*
>>
> Thanks for taking care of this.
>
Ping.

best regards,
Ranier Vilela

  5. Re: Fix array access (src/bin/pg_dump/pg_dump.c)

    Chao Li <li.evan.chao@gmail.com> — 2025-10-11T05:24:24Z

    On Oct 11, 2025, at 00:41, Ranier Vilela <ranier.vf@gmail.com> wrote:

Em ter., 12 de nov. de 2024 às 19:13, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

> Em ter., 12 de nov. de 2024 às 16:11, Alvaro Herrera <
> alvherre@alvh.no-ip.org> escreveu:
>
>> On 2024-Nov-12, Ranier Vilela wrote:
>>
>> > Per Coverity.
>> >
>> > The function *determineNotNullFlags* has a little oversight.
>> > The struct field *notnull_islocal* is an array.
>> >
>> > I think this is a simple typo.
>> > Fix using array notation access.
>>
>> Yeah, thanks, I had been made aware of this bug.  Before fixing I'd like
>> to construct a test case that tickles that code, because it's currently
>> uncovered  *shudder*
>>
> Thanks for taking care of this.
>
Ping.


I tried to debug this bug, and it looks like this bug can never be
triggered.

To do the test, I created two tables:
```
evantest=# CREATE TABLE parent_nn(a int NOT NULL, b int);
CREATE TABLE
evantest=# CREATE TABLE child_nn() INHERITS (parent_nn);
CREATE TABLE
evantest=# ALTER TABLE child_nn ALTER COLUMN b SET NOT NULL;
ALTER TABLE
```

Then let’s look at the code:

/*
* 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) ||
!PQgetisnull(res, r, i_notnull_comment)) // A
{
const char *val = PQgetvalue(res, r, i_notnull_name); // B
tbinfo->notnull_constrs[j] =
pstrdup(val);
}
else
{
char *default_name;
const char *val = PQgetvalue(res, r, i_notnull_name);

/* XXX should match ChooseConstraintName better */
default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
tbinfo->attnames[j]);
if (strcmp(default_name, // C
val) == 0)
tbinfo->notnull_constrs[j] = "";
else
{
tbinfo->notnull_constrs[j] = // D
pstrdup(val);
}
free(default_name);
}

Notice that I marked four lines with A/B/C/D.

For child table’s column “b”, when it reaches line A, it hits the bug,
as tbinfo->notnull_islocal[j] should be false, but tbinfo->notnull_islocal
is always true.

If the bug is fixed, as tbinfo->notnull_islocal[j] is false, it will enter
the “if” clause, in line B, PGgetvalue() will return “parent_nn_a_not_null”.

With this bug, if will go the the “else” clause, run strcmp()  in line C.
Here, “default_name” is built from the table name, its value is
“child_nn_a_not_null”, while PGgetvalue() is “parent_nn_a_not_null”, thus
it won’t meet the “if” of “strcmp”, instead it goes to line D, that runs
the same assignment as in line B.

So, Ranier, please let me know if you have an example that generates the
wrong result, then I can verify the fix with your test case.

But I believe we should still fix the bug: 1) otherwise the code is
confusing 2) after fixing, when tbinfo->notnull_islocal[j] is false, the
execution path is shorter 3) to make Coverity happy.

I also added a TAP test with test cases for determining NULL:

```
chaol@ChaodeMacBook-Air pg_dump % make check PROVE_TESTS='t/
011_dump_determine_null.pl'
# +++ tap check in src/bin/pg_dump +++
t/011_dump_determine_null.pl .. ok
All tests successful.
Files=1, Tests=5,  2 wallclock secs ( 0.00 usr  0.01 sys +  0.08 cusr  0.31
csys =  0.40 CPU)
Result: PASS
```

Please see the attached patch.

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

  6. Re: Fix array access (src/bin/pg_dump/pg_dump.c)

    Ranier Vilela <ranier.vf@gmail.com> — 2025-10-11T10:10:33Z

    Em sáb., 11 de out. de 2025 às 02:24, Chao Li <li.evan.chao@gmail.com>
escreveu:

>
> On Oct 11, 2025, at 00:41, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em ter., 12 de nov. de 2024 às 19:13, Ranier Vilela <ranier.vf@gmail.com>
> escreveu:
>
>> Em ter., 12 de nov. de 2024 às 16:11, Alvaro Herrera <
>> alvherre@alvh.no-ip.org> escreveu:
>>
>>> On 2024-Nov-12, Ranier Vilela wrote:
>>>
>>> > Per Coverity.
>>> >
>>> > The function *determineNotNullFlags* has a little oversight.
>>> > The struct field *notnull_islocal* is an array.
>>> >
>>> > I think this is a simple typo.
>>> > Fix using array notation access.
>>>
>>> Yeah, thanks, I had been made aware of this bug.  Before fixing I'd like
>>> to construct a test case that tickles that code, because it's currently
>>> uncovered  *shudder*
>>>
>> Thanks for taking care of this.
>>
> Ping.
>
>
> I tried to debug this bug, and it looks like this bug can never be
> triggered.
>
> To do the test, I created two tables:
> ```
> evantest=# CREATE TABLE parent_nn(a int NOT NULL, b int);
> CREATE TABLE
> evantest=# CREATE TABLE child_nn() INHERITS (parent_nn);
> CREATE TABLE
> evantest=# ALTER TABLE child_nn ALTER COLUMN b SET NOT NULL;
> ALTER TABLE
> ```
>
> Then let’s look at the code:
>
> /*
> * 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) ||
> !PQgetisnull(res, r, i_notnull_comment)) // A
> {
> const char *val = PQgetvalue(res, r, i_notnull_name); // B
> tbinfo->notnull_constrs[j] =
> pstrdup(val);
> }
> else
> {
> char *default_name;
> const char *val = PQgetvalue(res, r, i_notnull_name);
>
> /* XXX should match ChooseConstraintName better */
> default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
> tbinfo->attnames[j]);
> if (strcmp(default_name, // C
> val) == 0)
> tbinfo->notnull_constrs[j] = "";
> else
> {
> tbinfo->notnull_constrs[j] = // D
> pstrdup(val);
> }
> free(default_name);
> }
>
> Notice that I marked four lines with A/B/C/D.
>
> For child table’s column “b”, when it reaches line A, it hits the bug,
> as tbinfo->notnull_islocal[j] should be false, but tbinfo->notnull_islocal
> is always true.
>
> If the bug is fixed, as tbinfo->notnull_islocal[j] is false, it will enter
> the “if” clause, in line B, PGgetvalue() will return “parent_nn_a_not_null”.
>
> With this bug, if will go the the “else” clause, run strcmp()  in line C.
> Here, “default_name” is built from the table name, its value is
> “child_nn_a_not_null”, while PGgetvalue() is “parent_nn_a_not_null”, thus
> it won’t meet the “if” of “strcmp”, instead it goes to line D, that runs
> the same assignment as in line B.
>
> So, Ranier, please let me know if you have an example that generates the
> wrong result, then I can verify the fix with your test case.
>
> But I believe we should still fix the bug: 1) otherwise the code is
> confusing 2) after fixing, when tbinfo->notnull_islocal[j] is false, the
> execution path is shorter 3) to make Coverity happy.
>
> I also added a TAP test with test cases for determining NULL:
>
> ```
> chaol@ChaodeMacBook-Air pg_dump % make check PROVE_TESTS='t/
> 011_dump_determine_null.pl'
> # +++ tap check in src/bin/pg_dump +++
> t/011_dump_determine_null.pl .. ok
> All tests successful.
> Files=1, Tests=5,  2 wallclock secs ( 0.00 usr  0.01 sys +  0.08 cusr
>  0.31 csys =  0.40 CPU)
> Result: PASS
> ```
>
> Please see the attached patch.
>
Did you read the entire thread?
I think it's very rude and inelegant to suggest a patch while taking
advantage of another patch.

Best regards,
Ranier Vilela

  7. Re: Fix array access (src/bin/pg_dump/pg_dump.c)

    Chao Li <li.evan.chao@gmail.com> — 2025-10-11T11:59:21Z

    
> On Oct 11, 2025, at 18:10, Ranier Vilela <ranier.vf@gmail.com> wrote:
> 
> 
> Did you read the entire thread?
> I think it's very rude and inelegant to suggest a patch while taking advantage of another patch.
> 
> Best regards,
> Ranier Vilela


No, I guess I didn’t read the entire thread. I just saw ping and thought to help. If you don’t like, just ignore my previous response.

  8. Re: Fix array access (src/bin/pg_dump/pg_dump.c)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2026-03-07T13:33:08Z

    On 2024-Nov-12, Ranier Vilela wrote:

> Em ter., 12 de nov. de 2024 às 16:11, Alvaro Herrera <
> alvherre@alvh.no-ip.org> escreveu:
> 
> > On 2024-Nov-12, Ranier Vilela wrote:
> >
> > > Per Coverity.
> > >
> > > The function *determineNotNullFlags* has a little oversight.
> > > The struct field *notnull_islocal* is an array.
> > >
> > > I think this is a simple typo.
> > > Fix using array notation access.
> >
> > Yeah, thanks, I had been made aware of this bug.  Before fixing I'd like
> > to construct a test case that tickles that code, because it's currently
> > uncovered  *shudder*
>
> Thanks for taking care of this.

Fixed, thanks.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/