Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize

On Tue, 30 Apr 2024 at 02:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alexander Lakhin <exclusion@gmail.com> writes:
> > 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.

I forgot about INCLUDE.  Thanks.

> 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.

I did that research in the form of setting a breakpoint in
index_deform_tuple() and verified the tup->t_info & 0xFFF grows by
64-bytes for each extra name column I INCLUDE.  Also verified those
extra bytes are all zero'd

> 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?

I think this is a good idea.  It seems reasonable that some other AM
might want to do the same thing. Also, it allows me to choose a better
name for these new IndexOnlyScanState fields. ioss_NameCStringAttNums
and ioss_NameCStringCount seems more on point. However, I couldn't
really decide which way around to have Name and CString.

I added a regression test to index_including.sql to give this some coverage.

David