[HACKERS] 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/
inet-test.tgz
Description: application/compressed-tar
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DatumGetInetP buggy
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
diff --git a/src/backend/utils/adt/network.c b/src/backend/utils/adt/network.c
index 9aca1cc..a276d04 100644
--- a/src/backend/utils/adt/network.c
+++ b/src/backend/utils/adt/network.c
@@ -29,37 +29,6 @@ static int ip_addrsize(inet *inetptr);
static inet *internal_inetpl(inet *ip, int64 addend);
/*
- * Access macros. We use VARDATA_ANY so that we can process short-header
- * varlena values without detoasting them. This requires a trick:
- * VARDATA_ANY assumes the varlena header is already filled in, which is
- * not the case when constructing a new value (until SET_INET_VARSIZE is
- * called, which we typically can't do till the end). Therefore, we
- * always initialize the newly-allocated value to zeroes (using palloc0).
- * A zero length word will look like the not-1-byte case to VARDATA_ANY,
- * and so we correctly construct an uncompressed value.
- *
- * Note that ip_maxbits() and SET_INET_VARSIZE() require
- * the family field to be set correctly.
- */
-
-#define ip_family(inetptr) \
- (((inet_struct *) VARDATA_ANY(inetptr))-family)
-
-#define ip_bits(inetptr) \
- (((inet_struct *) VARDATA_ANY(inetptr))-bits)
-
-#define ip_addr(inetptr) \
- (((inet_struct *) VARDATA_ANY(inetptr))-ipaddr)
-
-#define ip_maxbits(inetptr) \
- (ip_family(inetptr) == PGSQL_AF_INET ? 32 : 128)
-
-#define SET_INET_VARSIZE(dst) \
- SET_VARSIZE(dst, VARHDRSZ + offsetof(inet_struct, ipaddr) + \
-ip_addrsize(dst))
-
-
-/*
* Return the number of bytes of address storage needed for this data type.
*/
static int
@@ -907,7 +876,7 @@ convert_network_to_scalar(Datum value, Oid typid)
case INETOID:
case CIDROID:
{
-inet *ip = DatumGetInetP(value);
+inet *ip = DatumGetInetPP(value);
int len;
double res;
int i;
diff --git a/src/include/utils/inet.h b/src/include/utils/inet.h
index 9626a2d..7cb7337 100644
--- a/src/include/utils/inet.h
+++ b/src/include/utils/inet.h
@@ -53,6 +53,36 @@ typedef struct
inet_struct inet_data;
} inet;
+/*
+ * Access macros. We use VARDATA_ANY so that we can process short-header
+ * varlena values without detoasting them. This requires a trick:
+ * VARDATA_ANY assumes the varlena header is already filled in, which is
+ * not the case when constructing a new value (until SET_INET_VARSIZE is
+ * called, which we typically can't do till the end). Therefore, we
+ * always initialize the newly-allocated value to zeroes (using palloc0).
+ * A zero length word will look like the not-1-byte case to VARDATA_ANY,
+ * and so we correctly construct an uncompressed value.
+ *
+ * Note that ip_maxbits() and SET_INET_VARSIZE() require
+ * the family field to be set correctly.
+ */
+
+#define ip_family(inetptr) \
+ (((inet_struct *) VARDATA_ANY(inetptr))-family)
+
+#define ip_bits(inetptr) \
+ (((inet_struct *) VARDATA_ANY(inetptr))-bits)
+
+#define ip_addr(inetptr) \
+ (((inet_struct *) VARDATA_ANY(inetptr))-ipaddr)
+
+#define ip_maxbits(inetptr) \
+ (ip_family(inetptr) == PGSQL_AF_INET ? 32 : 128)
+
+#define SET_INET_VARSIZE(dst) \
+ SET_VARSIZE(dst, VARHDRSZ + offsetof(inet_struct, ipaddr) + \
+ip_addrsize(dst))
+
/*
* This is the internal storage format for MAC addresses:
@@ -70,9 +100,11 @@ typedef struct macaddr
/*
* fmgr interface macros
*/
-#define DatumGetInetP(X) ((inet *) PG_DETOAST_DATUM_PACKED(X))
+#define DatumGetInetP(X) ((inet *) PG_DETOAST_DATUM(X))
+#define DatumGetInetPP(X) ((inet *) PG_DETOAST_DATUM_PACKED(X))
#define
Re: [HACKERS] DatumGetInetP buggy
Heikki Linnakangas heikki.linnakan...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DatumGetInetP buggy
On 08.11.2011 17:06, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DatumGetInetP buggy
2011-11-08 18:53 keltezéssel, Heikki Linnakangas írta: On 08.11.2011 17:06, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] DatumGetInetP buggy
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
