From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Lakhin <exclusion@gmail.com>
cc: David Rowley <dgrowleyml@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17855: Uninitialised memory used when the name type value
 processed in binary mode of Memoize
In-reply-to: <c898e301-ea62-e551-bfbc-2b7579150c14@gmail.com>
References: <17855-5f523e0f9769a566@postgresql.org>
 <CAApHDvqwwdO1EAo-6osu-LtTk1SD2iQjLQXhVd7Wamc-JfhfrQ@mail.gmail.com>
 <CAApHDvokmLEriy3+4vB0KjCQip5pMq0g_JJX-wHNDvBJKukr6w@mail.gmail.com>
 <CAApHDvqNo_7yipfLjVXatUUosEkKZrdhHxAObPuxSat8UQaUTw@mail.gmail.com>
 <b2a382d6-d261-fdb6-176c-d0cdbf63dce8@gmail.com>
 <CAApHDvqqvkorexCuVkSJ16LcqGKfUdLAn3Y+YsqKNT3Pe56oOA@mail.gmail.com>
 <3943232.1714060238@sss.pgh.pa.us>
 <CAApHDvpW3ZEyTyogjD6qJsnuTFujLk3yTyCPX-to7VhftkrakQ@mail.gmail.com>
 <c898e301-ea62-e551-bfbc-2b7579150c14@gmail.com>
Comments: In-reply-to Alexander Lakhin <exclusion@gmail.com>
	message dated "Mon, 29 Apr 2024 17:00:00 +0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1228402.1714400922.1@sss.pgh.pa.us>
Date: Mon, 29 Apr 2024 10:28:42 -0400
Message-ID: <1228403.1714400922@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1228403.1714400922%40sss.pgh.pa.us>
Precedence: bulk

Alexander Lakhin <exclusion@gmail.com> writes:
> 29.04.2024 03:54, David Rowley wrote:
>> On looking at the relcache code, I don't see anywhere that we store
>> the opclass Oid in RelationData. IndexSupportInitialize() only records
>> the opcfamily and opcintype. Assuming we can't add new fields to
>> RelationData in the backbranches, is there a reason why we can't check
>> for rd_opfamily of TEXT_BTREE_FAM_OID and rd_opcintype of NAMEOID?

That should be all right, but ...

> With the v3 patch applied, `make check` fails for me under Valgrind:

... yeah, because those arrays are only of length indnkeyatts,
while the loop is trying to iterate over indnatts columns.

One bit of research that needs to be done is whether btree will
truncate an "include"'d column of type name.  I think it will not,
because that behavior is driven by the opclass and there isn't one
for an included column, but it wouldn't hurt to check.  If so,
just restricting these setup loops to consider only indnkeyatts
columns should fix this.

Something that's bothering me more is expressed in this comment
near the head of ExecInitIndexOnlyScan:

     * Build the scan tuple type using the indextlist generated by the
     * planner.  We use this, rather than the index's physical tuple
     * descriptor, because the latter contains storage column types not the
     * types of the original datums.  (It's the AM's responsibility to return
     * suitable data anyway.)

Per this, the system structure ought to be that btree makes this mess
and btree should clean it up.  Now maybe there's an argument that it's
better to do it in nodeIndexonlyscan.c in case any other index AM
wants to make a similar optimization ... but that seems unlikely to me
since "name" isn't really a general-purpose data type, and we don't
use anything but btree for the system catalogs.  Besides, this code
wouldn't work anyway because it's specific to a btree opfamily.
So I think we ought to look to see if there's someplace in nbtree
where this responsibility could be shoved.

On the other hand, looking at that comment again, I wonder if we could
drive this off the physical tuple descriptor containing atttypid
CSTRINGOID.  I don't say that's better than the v3 logic, but it's an
alternative to consider, especially if it'd let us avoid writing this
in a btree-specific way.  Maybe "storage type is cstring and
rd_opcintype is name" would be a sufficient test that would preserve
some independence from btree?

			regards, tom lane