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

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 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().

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(at)paquier(dot)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 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().
>
> 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(at)gmail(dot)com> wrote:
>
> On Mon, Jan 6, 2020 at 9:21 AM Michael Paquier <michael(at)paquier(dot)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 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().
> >
> > 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

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(at)gmail(dot)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(at)gmail(dot)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(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> On 2020-01-11 17:47, Magnus Hagander wrote:
> > On Sat, Jan 11, 2020 at 5:44 PM Julien Rouhaud <rjuju123(at)gmail(dot)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!