Horton

Thread

  1. DatumGetInetP buggy

    Boszormenyi Zoltan <zb@cybertec.at> — 2011-11-08T09:22:38Z

    Hi,

I wanted to do some transformation on an inet value from
an SPI-using function. The inet Datum passed from SPI_getbinval()
to the values array in heap_form_tuple() obviously gives good data
to the frontend. When I use DatumGetInetP() on the Datum,
the structure passed to me is corrupt:

zozo=# select * from inet_test() as (id integer, i1 inet, i2 inet);
NOTICE:  i1 family=CORRUPTED
NOTICE:  i1 family=CORRUPTED
NOTICE:  i1 family=CORRUPTED
 id |     i1      |      i2      
----+-------------+---------------
  1 | 192.168.0.1 | 192.168.0.101
  2 | 192.168.0.2 | 192.168.0.102
  3 | 192.168.0.3 | 192.168.0.103
(3 rows)

I looked at utils/inet.h and DatumGetInetP() uses PG_DETOAST_DATUM_PACKED().
fmgr.h warns about PG_DETOAST_DATUM_PACKED() that it may give you
an unaligned pointer. Indeed, using PG_DETOAST_DATUM() instead of the
_PACKED variant on the Datum give me a well formed inet structure:

zozo=# select * from inet_test() as (id integer, i1 inet, i2 inet);
NOTICE:  i1 family=AF_INET
NOTICE:  i1 netmask=32 bits
NOTICE:  i1 address=192.168.0.1
NOTICE:  i1 family=AF_INET
NOTICE:  i1 netmask=32 bits
NOTICE:  i1 address=192.168.0.2
NOTICE:  i1 family=AF_INET
NOTICE:  i1 netmask=32 bits
NOTICE:  i1 address=192.168.0.3
 id |     i1      |      i2      
----+-------------+---------------
  1 | 192.168.0.1 | 192.168.0.101
  2 | 192.168.0.2 | 192.168.0.102
  3 | 192.168.0.3 | 192.168.0.103
(3 rows)

System is Fedora 16/x86_64, PostgreSQL 9.1.1 as provided by the OS.
The same error occurs on PostgreSQL 9.0.4 on another system
which is also Linux/x86_64

Example code is attached, the tables used by the code are:

create table t1 (id serial primary key, i1 inet);
create table t2 (id serial primary key, id2 integer references t1(id), i2 inet);
insert into t1 (i1) values ('192.168.0.1');
insert into t1 (i1) values ('192.168.0.2');
insert into t1 (i1) values ('192.168.0.3');
insert into t2 (id2, i2) values (1, '192.168.0.101');
insert into t2 (id2, i2) values (2, '192.168.0.102');
insert into t2 (id2, i2) values (3, '192.168.0.103');

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
     http://www.postgresql.at/

  2. Re: DatumGetInetP buggy

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-11-08T12:15:52Z

    On 08.11.2011 11:22, Boszormenyi Zoltan wrote:
> Hi,
>
> I wanted to do some transformation on an inet value from
> an SPI-using function. The inet Datum passed from SPI_getbinval()
> to the values array in heap_form_tuple() obviously gives good data
> to the frontend. When I use DatumGetInetP() on the Datum,
> the structure passed to me is corrupt:
>
> zozo=# select * from inet_test() as (id integer, i1 inet, i2 inet);
> NOTICE:  i1 family=CORRUPTED
> NOTICE:  i1 family=CORRUPTED
> NOTICE:  i1 family=CORRUPTED
>   id |     i1      |      i2
> ----+-------------+---------------
>    1 | 192.168.0.1 | 192.168.0.101
>    2 | 192.168.0.2 | 192.168.0.102
>    3 | 192.168.0.3 | 192.168.0.103
> (3 rows)
>
> I looked at utils/inet.h and DatumGetInetP() uses PG_DETOAST_DATUM_PACKED().
> fmgr.h warns about PG_DETOAST_DATUM_PACKED() that it may give you
> an unaligned pointer. Indeed, using PG_DETOAST_DATUM() instead of the
> _PACKED variant on the Datum give me a well formed inet structure:

Hmm, it seems to be intentional, but I agree it's very much contrary to 
the usual convention that DatumGetXXXP() returns a detoasted and 
depacked datum. I think we should change it. I propose the attached 
patch. It changes DatumGetInetP() to do PG_DETOAST_DATUM(), and adds new 
DatumGetInetPP() macro to return the packed version. I also moved the 
access macros like ip_family() from network.c to inet.h, so that they're 
available for whoever wants to look at the fields without having to depack.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  3. Re: DatumGetInetP buggy

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-11-08T15:06:26Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Hmm, it seems to be intentional, but I agree it's very much contrary to 
> the usual convention that DatumGetXXXP() returns a detoasted and 
> depacked datum. I think we should change it. I propose the attached 
> patch. It changes DatumGetInetP() to do PG_DETOAST_DATUM(), and adds new 
> DatumGetInetPP() macro to return the packed version. I also moved the 
> access macros like ip_family() from network.c to inet.h, so that they're 
> available for whoever wants to look at the fields without having to depack.

No objection to making the DatumGet macro names conform to common
convention, but I'm not thrilled with moving those special-purpose
accessor macros into wider circulation.  It's not necessary and the
macros don't work unless used in a particular way per the comment,
so I don't think they can be considered general purpose.

			regards, tom lane

  4. Re: DatumGetInetP buggy

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-11-08T17:53:03Z

    On 08.11.2011 17:06, Tom Lane wrote:
> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  writes:
>> Hmm, it seems to be intentional, but I agree it's very much contrary to
>> the usual convention that DatumGetXXXP() returns a detoasted and
>> depacked datum. I think we should change it. I propose the attached
>> patch. It changes DatumGetInetP() to do PG_DETOAST_DATUM(), and adds new
>> DatumGetInetPP() macro to return the packed version. I also moved the
>> access macros like ip_family() from network.c to inet.h, so that they're
>> available for whoever wants to look at the fields without having to depack.
>
> No objection to making the DatumGet macro names conform to common
> convention, but I'm not thrilled with moving those special-purpose
> accessor macros into wider circulation.  It's not necessary and the
> macros don't work unless used in a particular way per the comment,
> so I don't think they can be considered general purpose.

Ok.

What do people think of backpatching this? I'm inclined to backpatch, on 
the grounds that the macro that detoasts and depacks should always work 
(ie. after this patch), even if the depacking isn't necessary and 
introduces an extra palloc+copy, but the reverse is not true.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

  5. Re: DatumGetInetP buggy

    Boszormenyi Zoltan <zb@cybertec.at> — 2011-11-08T18:46:21Z

    2011-11-08 18:53 keltezéssel, Heikki Linnakangas írta:
> On 08.11.2011 17:06, Tom Lane wrote:
>> Heikki Linnakangas<heikki.linnakangas@enterprisedb.com>  writes:
>>> Hmm, it seems to be intentional, but I agree it's very much contrary to
>>> the usual convention that DatumGetXXXP() returns a detoasted and
>>> depacked datum. I think we should change it. I propose the attached
>>> patch. It changes DatumGetInetP() to do PG_DETOAST_DATUM(), and adds new
>>> DatumGetInetPP() macro to return the packed version. I also moved the
>>> access macros like ip_family() from network.c to inet.h, so that they're
>>> available for whoever wants to look at the fields without having to depack.
>>
>> No objection to making the DatumGet macro names conform to common
>> convention, but I'm not thrilled with moving those special-purpose
>> accessor macros into wider circulation.  It's not necessary and the
>> macros don't work unless used in a particular way per the comment,
>> so I don't think they can be considered general purpose.
>
> Ok.
>
> What do people think of backpatching this? I'm inclined to backpatch, on the grounds
> that the macro that detoasts and depacks should always work (ie. after this patch), even
> if the depacking isn't necessary and introduces an extra palloc+copy, but the reverse is
> not true.

On the grounds that 9.0.x also shows the problem with
the stock macro, backporting would be nice. I don't know
which older trees may show the problem, I only tested with
9.0 and 9.1.

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
     http://www.postgresql.at/

  6. Re: DatumGetInetP buggy

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-11-08T20:52:43Z

    On 08.11.2011 20:46, Boszormenyi Zoltan wrote:
> 2011-11-08 18:53 keltezéssel, Heikki Linnakangas írta:
>> What do people think of backpatching this? I'm inclined to backpatch, on the grounds
>> that the macro that detoasts and depacks should always work (ie. after this patch), even
>> if the depacking isn't necessary and introduces an extra palloc+copy, but the reverse is
>> not true.
>
> On the grounds that 9.0.x also shows the problem with
> the stock macro, backporting would be nice. I don't know
> which older trees may show the problem, I only tested with
> 9.0 and 9.1.

Packed varlenas were introduced in 8.3 - this hasn't changed since then. 
Committed and backpatched all the way to 8.3.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com