public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v5 3/3] Change policy of XLog read-buffer allocation
8+ messages / 4 participants
[nested] [flat]
* [PATCH v5 3/3] Change policy of XLog read-buffer allocation
@ 2019-09-05 12:29 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Kyotaro Horiguchi @ 2019-09-05 12:29 UTC (permalink / raw)
Page buffer in XLogReaderState was allocated by XLogReaderAllcoate but
actually it'd be the responsibility to the callers of XLogReadRecord,
which now actually reads in pages. This patch does that.
---
src/backend/access/transam/twophase.c | 2 ++
src/backend/access/transam/xlog.c | 2 ++
src/backend/access/transam/xlogreader.c | 18 ------------------
src/backend/replication/logical/logical.c | 2 ++
src/bin/pg_rewind/parsexlog.c | 6 ++++++
src/bin/pg_waldump/pg_waldump.c | 2 ++
6 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 929da4eef2..9ec88b35ef 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1392,6 +1392,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory"),
errdetail("Failed while allocating a WAL reading processor.")));
+ xlogreader->readBuf = palloc(XLOG_BLCKSZ);
while (XLogReadRecord(xlogreader, lsn, &record, &errormsg) ==
XLREAD_NEED_DATA)
@@ -1421,6 +1422,7 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
*buf = palloc(sizeof(char) * XLogRecGetDataLen(xlogreader));
memcpy(*buf, XLogRecGetData(xlogreader), sizeof(char) * XLogRecGetDataLen(xlogreader));
+ pfree(xlogreader->readBuf);
XLogReaderFree(xlogreader);
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 96e2115aad..425f7a12bd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6351,6 +6351,7 @@ StartupXLOG(void)
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory"),
errdetail("Failed while allocating a WAL reading processor.")));
+ xlogreader->readBuf = palloc(XLOG_BLCKSZ);
xlogreader->system_identifier = ControlFile->system_identifier;
/*
@@ -7722,6 +7723,7 @@ StartupXLOG(void)
close(readFile);
readFile = -1;
}
+ pfree(xlogreader->readBuf);
XLogReaderFree(xlogreader);
/*
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 66bd9eb8d7..26f6834b8f 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -79,27 +79,11 @@ XLogReaderAllocate(int wal_segment_size)
state->max_block_id = -1;
- /*
- * Permanently allocate readBuf. We do it this way, rather than just
- * making a static array, for two reasons: (1) no need to waste the
- * storage in most instantiations of the backend; (2) a static char array
- * isn't guaranteed to have any particular alignment, whereas
- * palloc_extended() will provide MAXALIGN'd storage.
- */
- state->readBuf = (char *) palloc_extended(XLOG_BLCKSZ,
- MCXT_ALLOC_NO_OOM);
- if (!state->readBuf)
- {
- pfree(state);
- return NULL;
- }
-
state->wal_segment_size = wal_segment_size;
state->errormsg_buf = palloc_extended(MAX_ERRORMSG_LEN + 1,
MCXT_ALLOC_NO_OOM);
if (!state->errormsg_buf)
{
- pfree(state->readBuf);
pfree(state);
return NULL;
}
@@ -112,7 +96,6 @@ XLogReaderAllocate(int wal_segment_size)
if (!allocate_recordbuf(state, 0))
{
pfree(state->errormsg_buf);
- pfree(state->readBuf);
pfree(state);
return NULL;
}
@@ -136,7 +119,6 @@ XLogReaderFree(XLogReaderState *state)
pfree(state->errormsg_buf);
if (state->readRecordBuf)
pfree(state->readRecordBuf);
- pfree(state->readBuf);
pfree(state);
}
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 11e52e4c01..ea027caa69 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -178,6 +178,7 @@ StartupDecodingContext(List *output_plugin_options,
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of memory")));
+ ctx->reader->readBuf = palloc(XLOG_BLCKSZ);
ctx->read_page = read_page;
ctx->reorder = ReorderBufferAllocate();
@@ -523,6 +524,7 @@ FreeDecodingContext(LogicalDecodingContext *ctx)
ReorderBufferFree(ctx->reorder);
FreeSnapshotBuilder(ctx->snapshot_builder);
+ pfree(ctx->reader->readBuf);
XLogReaderFree(ctx->reader);
MemoryContextDelete(ctx->context);
}
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index ff26b30f82..c60267e87e 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -60,6 +60,7 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
xlogreader = XLogReaderAllocate(WalSegSz);
if (xlogreader == NULL)
pg_fatal("out of memory");
+ xlogreader->readBuf = pg_malloc(XLOG_BLCKSZ);
do
{
@@ -92,6 +93,7 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex,
} while (xlogreader->ReadRecPtr != endpoint);
+ pg_free(xlogreader->readBuf);
XLogReaderFree(xlogreader);
if (xlogreadfd != -1)
{
@@ -115,6 +117,7 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
xlogreader = XLogReaderAllocate(WalSegSz);
if (xlogreader == NULL)
pg_fatal("out of memory");
+ xlogreader->readBuf = pg_malloc(XLOG_BLCKSZ);
while (XLogReadRecord(xlogreader, ptr, &record, &errormsg) ==
XLREAD_NEED_DATA)
@@ -133,6 +136,7 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex)
}
endptr = xlogreader->EndRecPtr;
+ pg_free(xlogreader->readBuf);
XLogReaderFree(xlogreader);
if (xlogreadfd != -1)
{
@@ -174,6 +178,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
xlogreader = XLogReaderAllocate(WalSegSz);
if (xlogreader == NULL)
pg_fatal("out of memory");
+ xlogreader->readBuf = pg_malloc(XLOG_BLCKSZ);
searchptr = forkptr;
for (;;)
@@ -222,6 +227,7 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
searchptr = record->xl_prev;
}
+ pg_free(xlogreader->readBuf);
XLogReaderFree(xlogreader);
if (xlogreadfd != -1)
{
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 56e2f8b0b0..bdf3c81b03 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -1108,6 +1108,7 @@ main(int argc, char **argv)
xlogreader_state = XLogReaderAllocate(WalSegSz);
if (!xlogreader_state)
fatal_error("out of memory");
+ xlogreader_state->readBuf = palloc(XLOG_BLCKSZ);
/* first find a valid recptr to start from */
first_record = XLogFindNextRecord(xlogreader_state, private.startptr,
@@ -1190,6 +1191,7 @@ main(int argc, char **argv)
(uint32) xlogreader_state->ReadRecPtr,
errormsg);
+ pfree(xlogreader_state->readBuf);
XLogReaderFree(xlogreader_state);
return EXIT_SUCCESS;
--
2.16.3
----Next_Part(Fri_Sep_06_16_33_18_2019_198)----
^ permalink raw reply [nested|flat] 8+ messages in thread
* Fix array access (src/bin/pg_dump/pg_dump.c)
@ 2024-11-12 17:05 Ranier Vilela <[email protected]>
2024-11-12 19:11 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Alvaro Herrera <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ranier Vilela @ 2024-11-12 17:05 UTC (permalink / raw)
To: pgsql-hackers
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:
[application/octet-stream] fix_array_access_pg_dump.patch (473B, ../../CAEudQAo7ah=4TDheuEjtb0dsv6bHoK7uBNqv53Tsub2h-xBSJw@mail.gmail.com/3-fix_array_access_pg_dump.patch)
download | inline diff:
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));
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix array access (src/bin/pg_dump/pg_dump.c)
2024-11-12 17:05 Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
@ 2024-11-12 19:11 ` Alvaro Herrera <[email protected]>
2024-11-12 22:13 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2024-11-12 19:11 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: pgsql-hackers
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)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix array access (src/bin/pg_dump/pg_dump.c)
2024-11-12 17:05 Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2024-11-12 19:11 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Alvaro Herrera <[email protected]>
@ 2024-11-12 22:13 ` Ranier Vilela <[email protected]>
2025-10-10 16:41 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ranier Vilela @ 2024-11-12 22:13 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: pgsql-hackers
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.
best regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix array access (src/bin/pg_dump/pg_dump.c)
2024-11-12 17:05 Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2024-11-12 19:11 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Alvaro Herrera <[email protected]>
2024-11-12 22:13 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
@ 2025-10-10 16:41 ` Ranier Vilela <[email protected]>
2025-10-11 05:24 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Chao Li <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ranier Vilela @ 2025-10-10 16:41 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: pgsql-hackers
Hi.
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.
best regards,
Ranier Vilela
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix array access (src/bin/pg_dump/pg_dump.c)
2024-11-12 17:05 Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2024-11-12 19:11 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Alvaro Herrera <[email protected]>
2024-11-12 22:13 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-10 16:41 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
@ 2025-10-11 05:24 ` Chao Li <[email protected]>
2025-10-11 10:10 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Chao Li @ 2025-10-11 05:24 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers
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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-Fixed-a-bug-in-pg_dump-about-determining-NotNull.patch (3.9K, ../../CAEoWx2nJXLDb_gZuGcAE4rCfwUX=ndtkcBx55ynNSyFPOgXpEQ@mail.gmail.com/3-v1-0001-Fixed-a-bug-in-pg_dump-about-determining-NotNull.patch)
download | inline diff:
From 92608d497d779000077d13dae6c2064549c3785f Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
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 <[email protected]>
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)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix array access (src/bin/pg_dump/pg_dump.c)
2024-11-12 17:05 Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2024-11-12 19:11 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Alvaro Herrera <[email protected]>
2024-11-12 22:13 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-10 16:41 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-11 05:24 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Chao Li <[email protected]>
@ 2025-10-11 10:10 ` Ranier Vilela <[email protected]>
2025-10-11 11:59 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Chao Li <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ranier Vilela @ 2025-10-11 10:10 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Fix array access (src/bin/pg_dump/pg_dump.c)
2024-11-12 17:05 Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2024-11-12 19:11 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Alvaro Herrera <[email protected]>
2024-11-12 22:13 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-10 16:41 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-11 05:24 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Chao Li <[email protected]>
2025-10-11 10:10 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
@ 2025-10-11 11:59 ` Chao Li <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Chao Li @ 2025-10-11 11:59 UTC (permalink / raw)
To: Ranier Vilela <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; pgsql-hackers
> On Oct 11, 2025, at 18:10, Ranier Vilela <[email protected]> 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.
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-10-11 11:59 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 12:29 [PATCH v5 3/3] Change policy of XLog read-buffer allocation Kyotaro Horiguchi <[email protected]>
2024-11-12 17:05 Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2024-11-12 19:11 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Alvaro Herrera <[email protected]>
2024-11-12 22:13 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-10 16:41 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-11 05:24 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Chao Li <[email protected]>
2025-10-11 10:10 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Ranier Vilela <[email protected]>
2025-10-11 11:59 ` Re: Fix array access (src/bin/pg_dump/pg_dump.c) Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox