Date: Thu, 11 Jan 2018 13:17:13 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Ildar Musin <i.musin@postgrespro.ru>,
	Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: IndexTupleDSize macro seems redundant
Message-ID: <20180111181713.GN2416@tamriel.snowman.net>
References: <0274288e-9e88-13b6-c61c-7b36928bf221@postgrespro.ru>
 <CAA4eK1+qNuT2iY+gL-yi-Sn-aj=2y7hBD3GFFRrxh6hV-U+X6w@mail.gmail.com>
 <CA+TgmoaBZi-oNU4dK-FBDeqRWijxYKc5Lne2hD7+bn+sXKuT3g@mail.gmail.com>
 <20180111172241.GK2416@tamriel.snowman.net>
 <8984.1515692863@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="wNtmCi+hDt54JonX"
Content-Disposition: inline
In-Reply-To: <8984.1515692863@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--wNtmCi+hDt54JonX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > I'll leave the patch status in 'Needs review' since there's more
> > changes, but hopefully someone can take a look and we can move this
> > along, seems like a pretty small and reasonable improvement.
>=20
> I'm on board with Stephen's changes, except in _bt_restore_page.
> The issue there is that the "from" pointer isn't necessarily adequately
> aligned to be considered an IndexTuple pointer; that's why we're doing
> the memcpy dance to get a length out of it.  "Item" doesn't connote
> anything about alignment (it's the same as Pointer, ie char*).  Even
> though we don't do anything with items[i] except pass it as an Item
> to PageAddItem, the proposed change could result in breakage, because
> the compiler could take it as license to assume that "from" is aligned,
> and perhaps change what it generates for the memcpy.

I certainly hadn't been thinking about that.  I didn't see any
issues in my testing (where I created a table with a btree index and
insert'd a bunch of records into and then killed the server, forcing WAL
replay and then checked that the index appeared to be valid using order
by queries; perhaps I should have tried amcheck, but doesn't sound like
this is something that would have been guaranteed to break anyway).

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.

> I think that in the other places where Stephen wants to change Item
> to something else, the alignment expectation actually does hold,
> so we're OK if we want to do it in those places.

Yes, everywhere else it's a pointer returned from PageGetItem() or
XLogRecGetBlockData() (which always returns a MAXALIGNed pointer).

Thanks!

Stephen

--wNtmCi+hDt54JonX
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJaV6opAAoJEO1sijiDR2RVB14P/1LrKLrLSSN32lZuJoQDmeEV
kFihyhfqMXWrF/JydviompxXoR9+GGxeGYwtMgYTuh6m2DBAX9FgOK+vw4X+NsJ2
uTHP5UcgW9TjVmIU1pC3oJgcjnYKysjPK9HOMEQ1IMt6rfVe3tfNwk49Y4sTNSaB
C9hf5ctAN7NUMEZRmddWlJAeLrkljknA3UDnbtm9eZ7MugqWpiOM7nEEKMBVqlCT
52N7q/y86IJxaW/NHthkAUxdPDBkJQTNvnYEwU+S7PCkceZY8kGMQgCljB/0WSRN
X7KorUbKRnBo417Om8doAYG3l8q4Y4z3y3c8ENj8Kqi8Nr7XgWDpu4jv6KCjO1Gq
lkYPnYou8v2oFjl7Tv56kIYWi66iKAySmwNDhnRPw6oSCw63SGKrksL8yM4SlN1k
lgHvsvDBab+7KAJ0Uy8L6+vyHMM3I0I0mPbukamyuAA9kQQSHnDTUZ5FBVZmHpie
hTl/SBMfE2banaYRP/9h61VEK9CItgx8pANJ2vPzefGSbUtb5H+TyVCfv12G8Kgh
nEcD6z6gwpBIdivUp35Z+5dKg+oWrj2Ir/9I2oWp717WByy7YG/QLSSuWUAMA6zI
spOFb7o+Srdy+d9KGRLd1RthTGMrK6Ezsj3RuynPHatRJXKoq9e4HhWUGyuqfATi
Cx/8AiFT5Gxt6lh5BdYd
=kKxa
-----END PGP SIGNATURE-----

--wNtmCi+hDt54JonX--