Em sáb., 11 de out. de 2025 às 02:24, Chao Li <[email protected]> escreveu:
> > On Oct 11, 2025, at 00:41, Ranier Vilela <[email protected]> wrote: > > Em ter., 12 de nov. de 2024 às 19:13, Ranier Vilela <[email protected]> > escreveu: > >> Em ter., 12 de nov. de 2024 às 16:11, Alvaro Herrera < >> [email protected]> 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
