Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eZjX0-0003ie-2Q for pgsql-hackers@arkaria.postgresql.org; Thu, 11 Jan 2018 20:32:58 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eZjWz-0007kv-C1 for pgsql-hackers@arkaria.postgresql.org; Thu, 11 Jan 2018 20:32:57 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1eZjWz-0007kl-48 for pgsql-hackers@lists.postgresql.org; Thu, 11 Jan 2018 20:32:57 +0000 Received: from tamriel.snowman.net ([96.255.250.162]) by magus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1eZjWu-0001cz-SU for pgsql-hackers@postgresql.org; Thu, 11 Jan 2018 20:32:56 +0000 Received: by tamriel.snowman.net (Postfix, from userid 1000) id 02B6D5F79C; Thu, 11 Jan 2018 15:32:50 -0500 (EST) Date: Thu, 11 Jan 2018 15:32:50 -0500 From: Stephen Frost To: Tom Lane Cc: Robert Haas , Amit Kapila , Ildar Musin , Pg Hackers Subject: Re: IndexTupleDSize macro seems redundant Message-ID: <20180111203250.GR2416@tamriel.snowman.net> References: <0274288e-9e88-13b6-c61c-7b36928bf221@postgrespro.ru> <20180111172241.GK2416@tamriel.snowman.net> <8984.1515692863@sss.pgh.pa.us> <20180111181713.GN2416@tamriel.snowman.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="4PfAL0PD1CaqNo98" Content-Disposition: inline In-Reply-To: <20180111181713.GN2416@tamriel.snowman.net> User-Agent: Mutt/1.5.24 (2015-08-30) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --4PfAL0PD1CaqNo98 Content-Type: multipart/mixed; boundary="WcEzarxnNIg1d+Go" Content-Disposition: inline --WcEzarxnNIg1d+Go Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Greetings Tom, Robert, Ildar, all, * Stephen Frost (sfrost@snowman.net) wrote: > That said, since it's not aligned, regardless of the what craziness the > compiler might try to pull, we probably shouldn't go casting it > to something that later hackers might think will be aligned, but we > should add a comment to clarify that it's not aligned and that we can't > act like it is. Updated (combined) patch attached for review. I went through and looked again to make sure there weren't any cases of making an unaligned pointer to a struct and didn't see any, and I added some comments to _bt_restore_page(). Thanks! Stephen --WcEzarxnNIg1d+Go Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="remove-indextupledsize-v4.patch" Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/= hash_xlog.c new file mode 100644 index b38208e..ab5aaff *** a/src/backend/access/hash/hash_xlog.c --- b/src/backend/access/hash/hash_xlog.c *************** hash_xlog_move_page_contents(XLogReaderS *** 558,564 **** Size itemsz; OffsetNumber l; =20 ! itemsz =3D IndexTupleDSize(*itup); itemsz =3D MAXALIGN(itemsz); =20 data +=3D itemsz; --- 558,564 ---- Size itemsz; OffsetNumber l; =20 ! itemsz =3D IndexTupleSize(itup); itemsz =3D MAXALIGN(itemsz); =20 data +=3D itemsz; *************** hash_xlog_squeeze_page(XLogReaderState * *** 686,692 **** Size itemsz; OffsetNumber l; =20 ! itemsz =3D IndexTupleDSize(*itup); itemsz =3D MAXALIGN(itemsz); =20 data +=3D itemsz; --- 686,692 ---- Size itemsz; OffsetNumber l; =20 ! itemsz =3D IndexTupleSize(itup); itemsz =3D MAXALIGN(itemsz); =20 data +=3D itemsz; diff --git a/src/backend/access/hash/hashinsert.c b/src/backend/access/hash= /hashinsert.c new file mode 100644 index f668dcf..f121286 *** a/src/backend/access/hash/hashinsert.c --- b/src/backend/access/hash/hashinsert.c *************** _hash_doinsert(Relation rel, IndexTuple *** 55,61 **** hashkey =3D _hash_get_indextuple_hashkey(itup); =20 /* compute item size too */ ! itemsz =3D IndexTupleDSize(*itup); itemsz =3D MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we * need to be consistent */ =20 --- 55,61 ---- hashkey =3D _hash_get_indextuple_hashkey(itup); =20 /* compute item size too */ ! itemsz =3D IndexTupleSize(itup); itemsz =3D MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we * need to be consistent */ =20 *************** restart_insert: *** 222,228 **** XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD); =20 XLogRegisterBuffer(0, buf, REGBUF_STANDARD); ! XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup)); =20 recptr =3D XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT); =20 --- 222,228 ---- XLogRegisterBuffer(1, metabuf, REGBUF_STANDARD); =20 XLogRegisterBuffer(0, buf, REGBUF_STANDARD); ! XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup)); =20 recptr =3D XLogInsert(RM_HASH_ID, XLOG_HASH_INSERT); =20 *************** _hash_pgaddmultitup(Relation rel, Buffer *** 309,315 **** { Size itemsize; =20 ! itemsize =3D IndexTupleDSize(*itups[i]); itemsize =3D MAXALIGN(itemsize); =20 /* Find where to insert the tuple (preserving page's hashkey ordering) = */ --- 309,315 ---- { Size itemsize; =20 ! itemsize =3D IndexTupleSize(itups[i]); itemsize =3D MAXALIGN(itemsize); =20 /* Find where to insert the tuple (preserving page's hashkey ordering) = */ diff --git a/src/backend/access/hash/hashovfl.c b/src/backend/access/hash/h= ashovfl.c new file mode 100644 index c9de128..d92ecd7 *** a/src/backend/access/hash/hashovfl.c --- b/src/backend/access/hash/hashovfl.c *************** readpage: *** 890,896 **** =20 itup =3D (IndexTuple) PageGetItem(rpage, PageGetItemId(rpage, roffnum)); ! itemsz =3D IndexTupleDSize(*itup); itemsz =3D MAXALIGN(itemsz); =20 /* --- 890,896 ---- =20 itup =3D (IndexTuple) PageGetItem(rpage, PageGetItemId(rpage, roffnum)); ! itemsz =3D IndexTupleSize(itup); itemsz =3D MAXALIGN(itemsz); =20 /* diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/h= ashpage.c new file mode 100644 index e3c8721..3859e3b *** a/src/backend/access/hash/hashpage.c --- b/src/backend/access/hash/hashpage.c *************** _hash_splitbucket(Relation rel, *** 1173,1179 **** * the current page in the new bucket, we must allocate a new * overflow page and place the tuple on that page instead. */ ! itemsz =3D IndexTupleDSize(*new_itup); itemsz =3D MAXALIGN(itemsz); =20 if (PageGetFreeSpaceForMultipleTuples(npage, nitups + 1) < (all_tups_= size + itemsz)) --- 1173,1179 ---- * the current page in the new bucket, we must allocate a new * overflow page and place the tuple on that page instead. */ ! itemsz =3D IndexTupleSize(new_itup); itemsz =3D MAXALIGN(itemsz); =20 if (PageGetFreeSpaceForMultipleTuples(npage, nitups + 1) < (all_tups_= size + itemsz)) diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c new file mode 100644 index 0d7bc68..42e75ec *** a/src/backend/access/heap/hio.c --- b/src/backend/access/heap/hio.c *************** RelationPutHeapTuple(Relation relation, *** 67,75 **** if (!token) { ItemId itemId =3D PageGetItemId(pageHeader, offnum); ! Item item =3D PageGetItem(pageHeader, itemId); =20 ! ((HeapTupleHeader) item)->t_ctid =3D tuple->t_self; } } =20 --- 67,75 ---- if (!token) { ItemId itemId =3D PageGetItemId(pageHeader, offnum); ! HeapTupleHeader item =3D (HeapTupleHeader) PageGetItem(pageHeader, item= Id); =20 ! item->t_ctid =3D tuple->t_self; } } =20 diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbt= ree/nbtinsert.c new file mode 100644 index 51059c0..2fe9867 *** a/src/backend/access/nbtree/nbtinsert.c --- b/src/backend/access/nbtree/nbtinsert.c *************** _bt_findinsertloc(Relation rel, *** 558,564 **** =20 lpageop =3D (BTPageOpaque) PageGetSpecialPointer(page); =20 ! itemsz =3D IndexTupleDSize(*newtup); itemsz =3D MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we * need to be consistent */ =20 --- 558,564 ---- =20 lpageop =3D (BTPageOpaque) PageGetSpecialPointer(page); =20 ! itemsz =3D IndexTupleSize(newtup); itemsz =3D MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we * need to be consistent */ =20 *************** _bt_insertonpg(Relation rel, *** 755,761 **** elog(ERROR, "cannot insert to incompletely split page %u", BufferGetBlockNumber(buf)); =20 ! itemsz =3D IndexTupleDSize(*itup); itemsz =3D MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we * need to be consistent */ =20 --- 755,761 ---- elog(ERROR, "cannot insert to incompletely split page %u", BufferGetBlockNumber(buf)); =20 ! itemsz =3D IndexTupleSize(itup); itemsz =3D MAXALIGN(itemsz); /* be safe, PageAddItem will do this but we * need to be consistent */ =20 *************** _bt_insertonpg(Relation rel, *** 914,920 **** sizeof(IndexTupleData)); } else ! XLogRegisterBufData(0, (char *) itup, IndexTupleDSize(*itup)); =20 recptr =3D XLogInsert(RM_BTREE_ID, xlinfo); =20 --- 914,920 ---- sizeof(IndexTupleData)); } else ! XLogRegisterBufData(0, (char *) itup, IndexTupleSize(itup)); =20 recptr =3D XLogInsert(RM_BTREE_ID, xlinfo); =20 diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtre= e/nbtsort.c new file mode 100644 index f6159db..4a909ea *** a/src/backend/access/nbtree/nbtsort.c --- b/src/backend/access/nbtree/nbtsort.c *************** _bt_buildadd(BTWriteState *wstate, BTPag *** 468,474 **** last_off =3D state->btps_lastoff; =20 pgspc =3D PageGetFreeSpace(npage); ! itupsz =3D IndexTupleDSize(*itup); itupsz =3D MAXALIGN(itupsz); =20 /* --- 468,474 ---- last_off =3D state->btps_lastoff; =20 pgspc =3D PageGetFreeSpace(npage); ! itupsz =3D IndexTupleSize(itup); itupsz =3D MAXALIGN(itupsz); =20 /* diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtre= e/nbtxlog.c new file mode 100644 index bed1dd2..454bfe2 *** a/src/backend/access/nbtree/nbtxlog.c --- b/src/backend/access/nbtree/nbtxlog.c *************** _bt_restore_page(Page page, char *from, *** 51,59 **** i =3D 0; while (from < end) { ! /* Need to copy tuple header due to alignment considerations */ memcpy(&itupdata, from, sizeof(IndexTupleData)); ! itemsz =3D IndexTupleDSize(itupdata); itemsz =3D MAXALIGN(itemsz); =20 items[i] =3D (Item) from; --- 51,65 ---- i =3D 0; while (from < end) { ! /*=20 ! * As we step through the items, 'from' won't always be properly ! * aligned, so we need to use memcpy(). Further, we use Item (which ! * is just a char*) here for our items array for the same reason; ! * wouldn't want the compiler or anyone thinking that an item is ! * aligned when it isn't. ! */ memcpy(&itupdata, from, sizeof(IndexTupleData)); ! itemsz =3D IndexTupleSize(&itupdata); itemsz =3D MAXALIGN(itemsz); =20 items[i] =3D (Item) from; *************** btree_xlog_split(bool onleft, XLogReader *** 205,211 **** BTPageOpaque ropaque; char *datapos; Size datalen; ! Item left_hikey =3D NULL; Size left_hikeysz =3D 0; BlockNumber leftsib; BlockNumber rightsib; --- 211,217 ---- BTPageOpaque ropaque; char *datapos; Size datalen; ! IndexTuple left_hikey =3D NULL; Size left_hikeysz =3D 0; BlockNumber leftsib; BlockNumber rightsib; *************** btree_xlog_split(bool onleft, XLogReader *** 248,254 **** { ItemId hiItemId =3D PageGetItemId(rpage, P_FIRSTDATAKEY(ropaque)); =20 ! left_hikey =3D PageGetItem(rpage, hiItemId); left_hikeysz =3D ItemIdGetLength(hiItemId); } =20 --- 254,260 ---- { ItemId hiItemId =3D PageGetItemId(rpage, P_FIRSTDATAKEY(ropaque)); =20 ! left_hikey =3D (IndexTuple) PageGetItem(rpage, hiItemId); left_hikeysz =3D ItemIdGetLength(hiItemId); } =20 *************** btree_xlog_split(bool onleft, XLogReader *** 272,278 **** Page lpage =3D (Page) BufferGetPage(lbuf); BTPageOpaque lopaque =3D (BTPageOpaque) PageGetSpecialPointer(lpage); OffsetNumber off; ! Item newitem =3D NULL; Size newitemsz =3D 0; Page newlpage; OffsetNumber leftoff; --- 278,284 ---- Page lpage =3D (Page) BufferGetPage(lbuf); BTPageOpaque lopaque =3D (BTPageOpaque) PageGetSpecialPointer(lpage); OffsetNumber off; ! IndexTuple newitem =3D NULL; Size newitemsz =3D 0; Page newlpage; OffsetNumber leftoff; *************** btree_xlog_split(bool onleft, XLogReader *** 281,287 **** =20 if (onleft) { ! newitem =3D (Item) datapos; newitemsz =3D MAXALIGN(IndexTupleSize(newitem)); datapos +=3D newitemsz; datalen -=3D newitemsz; --- 287,293 ---- =20 if (onleft) { ! newitem =3D (IndexTuple) datapos; newitemsz =3D MAXALIGN(IndexTupleSize(newitem)); datapos +=3D newitemsz; datalen -=3D newitemsz; *************** btree_xlog_split(bool onleft, XLogReader *** 290,296 **** /* Extract left hikey and its size (assuming 16-bit alignment) */ if (!isleaf) { ! left_hikey =3D (Item) datapos; left_hikeysz =3D MAXALIGN(IndexTupleSize(left_hikey)); datapos +=3D left_hikeysz; datalen -=3D left_hikeysz; --- 296,302 ---- /* Extract left hikey and its size (assuming 16-bit alignment) */ if (!isleaf) { ! left_hikey =3D (IndexTuple) datapos; left_hikeysz =3D MAXALIGN(IndexTupleSize(left_hikey)); datapos +=3D left_hikeysz; datalen -=3D left_hikeysz; *************** btree_xlog_split(bool onleft, XLogReader *** 301,307 **** =20 /* Set high key */ leftoff =3D P_HIKEY; ! if (PageAddItem(newlpage, left_hikey, left_hikeysz, P_HIKEY, false, false) =3D=3D InvalidOffsetNumber) elog(PANIC, "failed to add high key to left page after split"); leftoff =3D OffsetNumberNext(leftoff); --- 307,313 ---- =20 /* Set high key */ leftoff =3D P_HIKEY; ! if (PageAddItem(newlpage, (Item) left_hikey, left_hikeysz, P_HIKEY, false, false) =3D=3D InvalidOffsetNumber) elog(PANIC, "failed to add high key to left page after split"); leftoff =3D OffsetNumberNext(leftoff); *************** btree_xlog_split(bool onleft, XLogReader *** 310,321 **** { ItemId itemid; Size itemsz; ! Item item; =20 /* add the new item if it was inserted on left page */ if (onleft && off =3D=3D xlrec->newitemoff) { ! if (PageAddItem(newlpage, newitem, newitemsz, leftoff, false, false) =3D=3D InvalidOffsetNumber) elog(ERROR, "failed to add new item to left page after split"); leftoff =3D OffsetNumberNext(leftoff); --- 316,327 ---- { ItemId itemid; Size itemsz; ! IndexTuple item; =20 /* add the new item if it was inserted on left page */ if (onleft && off =3D=3D xlrec->newitemoff) { ! if (PageAddItem(newlpage, (Item) newitem, newitemsz, leftoff, false, false) =3D=3D InvalidOffsetNumber) elog(ERROR, "failed to add new item to left page after split"); leftoff =3D OffsetNumberNext(leftoff); *************** btree_xlog_split(bool onleft, XLogReader *** 323,330 **** =20 itemid =3D PageGetItemId(lpage, off); itemsz =3D ItemIdGetLength(itemid); ! item =3D PageGetItem(lpage, itemid); ! if (PageAddItem(newlpage, item, itemsz, leftoff, false, false) =3D=3D InvalidOffsetNumber) elog(ERROR, "failed to add old item to left page after split"); leftoff =3D OffsetNumberNext(leftoff); --- 329,336 ---- =20 itemid =3D PageGetItemId(lpage, off); itemsz =3D ItemIdGetLength(itemid); ! item =3D (IndexTuple) PageGetItem(lpage, itemid); ! if (PageAddItem(newlpage, (Item) item, itemsz, leftoff, false, false) =3D=3D InvalidOffsetNumber) elog(ERROR, "failed to add old item to left page after split"); leftoff =3D OffsetNumberNext(leftoff); *************** btree_xlog_split(bool onleft, XLogReader *** 333,339 **** /* cope with possibility that newitem goes at the end */ if (onleft && off =3D=3D xlrec->newitemoff) { ! if (PageAddItem(newlpage, newitem, newitemsz, leftoff, false, false) =3D=3D InvalidOffsetNumber) elog(ERROR, "failed to add new item to left page after split"); leftoff =3D OffsetNumberNext(leftoff); --- 339,345 ---- /* cope with possibility that newitem goes at the end */ if (onleft && off =3D=3D xlrec->newitemoff) { ! if (PageAddItem(newlpage, (Item) newitem, newitemsz, leftoff, false, false) =3D=3D InvalidOffsetNumber) elog(ERROR, "failed to add new item to left page after split"); leftoff =3D OffsetNumberNext(leftoff); diff --git a/src/include/access/itup.h b/src/include/access/itup.h new file mode 100644 index 0ffa91d..9be3442 *** a/src/include/access/itup.h --- b/src/include/access/itup.h *************** typedef IndexAttributeBitMapData * Index *** 67,74 **** #define INDEX_VAR_MASK 0x4000 #define INDEX_NULL_MASK 0x8000 =20 ! #define IndexTupleSize(itup) ((Size) (((IndexTuple) (itup))->t_info & IN= DEX_SIZE_MASK)) ! #define IndexTupleDSize(itup) ((Size) ((itup).t_info & INDEX_SIZE_MASK)) #define IndexTupleHasNulls(itup) ((((IndexTuple) (itup))->t_info & INDEX_= NULL_MASK)) #define IndexTupleHasVarwidths(itup) ((((IndexTuple) (itup))->t_info & IN= DEX_VAR_MASK)) =20 --- 67,73 ---- #define INDEX_VAR_MASK 0x4000 #define INDEX_NULL_MASK 0x8000 =20 ! #define IndexTupleSize(itup) ((Size) ((itup)->t_info & INDEX_SIZE_MASK)) #define IndexTupleHasNulls(itup) ((((IndexTuple) (itup))->t_info & INDEX_= NULL_MASK)) #define IndexTupleHasVarwidths(itup) ((((IndexTuple) (itup))->t_info & IN= DEX_VAR_MASK)) =20 --WcEzarxnNIg1d+Go-- --4PfAL0PD1CaqNo98 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJaV8nyAAoJEO1sijiDR2RV838P/jBHYdWehrhCJl/qQ+TQZcGj X3mskyd+JhCQP2XAxeJJRdneJ+v/WHZBeYy/RwtQV74ZsDkOzkTiMgTp6ZkaK6X6 0J0fVQ323TU020IV7XQROu1l1+qxz584Fa+5SI2Lol4R+PSnHU+E0ARkt2dzTJhh IbIFZtcxw6artIkcPQ6Qd213xQdqP/9u66+m28FWtRJHqWPc9p6LJ2QR4iZ/vKG7 RQ1RtPOX5YL0m5XCI+PEaKJVsaRRVhnyPvGWZLcKoEEur6GKZ0b+3Y9KxD20TuJd iAFHXWx5cFBeSsgxa4wuOl6puH71EV97hjSuOl6qeS2pkMymZW6NisOI2y7ZJ22d pNkGs1aO8f/whdNPF4rZXLrT16FVDDv8p1EFmrNc+xtYZmSUxT2xXjjaV8HYHBkh benmvV/q7l30ZarKiOexMgX2SGXgiOkXo6gsRxVGBxeQO3aeMpBFKjQT4oetF4EG i1vnn2gEwNQ0YMurdUdXYC4vZTCSuSdWUsiTRgE8pWg0EqZhzdmJuESf7Wck61PM 4UF2d88LQIu+nGl5HAInMhC+hYMjrk4b3C06q7bZo02IYXZJKHYTFB2dmwm3qcP/ MhKaDr/4ssoXUx9MLCgZ3A2YWMR8to4bmDO/I52X3QWeMBoTrLL3rP0VkHHenpH6 TNZCGyjXYgM3C17TLZS9 =fijS -----END PGP SIGNATURE----- --4PfAL0PD1CaqNo98--