pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
19+ messages / 5 participants
[nested] [flat]
* BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
@ 2023-03-20 14:00 PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: PG Bug reporting form @ 2023-03-20 14:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 17855
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 15.2
Operating system: Ubuntu 22.04
Description:
When the following query executed under valgrind:
SET enable_seqscan TO off;
CREATE TABLE t (n name);
INSERT INTO t VALUES('name'), ('name');
CREATE INDEX t_idx ON t(n);
ANALYZE t;
SELECT COUNT(*) FROM t t1 INNER JOIN t t2 ON t1.n >= t2.n;
An incorrect memory access detected:
==00:00:00:06.975 2318810== Use of uninitialised value of size 8
==00:00:00:06.975 2318810== at 0x4348B9: memoize_insert_hash_internal
(simplehash.h:633)
==00:00:00:06.975 2318810== by 0x43515F: memoize_insert
(simplehash.h:763)
==00:00:00:06.975 2318810== by 0x43515F: cache_lookup
(nodeMemoize.c:519)
==00:00:00:06.975 2318810== by 0x435832: ExecMemoize
(nodeMemoize.c:705)
==00:00:00:06.975 2318810== by 0x40EA14: ExecProcNodeFirst
(execProcnode.c:464)
==00:00:00:06.975 2318810== by 0x43F32B: ExecProcNode (executor.h:259)
==00:00:00:06.975 2318810== by 0x43F32B: ExecNestLoop
(nodeNestloop.c:160)
==00:00:00:06.975 2318810== by 0x40EA14: ExecProcNodeFirst
(execProcnode.c:464)
==00:00:00:06.975 2318810== by 0x41A9BF: ExecProcNode (executor.h:259)
==00:00:00:06.975 2318810== by 0x41A9BF: fetch_input_tuple
(nodeAgg.c:563)
==00:00:00:06.975 2318810== by 0x41E2D4: agg_retrieve_direct
(nodeAgg.c:2346)
==00:00:00:06.975 2318810== by 0x41E5AA: ExecAgg (nodeAgg.c:2161)
==00:00:00:06.975 2318810== by 0x40EA14: ExecProcNodeFirst
(execProcnode.c:464)
==00:00:00:06.975 2318810== by 0x407008: ExecProcNode (executor.h:259)
==00:00:00:06.975 2318810== by 0x407008: ExecutePlan (execMain.c:1636)
==00:00:00:06.975 2318810== by 0x4071E8: standard_ExecutorRun
(execMain.c:363)
==00:00:00:06.975 2318810== Uninitialised value was created by a heap
allocation
==00:00:00:06.975 2318810== at 0x74394C: palloc (mcxt.c:1093)
==00:00:00:06.975 2318810== by 0x262F07: btrescan (nbtree.c:427)
==00:00:00:06.975 2318810== by 0x2511F6: index_rescan (indexam.c:314)
==00:00:00:06.975 2318810== by 0x42F4E7: IndexOnlyNext
(nodeIndexonlyscan.c:111)
==00:00:00:06.975 2318810== by 0x411CCD: ExecScanFetch (execScan.c:133)
==00:00:00:06.975 2318810== by 0x411D68: ExecScan (execScan.c:182)
==00:00:00:06.975 2318810== by 0x42F25F: ExecIndexOnlyScan
(nodeIndexonlyscan.c:315)
==00:00:00:06.975 2318810== by 0x40EA14: ExecProcNodeFirst
(execProcnode.c:464)
==00:00:00:06.975 2318810== by 0x43EEB8: ExecProcNode (executor.h:259)
==00:00:00:06.975 2318810== by 0x43EEB8: ExecNestLoop
(nodeNestloop.c:109)
==00:00:00:06.975 2318810== by 0x40EA14: ExecProcNodeFirst
(execProcnode.c:464)
==00:00:00:06.975 2318810== by 0x41A9BF: ExecProcNode (executor.h:259)
==00:00:00:06.975 2318810== by 0x41A9BF: fetch_input_tuple
(nodeAgg.c:563)
==00:00:00:06.975 2318810== by 0x41E2D4: agg_retrieve_direct
(nodeAgg.c:2346)
==00:00:00:06.975 2318810==
EXPLAIN shows:
Aggregate (cost=21.42..21.43 rows=1 width=8)
-> Nested Loop (cost=0.27..21.42 rows=1 width=0)
-> Index Only Scan using t_idx on t t1 (cost=0.13..12.16 rows=2
width=64)
-> Memoize (cost=0.14..6.16 rows=1 width=64)
Cache Key: t1.n
Cache Mode: binary
-> Index Only Scan using t_idx on t t2 (cost=0.13..6.15
rows=1 width=64)
Index Cond: (n <= t1.n)
If I understand the issue and nodeMemoize.c correctly, here
prepare_probe_slot() fills pslot->tts_values[] with a result of
ExecEvalExpr(),
which has length of 'name', but later MemoizeHash_hash() gets
attrs[]->attlen
(64) and passes it to datum_image_hash() for calculating a hash over this
number of bytes.
Reproduced starting from e502150f7.
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
@ 2023-03-22 06:18 ` David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: David Rowley @ 2023-03-22 06:18 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Tue, 21 Mar 2023 at 10:27, PG Bug reporting form
<noreply@postgresql.org> wrote:
> SET enable_seqscan TO off;
>
> CREATE TABLE t (n name);
> INSERT INTO t VALUES('name'), ('name');
> CREATE INDEX t_idx ON t(n);
> ANALYZE t;
>
> SELECT COUNT(*) FROM t t1 INNER JOIN t t2 ON t1.n >= t2.n;
>
> An incorrect memory access detected:
> ==00:00:00:06.975 2318810== Use of uninitialised value of size 8
This is very strange. I tracked this down to the fact that the
TupleDesc for the heap and the index don't match.
postgres=# select attrelid::regclass,attname,attlen from pg_attribute
where attnum=1 and attrelid in ('t'::regclass,'t_idx'::regclass);
attrelid | attname | attlen
----------+---------+--------
t | n | 64
t_idx | n | -2
This seems to be due to pg_opclass defining that cstring should be the
opckeytype for btree index for the name type:
postgres=# select *,opckeytype::regtype from pg_opclass where
opcintype='name'::regtype and opcdefault and opcmethod=403;
oid | opcmethod | opcname | opcnamespace | opcowner | opcfamily |
opcintype | opcdefault | opckeytype | opckeytype
-------+-----------+----------+--------------+----------+-----------+-----------+------------+------------+------------
10028 | 403 | name_ops | 11 | 10 | 1994 |
19 | t | 2275 | cstring
(1 row)
The problem valgrind finds is due to index_deform_tuple_internal(), if
I put a breakpoint in indextuple.c:536 after the values[attnum] =
fetchatt(thisatt, tp + off); then output the stored Datum in my
debugger, I see:
((char *) values[attnum])[0]
110 'n'
((char *) values[attnum])[1]
97 'a'
((char *) values[attnum])[2]
109 'm'
((char *) values[attnum])[3]
101 'e'
((char *) values[attnum])[4]
0 '\0'
...
((char *) values[attnum])[7]
0 '\0'
((char *) values[attnum])[8]
-51 'Í'
Note the value at element 8 (I'd expect) should be 0. In
nodeMemoize.c when we call datum_image_hash(), we'll hash the full 64
bytes of the name type and when we access element 8, we read
uninitialised memory.
The reason this only seems to happen when Memoize is in binary mode is
because we use datum_image_hash() which will hash the full 64 bytes of
the name Datum. When in logical mode and in hash joins, we'll use
hashname(), which calls strlen() on the input name Datum and only
hashes the bytes as if the name were a cstring:
return hash_any((unsigned char *) key, strlen(key));
I really don't know if I made a wrong assumption about the safety of
using datum_image_hash(). It really surprises me that it's unsafe to
not access the full 64 bytes of a name Datum.
David
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2023-03-23 03:10 ` David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Peter Geoghegan <pg@bowt.ie>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
0 siblings, 2 replies; 19+ messages in thread
From: David Rowley @ 2023-03-23 03:10 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, 22 Mar 2023 at 19:18, David Rowley <dgrowleyml@gmail.com> wrote:
> The reason this only seems to happen when Memoize is in binary mode is
> because we use datum_image_hash() which will hash the full 64 bytes of
> the name Datum. When in logical mode and in hash joins, we'll use
> hashname(), which calls strlen() on the input name Datum and only
> hashes the bytes as if the name were a string:
A relevant comment is in StoreIndexTuple():
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
* not the slot's tupdesc, in case the latter has different datatypes
* (this happens for btree name_ops in particular). They'd better have
* the same number of columns though, as well as being datatype-compatible
* which is something we can't so easily check.
*/
I'm just not really certain if we can say name is
"datatype-compatible" with cstring or not. It seems that namehash,
namecmp, nameout etc are all coded so that they can accept cstrings as
inputs. It's just not going to be safe for anything that wants to
access all of the NAMEDATALEN bytes.
Another bug I've found relating to this is:
CREATE TABLE t (n name);
INSERT INTO t VALUES('name'), ('name');
CREATE INDEX t_idx ON t(n);
ANALYZE t;
set enable_seqscan=0;
set enable_indexscan=0;
postgres=# select record_image_eq(row('name'::name), row(t.n)) from t
order by n;
record_image_eq
-----------------
f <- incorrect result
f <- incorrect result
(2 rows)
set enable_indexscan=1;
set enable_indexonlyscan=0;
postgres=# select record_image_eq(row('name'::name), row(t.n)) from t
order by n;
record_image_eq
-----------------
t <- correct result
t <- correct result
(2 rows)
It just seems to me that this whole reading one type from an index and
assuming it's some other different time is going to be a source of
bugs, even if we were to fix the Memoize and record_image_eq() bugs.
I tried and failed to find problems with the usage of datum_image_eq()
used in ri_KeysEqual(). This is because to get an UPDATE to perform an
Index Only Scan would require that the index had the ctid column. It
does not seem impossible that we might want to special case that,
since the index does contain the heap ctid, after all. If we did
that, we'd subtly break foreign key checks when performing an
UPDATE/DELETE on the referenced table when the foreign key column is
of type "name".
Right now, I don't have any bright ideas on how we might fix this.
The only thing I can think of right now is to adjust StoreIndexTuple()
to check for mismatching data types and if it finds one, convert with
the index type's output function and import with the heap type's input
function. Sounds like painful overhead not to mention how to manage
the memory allocations so it does not leak any copied Datum values.
David
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2023-03-23 03:25 ` Peter Geoghegan <pg@bowt.ie>
2023-03-23 04:01 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 19+ messages in thread
From: Peter Geoghegan @ 2023-03-23 03:25 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Mar 22, 2023 at 8:11 PM David Rowley <dgrowleyml@gmail.com> wrote:
> A relevant comment is in StoreIndexTuple():
>
> /*
> * Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
> * not the slot's tupdesc, in case the latter has different datatypes
> * (this happens for btree name_ops in particular). They'd better have
> * the same number of columns though, as well as being datatype-compatible
> * which is something we can't so easily check.
> */
>
> I'm just not really certain if we can say name is
> "datatype-compatible" with cstring or not. It seems that namehash,
> namecmp, nameout etc are all coded so that they can accept cstrings as
> inputs. It's just not going to be safe for anything that wants to
> access all of the NAMEDATALEN bytes.
I doubt that there is a clear answer to that question.
Have you seen the comments about the cstring/name_ops hack mentioning
a SIGSEGV in btrescan()? Those were added around the time index-only
scans first went in.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Peter Geoghegan <pg@bowt.ie>
@ 2023-03-23 04:01 ` David Rowley <dgrowleyml@gmail.com>
2023-03-23 04:27 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Peter Geoghegan <pg@bowt.ie>
2023-03-23 04:56 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
0 siblings, 2 replies; 19+ messages in thread
From: David Rowley @ 2023-03-23 04:01 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, 23 Mar 2023 at 16:25, Peter Geoghegan <pg@bowt.ie> wrote:
> Have you seen the comments about the cstring/name_ops hack mentioning
> a SIGSEGV in btrescan()? Those were added around the time index-only
> scans first went in.
I'd not seen it. That's a bit disappointing. Is all this just to work
around not having to store the full 64 bytes for a name in indexes?
Seems it there are a few hacks that try to make this work. I wonder
if we should just invent new hacks in the form of a new version of
datum_image_eq that accepts a pointer to a FormData_pg_attribute
instead of typByVal and typLen then just special case NAMEOID types to
always compare as cstrings. Same for datum_image_hash().
David
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Peter Geoghegan <pg@bowt.ie>
2023-03-23 04:01 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2023-03-23 04:27 ` Peter Geoghegan <pg@bowt.ie>
1 sibling, 0 replies; 19+ messages in thread
From: Peter Geoghegan @ 2023-03-23 04:27 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Mar 22, 2023 at 9:01 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Thu, 23 Mar 2023 at 16:25, Peter Geoghegan <pg@bowt.ie> wrote:
> > Have you seen the comments about the cstring/name_ops hack mentioning
> > a SIGSEGV in btrescan()? Those were added around the time index-only
> > scans first went in.
>
> I'd not seen it. That's a bit disappointing. Is all this just to work
> around not having to store the full 64 bytes for a name in indexes?
Yes, that's the whole point of the hack.
> Seems it there are a few hacks that try to make this work. I wonder
> if we should just invent new hacks in the form of a new version of
> datum_image_eq that accepts a pointer to a FormData_pg_attribute
> instead of typByVal and typLen then just special case NAMEOID types to
> always compare as cstrings. Same for datum_image_hash().
The only reason why datum_image_eq() has support for cstring is B-Tree
deduplication; cstring support was added to allow nbtdedup.c to deal
with name/cstring indexes sensibly. So the fact that datum_image_eq()
can compare cstrings in the first place is in fact related to the same
general issue with name_ops.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Peter Geoghegan <pg@bowt.ie>
2023-03-23 04:01 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2023-03-23 04:56 ` David Rowley <dgrowleyml@gmail.com>
1 sibling, 0 replies; 19+ messages in thread
From: David Rowley @ 2023-03-23 04:56 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, 23 Mar 2023 at 17:01, David Rowley <dgrowleyml@gmail.com> wrote:
> Seems it there are a few hacks that try to make this work. I wonder
> if we should just invent new hacks in the form of a new version of
> datum_image_eq that accepts a pointer to a FormData_pg_attribute
> instead of typByVal and typLen then just special case NAMEOID types to
> always compare as cstrings. Same for datum_image_hash().
Here are patches to implement versions of datum_image_eq() and
datum_image_hash() which accept FormData_pg_attribute pointers instead
of the byval and len parameters.
The 0002 patch just removes the now unused functions.
David
Attachments:
[application/octet-stream] v1-0001-Work-around-name-cstring-problem-in-datum_image_e.patch (10.8K, ../../CAApHDvo2xvOqv=tiQx4DXm1yHG_HUsTz0Uzuu60kzuQQx+D90A@mail.gmail.com/2-v1-0001-Work-around-name-cstring-problem-in-datum_image_e.patch)
download | inline diff:
From 2a7d233ea36f3ba552b32e527c5e41998c7e0baa Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Thu, 23 Mar 2023 17:34:52 +1300
Subject: [PATCH v1 1/2] Work around name / cstring problem in datum_image_eq
Indexes using name_ops store name types as cstrings. When performing an
Index Only Scan on such an index, name values will arrive as cstrings
which means they're not padded out to the full NAMEDATALEN bytes. This
caused problems in datum_image_eq and datum_image_hash as if the Datum
came from an Index Only Scan, we'd try to look at the full 64 bytes. If
the Datum being compared to didn't come from an Index Only Scan then the
Datums may appear not equal when they actually are equal.
The same problem existed in datum_image_hash.
Here we add new versions of datum_image_eq and datum_image_hash which
accept FormData_pg_attribute and special case NAMEOID so that we always
compare those as cstrings. This is pretty dirty, but it seems no worse
than what's mentioned in btrescan().
Bug: #17855
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/17855-5f523e0f9769a566@postgresql.org
---
src/backend/access/nbtree/nbtutils.c | 2 +-
src/backend/executor/nodeMemoize.c | 7 +-
src/backend/utils/adt/datum.c | 155 ++++++++++++++++++++++++++-
src/backend/utils/adt/ri_triggers.c | 2 +-
src/backend/utils/adt/rowtypes.c | 2 +-
src/include/utils/datum.h | 24 ++++-
6 files changed, 182 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 7da499c4dd..f19094b89a 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -2444,7 +2444,7 @@ _bt_keep_natts_fast(Relation rel, IndexTuple lastleft, IndexTuple firstright)
break;
if (!isNull1 &&
- !datum_image_eq(datum1, datum2, att->attbyval, att->attlen))
+ !datum_image_attr_eq(datum1, datum2, att))
break;
keepnatts++;
diff --git a/src/backend/executor/nodeMemoize.c b/src/backend/executor/nodeMemoize.c
index 4f04269e26..e519dc31d5 100644
--- a/src/backend/executor/nodeMemoize.c
+++ b/src/backend/executor/nodeMemoize.c
@@ -176,7 +176,7 @@ MemoizeHash_hash(struct memoize_hash *tb, const MemoizeKey *key)
attr = &pslot->tts_tupleDescriptor->attrs[i];
- hkey = datum_image_hash(pslot->tts_values[i], attr->attbyval, attr->attlen);
+ hkey = datum_image_attr_hash(pslot->tts_values[i], attr);
hashkey ^= hkey;
}
@@ -244,8 +244,9 @@ MemoizeHash_equal(struct memoize_hash *tb, const MemoizeKey *key1,
/* perform binary comparison on the two datums */
attr = &tslot->tts_tupleDescriptor->attrs[i];
- if (!datum_image_eq(tslot->tts_values[i], pslot->tts_values[i],
- attr->attbyval, attr->attlen))
+ if (!datum_image_attr_eq(tslot->tts_values[i],
+ pslot->tts_values[i],
+ attr))
return false;
}
return true;
diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c
index 9f06ee7626..3947120a42 100644
--- a/src/backend/utils/adt/datum.c
+++ b/src/backend/utils/adt/datum.c
@@ -43,6 +43,8 @@
#include "postgres.h"
#include "access/detoast.h"
+#include "catalog/pg_attribute.h"
+#include "catalog/pg_type.h"
#include "common/hashfn.h"
#include "fmgr.h"
#include "utils/builtins.h"
@@ -256,7 +258,7 @@ datumIsEqual(Datum value1, Datum value2, bool typByVal, int typLen)
}
/*-------------------------------------------------------------------------
- * datum_image_eq
+ * datum_image_eq -- Use datum_image_attr_eq instead
*
* Compares two datums for identical contents, based on byte images. Return
* true if the two datums are equal, false otherwise.
@@ -326,7 +328,91 @@ datum_image_eq(Datum value1, Datum value2, bool typByVal, int typLen)
}
/*-------------------------------------------------------------------------
- * datum_image_hash
+ * datum_image_attr_eq
+ *
+ * Compares two datums for identical contents, based on byte images. Return
+ * true if the two datums are equal, false otherwise.
+ *-------------------------------------------------------------------------
+ */
+bool
+datum_image_attr_eq(Datum value1, Datum value2, FormData_pg_attribute *atttype)
+{
+ bool typByVal = atttype->attbyval;
+ int16 typLen;
+ Size len1,
+ len2;
+ bool result = true;
+
+ /*
+ * Always special case names to compare as cstrings instead of comparing
+ * NAMEDATALEN bytes. We must do this due to btree name_ops using an
+ * underlying storage datatype of cstring. If the Datum comes from an
+ * index only scan, then the Datum won't be padded out with NULs for the
+ * full NAMEDATALEN bytes.
+ */
+ if (atttype->atttypid == NAMEOID)
+ typLen = -2;
+ else
+ typLen = atttype->attlen;
+
+ if (typByVal)
+ {
+ result = (value1 == value2);
+ }
+ else if (typLen > 0)
+ {
+ result = (memcmp(DatumGetPointer(value1),
+ DatumGetPointer(value2),
+ typLen) == 0);
+ }
+ else if (typLen == -1)
+ {
+ len1 = toast_raw_datum_size(value1);
+ len2 = toast_raw_datum_size(value2);
+ /* No need to de-toast if lengths don't match. */
+ if (len1 != len2)
+ result = false;
+ else
+ {
+ struct varlena *arg1val;
+ struct varlena *arg2val;
+
+ arg1val = PG_DETOAST_DATUM_PACKED(value1);
+ arg2val = PG_DETOAST_DATUM_PACKED(value2);
+
+ result = (memcmp(VARDATA_ANY(arg1val),
+ VARDATA_ANY(arg2val),
+ len1 - VARHDRSZ) == 0);
+
+ /* Only free memory if it's a copy made here. */
+ if ((Pointer) arg1val != (Pointer) value1)
+ pfree(arg1val);
+ if ((Pointer) arg2val != (Pointer) value2)
+ pfree(arg2val);
+ }
+ }
+ else if (typLen == -2)
+ {
+ char *s1,
+ *s2;
+
+ /* Compare cstring datums */
+ s1 = DatumGetCString(value1);
+ s2 = DatumGetCString(value2);
+ len1 = strlen(s1) + 1;
+ len2 = strlen(s2) + 1;
+ if (len1 != len2)
+ return false;
+ result = (memcmp(s1, s2, len1) == 0);
+ }
+ else
+ elog(ERROR, "unexpected typLen: %d", typLen);
+
+ return result;
+}
+
+/*-------------------------------------------------------------------------
+ * datum_image_hash -- Use datum_image_attr_hash instead
*
* Generate a hash value based on the binary representation of 'value'. Most
* use cases will want to use the hash function specific to the Datum's type,
@@ -376,6 +462,71 @@ datum_image_hash(Datum value, bool typByVal, int typLen)
return result;
}
+/*-------------------------------------------------------------------------
+ * datum_image_attr_hash
+ *
+ * Generate a hash value based on the binary representation of 'value'. Most
+ * use cases will want to use the hash function specific to the Datum's type,
+ * however, some corner cases require generating a hash value based on the
+ * actual bits rather than the logical value.
+ *-------------------------------------------------------------------------
+ */
+uint32
+datum_image_attr_hash(Datum value, FormData_pg_attribute *atttype)
+{
+ bool typByVal = atttype->attbyval;
+ int16 typLen;
+ uint32 result;
+ Size len;
+
+ /*
+ * Always special case names to compare as cstrings instead of comparing
+ * NAMEDATALEN bytes. We must do this due to btree name_ops using an
+ * underlying storage datatype of cstring. If the Datum comes from an
+ * index only scan, then the Datum won't be padded out with NULs for the
+ * full NAMEDATALEN bytes.
+ */
+ if (atttype->atttypid == NAMEOID)
+ typLen = -2;
+ else
+ typLen = atttype->attlen;
+
+ if (typByVal)
+ result = hash_bytes((unsigned char *) &value, sizeof(Datum));
+ else if (typLen > 0)
+ result = hash_bytes((unsigned char *) DatumGetPointer(value), typLen);
+ else if (typLen == -1)
+ {
+ struct varlena *val;
+
+ len = toast_raw_datum_size(value);
+
+ val = PG_DETOAST_DATUM_PACKED(value);
+
+ result = hash_bytes((unsigned char *) VARDATA_ANY(val), len - VARHDRSZ);
+
+ /* Only free memory if it's a copy made here. */
+ if ((Pointer) val != (Pointer) value)
+ pfree(val);
+ }
+ else if (typLen == -2)
+ {
+ char *s;
+
+ s = DatumGetCString(value);
+ len = strlen(s) + 1;
+
+ result = hash_bytes((unsigned char *) s, len);
+ }
+ else
+ {
+ elog(ERROR, "unexpected typLen: %d", typLen);
+ result = 0; /* keep compiler quiet */
+ }
+
+ return result;
+}
+
/*-------------------------------------------------------------------------
* btequalimage
*
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 375b17b9f3..82b1cbcb9e 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2838,7 +2838,7 @@ ri_KeysEqual(Relation rel, TupleTableSlot *oldslot, TupleTableSlot *newslot,
*/
Form_pg_attribute att = TupleDescAttr(oldslot->tts_tupleDescriptor, attnums[i] - 1);
- if (!datum_image_eq(oldvalue, newvalue, att->attbyval, att->attlen))
+ if (!datum_image_attr_eq(oldvalue, newvalue, att))
return false;
}
else
diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index ad176651d8..11c4460fed 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -1723,7 +1723,7 @@ record_image_eq(PG_FUNCTION_ARGS)
}
/* Compare the pair of elements */
- result = datum_image_eq(values1[i1], values2[i2], att1->attbyval, att2->attlen);
+ result = datum_image_attr_eq(values1[i1], values2[i2], att1);
if (!result)
break;
}
diff --git a/src/include/utils/datum.h b/src/include/utils/datum.h
index 70a47f33c1..046e789f7c 100644
--- a/src/include/utils/datum.h
+++ b/src/include/utils/datum.h
@@ -18,6 +18,8 @@
#ifndef DATUM_H
#define DATUM_H
+struct FormData_pg_attribute;
+
/*
* datumGetSize - find the "real" length of a datum
*/
@@ -47,7 +49,7 @@ extern bool datumIsEqual(Datum value1, Datum value2,
bool typByVal, int typLen);
/*
- * datum_image_eq
+ * datum_image_eq -- Use datum_image_attr_eq instead
*
* Compares two datums for identical contents, based on byte images. Return
* true if the two datums are equal, false otherwise.
@@ -56,13 +58,31 @@ extern bool datum_image_eq(Datum value1, Datum value2,
bool typByVal, int typLen);
/*
- * datum_image_hash
+ * datum_image_attr_eq
+ *
+ * Compares two datums for identical contents, based on byte images.Return
+ * true if the two datums are equal, false otherwise.
+ */
+extern bool datum_image_attr_eq(Datum value1, Datum value2,
+ struct FormData_pg_attribute *atttype);
+
+/*
+ * datum_image_hash -- Use datum_image_attr_hash instead
*
* Generates hash value for 'value' based on its bits rather than logical
* value.
*/
extern uint32 datum_image_hash(Datum value, bool typByVal, int typLen);
+/*
+ * datum_image_attr_hash
+ *
+ * Generates hash value for 'value' based on its bits rather than logical
+ * value.
+ */
+extern uint32 datum_image_attr_hash(Datum value,
+ struct FormData_pg_attribute *atttype);
+
/*
* Serialize and restore datums so that we can transfer them to parallel
* workers.
--
2.37.2
[application/octet-stream] v1-0002-Remove-unused-datum_image_eq-and-datum_image_hash.patch (5.2K, ../../CAApHDvo2xvOqv=tiQx4DXm1yHG_HUsTz0Uzuu60kzuQQx+D90A@mail.gmail.com/3-v1-0002-Remove-unused-datum_image_eq-and-datum_image_hash.patch)
download | inline diff:
From 26bba5c38e83eea061bcfd1f3a8a0a228c466c58 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Thu, 23 Mar 2023 17:44:03 +1300
Subject: [PATCH v1 2/2] Remove unused datum_image_eq() and datum_image_hash()
functions
---
src/backend/utils/adt/datum.c | 121 ----------------------------------
src/include/utils/datum.h | 17 -----
2 files changed, 138 deletions(-)
diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c
index 3947120a42..db095d7f7c 100644
--- a/src/backend/utils/adt/datum.c
+++ b/src/backend/utils/adt/datum.c
@@ -257,76 +257,6 @@ datumIsEqual(Datum value1, Datum value2, bool typByVal, int typLen)
return res;
}
-/*-------------------------------------------------------------------------
- * datum_image_eq -- Use datum_image_attr_eq instead
- *
- * Compares two datums for identical contents, based on byte images. Return
- * true if the two datums are equal, false otherwise.
- *-------------------------------------------------------------------------
- */
-bool
-datum_image_eq(Datum value1, Datum value2, bool typByVal, int typLen)
-{
- Size len1,
- len2;
- bool result = true;
-
- if (typByVal)
- {
- result = (value1 == value2);
- }
- else if (typLen > 0)
- {
- result = (memcmp(DatumGetPointer(value1),
- DatumGetPointer(value2),
- typLen) == 0);
- }
- else if (typLen == -1)
- {
- len1 = toast_raw_datum_size(value1);
- len2 = toast_raw_datum_size(value2);
- /* No need to de-toast if lengths don't match. */
- if (len1 != len2)
- result = false;
- else
- {
- struct varlena *arg1val;
- struct varlena *arg2val;
-
- arg1val = PG_DETOAST_DATUM_PACKED(value1);
- arg2val = PG_DETOAST_DATUM_PACKED(value2);
-
- result = (memcmp(VARDATA_ANY(arg1val),
- VARDATA_ANY(arg2val),
- len1 - VARHDRSZ) == 0);
-
- /* Only free memory if it's a copy made here. */
- if ((Pointer) arg1val != (Pointer) value1)
- pfree(arg1val);
- if ((Pointer) arg2val != (Pointer) value2)
- pfree(arg2val);
- }
- }
- else if (typLen == -2)
- {
- char *s1,
- *s2;
-
- /* Compare cstring datums */
- s1 = DatumGetCString(value1);
- s2 = DatumGetCString(value2);
- len1 = strlen(s1) + 1;
- len2 = strlen(s2) + 1;
- if (len1 != len2)
- return false;
- result = (memcmp(s1, s2, len1) == 0);
- }
- else
- elog(ERROR, "unexpected typLen: %d", typLen);
-
- return result;
-}
-
/*-------------------------------------------------------------------------
* datum_image_attr_eq
*
@@ -411,57 +341,6 @@ datum_image_attr_eq(Datum value1, Datum value2, FormData_pg_attribute *atttype)
return result;
}
-/*-------------------------------------------------------------------------
- * datum_image_hash -- Use datum_image_attr_hash instead
- *
- * Generate a hash value based on the binary representation of 'value'. Most
- * use cases will want to use the hash function specific to the Datum's type,
- * however, some corner cases require generating a hash value based on the
- * actual bits rather than the logical value.
- *-------------------------------------------------------------------------
- */
-uint32
-datum_image_hash(Datum value, bool typByVal, int typLen)
-{
- Size len;
- uint32 result;
-
- if (typByVal)
- result = hash_bytes((unsigned char *) &value, sizeof(Datum));
- else if (typLen > 0)
- result = hash_bytes((unsigned char *) DatumGetPointer(value), typLen);
- else if (typLen == -1)
- {
- struct varlena *val;
-
- len = toast_raw_datum_size(value);
-
- val = PG_DETOAST_DATUM_PACKED(value);
-
- result = hash_bytes((unsigned char *) VARDATA_ANY(val), len - VARHDRSZ);
-
- /* Only free memory if it's a copy made here. */
- if ((Pointer) val != (Pointer) value)
- pfree(val);
- }
- else if (typLen == -2)
- {
- char *s;
-
- s = DatumGetCString(value);
- len = strlen(s) + 1;
-
- result = hash_bytes((unsigned char *) s, len);
- }
- else
- {
- elog(ERROR, "unexpected typLen: %d", typLen);
- result = 0; /* keep compiler quiet */
- }
-
- return result;
-}
-
/*-------------------------------------------------------------------------
* datum_image_attr_hash
*
diff --git a/src/include/utils/datum.h b/src/include/utils/datum.h
index 046e789f7c..cbe45333e8 100644
--- a/src/include/utils/datum.h
+++ b/src/include/utils/datum.h
@@ -48,15 +48,6 @@ extern Datum datumTransfer(Datum value, bool typByVal, int typLen);
extern bool datumIsEqual(Datum value1, Datum value2,
bool typByVal, int typLen);
-/*
- * datum_image_eq -- Use datum_image_attr_eq instead
- *
- * Compares two datums for identical contents, based on byte images. Return
- * true if the two datums are equal, false otherwise.
- */
-extern bool datum_image_eq(Datum value1, Datum value2,
- bool typByVal, int typLen);
-
/*
* datum_image_attr_eq
*
@@ -66,14 +57,6 @@ extern bool datum_image_eq(Datum value1, Datum value2,
extern bool datum_image_attr_eq(Datum value1, Datum value2,
struct FormData_pg_attribute *atttype);
-/*
- * datum_image_hash -- Use datum_image_attr_hash instead
- *
- * Generates hash value for 'value' based on its bits rather than logical
- * value.
- */
-extern uint32 datum_image_hash(Datum value, bool typByVal, int typLen);
-
/*
* datum_image_attr_hash
*
--
2.37.2
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2023-03-25 05:32 ` David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
1 sibling, 1 reply; 19+ messages in thread
From: David Rowley @ 2023-03-25 05:32 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, 23 Mar 2023 at 16:10, David Rowley <dgrowleyml@gmail.com> wrote:
> Right now, I don't have any bright ideas on how we might fix this.
> The only thing I can think of right now is to adjust StoreIndexTuple()
> to check for mismatching data types and if it finds one, convert with
> the index type's output function and import with the heap type's input
> function. Sounds like painful overhead not to mention how to manage
> the memory allocations so it does not leak any copied Datum values.
I had a bit of an offline chat about this bug to Andres. We talked
about the method described in the above paragraph as a means to fix
this.
We talked about the possibilities of bugs being a bit more widespread.
For example, anywhere that does datumCopy() (i.e. lots of places) on a
name type could read too many bytes.
I've drafted up a patch which adds some code to nodeIndexonlyscan.c.
During ExecInitIndexOnlyScan() to looks for any name types in the
scan's tupleDesc and if it finds some, it marks the position of each
of these in a new array in IndexOnlyScanState. When we call
StoreIndexTuple(), if that array has any elements, we spin through it
and convert the cstring names to Names correctly padded out to
64-bytes.
The attached is just a draft so far. It'll need more comments to
document what the code is all about. I don't want to spend too much
time on it if this isn't going to be the final solution.
I'd be happy to hear from anyone who has any thoughts on this as a fix
for the issue.
David
Attachments:
[application/octet-stream] convert_cstring_names_to_proper_names_in_ios.patch (3.8K, ../../CAApHDvqNo_7yipfLjVXatUUosEkKZrdhHxAObPuxSat8UQaUTw@mail.gmail.com/2-convert_cstring_names_to_proper_names_in_ios.patch)
download | inline diff:
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index 0b43a9b969..87c02e8751 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -41,13 +41,14 @@
#include "miscadmin.h"
#include "storage/bufmgr.h"
#include "storage/predicate.h"
+#include "utils/builtins.h"
#include "utils/memutils.h"
#include "utils/rel.h"
static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node);
-static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup,
- TupleDesc itupdesc);
+static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc);
/* ----------------------------------------------------------------
@@ -206,7 +207,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
}
else if (scandesc->xs_itup)
- StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
+ StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc);
else
elog(ERROR, "no data returned for index-only scan");
@@ -264,7 +265,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
* right now we don't need it elsewhere.
*/
static void
-StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
+StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc)
{
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
@@ -277,6 +279,26 @@ StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
ExecClearTuple(slot);
index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull);
+
+ if (node->ioss_ConvertDatumIdxs != NULL)
+ {
+ for (int i = 0; i < node->ioss_ConvertDatumCount; i++)
+ {
+ int j = node->ioss_ConvertDatumIdxs[i];
+ Name name;
+
+ /* skip null Datums */
+ if (slot->tts_isnull[j])
+ continue;
+
+ name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
+ NAMEDATALEN);
+
+ namestrcpy(name, DatumGetCString(slot->tts_values[j]));
+ slot->tts_values[j] = NameGetDatum(name);
+ }
+ }
+
ExecStoreVirtualTuple(slot);
}
@@ -620,6 +642,32 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ioss_RuntimeContext = NULL;
}
+ indexstate->ioss_ConvertDatumIdxs = NULL;
+ indexstate->ioss_ConvertDatumCount = 0;
+
+ {
+ int name_count = 0;
+ for (int i = 0; i < tupDesc->natts; i++)
+ {
+ if (tupDesc->attrs[i].atttypid == NAMEOID)
+ name_count++;
+ }
+
+ if (name_count > 0)
+ {
+ int j = 0;
+ indexstate->ioss_ConvertDatumIdxs = palloc(sizeof(int) * name_count);
+
+ for (int i = 0; i < tupDesc->natts; i++)
+ {
+ if (tupDesc->attrs[i].atttypid == NAMEOID)
+ indexstate->ioss_ConvertDatumIdxs[j++] = i;
+ }
+ }
+
+ indexstate->ioss_ConvertDatumCount = name_count;
+ }
+
/*
* all done.
*/
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d97f5a8e7d..6a64d4aa35 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1587,6 +1587,8 @@ typedef struct IndexScanState
* TableSlot slot for holding tuples fetched from the table
* VMBuffer buffer in use for visibility map testing, if any
* PscanLen size of parallel index-only scan descriptor
+ * ConvertDatumIdxs Index of Datums from the index to convert to heap type
+ * ConvertDatumCount Number of elements in ConvertDatumIdxs
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1606,6 +1608,8 @@ typedef struct IndexOnlyScanState
TupleTableSlot *ioss_TableSlot;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ int *ioss_ConvertDatumIdxs;
+ int ioss_ConvertDatumCount;
} IndexOnlyScanState;
/* ----------------
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2023-09-09 08:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Alexander Lakhin @ 2023-09-09 08:00 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; pgsql-bugs@lists.postgresql.org
Hi David,
25.03.2023 08:32, David Rowley wrote:
> The attached is just a draft so far. It'll need more comments to
> document what the code is all about. I don't want to spend too much
> time on it if this isn't going to be the final solution.
I've stumbled upon this issue one more time. With a query like this:
CREATE TABLE t(id integer, node name);
CREATE INDEX t_id_node_idx ON t(id, node);
INSERT INTO t VALUES (1, 'node1');
EXPLAIN SELECT array_agg(node ORDER BY node) AS node_list FROM t GROUP BY id;
SELECT array_agg(node ORDER BY node) AS node_list FROM t GROUP BY id;
I get (on REL_16_STABLE):
QUERY PLAN
--------------------------------------------------------------------------------------
GroupAggregate (cost=0.15..67.65 rows=200 width=36)
Group Key: id
-> Index Only Scan using t_id_node_idx on t (cost=0.15..60.90 rows=850 width=68)
(Note that this time the error is triggered without the Memoize node.)
==00:00:00:04.385 3941088== Uninitialised byte(s) found during client check request
==00:00:00:04.385 3941088== at 0x1F0CF7: printtup (printtup.c:349)
==00:00:00:04.385 3941088== by 0x410521: ExecutePlan (execMain.c:1701)
==00:00:00:04.385 3941088== by 0x4106B3: standard_ExecutorRun (execMain.c:365)
==00:00:00:04.385 3941088== by 0x41078D: ExecutorRun (execMain.c:309)
==00:00:00:04.385 3941088== by 0x5FAC9A: PortalRunSelect (pquery.c:924)
==00:00:00:04.385 3941088== by 0x5FC63B: PortalRun (pquery.c:768)
==00:00:00:04.385 3941088== by 0x5F85F3: exec_simple_query (postgres.c:1274)
==00:00:00:04.385 3941088== by 0x5FA593: PostgresMain (postgres.c:4637)
==00:00:00:04.385 3941088== by 0x5514A2: BackendRun (postmaster.c:4464)
==00:00:00:04.385 3941088== by 0x554658: BackendStartup (postmaster.c:4192)
==00:00:00:04.385 3941088== by 0x5547F6: ServerLoop (postmaster.c:1782)
==00:00:00:04.385 3941088== by 0x555D26: PostmasterMain (postmaster.c:1466)
==00:00:00:04.385 3941088== Address 0x1121ecdc is 36 bytes inside a block of size 88 client-defined
==00:00:00:04.385 3941088== at 0x77EDA5: palloc0 (mcxt.c:1282)
==00:00:00:04.385 3941088== by 0x62B6BC: construct_md_array (arrayfuncs.c:3528)
==00:00:00:04.385 3941088== by 0x631013: makeMdArrayResult (arrayfuncs.c:5427)
==00:00:00:04.385 3941088== by 0x6242AF: array_agg_finalfn (array_userfuncs.c:858)
==00:00:00:04.385 3941088== by 0x4247F5: finalize_aggregate (nodeAgg.c:1120)
==00:00:00:04.385 3941088== by 0x42620A: finalize_aggregates (nodeAgg.c:1361)
==00:00:00:04.385 3941088== by 0x426E71: agg_retrieve_direct (nodeAgg.c:2520)
==00:00:00:04.385 3941088== by 0x4272B9: ExecAgg (nodeAgg.c:2180)
==00:00:00:04.385 3941088== by 0x417EE1: ExecProcNodeFirst (execProcnode.c:464)
==00:00:00:04.385 3941088== by 0x4104F0: ExecProcNode (executor.h:273)
==00:00:00:04.385 3941088== by 0x4104F0: ExecutePlan (execMain.c:1670)
==00:00:00:04.385 3941088== by 0x4106B3: standard_ExecutorRun (execMain.c:365)
==00:00:00:04.385 3941088== by 0x41078D: ExecutorRun (execMain.c:309)
==00:00:00:04.385 3941088==
With your last patch applied I see no this valgrind complaint.
Maybe it makes sense to register the proposed patch on the commitfest at
least to keep it in sight?
Best regards,
Alexander Lakhin
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
@ 2024-04-24 02:25 ` David Rowley <dgrowleyml@gmail.com>
2024-04-24 13:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 19+ messages in thread
From: David Rowley @ 2024-04-24 02:25 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Sat, 9 Sept 2023 at 20:00, Alexander Lakhin <exclusion@gmail.com> wrote:
> I've stumbled upon this issue one more time. With a query like this:
> CREATE TABLE t(id integer, node name);
> CREATE INDEX t_id_node_idx ON t(id, node);
> INSERT INTO t VALUES (1, 'node1');
>
> (Note that this time the error is triggered without the Memoize node.)
Yeah, it's really not a Memoize bug. It's an Index Only Scan bug.
I've added Robert to get his views.
> Maybe it makes sense to register the proposed patch on the commitfest at
> least to keep it in sight?
I've attached another patch which uses another method to fix this, per
an idea from Andres Freund. I'd class it as a hack, but I don't have
any better ideas aside from the mammoth task of making name variable
length. Indexes on name typed columns simply don't store all 64 bytes
of the name, so it's not safe to have code that assumes a name Datum
points to 64 bytes. The patch makes it so such a Datum *will* point to
64 bytes. I've tried to do this as cheaply as possible by saving the
indexes to name columns in a new array in IndexOnlyScanState. That
should make the overhead very small when indexes don't contain any
name-typed columns.
David
From ba0c2f4045a58c2d11e12d5f0e5d770765be6c82 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 24 Apr 2024 14:14:44 +1200
Subject: [PATCH v2] Ensure we allocate NAMEDATALEN bytes for names in Index
Only Scans
---
src/backend/executor/nodeIndexonlyscan.c | 55 ++++++++++++++++++++++--
src/include/nodes/execnodes.h | 4 ++
2 files changed, 55 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index fcf6d1d932..93ab69ba50 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -45,8 +45,8 @@
static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node);
-static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup,
- TupleDesc itupdesc);
+static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc);
/* ----------------------------------------------------------------
@@ -205,7 +205,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
}
else if (scandesc->xs_itup)
- StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
+ StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc);
else
elog(ERROR, "no data returned for index-only scan");
@@ -263,7 +263,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
* right now we don't need it elsewhere.
*/
static void
-StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
+StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc)
{
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
@@ -276,6 +277,26 @@ StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
ExecClearTuple(slot);
index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull);
+
+ if (node->ioss_ConvertDatumIdxs != NULL)
+ {
+ for (int i = 0; i < node->ioss_ConvertDatumCount; i++)
+ {
+ int j = node->ioss_ConvertDatumIdxs[i];
+ Name name;
+
+ /* skip null Datums */
+ if (slot->tts_isnull[j])
+ continue;
+
+ name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
+ NAMEDATALEN);
+
+ namestrcpy(name, DatumGetCString(slot->tts_values[j]));
+ slot->tts_values[j] = NameGetDatum(name);
+ }
+ }
+
ExecStoreVirtualTuple(slot);
}
@@ -603,6 +624,32 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ioss_RuntimeContext = NULL;
}
+ indexstate->ioss_ConvertDatumIdxs = NULL;
+ indexstate->ioss_ConvertDatumCount = 0;
+
+ {
+ int name_count = 0;
+ for (int i = 0; i < tupDesc->natts; i++)
+ {
+ if (tupDesc->attrs[i].atttypid == NAMEOID)
+ name_count++;
+ }
+
+ if (name_count > 0)
+ {
+ int j = 0;
+ indexstate->ioss_ConvertDatumIdxs = palloc(sizeof(int) * name_count);
+
+ for (int i = 0; i < tupDesc->natts; i++)
+ {
+ if (tupDesc->attrs[i].atttypid == NAMEOID)
+ indexstate->ioss_ConvertDatumIdxs[j++] = i;
+ }
+ }
+
+ indexstate->ioss_ConvertDatumCount = name_count;
+ }
+
/*
* all done.
*/
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d927ac44a8..6d72e04fe7 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1690,6 +1690,8 @@ typedef struct IndexScanState
* TableSlot slot for holding tuples fetched from the table
* VMBuffer buffer in use for visibility map testing, if any
* PscanLen size of parallel index-only scan descriptor
+ * ConvertDatumIdxs Index of Datums from the index to convert to heap type
+ * ConvertDatumCount Number of elements in ConvertDatumIdxs
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1709,6 +1711,8 @@ typedef struct IndexOnlyScanState
TupleTableSlot *ioss_TableSlot;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ int *ioss_ConvertDatumIdxs;
+ int ioss_ConvertDatumCount;
} IndexOnlyScanState;
/* ----------------
--
2.40.1.windows.1
Attachments:
[text/plain] v2-0001-Ensure-we-allocate-NAMEDATALEN-bytes-for-names-in.patch (4.0K, ../../CAApHDvqqvkorexCuVkSJ16LcqGKfUdLAn3Y+YsqKNT3Pe56oOA@mail.gmail.com/2-v2-0001-Ensure-we-allocate-NAMEDATALEN-bytes-for-names-in.patch)
download | inline diff:
From ba0c2f4045a58c2d11e12d5f0e5d770765be6c82 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 24 Apr 2024 14:14:44 +1200
Subject: [PATCH v2] Ensure we allocate NAMEDATALEN bytes for names in Index
Only Scans
---
src/backend/executor/nodeIndexonlyscan.c | 55 ++++++++++++++++++++++--
src/include/nodes/execnodes.h | 4 ++
2 files changed, 55 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index fcf6d1d932..93ab69ba50 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -45,8 +45,8 @@
static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node);
-static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup,
- TupleDesc itupdesc);
+static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc);
/* ----------------------------------------------------------------
@@ -205,7 +205,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
}
else if (scandesc->xs_itup)
- StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
+ StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc);
else
elog(ERROR, "no data returned for index-only scan");
@@ -263,7 +263,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
* right now we don't need it elsewhere.
*/
static void
-StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
+StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc)
{
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
@@ -276,6 +277,26 @@ StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
ExecClearTuple(slot);
index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull);
+
+ if (node->ioss_ConvertDatumIdxs != NULL)
+ {
+ for (int i = 0; i < node->ioss_ConvertDatumCount; i++)
+ {
+ int j = node->ioss_ConvertDatumIdxs[i];
+ Name name;
+
+ /* skip null Datums */
+ if (slot->tts_isnull[j])
+ continue;
+
+ name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
+ NAMEDATALEN);
+
+ namestrcpy(name, DatumGetCString(slot->tts_values[j]));
+ slot->tts_values[j] = NameGetDatum(name);
+ }
+ }
+
ExecStoreVirtualTuple(slot);
}
@@ -603,6 +624,32 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ioss_RuntimeContext = NULL;
}
+ indexstate->ioss_ConvertDatumIdxs = NULL;
+ indexstate->ioss_ConvertDatumCount = 0;
+
+ {
+ int name_count = 0;
+ for (int i = 0; i < tupDesc->natts; i++)
+ {
+ if (tupDesc->attrs[i].atttypid == NAMEOID)
+ name_count++;
+ }
+
+ if (name_count > 0)
+ {
+ int j = 0;
+ indexstate->ioss_ConvertDatumIdxs = palloc(sizeof(int) * name_count);
+
+ for (int i = 0; i < tupDesc->natts; i++)
+ {
+ if (tupDesc->attrs[i].atttypid == NAMEOID)
+ indexstate->ioss_ConvertDatumIdxs[j++] = i;
+ }
+ }
+
+ indexstate->ioss_ConvertDatumCount = name_count;
+ }
+
/*
* all done.
*/
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d927ac44a8..6d72e04fe7 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1690,6 +1690,8 @@ typedef struct IndexScanState
* TableSlot slot for holding tuples fetched from the table
* VMBuffer buffer in use for visibility map testing, if any
* PscanLen size of parallel index-only scan descriptor
+ * ConvertDatumIdxs Index of Datums from the index to convert to heap type
+ * ConvertDatumCount Number of elements in ConvertDatumIdxs
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1709,6 +1711,8 @@ typedef struct IndexOnlyScanState
TupleTableSlot *ioss_TableSlot;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ int *ioss_ConvertDatumIdxs;
+ int ioss_ConvertDatumCount;
} IndexOnlyScanState;
/* ----------------
--
2.40.1.windows.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2024-04-24 13:00 ` Alexander Lakhin <exclusion@gmail.com>
1 sibling, 0 replies; 19+ messages in thread
From: Alexander Lakhin @ 2024-04-24 13:00 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Hello David.
24.04.2024 05:25, David Rowley wrote:
> On Sat, 9 Sept 2023 at 20:00, Alexander Lakhin <exclusion@gmail.com> wrote:
>> I've stumbled upon this issue one more time. With a query like this:
>> CREATE TABLE t(id integer, node name);
>> CREATE INDEX t_id_node_idx ON t(id, node);
>> INSERT INTO t VALUES (1, 'node1');
>>
>> (Note that this time the error is triggered without the Memoize node.)
> Yeah, it's really not a Memoize bug. It's an Index Only Scan bug.
> I've added Robert to get his views.
>
>> Maybe it makes sense to register the proposed patch on the commitfest at
>> least to keep it in sight?
> I've attached another patch which uses another method to fix this, per
> an idea from Andres Freund. I'd class it as a hack, but I don't have
> any better ideas aside from the mammoth task of making name variable
> length. Indexes on name typed columns simply don't store all 64 bytes
> of the name, so it's not safe to have code that assumes a name Datum
> points to 64 bytes. The patch makes it so such a Datum *will* point to
> 64 bytes. I've tried to do this as cheaply as possible by saving the
> indexes to name columns in a new array in IndexOnlyScanState. That
> should make the overhead very small when indexes don't contain any
> name-typed columns.
I've tested this patch with an additional #include in nodeIndexonlyscan.c
(for namestrcpy), it works, but I'm unsure about the check for NAMEOID —
it seems like the fix could break for a type based on name. Say, with:
CREATE DOMAIN named AS name;
CREATE TABLE t(id integer, node named);
CREATE INDEX t_id_node_idx ON t(id, node);
INSERT INTO t VALUES (1, 'node1');
SELECT array_agg(node ORDER BY node) AS node_list FROM t GROUP BY id;
I still get the Valgrind complaint.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2024-04-25 15:50 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 19+ messages in thread
From: Tom Lane @ 2024-04-25 15:50 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> I've attached another patch which uses another method to fix this, per
> an idea from Andres Freund. I'd class it as a hack, but I don't have
> any better ideas aside from the mammoth task of making name variable
> length. Indexes on name typed columns simply don't store all 64 bytes
> of the name, so it's not safe to have code that assumes a name Datum
> points to 64 bytes. The patch makes it so such a Datum *will* point to
> 64 bytes. I've tried to do this as cheaply as possible by saving the
> indexes to name columns in a new array in IndexOnlyScanState. That
> should make the overhead very small when indexes don't contain any
> name-typed columns.
I realize that this is just a draft patch, but the near-total lack
of comments is still disappointing. Even at the draft stage,
good comments are important to make it reviewable.
I do not like testing "atttypid == NAMEOID" one bit. As noted,
that will fail on domains over name. It will also result in
unnecessary work when reading non-btree indexes that contain
name, since I don't think anything else has the same hack
that btree name_ops does (grep for cstring in pg_opclass.dat).
I think the correct thing is to see whether the index opclass
for the column is btree name_ops. We don't seem to have an
oid_symbol macro for that, but it shouldn't be hard to add,
at least in HEAD.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-04-29 00:54 ` David Rowley <dgrowleyml@gmail.com>
2024-04-29 14:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: David Rowley @ 2024-04-29 00:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org
On Fri, 26 Apr 2024 at 03:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I do not like testing "atttypid == NAMEOID" one bit. As noted,
> that will fail on domains over name. It will also result in
> unnecessary work when reading non-btree indexes that contain
> name, since I don't think anything else has the same hack
> that btree name_ops does (grep for cstring in pg_opclass.dat).
> I think the correct thing is to see whether the index opclass
> for the column is btree name_ops. We don't seem to have an
> oid_symbol macro for that, but it shouldn't be hard to add,
> at least in HEAD.
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?
I've attached an updated patch that does it that way and also did a
round of commenting the code.
David
From 9d261c24072c781dd8bd82b0d51bfb950a4fcb1e Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 24 Apr 2024 14:14:44 +1200
Subject: [PATCH v3] Ensure we allocate NAMEDATALEN bytes for names in Index
Only Scans
---
src/backend/executor/nodeIndexonlyscan.c | 85 ++++++++++++++++++++++--
src/include/catalog/pg_opclass.dat | 7 +-
src/include/nodes/execnodes.h | 4 ++
3 files changed, 87 insertions(+), 9 deletions(-)
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index fcf6d1d932..bec34a62f1 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -35,18 +35,20 @@
#include "access/tableam.h"
#include "access/tupdesc.h"
#include "access/visibilitymap.h"
+#include "catalog/pg_opfamily_d.h"
#include "executor/executor.h"
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeIndexscan.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
#include "storage/predicate.h"
+#include "utils/builtins.h"
#include "utils/rel.h"
static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node);
-static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup,
- TupleDesc itupdesc);
+static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc);
/* ----------------------------------------------------------------
@@ -205,7 +207,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
}
else if (scandesc->xs_itup)
- StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
+ StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc);
else
elog(ERROR, "no data returned for index-only scan");
@@ -263,7 +265,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
* right now we don't need it elsewhere.
*/
static void
-StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
+StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc)
{
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
@@ -276,6 +279,37 @@ StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
ExecClearTuple(slot);
index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull);
+
+ /*
+ * Because the btree opclass for "name" is defined to store cstrings to
+ * avoid having to store the full NAMEDATALEN bytes in indexes, here we
+ * must copy the name Datum into a NAMEDATALEN-byte memory allocation. We
+ * mark this branch as unlikely as "name", or a derivative thereof,
+ * shouldn't be commonly used in tables other than the catalog tables and
+ * we'll only index only scan those from user queries to catalog tables,
+ * never from internal code performing a catalog lookup.
+ */
+ if (unlikely(node->ioss_NameColumnAttNums != NULL))
+ {
+ for (int idx = 0; idx < node->ioss_NameColumnAttCount; idx++)
+ {
+ int attnum = node->ioss_NameColumnAttNums[idx];
+ Name name;
+
+ /* skip null Datums */
+ if (slot->tts_isnull[attnum])
+ continue;
+
+ /* allocate the NAMEDATALEN and copy the datum into that memory */
+ name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
+ NAMEDATALEN);
+
+ /* use namestrcpy to zero-pad all trailing bytes */
+ namestrcpy(name, DatumGetCString(slot->tts_values[attnum]));
+ slot->tts_values[attnum] = NameGetDatum(name);
+ }
+ }
+
ExecStoreVirtualTuple(slot);
}
@@ -473,8 +507,10 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
{
IndexOnlyScanState *indexstate;
Relation currentRelation;
+ Relation indexRelation;
LOCKMODE lockmode;
TupleDesc tupDesc;
+ int namecount;
/*
* create state structure
@@ -547,7 +583,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
/* Open the index relation. */
lockmode = exec_rt_fetch(node->scan.scanrelid, estate)->rellockmode;
- indexstate->ioss_RelationDesc = index_open(node->indexid, lockmode);
+ indexRelation = index_open(node->indexid, lockmode);
+ indexstate->ioss_RelationDesc = indexRelation;
/*
* Initialize index-specific scan state
@@ -560,7 +597,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* build the index scan keys from the index qualification
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexqual,
false,
&indexstate->ioss_ScanKeys,
@@ -574,7 +611,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* any ORDER BY exprs have to be turned into scankeys in the same way
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexorderby,
true,
&indexstate->ioss_OrderByKeys,
@@ -603,6 +640,40 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ioss_RuntimeContext = NULL;
}
+ indexstate->ioss_NameColumnAttNums = NULL;
+ namecount = 0;
+
+ /*
+ * For indexes using the name_ops opclass, because that opclass is defined
+ * to store cstrings rather than NAMEDATALEN padded strings, we must
+ * detect this case so that we can properly pad name Datums with the full
+ * NAMEDATALEN bytes. Here we first determine if any such columns exist,
+ * if so, we create an array marking the index attribute number of each of
+ * them. StoreIndexTuple() handles copying the name Datums into a
+ * NAMEDATALEN-byte allocation.
+ */
+ for (int attnum = 0; attnum < tupDesc->natts; attnum++)
+ {
+ if (indexRelation->rd_opfamily[attnum] == TEXT_BTREE_FAM_OID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ namecount++;
+ }
+
+ if (namecount > 0)
+ {
+ int idx = 0;
+
+ indexstate->ioss_NameColumnAttNums = (int *) palloc(sizeof(int) * namecount);
+
+ for (int attnum = 0; attnum < tupDesc->natts; attnum++)
+ {
+ if (indexRelation->rd_opfamily[attnum] == TEXT_BTREE_FAM_OID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ indexstate->ioss_NameColumnAttNums[idx++] = attnum;
+ }
+ }
+ indexstate->ioss_NameColumnAttCount = namecount;
+
/*
* all done.
*/
diff --git a/src/include/catalog/pg_opclass.dat b/src/include/catalog/pg_opclass.dat
index 6c30770fe7..f503c652eb 100644
--- a/src/include/catalog/pg_opclass.dat
+++ b/src/include/catalog/pg_opclass.dat
@@ -91,8 +91,11 @@
# Here's an ugly little hack to save space in the system catalog indexes.
# btree doesn't ordinarily allow a storage type different from input type;
# but cstring and name are the same thing except for trailing padding,
-# and we can safely omit that within an index entry. So we declare the
-# btree opclass for name as using cstring storage type.
+# so we choose to omit that within an index entry. Here we declare the
+# btree opclass for name as using cstring storage type. This does require
+# that we pad the cstring out with the full NAMEDATALEN bytes when performing
+# index-only scans. See corresponding hacks in ExecInitIndexOnlyScan() and
+# StoreIndexTuple().
{ opcmethod => 'btree', opcname => 'name_ops', opcfamily => 'btree/text_ops',
opcintype => 'name', opckeytype => 'cstring' },
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d927ac44a8..5b98bbed91 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1690,6 +1690,8 @@ typedef struct IndexScanState
* TableSlot slot for holding tuples fetched from the table
* VMBuffer buffer in use for visibility map testing, if any
* PscanLen size of parallel index-only scan descriptor
+ * NameColumnAttNums attnums of name typed columns to pad to NAMEDATALEN
+ * NameColumnAttCount number of elements in the NameColumnAttNums array
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1709,6 +1711,8 @@ typedef struct IndexOnlyScanState
TupleTableSlot *ioss_TableSlot;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ int *ioss_NameColumnAttNums;
+ int ioss_NameColumnAttCount;
} IndexOnlyScanState;
/* ----------------
--
2.40.1
Attachments:
[text/plain] v3-0001-Ensure-we-allocate-NAMEDATALEN-bytes-for-names-in.patch (8.1K, ../../CAApHDvpW3ZEyTyogjD6qJsnuTFujLk3yTyCPX-to7VhftkrakQ@mail.gmail.com/2-v3-0001-Ensure-we-allocate-NAMEDATALEN-bytes-for-names-in.patch)
download | inline diff:
From 9d261c24072c781dd8bd82b0d51bfb950a4fcb1e Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 24 Apr 2024 14:14:44 +1200
Subject: [PATCH v3] Ensure we allocate NAMEDATALEN bytes for names in Index
Only Scans
---
src/backend/executor/nodeIndexonlyscan.c | 85 ++++++++++++++++++++++--
src/include/catalog/pg_opclass.dat | 7 +-
src/include/nodes/execnodes.h | 4 ++
3 files changed, 87 insertions(+), 9 deletions(-)
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index fcf6d1d932..bec34a62f1 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -35,18 +35,20 @@
#include "access/tableam.h"
#include "access/tupdesc.h"
#include "access/visibilitymap.h"
+#include "catalog/pg_opfamily_d.h"
#include "executor/executor.h"
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeIndexscan.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
#include "storage/predicate.h"
+#include "utils/builtins.h"
#include "utils/rel.h"
static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node);
-static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup,
- TupleDesc itupdesc);
+static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc);
/* ----------------------------------------------------------------
@@ -205,7 +207,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
}
else if (scandesc->xs_itup)
- StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
+ StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc);
else
elog(ERROR, "no data returned for index-only scan");
@@ -263,7 +265,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
* right now we don't need it elsewhere.
*/
static void
-StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
+StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc)
{
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
@@ -276,6 +279,37 @@ StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
ExecClearTuple(slot);
index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull);
+
+ /*
+ * Because the btree opclass for "name" is defined to store cstrings to
+ * avoid having to store the full NAMEDATALEN bytes in indexes, here we
+ * must copy the name Datum into a NAMEDATALEN-byte memory allocation. We
+ * mark this branch as unlikely as "name", or a derivative thereof,
+ * shouldn't be commonly used in tables other than the catalog tables and
+ * we'll only index only scan those from user queries to catalog tables,
+ * never from internal code performing a catalog lookup.
+ */
+ if (unlikely(node->ioss_NameColumnAttNums != NULL))
+ {
+ for (int idx = 0; idx < node->ioss_NameColumnAttCount; idx++)
+ {
+ int attnum = node->ioss_NameColumnAttNums[idx];
+ Name name;
+
+ /* skip null Datums */
+ if (slot->tts_isnull[attnum])
+ continue;
+
+ /* allocate the NAMEDATALEN and copy the datum into that memory */
+ name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
+ NAMEDATALEN);
+
+ /* use namestrcpy to zero-pad all trailing bytes */
+ namestrcpy(name, DatumGetCString(slot->tts_values[attnum]));
+ slot->tts_values[attnum] = NameGetDatum(name);
+ }
+ }
+
ExecStoreVirtualTuple(slot);
}
@@ -473,8 +507,10 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
{
IndexOnlyScanState *indexstate;
Relation currentRelation;
+ Relation indexRelation;
LOCKMODE lockmode;
TupleDesc tupDesc;
+ int namecount;
/*
* create state structure
@@ -547,7 +583,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
/* Open the index relation. */
lockmode = exec_rt_fetch(node->scan.scanrelid, estate)->rellockmode;
- indexstate->ioss_RelationDesc = index_open(node->indexid, lockmode);
+ indexRelation = index_open(node->indexid, lockmode);
+ indexstate->ioss_RelationDesc = indexRelation;
/*
* Initialize index-specific scan state
@@ -560,7 +597,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* build the index scan keys from the index qualification
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexqual,
false,
&indexstate->ioss_ScanKeys,
@@ -574,7 +611,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* any ORDER BY exprs have to be turned into scankeys in the same way
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexorderby,
true,
&indexstate->ioss_OrderByKeys,
@@ -603,6 +640,40 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ioss_RuntimeContext = NULL;
}
+ indexstate->ioss_NameColumnAttNums = NULL;
+ namecount = 0;
+
+ /*
+ * For indexes using the name_ops opclass, because that opclass is defined
+ * to store cstrings rather than NAMEDATALEN padded strings, we must
+ * detect this case so that we can properly pad name Datums with the full
+ * NAMEDATALEN bytes. Here we first determine if any such columns exist,
+ * if so, we create an array marking the index attribute number of each of
+ * them. StoreIndexTuple() handles copying the name Datums into a
+ * NAMEDATALEN-byte allocation.
+ */
+ for (int attnum = 0; attnum < tupDesc->natts; attnum++)
+ {
+ if (indexRelation->rd_opfamily[attnum] == TEXT_BTREE_FAM_OID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ namecount++;
+ }
+
+ if (namecount > 0)
+ {
+ int idx = 0;
+
+ indexstate->ioss_NameColumnAttNums = (int *) palloc(sizeof(int) * namecount);
+
+ for (int attnum = 0; attnum < tupDesc->natts; attnum++)
+ {
+ if (indexRelation->rd_opfamily[attnum] == TEXT_BTREE_FAM_OID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ indexstate->ioss_NameColumnAttNums[idx++] = attnum;
+ }
+ }
+ indexstate->ioss_NameColumnAttCount = namecount;
+
/*
* all done.
*/
diff --git a/src/include/catalog/pg_opclass.dat b/src/include/catalog/pg_opclass.dat
index 6c30770fe7..f503c652eb 100644
--- a/src/include/catalog/pg_opclass.dat
+++ b/src/include/catalog/pg_opclass.dat
@@ -91,8 +91,11 @@
# Here's an ugly little hack to save space in the system catalog indexes.
# btree doesn't ordinarily allow a storage type different from input type;
# but cstring and name are the same thing except for trailing padding,
-# and we can safely omit that within an index entry. So we declare the
-# btree opclass for name as using cstring storage type.
+# so we choose to omit that within an index entry. Here we declare the
+# btree opclass for name as using cstring storage type. This does require
+# that we pad the cstring out with the full NAMEDATALEN bytes when performing
+# index-only scans. See corresponding hacks in ExecInitIndexOnlyScan() and
+# StoreIndexTuple().
{ opcmethod => 'btree', opcname => 'name_ops', opcfamily => 'btree/text_ops',
opcintype => 'name', opckeytype => 'cstring' },
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d927ac44a8..5b98bbed91 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1690,6 +1690,8 @@ typedef struct IndexScanState
* TableSlot slot for holding tuples fetched from the table
* VMBuffer buffer in use for visibility map testing, if any
* PscanLen size of parallel index-only scan descriptor
+ * NameColumnAttNums attnums of name typed columns to pad to NAMEDATALEN
+ * NameColumnAttCount number of elements in the NameColumnAttNums array
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1709,6 +1711,8 @@ typedef struct IndexOnlyScanState
TupleTableSlot *ioss_TableSlot;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ int *ioss_NameColumnAttNums;
+ int ioss_NameColumnAttCount;
} IndexOnlyScanState;
/* ----------------
--
2.40.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2024-04-29 14:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-04-29 14:28 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 19+ messages in thread
From: Alexander Lakhin @ 2024-04-29 14:00 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
Hello David,
29.04.2024 03:54, David Rowley wrote:
> On Fri, 26 Apr 2024 at 03:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I do not like testing "atttypid == NAMEOID" one bit. As noted,
>> that will fail on domains over name. It will also result in
>> unnecessary work when reading non-btree indexes that contain
>> name, since I don't think anything else has the same hack
>> that btree name_ops does (grep for cstring in pg_opclass.dat).
>> I think the correct thing is to see whether the index opclass
>> for the column is btree name_ops. We don't seem to have an
>> oid_symbol macro for that, but it shouldn't be hard to add,
>> at least in HEAD.
> 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?
>
> I've attached an updated patch that does it that way and also did a
> round of commenting the code.
With the v3 patch applied, `make check` fails for me under Valgrind:
ok 66 + create_index 33585 ms
not ok 67 + create_index_spgist 14446 ms
# (test process exited with exit code 2)
==00:00:04:57.033 4152513== Invalid read of size 4
==00:00:04:57.033 4152513== at 0x456499: ExecInitIndexOnlyScan (nodeIndexonlyscan.c:657)
==00:00:04:57.033 4152513== by 0x435EE6: ExecInitNode (execProcnode.c:225)
==00:00:04:57.033 4152513== by 0x459780: ExecInitLimit (nodeLimit.c:477)
...
==00:00:04:57.033 4152513== Address 0x73263b4 is 612 bytes inside a recently re-allocated block of size 1,024 alloc'd
==00:00:04:57.033 4152513== at 0x4848899: malloc (vg_replace_malloc.c:381)
==00:00:04:57.033 4152513== by 0x7A7DFC: AllocSetContextCreateInternal (aset.c:444)
==00:00:04:57.033 4152513== by 0x775644: RelationInitIndexAccessInfo (relcache.c:1481)
==00:00:04:57.033 4152513== by 0x775E4B: RelationBuildDesc (relcache.c:1207)
...
==00:00:04:57.033 4152513==
==00:00:04:57.033 4152513== Exit program on first error (--exit-on-first-error=yes)
2024-04-29 12:32:14.965 UTC postmaster[4109162] LOG: server process (PID 4152513) exited with exit code 1
2024-04-29 12:32:14.965 UTC postmaster[4109162] DETAIL: Failed process was running: SELECT p, dist FROM
quad_point_tbl_ord_seq1 ORDER BY p <-> '0,0' LIMIT 10;
The quad_point_tbl_ord_seq1 index is defined as
CREATE INDEX ON quad_point_tbl_ord_seq1 USING spgist(p) INCLUDE(dist);
Best regards,
Alexander
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-29 14:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
@ 2024-04-29 14:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-30 00:48 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 19+ messages in thread
From: Tom Lane @ 2024-04-29 14:28 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: David Rowley <dgrowleyml@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-29 14:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-29 14:28 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-04-30 00:48 ` David Rowley <dgrowleyml@gmail.com>
2024-04-30 14:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-30 15:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
0 siblings, 2 replies; 19+ messages in thread
From: David Rowley @ 2024-04-30 00:48 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org
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
From bd9ee5e164f7a7f9f4489d617383f83cbb669fb1 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 24 Apr 2024 14:14:44 +1200
Subject: [PATCH v4] Ensure we allocate NAMEDATALEN bytes for names in Index
Only Scans
---
src/backend/executor/nodeIndexonlyscan.c | 94 +++++++++++++++++--
src/include/catalog/pg_opclass.dat | 7 +-
src/include/nodes/execnodes.h | 4 +
src/test/regress/expected/index_including.out | 25 +++++
src/test/regress/sql/index_including.sql | 19 ++++
5 files changed, 140 insertions(+), 9 deletions(-)
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index fcf6d1d932..d82da50a69 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -35,18 +35,20 @@
#include "access/tableam.h"
#include "access/tupdesc.h"
#include "access/visibilitymap.h"
+#include "catalog/pg_opfamily_d.h"
#include "executor/executor.h"
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeIndexscan.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
#include "storage/predicate.h"
+#include "utils/builtins.h"
#include "utils/rel.h"
static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node);
-static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup,
- TupleDesc itupdesc);
+static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc);
/* ----------------------------------------------------------------
@@ -205,7 +207,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
}
else if (scandesc->xs_itup)
- StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
+ StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc);
else
elog(ERROR, "no data returned for index-only scan");
@@ -263,7 +265,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
* right now we don't need it elsewhere.
*/
static void
-StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
+StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc)
{
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
@@ -276,6 +279,37 @@ StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
ExecClearTuple(slot);
index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull);
+
+ /*
+ * Copy all name columns stored as cstrings back into a NAMEDATALEN byte
+ * sized allocation. We mark this branch as unlikely as generally "name"
+ * is used only for the system catalogs and this would have to be a user
+ * query running on those or some other user table with an index on a name
+ * column.
+ */
+ if (unlikely(node->ioss_NameCStringAttNums != NULL))
+ {
+ int attcount = node->ioss_NameCStringCount;
+
+ for (int idx = 0; idx < attcount; idx++)
+ {
+ int attnum = node->ioss_NameCStringAttNums[idx];
+ Name name;
+
+ /* skip null Datums */
+ if (slot->tts_isnull[attnum])
+ continue;
+
+ /* allocate the NAMEDATALEN and copy the datum into that memory */
+ name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
+ NAMEDATALEN);
+
+ /* use namestrcpy to zero-pad all trailing bytes */
+ namestrcpy(name, DatumGetCString(slot->tts_values[attnum]));
+ slot->tts_values[attnum] = NameGetDatum(name);
+ }
+ }
+
ExecStoreVirtualTuple(slot);
}
@@ -473,8 +507,11 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
{
IndexOnlyScanState *indexstate;
Relation currentRelation;
+ Relation indexRelation;
LOCKMODE lockmode;
TupleDesc tupDesc;
+ int indnkeyatts;
+ int namecount;
/*
* create state structure
@@ -547,7 +584,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
/* Open the index relation. */
lockmode = exec_rt_fetch(node->scan.scanrelid, estate)->rellockmode;
- indexstate->ioss_RelationDesc = index_open(node->indexid, lockmode);
+ indexRelation = index_open(node->indexid, lockmode);
+ indexstate->ioss_RelationDesc = indexRelation;
/*
* Initialize index-specific scan state
@@ -560,7 +598,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* build the index scan keys from the index qualification
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexqual,
false,
&indexstate->ioss_ScanKeys,
@@ -574,7 +612,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* any ORDER BY exprs have to be turned into scankeys in the same way
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexorderby,
true,
&indexstate->ioss_OrderByKeys,
@@ -603,6 +641,48 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ioss_RuntimeContext = NULL;
}
+ indexstate->ioss_NameCStringAttNums = NULL;
+ indnkeyatts = indexRelation->rd_index->indnkeyatts;
+ namecount = 0;
+
+ /*
+ * The "name" type for btree uses text_ops which results in storing
+ * cstrings in the indexed keys rather than names. Here we detect that in
+ * a generic way in case other index AMs want to do the same optimization.
+ * Check for opclasses with an opcintype of NAMEOID and a index tuple
+ * descriptor with CSTRINGOID. If any of these are found, create an array
+ * marking the index attribute number of each of them. StoreIndexTuple()
+ * handles copying the name Datums into a NAMEDATALEN-byte allocation.
+ */
+
+ /* First count the number of such index keys */
+ for (int attnum = 0; attnum < indnkeyatts; attnum++)
+ {
+ if (indexRelation->rd_att->attrs[attnum].atttypid == CSTRINGOID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ namecount++;
+ }
+
+ if (namecount > 0)
+ {
+ int idx = 0;
+
+ /*
+ * Now create an array to mark the attribute numbers of the keys that
+ * need to be converted from cstring to name.
+ */
+ indexstate->ioss_NameCStringAttNums = (int *) palloc(sizeof(int) * namecount);
+
+ for (int attnum = 0; attnum < indnkeyatts; attnum++)
+ {
+ if (indexRelation->rd_att->attrs[attnum].atttypid == CSTRINGOID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ indexstate->ioss_NameCStringAttNums[idx++] = attnum;
+ }
+ }
+
+ indexstate->ioss_NameCStringCount = namecount;
+
/*
* all done.
*/
diff --git a/src/include/catalog/pg_opclass.dat b/src/include/catalog/pg_opclass.dat
index 6c30770fe7..f503c652eb 100644
--- a/src/include/catalog/pg_opclass.dat
+++ b/src/include/catalog/pg_opclass.dat
@@ -91,8 +91,11 @@
# Here's an ugly little hack to save space in the system catalog indexes.
# btree doesn't ordinarily allow a storage type different from input type;
# but cstring and name are the same thing except for trailing padding,
-# and we can safely omit that within an index entry. So we declare the
-# btree opclass for name as using cstring storage type.
+# so we choose to omit that within an index entry. Here we declare the
+# btree opclass for name as using cstring storage type. This does require
+# that we pad the cstring out with the full NAMEDATALEN bytes when performing
+# index-only scans. See corresponding hacks in ExecInitIndexOnlyScan() and
+# StoreIndexTuple().
{ opcmethod => 'btree', opcname => 'name_ops', opcfamily => 'btree/text_ops',
opcintype => 'name', opckeytype => 'cstring' },
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d927ac44a8..c7b1c008cb 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1690,6 +1690,8 @@ typedef struct IndexScanState
* TableSlot slot for holding tuples fetched from the table
* VMBuffer buffer in use for visibility map testing, if any
* PscanLen size of parallel index-only scan descriptor
+ * NameCStringAttNums attnums of name typed columns to pad to NAMEDATALEN
+ * NameCStringCount number of elements in the NameCStringAttNums array
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1709,6 +1711,8 @@ typedef struct IndexOnlyScanState
TupleTableSlot *ioss_TableSlot;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ int *ioss_NameCStringAttNums;
+ int ioss_NameCStringCount;
} IndexOnlyScanState;
/* ----------------
diff --git a/src/test/regress/expected/index_including.out b/src/test/regress/expected/index_including.out
index 86510687c7..ea8b2454bf 100644
--- a/src/test/regress/expected/index_including.out
+++ b/src/test/regress/expected/index_including.out
@@ -398,3 +398,28 @@ Indexes:
"tbl_c1_c2_c3_c4_key" UNIQUE CONSTRAINT, btree (c1, c2) INCLUDE (c3, c4)
DROP TABLE tbl;
+/*
+ * 10. Test coverage for names stored as cstrings in indexes
+ */
+CREATE TABLE nametbl (c1 int, c2 name, c3 float);
+CREATE INDEX nametbl_c1_c2_idx ON nametbl (c2, c1) INCLUDE (c3);
+INSERT INTO nametbl VALUES(1, 'two', 3.0);
+VACUUM nametbl;
+SET enable_seqscan = 0;
+-- Ensure we get an index only scan plan
+EXPLAIN (COSTS OFF) SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Only Scan using nametbl_c1_c2_idx on nametbl
+ Index Cond: ((c2 = 'two'::name) AND (c1 = 1))
+(2 rows)
+
+-- Validate the results look sane
+SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+ c2 | c1 | c3
+-----+----+----
+ two | 1 | 3
+(1 row)
+
+RESET enable_seqscan;
+DROP TABLE nametbl;
diff --git a/src/test/regress/sql/index_including.sql b/src/test/regress/sql/index_including.sql
index 44b340053b..ad9cbdd028 100644
--- a/src/test/regress/sql/index_including.sql
+++ b/src/test/regress/sql/index_including.sql
@@ -217,3 +217,22 @@ ALTER TABLE tbl ALTER c1 TYPE bigint;
ALTER TABLE tbl ALTER c3 TYPE bigint;
\d tbl
DROP TABLE tbl;
+
+/*
+ * 10. Test coverage for names stored as cstrings in indexes
+ */
+CREATE TABLE nametbl (c1 int, c2 name, c3 float);
+CREATE INDEX nametbl_c1_c2_idx ON nametbl (c2, c1) INCLUDE (c3);
+INSERT INTO nametbl VALUES(1, 'two', 3.0);
+VACUUM nametbl;
+SET enable_seqscan = 0;
+
+-- Ensure we get an index only scan plan
+EXPLAIN (COSTS OFF) SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+
+-- Validate the results look sane
+SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+
+RESET enable_seqscan;
+
+DROP TABLE nametbl;
\ No newline at end of file
--
2.40.1
Attachments:
[text/plain] v4-0001-Ensure-we-allocate-NAMEDATALEN-bytes-for-names-in.patch (10.5K, ../../CAApHDvrG=X6YK3WajkvybT5k=nnWt3W+yr8tMcFHYVVjej8zKw@mail.gmail.com/2-v4-0001-Ensure-we-allocate-NAMEDATALEN-bytes-for-names-in.patch)
download | inline diff:
From bd9ee5e164f7a7f9f4489d617383f83cbb669fb1 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Wed, 24 Apr 2024 14:14:44 +1200
Subject: [PATCH v4] Ensure we allocate NAMEDATALEN bytes for names in Index
Only Scans
---
src/backend/executor/nodeIndexonlyscan.c | 94 +++++++++++++++++--
src/include/catalog/pg_opclass.dat | 7 +-
src/include/nodes/execnodes.h | 4 +
src/test/regress/expected/index_including.out | 25 +++++
src/test/regress/sql/index_including.sql | 19 ++++
5 files changed, 140 insertions(+), 9 deletions(-)
diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c
index fcf6d1d932..d82da50a69 100644
--- a/src/backend/executor/nodeIndexonlyscan.c
+++ b/src/backend/executor/nodeIndexonlyscan.c
@@ -35,18 +35,20 @@
#include "access/tableam.h"
#include "access/tupdesc.h"
#include "access/visibilitymap.h"
+#include "catalog/pg_opfamily_d.h"
#include "executor/executor.h"
#include "executor/nodeIndexonlyscan.h"
#include "executor/nodeIndexscan.h"
#include "miscadmin.h"
#include "storage/bufmgr.h"
#include "storage/predicate.h"
+#include "utils/builtins.h"
#include "utils/rel.h"
static TupleTableSlot *IndexOnlyNext(IndexOnlyScanState *node);
-static void StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup,
- TupleDesc itupdesc);
+static void StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc);
/* ----------------------------------------------------------------
@@ -205,7 +207,7 @@ IndexOnlyNext(IndexOnlyScanState *node)
ExecForceStoreHeapTuple(scandesc->xs_hitup, slot, false);
}
else if (scandesc->xs_itup)
- StoreIndexTuple(slot, scandesc->xs_itup, scandesc->xs_itupdesc);
+ StoreIndexTuple(node, slot, scandesc->xs_itup, scandesc->xs_itupdesc);
else
elog(ERROR, "no data returned for index-only scan");
@@ -263,7 +265,8 @@ IndexOnlyNext(IndexOnlyScanState *node)
* right now we don't need it elsewhere.
*/
static void
-StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
+StoreIndexTuple(IndexOnlyScanState *node, TupleTableSlot *slot,
+ IndexTuple itup, TupleDesc itupdesc)
{
/*
* Note: we must use the tupdesc supplied by the AM in index_deform_tuple,
@@ -276,6 +279,37 @@ StoreIndexTuple(TupleTableSlot *slot, IndexTuple itup, TupleDesc itupdesc)
ExecClearTuple(slot);
index_deform_tuple(itup, itupdesc, slot->tts_values, slot->tts_isnull);
+
+ /*
+ * Copy all name columns stored as cstrings back into a NAMEDATALEN byte
+ * sized allocation. We mark this branch as unlikely as generally "name"
+ * is used only for the system catalogs and this would have to be a user
+ * query running on those or some other user table with an index on a name
+ * column.
+ */
+ if (unlikely(node->ioss_NameCStringAttNums != NULL))
+ {
+ int attcount = node->ioss_NameCStringCount;
+
+ for (int idx = 0; idx < attcount; idx++)
+ {
+ int attnum = node->ioss_NameCStringAttNums[idx];
+ Name name;
+
+ /* skip null Datums */
+ if (slot->tts_isnull[attnum])
+ continue;
+
+ /* allocate the NAMEDATALEN and copy the datum into that memory */
+ name = (Name) MemoryContextAlloc(node->ss.ps.ps_ExprContext->ecxt_per_tuple_memory,
+ NAMEDATALEN);
+
+ /* use namestrcpy to zero-pad all trailing bytes */
+ namestrcpy(name, DatumGetCString(slot->tts_values[attnum]));
+ slot->tts_values[attnum] = NameGetDatum(name);
+ }
+ }
+
ExecStoreVirtualTuple(slot);
}
@@ -473,8 +507,11 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
{
IndexOnlyScanState *indexstate;
Relation currentRelation;
+ Relation indexRelation;
LOCKMODE lockmode;
TupleDesc tupDesc;
+ int indnkeyatts;
+ int namecount;
/*
* create state structure
@@ -547,7 +584,8 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
/* Open the index relation. */
lockmode = exec_rt_fetch(node->scan.scanrelid, estate)->rellockmode;
- indexstate->ioss_RelationDesc = index_open(node->indexid, lockmode);
+ indexRelation = index_open(node->indexid, lockmode);
+ indexstate->ioss_RelationDesc = indexRelation;
/*
* Initialize index-specific scan state
@@ -560,7 +598,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* build the index scan keys from the index qualification
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexqual,
false,
&indexstate->ioss_ScanKeys,
@@ -574,7 +612,7 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
* any ORDER BY exprs have to be turned into scankeys in the same way
*/
ExecIndexBuildScanKeys((PlanState *) indexstate,
- indexstate->ioss_RelationDesc,
+ indexRelation,
node->indexorderby,
true,
&indexstate->ioss_OrderByKeys,
@@ -603,6 +641,48 @@ ExecInitIndexOnlyScan(IndexOnlyScan *node, EState *estate, int eflags)
indexstate->ioss_RuntimeContext = NULL;
}
+ indexstate->ioss_NameCStringAttNums = NULL;
+ indnkeyatts = indexRelation->rd_index->indnkeyatts;
+ namecount = 0;
+
+ /*
+ * The "name" type for btree uses text_ops which results in storing
+ * cstrings in the indexed keys rather than names. Here we detect that in
+ * a generic way in case other index AMs want to do the same optimization.
+ * Check for opclasses with an opcintype of NAMEOID and a index tuple
+ * descriptor with CSTRINGOID. If any of these are found, create an array
+ * marking the index attribute number of each of them. StoreIndexTuple()
+ * handles copying the name Datums into a NAMEDATALEN-byte allocation.
+ */
+
+ /* First count the number of such index keys */
+ for (int attnum = 0; attnum < indnkeyatts; attnum++)
+ {
+ if (indexRelation->rd_att->attrs[attnum].atttypid == CSTRINGOID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ namecount++;
+ }
+
+ if (namecount > 0)
+ {
+ int idx = 0;
+
+ /*
+ * Now create an array to mark the attribute numbers of the keys that
+ * need to be converted from cstring to name.
+ */
+ indexstate->ioss_NameCStringAttNums = (int *) palloc(sizeof(int) * namecount);
+
+ for (int attnum = 0; attnum < indnkeyatts; attnum++)
+ {
+ if (indexRelation->rd_att->attrs[attnum].atttypid == CSTRINGOID &&
+ indexRelation->rd_opcintype[attnum] == NAMEOID)
+ indexstate->ioss_NameCStringAttNums[idx++] = attnum;
+ }
+ }
+
+ indexstate->ioss_NameCStringCount = namecount;
+
/*
* all done.
*/
diff --git a/src/include/catalog/pg_opclass.dat b/src/include/catalog/pg_opclass.dat
index 6c30770fe7..f503c652eb 100644
--- a/src/include/catalog/pg_opclass.dat
+++ b/src/include/catalog/pg_opclass.dat
@@ -91,8 +91,11 @@
# Here's an ugly little hack to save space in the system catalog indexes.
# btree doesn't ordinarily allow a storage type different from input type;
# but cstring and name are the same thing except for trailing padding,
-# and we can safely omit that within an index entry. So we declare the
-# btree opclass for name as using cstring storage type.
+# so we choose to omit that within an index entry. Here we declare the
+# btree opclass for name as using cstring storage type. This does require
+# that we pad the cstring out with the full NAMEDATALEN bytes when performing
+# index-only scans. See corresponding hacks in ExecInitIndexOnlyScan() and
+# StoreIndexTuple().
{ opcmethod => 'btree', opcname => 'name_ops', opcfamily => 'btree/text_ops',
opcintype => 'name', opckeytype => 'cstring' },
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index d927ac44a8..c7b1c008cb 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1690,6 +1690,8 @@ typedef struct IndexScanState
* TableSlot slot for holding tuples fetched from the table
* VMBuffer buffer in use for visibility map testing, if any
* PscanLen size of parallel index-only scan descriptor
+ * NameCStringAttNums attnums of name typed columns to pad to NAMEDATALEN
+ * NameCStringCount number of elements in the NameCStringAttNums array
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1709,6 +1711,8 @@ typedef struct IndexOnlyScanState
TupleTableSlot *ioss_TableSlot;
Buffer ioss_VMBuffer;
Size ioss_PscanLen;
+ int *ioss_NameCStringAttNums;
+ int ioss_NameCStringCount;
} IndexOnlyScanState;
/* ----------------
diff --git a/src/test/regress/expected/index_including.out b/src/test/regress/expected/index_including.out
index 86510687c7..ea8b2454bf 100644
--- a/src/test/regress/expected/index_including.out
+++ b/src/test/regress/expected/index_including.out
@@ -398,3 +398,28 @@ Indexes:
"tbl_c1_c2_c3_c4_key" UNIQUE CONSTRAINT, btree (c1, c2) INCLUDE (c3, c4)
DROP TABLE tbl;
+/*
+ * 10. Test coverage for names stored as cstrings in indexes
+ */
+CREATE TABLE nametbl (c1 int, c2 name, c3 float);
+CREATE INDEX nametbl_c1_c2_idx ON nametbl (c2, c1) INCLUDE (c3);
+INSERT INTO nametbl VALUES(1, 'two', 3.0);
+VACUUM nametbl;
+SET enable_seqscan = 0;
+-- Ensure we get an index only scan plan
+EXPLAIN (COSTS OFF) SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Only Scan using nametbl_c1_c2_idx on nametbl
+ Index Cond: ((c2 = 'two'::name) AND (c1 = 1))
+(2 rows)
+
+-- Validate the results look sane
+SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+ c2 | c1 | c3
+-----+----+----
+ two | 1 | 3
+(1 row)
+
+RESET enable_seqscan;
+DROP TABLE nametbl;
diff --git a/src/test/regress/sql/index_including.sql b/src/test/regress/sql/index_including.sql
index 44b340053b..ad9cbdd028 100644
--- a/src/test/regress/sql/index_including.sql
+++ b/src/test/regress/sql/index_including.sql
@@ -217,3 +217,22 @@ ALTER TABLE tbl ALTER c1 TYPE bigint;
ALTER TABLE tbl ALTER c3 TYPE bigint;
\d tbl
DROP TABLE tbl;
+
+/*
+ * 10. Test coverage for names stored as cstrings in indexes
+ */
+CREATE TABLE nametbl (c1 int, c2 name, c3 float);
+CREATE INDEX nametbl_c1_c2_idx ON nametbl (c2, c1) INCLUDE (c3);
+INSERT INTO nametbl VALUES(1, 'two', 3.0);
+VACUUM nametbl;
+SET enable_seqscan = 0;
+
+-- Ensure we get an index only scan plan
+EXPLAIN (COSTS OFF) SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+
+-- Validate the results look sane
+SELECT c2, c1, c3 FROM nametbl WHERE c2 = 'two' AND c1 = 1;
+
+RESET enable_seqscan;
+
+DROP TABLE nametbl;
\ No newline at end of file
--
2.40.1
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-29 14:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-29 14:28 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-30 00:48 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2024-04-30 14:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-05-01 01:30 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 19+ messages in thread
From: Tom Lane @ 2024-04-30 14:54 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org
David Rowley <dgrowleyml@gmail.com> writes:
> 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.
This version LGTM. The field names are fine, or at least I don't see
a reason to prefer the other way. One trivial nit: should we make
the array be AttrNumber* instead of int*? The space saving would be
negligible (it being very unlikely that there's ever more than one
entry), but AttrNumber is a more specific description of what it is.
regards, tom lane
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-29 14:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-29 14:28 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-30 00:48 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-30 14:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-05-01 01:30 ` David Rowley <dgrowleyml@gmail.com>
0 siblings, 0 replies; 19+ messages in thread
From: David Rowley @ 2024-05-01 01:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org
On Wed, 1 May 2024 at 02:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This version LGTM. The field names are fine, or at least I don't see
> a reason to prefer the other way. One trivial nit: should we make
> the array be AttrNumber* instead of int*? The space saving would be
> negligible (it being very unlikely that there's ever more than one
> entry), but AttrNumber is a more specific description of what it is.
I've pushed this after adjusting the int array to become an AttrNumber array.
I toyed with the idea of having the loop iterator in
ExecInitIndexOnlyScan() an AttrNumber type, but in the end went with a
cast. I felt this way was more robust against any possible future
widening of the AttrNumber typedef. I added the cast to prevent any
compilers from warning about truncation. This can't happen since
indnkeyatts is int16, but I don't know if all compilers would be able
to figure that out, so added a cast.
Thank you to both of you for reviewing this.
David
^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-25 15:50 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
2024-04-29 14:00 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Alexander Lakhin <exclusion@gmail.com>
2024-04-29 14:28 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize Tom Lane <tgl@sss.pgh.pa.us>
2024-04-30 00:48 ` Re: BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize David Rowley <dgrowleyml@gmail.com>
@ 2024-04-30 15:00 ` Alexander Lakhin <exclusion@gmail.com>
1 sibling, 0 replies; 19+ messages in thread
From: Alexander Lakhin @ 2024-04-30 15:00 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Robert Haas <robertmhaas@gmail.com>; pgsql-bugs@lists.postgresql.org
Hello David,
30.04.2024 03:48, David Rowley wrote:
> On Tue, 30 Apr 2024 at 02:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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
Yes, I saw the same with pageinspect.
I could not find cases (I've tested also ranges and arrays based on the
name type) where the v4 behaves incorrectly, so I think it solves the
problem.
As a side note, perhaps this addition:
+#include "catalog/pg_opfamily_d.h"
is not needed for the current implementation.
Thank you for the fix!
Best regards,
Alexander
^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2024-05-01 01:30 UTC | newest]
Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20 14:00 BUG #17855: Uninitialised memory used when the name type value processed in binary mode of Memoize PG Bug reporting form <noreply@postgresql.org>
2023-03-22 06:18 ` David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:10 ` David Rowley <dgrowleyml@gmail.com>
2023-03-23 03:25 ` Peter Geoghegan <pg@bowt.ie>
2023-03-23 04:01 ` David Rowley <dgrowleyml@gmail.com>
2023-03-23 04:27 ` Peter Geoghegan <pg@bowt.ie>
2023-03-23 04:56 ` David Rowley <dgrowleyml@gmail.com>
2023-03-25 05:32 ` David Rowley <dgrowleyml@gmail.com>
2023-09-09 08:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-04-24 02:25 ` David Rowley <dgrowleyml@gmail.com>
2024-04-24 13:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-04-25 15:50 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-29 00:54 ` David Rowley <dgrowleyml@gmail.com>
2024-04-29 14:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-04-29 14:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-30 00:48 ` David Rowley <dgrowleyml@gmail.com>
2024-04-30 14:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-05-01 01:30 ` David Rowley <dgrowleyml@gmail.com>
2024-04-30 15:00 ` Alexander Lakhin <exclusion@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox