Fix array access (src/bin/pg_dump/pg_dump.c)
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
Attachments:
fix_array_access_pg_dump.patchapplication/octet-stream; name=fix_array_access_pg_dump.patchDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a8c141b689..404f5d8675 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -9397,7 +9397,7 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
*/
if (dopt->binary_upgrade &&
!tbinfo->ispartition &&
- !tbinfo->notnull_islocal)
+ !tbinfo->notnull_islocal[j])
{
tbinfo->notnull_constrs[j] =
pstrdup(PQgetvalue(res, r, i_notnull_name));
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)
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
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
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/
Attachments:
v1-0001-Fixed-a-bug-in-pg_dump-about-determining-NotNull.patchapplication/octet-stream; name=v1-0001-Fixed-a-bug-in-pg_dump-about-determining-NotNull.patchDownload
From 92608d497d779000077d13dae6c2064549c3785f Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Sat, 11 Oct 2025 13:13:32 +0800
Subject: [PATCH v1] Fixed a bug in pg_dump about determining NotNull
In determineNotNullFlags(), a check should be done against
tbinfo->notnull_islocal[j], but "[j]" was missing. However,
this bug cannot be actually fired, see the discussion.
This commit fixes the bug and adds a TAP test of determining
NotNull for pg_dump.
Author: Chao Li <lic@highgo.com>
Discussion: https://www.postgresql.org/message-id/CAEudQAo7ah%3D4TDheuEjtb0dsv6bHoK7uBNqv53Tsub2h-xBSJw%40mail.gmail.com
---
src/bin/pg_dump/meson.build | 1 +
src/bin/pg_dump/pg_dump.c | 2 +-
src/bin/pg_dump/t/011_dump_determine_null.pl | 59 ++++++++++++++++++++
3 files changed, 61 insertions(+), 1 deletion(-)
create mode 100644 src/bin/pg_dump/t/011_dump_determine_null.pl
diff --git a/src/bin/pg_dump/meson.build b/src/bin/pg_dump/meson.build
index a2233b0a1b4..47225b818da 100644
--- a/src/bin/pg_dump/meson.build
+++ b/src/bin/pg_dump/meson.build
@@ -103,6 +103,7 @@ tests += {
't/004_pg_dump_parallel.pl',
't/005_pg_dump_filterfile.pl',
't/010_dump_connstr.pl',
+ 't/011_dump_determine_null.pl',
],
},
}
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 641bece12c7..9dc9396a34d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -10074,7 +10074,7 @@ determineNotNullFlags(Archive *fout, PGresult *res, int r,
*/
if ((dopt->binary_upgrade &&
!tbinfo->ispartition &&
- !tbinfo->notnull_islocal) ||
+ !tbinfo->notnull_islocal[j]) ||
!PQgetisnull(res, r, i_notnull_comment))
{
tbinfo->notnull_constrs[j] =
diff --git a/src/bin/pg_dump/t/011_dump_determine_null.pl b/src/bin/pg_dump/t/011_dump_determine_null.pl
new file mode 100644
index 00000000000..11d93135dd8
--- /dev/null
+++ b/src/bin/pg_dump/t/011_dump_determine_null.pl
@@ -0,0 +1,59 @@
+# Copyright (c) 2021-2025, PostgreSQL Global Development Group
+
+use strict;
+use warnings FATAL => 'all';
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+my $node = PostgreSQL::Test::Cluster->new('main');
+my $port = $node->port;
+my $backupdir = $node->backup_dir;
+my $plainfile = "$backupdir/plain.sql";
+
+# Init & start the node (no extra args; follow conventions used by other tests)
+$node->init;
+$node->start;
+
+# Setup parent/child table for binary upgrade NOT NULL check
+$node->safe_psql('postgres', "CREATE TABLE parent_nn(a int NOT NULL, b int)");
+$node->safe_psql('postgres', "CREATE TABLE child_nn() INHERITS (parent_nn)");
+
+# Add a local NOT NULL to child column 'b'
+$node->safe_psql('postgres', "ALTER TABLE child_nn ALTER COLUMN b SET NOT NULL");
+
+# Do a schema-only pg_dump through the tap helper
+command_ok(
+ [
+ 'pg_dump',
+ '--file' => $plainfile,
+ '--schema-only',
+ '--binary-upgrade',
+ '--port' => $port,
+ 'postgres'
+ ],
+ 'pg_dump schema-only succeeds'
+);
+
+# Read dump and assert expected NOT NULL markings
+my $dump = slurp_file($plainfile);
+
+# Check parent table that NOT NULL is there
+ok($dump =~ qr/CREATE TABLE public\.parent_nn\s*\(\s*a integer NOT NULL/m,
+ "parent NOT NULL column 'a' is there");
+
+# Check parent table that NULL-able is there
+ok($dump =~ qr/CREATE TABLE public\.parent_nn\s*\(\s*a.*,\s*b integer/m,
+ "parent NULL-able column 'b' is there");
+
+# Check child table that inherited NOT NULL is preserved
+ok($dump =~ qr/CREATE TABLE public\.child_nn\s*\(\s*a integer CONSTRAINT parent_nn_a_not_null NOT NULL/m,
+ "inherited NOT NULL column 'a' preserved");
+
+# Check child table that locally added NOT NULL is preserved
+ok($dump =~ qr/CREATE TABLE public\.child_nn\s*\(\s*a.*,\s*b integer NOT NULL/m,
+ "child local NOT NULL column 'b' preserved");
+
+done_testing();
--
2.39.5 (Apple Git-154)
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
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.