Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1v7WYs-004057-H0 for pgsql-hackers@arkaria.postgresql.org; Sat, 11 Oct 2025 10:10:50 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1v7WYp-00FpBI-95 for pgsql-hackers@arkaria.postgresql.org; Sat, 11 Oct 2025 10:10:48 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1v7WYo-00FpB9-T6 for pgsql-hackers@lists.postgresql.org; Sat, 11 Oct 2025 10:10:47 +0000 Received: from mail-pg1-x52d.google.com ([2607:f8b0:4864:20::52d]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1v7WYk-001ZBv-0o for pgsql-hackers@postgresql.org; Sat, 11 Oct 2025 10:10:47 +0000 Received: by mail-pg1-x52d.google.com with SMTP id 41be03b00d2f7-b631b435d59so1833115a12.0 for ; Sat, 11 Oct 2025 03:10:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1760177439; x=1760782239; darn=postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=MtGWK5EvXNu0AMAnW53aEh4qjSlZInF0e4dl7Ccf/4I=; b=E5c1XoJf2m4WRvaQETNs2IssBqpk16rBCyKDlRZOBSTsBFQw0csiTq7FMjAy1kM1YJ YHcAacEesmpZZde3H6Cn1K3sKAo/akmPGXnhT8MOvuM2kNOgr6IOYMYhNhxMkpDdycc9 Zg8Yyg1vwMBe7YUJ16mFXvV55MOSkHrpaVavftYxooDE5klCOgQfzfId0dKqQQ/PWq+P aMtAIzEk1s55MDdXpIUjlKDWT691Q7TI3zQspPJOJ3WR+9Z7Fe3Ghu8t4IN81K4sZbE7 TMiZ1BUUnErjGqx5u689NqjcPJmyEeaazYD/Ow2IbqdJahRrt5ZxmQzjy1CDZ6C08dfK +M2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1760177439; x=1760782239; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=MtGWK5EvXNu0AMAnW53aEh4qjSlZInF0e4dl7Ccf/4I=; b=Yomzt7jTjIwdQufwJVf39xs5PNYPnLw/yVCpTwBruF8jliGAfv4Q0kpkYdTV2sE2wt MzSW5/ax3GswmornmSf7Aw6xZabrfi1F1URjZ2lJnXBwdLiwxNiAyzHoTBlhZ7b8ikUj wsiXwohXEKTP3yOGOx3zQFv3GQkr+zgCUfCtQie7PKSnVnvsvd9WESX+Ykscn3k2QyEC YRpCxOxQsf8bBq/rZawmzonS0G36lEMCHQnmQmy0K0agMKt5aKm4LuVMa0l4kyv7DGW3 vBNtO91L4xUrWDWRwLF9ChLmZIr4J01qDox0QdqnZG9l+mGMk6hT2+8H9ExKWw1OzP6N 6r3A== X-Forwarded-Encrypted: i=1; AJvYcCX5159769w4Ird7B+lRGIfp4JqKjsgPwB8xQKrgNjLF4u1ptsUJbvQ+6vulI08jGiU5Q+YNMReOIUXPWBkM@postgresql.org X-Gm-Message-State: AOJu0YwDVNH3gctDY/ETeYaM8mW2CH5fFrCQkJm7AHAqi8iN+NDpjVWU AG3wZ5PBuVXvhjEX5lVcR/uOfOuClrGoldcBI1JrKtJCaiJoI3l1OPp/vYbmc+lwF56A4UWq7gx KLTh29CqOcz0W6+xtqHmhxZn5+3n+XQg= X-Gm-Gg: ASbGnctsTk+X5zho95wEAC25y+FoEXu+QiUbLp6Q/WO1JUP8jCMQG7/pQvNV+yXrVcD IpvwcQ1TWcOUFfxGmnBWJrIHYnSj7Wpa0bIeHiysd5gYLB7IqAs2c9mxNa9fF1p3MySvi1hSprY uamMYZX6vzXAE4OEnZjaGeJvPfswihWE1G3cCtmCONtQQEgPWz8QZweN6LQj24vZAgYETKtP6j/ s9CvTCF7iNsQraF5Amh8c+LQoVTEJPKFU+vbv9uNgY169dOz47eboaOqBbz0le/8F5TvQ== X-Google-Smtp-Source: AGHT+IEMYzyl51fJEkIwUyDFQnZdDPVtkxbLWFES07LAWJo43iqbLkNd2zA6RO9bEzbookNllO/DdF/vlCt/rIccxbA= X-Received: by 2002:a17:902:f693:b0:267:bd66:14f3 with SMTP id d9443c01a7336-29027402ed5mr174535995ad.51.1760177438677; Sat, 11 Oct 2025 03:10:38 -0700 (PDT) MIME-Version: 1.0 References: <202411121911.3vwdrzfgjt4o@alvherre.pgsql> In-Reply-To: From: Ranier Vilela Date: Sat, 11 Oct 2025 07:10:33 -0300 X-Gm-Features: AS18NWB7F-vaEf7Wg6albdWdrJfOappjysZ00MYTjvQhg8E6l5rELEDtJBzKtZ0 Message-ID: Subject: Re: Fix array access (src/bin/pg_dump/pg_dump.c) To: Chao Li Cc: Alvaro Herrera , Pg Hackers Content-Type: multipart/alternative; boundary="000000000000f95f810640df3e16" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000f95f810640df3e16 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Em s=C3=A1b., 11 de out. de 2025 =C3=A0s 02:24, Chao Li escreveu: > > On Oct 11, 2025, at 00:41, Ranier Vilela wrote: > > Em ter., 12 de nov. de 2024 =C3=A0s 19:13, Ranier Vilela > escreveu: > >> Em ter., 12 de nov. de 2024 =C3=A0s 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 lik= e >>> 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=3D# CREATE TABLE parent_nn(a int NOT NULL, b int); > CREATE TABLE > evantest=3D# CREATE TABLE child_nn() INHERITS (parent_nn); > CREATE TABLE > evantest=3D# ALTER TABLE child_nn ALTER COLUMN b SET NOT NULL; > ALTER TABLE > ``` > > Then let=E2=80=99s 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 =3D PQgetvalue(res, r, i_notnull_name); // B > tbinfo->notnull_constrs[j] =3D > pstrdup(val); > } > else > { > char *default_name; > const char *val =3D PQgetvalue(res, r, i_notnull_name); > > /* XXX should match ChooseConstraintName better */ > default_name =3D psprintf("%s_%s_not_null", tbinfo->dobj.name, > tbinfo->attnames[j]); > if (strcmp(default_name, // C > val) =3D=3D 0) > tbinfo->notnull_constrs[j] =3D ""; > else > { > tbinfo->notnull_constrs[j] =3D // D > pstrdup(val); > } > free(default_name); > } > > Notice that I marked four lines with A/B/C/D. > > For child table=E2=80=99s column =E2=80=9Cb=E2=80=9D, when it reaches lin= e A, it hits the bug, > as tbinfo->notnull_islocal[j] should be false, but tbinfo->notnull_isloca= l > is always true. > > If the bug is fixed, as tbinfo->notnull_islocal[j] is false, it will ente= r > the =E2=80=9Cif=E2=80=9D clause, in line B, PGgetvalue() will return =E2= =80=9Cparent_nn_a_not_null=E2=80=9D. > > With this bug, if will go the the =E2=80=9Celse=E2=80=9D clause, run strc= mp() in line C. > Here, =E2=80=9Cdefault_name=E2=80=9D is built from the table name, its va= lue is > =E2=80=9Cchild_nn_a_not_null=E2=80=9D, while PGgetvalue() is =E2=80=9Cpar= ent_nn_a_not_null=E2=80=9D, thus > it won=E2=80=99t meet the =E2=80=9Cif=E2=80=9D of =E2=80=9Cstrcmp=E2=80= =9D, 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=3D't/ > 011_dump_determine_null.pl' > # +++ tap check in src/bin/pg_dump +++ > t/011_dump_determine_null.pl .. ok > All tests successful. > Files=3D1, Tests=3D5, 2 wallclock secs ( 0.00 usr 0.01 sys + 0.08 cusr > 0.31 csys =3D 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 --000000000000f95f810640df3e16 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


Em s=C3=A1b., 1= 1 de out. de 2025 =C3=A0s 02:24, Chao Li <li.evan.chao@gmail.com> escreveu:
On Oct 11, 202= 5, at 00:41, Ranier Vilela <ranier.vf@gmail.com> wrote:
=

Em ter., 12 de nov. de 2024 =C3=A0s 19:13, Ranier Vilela <ranier.vf@gmail.com&g= t; escreveu:
Em ter., 12 de nov. de 2024 =C3=A0s 16:11, Alvaro Herrera <alvherre@alvh.no-ip.o= rg> 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.=C2=A0 Before fixing I'= d like
to construct a test case that tickles that code, because it's currently=
uncovered=C2=A0 *shudder*
Thanks for taking care of th= is.
Ping.


I tried to debu= g this bug, and it looks like this bug can never be triggered.

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

Then let=E2=80= =99s 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-&= gt;notnull_islocal) ||
!PQgetisnull(<= span style=3D"color:rgb(154,154,154)">res, r, i_notnull_comment)) // A
<= span style=3D"color:rgb(180,180,180)">{
const char *val =3D PQgetvalue(res, r, i_notnull_name); // B
tbinfo->notnull_constrs[j] =3D
= pstrdup(val);
= }
= else
= {
= char *defaul= t_name;
= const char *val =3D = PQgetvalue(res, r, i_not= null_name);

<= div> /* XXX should matc= h ChooseConstraintName better */
default_name =3D psprintf("%s_%s_not_null", tbinfo->dobj.name,<= /span>
tbinfo-&= gt;attnames[j]);
if (strcmp= (default_name,= // C
val) =3D=3D 0)
=
tbinfo->notnull_constrs[j] =3D &= quot;";
else
{
tbinfo<= span style=3D"color:rgb(180,180,180)">->notnull_constrs[j] =3D // D
pstrdup(val);
}
free(default_name);
}

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

For child table=E2=80=99s colu= mn =E2=80=9Cb=E2=80=9D, when it reaches line A, it hits the bug, as=C2=A0tb= info->notnull_islocal[j] should be false, but=C2=A0tbinfo->notnull_is= local is always true.

If the bug is fixed, as tbin= fo->notnull_islocal[j] is false, it will enter the =E2=80=9Cif=E2=80=9D = clause, in line B, PGgetvalue() will return =E2=80=9Cparent_nn_a_not_null= =E2=80=9D.

With this bug, if will go the the =E2= =80=9Celse=E2=80=9D clause, run strcmp() =C2=A0in line C. Here, =E2=80=9Cde= fault_name=E2=80=9D is built from the table name, its value is =E2=80=9Cchi= ld_nn_a_not_null=E2=80=9D, while PGgetvalue() is =E2=80=9Cparent_nn_a_not_n= ull=E2=80=9D, thus it won=E2=80=99t meet the =E2=80=9Cif=E2=80=9D of =E2=80= =9Cstrcmp=E2=80=9D, instead it goes to line D, that runs the same assignmen= t 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 s= till 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=3D't/011_dump_determin= e_null.pl'
# +++ tap check in src/bin/pg_dump +++
t/011_du= mp_determine_null.pl .. ok
All tests successful.
Fi= les=3D1, Tests=3D5, =C2=A02 wallclock secs ( 0.00 usr =C2=A00.01 sys + =C2= =A00.08 cusr =C2=A00.31 csys =3D =C2=A00.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
--000000000000f95f810640df3e16--