Обсуждение: DatumGetInetP buggy
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/
Вложения
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
Вложения
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
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
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/
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