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:
```
# +++ tap check in src/bin/pg_dump +++
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,
