pg_basebackup fails on databases with high OIDs
This is a new bug in PG12. When you have a database with an OID above
INT32_MAX (signed), then pg_basebackup fails thus:
pg_basebackup: error: could not get write-ahead log end position from
server: ERROR: value "3000000000" is out of range for type integer
The cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436.
A possible fix is attached. An alternative to using
OidInputFunctionCall() would be exporting something like oidin_subr().
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-base-backup-with-database-OIDs-larger-than-INT32.patchtext/plain; charset=UTF-8; name=0001-Fix-base-backup-with-database-OIDs-larger-than-INT32.patch; x-mac-creator=0; x-mac-type=0Download
From 19ee6b09568b4247c33c2920277dde2fbd3f0ac4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sun, 29 Dec 2019 20:15:50 +0100
Subject: [PATCH] Fix base backup with database OIDs larger than INT32_MAX
---
src/backend/replication/basebackup.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index a73893237a..0e3e0c7a38 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -38,6 +38,7 @@
#include "storage/ipc.h"
#include "storage/reinit.h"
#include "utils/builtins.h"
+#include "utils/fmgroids.h"
#include "utils/ps_status.h"
#include "utils/relcache.h"
#include "utils/timestamp.h"
@@ -1316,7 +1317,7 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
if (!sizeonly)
sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf,
- true, isDbDir ? pg_atoi(lastDir + 1, sizeof(Oid), 0) : InvalidOid);
+ true, isDbDir ? DatumGetObjectId(OidInputFunctionCall(F_OIDIN, unconstify(char *, lastDir + 1), 0, -1)) : InvalidOid);
if (sent || sizeonly)
{
--
2.24.1
On Mon, Jan 06, 2020 at 09:07:26AM +0100, Peter Eisentraut wrote:
This is a new bug in PG12. When you have a database with an OID above
INT32_MAX (signed), then pg_basebackup fails thus:
Yep. Introduced by 6b9e875.
pg_basebackup: error: could not get write-ahead log end position from
server: ERROR: value "3000000000" is out of range for type integerThe cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436.
A possible fix is attached. An alternative to using OidInputFunctionCall()
would be exporting something like oidin_subr().
I think that you would save yourself from a lot of trouble if you do
the latter with a subroutine. Not quite like that based on the
process context where the call is done, but remember 21f428eb..
--
Michael
On Mon, Jan 6, 2020 at 9:21 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jan 06, 2020 at 09:07:26AM +0100, Peter Eisentraut wrote:
This is a new bug in PG12. When you have a database with an OID above
INT32_MAX (signed), then pg_basebackup fails thus:Yep. Introduced by 6b9e875.
Indeed.
pg_basebackup: error: could not get write-ahead log end position from
server: ERROR: value "3000000000" is out of range for type integerThe cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436.
A possible fix is attached. An alternative to using OidInputFunctionCall()
would be exporting something like oidin_subr().I think that you would save yourself from a lot of trouble if you do
the latter with a subroutine. Not quite like that based on the
process context where the call is done, but remember 21f428eb..
+0.5 to avoid calling OidInputFunctionCall()
On Mon, Jan 6, 2020 at 9:31 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
On Mon, Jan 6, 2020 at 9:21 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jan 06, 2020 at 09:07:26AM +0100, Peter Eisentraut wrote:
This is a new bug in PG12. When you have a database with an OID above
INT32_MAX (signed), then pg_basebackup fails thus:Yep. Introduced by 6b9e875.
Indeed.
Yeah, clearly :/
pg_basebackup: error: could not get write-ahead log end position from
server: ERROR: value "3000000000" is out of range for type integerThe cause appears to be commit 6b9e875f7286d8535bff7955e5aa3602e188e436.
A possible fix is attached. An alternative to using OidInputFunctionCall()
would be exporting something like oidin_subr().I think that you would save yourself from a lot of trouble if you do
the latter with a subroutine. Not quite like that based on the
process context where the call is done, but remember 21f428eb..+0.5 to avoid calling OidInputFunctionCall()
Or just directly using atol() instead of atoi()? Well maybe not
directly but in a small wrapper that verifies it's not bigger than an
unsigned?
Unlike in cases where we use oidin etc, we are dealing with data that
is "mostly trusted" here, aren't we? Meaning we could call atol() on
it, and throw an error if it overflows, and be done with it?
Subdirectories in the data directory aren't exactly "untrusted enduser
data"...
I agree with the feelings that calling OidInputFunctionCall() from
this context leaves me slightly irked.
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On 2020-01-06 21:00, Magnus Hagander wrote:
+0.5 to avoid calling OidInputFunctionCall()
Or just directly using atol() instead of atoi()? Well maybe not
directly but in a small wrapper that verifies it's not bigger than an
unsigned?Unlike in cases where we use oidin etc, we are dealing with data that
is "mostly trusted" here, aren't we? Meaning we could call atol() on
it, and throw an error if it overflows, and be done with it?
Subdirectories in the data directory aren't exactly "untrusted enduser
data"...
Yeah, it looks like we are using strtoul() without additional error
checking in similar situations, so here is a patch doing it like that.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
v2-0001-Fix-base-backup-with-database-OIDs-larger-than-IN.patchtext/plain; charset=UTF-8; name=v2-0001-Fix-base-backup-with-database-OIDs-larger-than-IN.patch; x-mac-creator=0; x-mac-type=0Download
From 8a4d22b95bb9f54e6834bc5285c4d84582e6f128 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Sat, 11 Jan 2020 08:16:21 +0100
Subject: [PATCH v2] Fix base backup with database OIDs larger than INT32_MAX
The use of pg_atoi() for parsing a string into an Oid fails for values
larger than INT32_MAX, since OIDs are unsigned. Instead, use plain
strtoul(). While this has less error checking, the content of the
data directory are expected to be trustworthy, so we don't need to go
out of our way to do full error checking.
Discussion: https://www.postgresql.org/message-id/flat/dea47fc8-6c89-a2b1-07e3-754ff1ab094b%402ndquadrant.com
---
src/backend/replication/basebackup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index a73893237a..ae535a0565 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -1316,7 +1316,7 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
if (!sizeonly)
sent = sendFile(pathbuf, pathbuf + basepathlen + 1, &statbuf,
- true, isDbDir ? pg_atoi(lastDir + 1, sizeof(Oid), 0) : InvalidOid);
+ true, isDbDir ? (Oid) strtoul(lastDir + 1, NULL, 10) : InvalidOid);
if (sent || sizeonly)
{
--
2.24.1
On Sat, Jan 11, 2020 at 08:21:11AM +0100, Peter Eisentraut wrote:
On 2020-01-06 21:00, Magnus Hagander wrote:
+0.5 to avoid calling OidInputFunctionCall()
Or just directly using atol() instead of atoi()? Well maybe not
directly but in a small wrapper that verifies it's not bigger than an
unsigned?Unlike in cases where we use oidin etc, we are dealing with data that
is "mostly trusted" here, aren't we? Meaning we could call atol() on
it, and throw an error if it overflows, and be done with it?
Subdirectories in the data directory aren't exactly "untrusted enduser
data"...Yeah, it looks like we are using strtoul() without additional error checking
in similar situations, so here is a patch doing it like that.
- true, isDbDir ? pg_atoi(lastDir + 1, sizeof(Oid), 0) : InvalidOid); + true, isDbDir ? (Oid) strtoul(lastDir + 1, NULL, 10) : InvalidOid);
Looking at some other code, I just discovered the atooid() macro that already
does the same, maybe it'd be better for consistency to use that instead?
On Sat, Jan 11, 2020 at 5:44 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
On Sat, Jan 11, 2020 at 08:21:11AM +0100, Peter Eisentraut wrote:
On 2020-01-06 21:00, Magnus Hagander wrote:
+0.5 to avoid calling OidInputFunctionCall()
Or just directly using atol() instead of atoi()? Well maybe not
directly but in a small wrapper that verifies it's not bigger than an
unsigned?Unlike in cases where we use oidin etc, we are dealing with data that
is "mostly trusted" here, aren't we? Meaning we could call atol() on
it, and throw an error if it overflows, and be done with it?
Subdirectories in the data directory aren't exactly "untrusted enduser
data"...Yeah, it looks like we are using strtoul() without additional error checking
in similar situations, so here is a patch doing it like that.- true, isDbDir ? pg_atoi(lastDir + 1, sizeof(Oid), 0) : InvalidOid); + true, isDbDir ? (Oid) strtoul(lastDir + 1, NULL, 10) : InvalidOid);Looking at some other code, I just discovered the atooid() macro that already
does the same, maybe it'd be better for consistency to use that instead?
+1. Whie it does the same thing, consistency is good! :)
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On 2020-01-11 17:47, Magnus Hagander wrote:
On Sat, Jan 11, 2020 at 5:44 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
On Sat, Jan 11, 2020 at 08:21:11AM +0100, Peter Eisentraut wrote:
On 2020-01-06 21:00, Magnus Hagander wrote:
+0.5 to avoid calling OidInputFunctionCall()
Or just directly using atol() instead of atoi()? Well maybe not
directly but in a small wrapper that verifies it's not bigger than an
unsigned?Unlike in cases where we use oidin etc, we are dealing with data that
is "mostly trusted" here, aren't we? Meaning we could call atol() on
it, and throw an error if it overflows, and be done with it?
Subdirectories in the data directory aren't exactly "untrusted enduser
data"...Yeah, it looks like we are using strtoul() without additional error checking
in similar situations, so here is a patch doing it like that.- true, isDbDir ? pg_atoi(lastDir + 1, sizeof(Oid), 0) : InvalidOid); + true, isDbDir ? (Oid) strtoul(lastDir + 1, NULL, 10) : InvalidOid);Looking at some other code, I just discovered the atooid() macro that already
does the same, maybe it'd be better for consistency to use that instead?+1. Whie it does the same thing, consistency is good! :)
committed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jan 13, 2020 at 1:49 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
On 2020-01-11 17:47, Magnus Hagander wrote:
On Sat, Jan 11, 2020 at 5:44 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
On Sat, Jan 11, 2020 at 08:21:11AM +0100, Peter Eisentraut wrote:
On 2020-01-06 21:00, Magnus Hagander wrote:
+0.5 to avoid calling OidInputFunctionCall()
Or just directly using atol() instead of atoi()? Well maybe not
directly but in a small wrapper that verifies it's not bigger than an
unsigned?Unlike in cases where we use oidin etc, we are dealing with data that
is "mostly trusted" here, aren't we? Meaning we could call atol() on
it, and throw an error if it overflows, and be done with it?
Subdirectories in the data directory aren't exactly "untrusted enduser
data"...Yeah, it looks like we are using strtoul() without additional error checking
in similar situations, so here is a patch doing it like that.- true, isDbDir ? pg_atoi(lastDir + 1, sizeof(Oid), 0) : InvalidOid); + true, isDbDir ? (Oid) strtoul(lastDir + 1, NULL, 10) : InvalidOid);Looking at some other code, I just discovered the atooid() macro that already
does the same, maybe it'd be better for consistency to use that instead?+1. Whie it does the same thing, consistency is good! :)
committed
Thanks!