Feedback-ID: i0fe9450f:Fastmail
Date: Sun, 1 Oct 2023 09:31:39 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18135: Incorrect memory access occurs when attaching a
 partition with an index
Message-ID: <ZRi96-kUPFy0ILzb@paquier.xyz>
References: <18135-532f4a755e71e4d2@postgresql.org>
 <1043711.1695922299@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="P+kJnpnCtTxqIV0g"
Content-Disposition: inline
In-Reply-To: <1043711.1695922299@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/ZRi96-kUPFy0ILzb%40paquier.xyz>
Precedence: bulk


--P+kJnpnCtTxqIV0g
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Sep 28, 2023 at 01:31:39PM -0400, Tom Lane wrote:
> Agreed, that's pretty broken, and it's not just that it risks an
> invalid access.  I don't think this reliably rejects cases where
> one index has an expression and the other doesn't.  Even if it does
> work, it's way too complicated to convince oneself that that's
> rejected.  I think for clarity we should code it as attached.

-        /* ignore expressions at this stage */
-        if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
-            (attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
-             info1->ii_IndexAttrNumbers[i]))
-            return false;
+        /* ignore expressions for now (but check their collation/opfamily) */
+        if (!(info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber &&
+              info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber))
+        {
+            /* fail if just one index has an expression in this column */
+            if (info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber ||
+                info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber)
+                return false;

I can see that this has already been committed as 9f71e10d65, but are
you sure that this is correct?  I didn't take the time to reply back,
because I got the feeling that even what I proposed is not correct.
The previous code was careful enough to look at the information from
info2 only *through* the attribute map, which is not what this new
code is doing.  Rather than looking directly at the elements in info2
at each iteration, shouldn't this stuff loop over the elements in
attmap->attnums for each info1->ii_IndexAttrNumbers[i]?
--
Michael

--P+kJnpnCtTxqIV0g
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmUYvesACgkQnvQgOdby
QH2trxAAiU/hJOnZLtkCmXhFeoBgwgrDQa5UFackkO7CivLzBk7WAZVKps4MbZpV
4CzN8/iB4lmrHoKeQg7WM28JUQIN0gv1UFfT03en050ot6SbcoD82yj2rz39k3rW
lpu7fTzuOaObSfVw4mSJy/KOCQD/hYJknRkgtsOXjxzOtRWSQDk05AQfPLV7KFHl
fGzl8o5v2wt3XQ0JE2U0TehAaYokUWVGj1vQNtdmaRhpw+GvdjHNkL/h4FO/SooO
xeNbYCWpU02rHRN2Q1LZZp5oc6lQ3QEJfyJw//RsIleTwBfPiZLMt++Ih334kVAy
R7Ds86naxHSb0Xz5H5JV0e+zqAQvOI5qtbfAcMc766ejGfbJb3se3a9kN207P1Jg
7Xuq5v4lr90LTWsJzvMfg7lpQlb8XBKSioiEyRf2o2b9vERH6brAeEclH2RKk04o
7a9BL2qNQCkplGTfPy6JeQOUdxEwcPpvbq3VMnRzq4vFdZYO6TGPlCUOpXX7B0da
hyDN+NYcqJPODmWNL6Jaj2k13HaW2wYFa86DeAMsNgt9lEnn6iOruvx4IVxAABgC
rkG5ARQ/CbSkWNTY+85Gx4wIRy5atRkNibkWqJUkIbgNn8Q/wWtEhryUWedou2+A
bceuM+jTlLflLKjwOpGaySXJjZfQJ5XfWvbYnLwX+pimutzAEH0=
=V1nh
-----END PGP SIGNATURE-----

--P+kJnpnCtTxqIV0g--