pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18135: Incorrect memory access occurs when attaching a partition with an index
10+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2023-09-28 d6425987ba 9f71e10d65 12402b9311 59371f1aeb 9d6d8d7049 83ba050f92 3caedf2461 Fix checking of index expressions in CompareIndexInfo().
* BUG #18135: Incorrect memory access occurs when attaching a partition with an index
@ 2023-09-27 10:00 PG Bug reporting form <noreply@postgresql.org>
2023-09-28 03:30 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
2023-09-28 17:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 10+ messages in thread
From: PG Bug reporting form @ 2023-09-27 10: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: 18135
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 16.0
Operating system: Ubuntu 22.04
Description:
When the following query:
CREATE TABLE t(a int) PARTITION BY RANGE (a);
CREATE INDEX ON t((a + 1));
CREATE TABLE tp(a int not null) PARTITION BY RANGE (a);
CREATE INDEX ON tp((a));
ALTER TABLE t ATTACH PARTITION tp FOR VALUES FROM (0) TO (1);
executed under Valgrind, it leads to an incorrect memory access:
==00:00:00:03.947 396156== Invalid read of size 2
==00:00:00:03.947 396156== at 0x2E323D: CompareIndexInfo (index.c:2572)
==00:00:00:03.947 396156== by 0x3D009B: AttachPartitionEnsureIndexes
(tablecmds.c:18797)
==00:00:00:03.947 396156== by 0x3D8B4F: ATExecAttachPartition
(tablecmds.c:18578)
==00:00:00:03.947 396156== by 0x3D9A88: ATExecCmd (tablecmds.c:5379)
==00:00:00:03.947 396156== by 0x3D9BC7: ATRewriteCatalogs
(tablecmds.c:5063)
==00:00:00:03.947 396156== by 0x3D9E29: ATController (tablecmds.c:4638)
==00:00:00:03.947 396156== by 0x3D9EB3: AlterTable (tablecmds.c:4293)
==00:00:00:03.947 396156== by 0x5FAC30: ProcessUtilitySlow
(utility.c:1329)
==00:00:00:03.947 396156== by 0x5FA6C4: standard_ProcessUtility
(utility.c:1078)
==00:00:00:03.947 396156== by 0x5FA789: ProcessUtility (utility.c:530)
==00:00:00:03.947 396156== by 0x5F7B46: PortalRunUtility
(pquery.c:1158)
==00:00:00:03.947 396156== by 0x5F7E40: PortalRunMulti (pquery.c:1315)
==00:00:00:03.947 396156== Address 0x10736bbe is 2,446 bytes inside a
recently re-allocated block of size 8,192 alloc'd
==00:00:00:03.947 396156== at 0x4848899: malloc
(vg_replace_malloc.c:381)
==00:00:00:03.947 396156== by 0x768F55: AllocSetContextCreateInternal
(aset.c:444)
==00:00:00:03.947 396156== by 0x7512FF: hash_create (dynahash.c:385)
...
The function CompareIndexInfo() contains the code:
/* ignore expressions at this stage */
if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
(attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
info1->ii_IndexAttrNumbers[i]))
return false;
where info1->ii_IndexAttrNumbers[i] is checked for InvalidAttrNumber
(i. e. it's not an expression), but info2->ii_IndexAttrNumbers[i] is not.
In addition, there is a check whether both indexes are (are not)
expression
indexes, but it's placed below...
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
@ 2023-09-28 03:30 ` Michael Paquier <michael@paquier.xyz>
2023-09-28 05:00 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Alexander Lakhin <exclusion@gmail.com>
1 sibling, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2023-09-28 03:30 UTC (permalink / raw)
To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Sep 27, 2023 at 10:00:01AM +0000, PG Bug reporting form wrote:
> executed under Valgrind, it leads to an incorrect memory access:
> ==00:00:00:03.947 396156== Invalid read of size 2
> ==00:00:00:03.947 396156== at 0x2E323D: CompareIndexInfo (index.c:2572)
> ==00:00:00:03.947 396156== by 0x3D009B: AttachPartitionEnsureIndexes
> (tablecmds.c:18797)
> ==00:00:00:03.947 396156== by 0x3D8B4F: ATExecAttachPartition
> (tablecmds.c:18578)
> ==00:00:00:03.947 396156== by 0x3D9A88: ATExecCmd (tablecmds.c:5379)
> ==00:00:00:03.947 396156== by 0x3D9BC7: ATRewriteCatalogs
> (tablecmds.c:5063)
I have just tested that on HEAD and REL_16_STABLE, but fail to see
this report, which is weird (3.19.0 here). Are you using any specific
option of valgrind I should be aware of? Here is what I used, for
reference:
valgrind \
--suppressions=$PG_SOURCE/src/tools/valgrind.supp \
--trace-children=yes --track-origins=yes --read-var-info=yes \
postgres -D REST_OF_ARGS
> The function CompareIndexInfo() contains the code:
> /* ignore expressions at this stage */
> if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
> (attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
> info1->ii_IndexAttrNumbers[i]))
> return false;
>
> where info1->ii_IndexAttrNumbers[i] is checked for InvalidAttrNumber
> (i. e. it's not an expression), but info2->ii_IndexAttrNumbers[i] is not.
Anyway, I can see your point here. info2's first attnum is 0 so we
look at an imaginary position in attmap->attnums. So, yes, that's
wrong.
> In addition, there is a check whether both indexes are (are not)
> expression indexes, but it's placed below...
Sure, but this makes the check a bit cheaper if the indexes to compare
use expr and non-expr attributes at the same attnums, no? Except if I
am missing something, the attached should be sufficient.
--
Michael
Attachments:
compare-index-mem.patch (text/x-diff, 542B) download
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 716de171a4..bbc9b262a0 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2570,6 +2570,7 @@ CompareIndexInfo(const IndexInfo *info1, const IndexInfo *info2,
/* ignore expressions at this stage */
if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
+ (info2->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
(attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
info1->ii_IndexAttrNumbers[i]))
return false;
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 03:30 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
@ 2023-09-28 05:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-09-28 07:07 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Alexander Lakhin @ 2023-09-28 05:00 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; pgsql-bugs@lists.postgresql.org
Hi Michael,
28.09.2023 06:30, Michael Paquier wrote:
> On Wed, Sep 27, 2023 at 10:00:01AM +0000, PG Bug reporting form wrote:
>> executed under Valgrind, it leads to an incorrect memory access:
>> ==00:00:00:03.947 396156== Invalid read of size 2
>> ==00:00:00:03.947 396156== at 0x2E323D: CompareIndexInfo (index.c:2572)
>> ==00:00:00:03.947 396156== by 0x3D009B: AttachPartitionEnsureIndexes
>> (tablecmds.c:18797)
>> ==00:00:00:03.947 396156== by 0x3D8B4F: ATExecAttachPartition
>> (tablecmds.c:18578)
>> ==00:00:00:03.947 396156== by 0x3D9A88: ATExecCmd (tablecmds.c:5379)
>> ==00:00:00:03.947 396156== by 0x3D9BC7: ATRewriteCatalogs
>> (tablecmds.c:5063)
Thank you for paying attention to it!
> I have just tested that on HEAD and REL_16_STABLE, but fail to see
> this report, which is weird (3.19.0 here). Are you using any specific
> option of valgrind I should be aware of? Here is what I used, for
> reference:
> valgrind \
> --suppressions=$PG_SOURCE/src/tools/valgrind.supp \
> --trace-children=yes --track-origins=yes --read-var-info=yes \
> postgres -D REST_OF_ARGS
Please try the following procedure (I've simplified my own):
With the attached patch (for HEAD) applied
CPPFLAGS="-DUSE_VALGRIND -Og" ./configure -q --enable-debug --enable-cassert && make -s -j8
sed 's/create index on idxpart1 ((a + b));/create index on idxpart1 ((a));/' -i src/test/regress/sql/indexing.sql
TESTS=indexing make check-tests
I get:
not ok 1 - indexing 9844 ms
# (test process exited with exit code 2)
(I use valgrind-3.18.1.)
If you still see no error, please share details of your method for running
valgrind.
>
>> In addition, there is a check whether both indexes are (are not)
>> expression indexes, but it's placed below...
> Sure, but this makes the check a bit cheaper if the indexes to compare
> use expr and non-expr attributes at the same attnums, no? Except if I
> am missing something, the attached should be sufficient.
I thought about placing that check before the loop, but your fix looks
more clear (and my testing confirms that it works).
Best regards,
Alexander
Attachments:
install-vrunner-17.patch (text/x-patch, 1.9K) download
diff --git a/install-vrunner.sh b/install-vrunner.sh
new file mode 100755
index 00000000000..21881d4c729
--- /dev/null
+++ b/install-vrunner.sh
@@ -0,0 +1,31 @@
+#!/bin/bash
+set -e
+
+SRCDIR=$1
+PGBIN=$2
+
+cd $PGBIN
+cp $SRCDIR/src/tools/valgrind.supp ./
+
+echo ""
+cat << 'EOF' > vrunner
+#!/bin/bash
+set -e
+BD="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+SN="$(basename "${BASH_SOURCE[0]}")"
+OBIN="_$SN"
+[[ $LD_PRELOAD = *"valgrind"* ]] && exec $BD/$OBIN "$@"
+
+exec valgrind --quiet --exit-on-first-error=yes --track-origins=no --error-exitcode=1 --leak-check=no --time-stamp=yes \
+ --gen-suppressions=all --suppressions=$BD/valgrind.supp \
+ --trace-children=yes --trace-children-skip="/bin/*,/usr/bin/*" \
+ $BD/$OBIN "$@"
+EOF
+chmod a+x vrunner
+
+for f in *; do
+ if [ "$f" = "vrunner" ] || [ ! -x "$f" ]; then continue; fi
+ mv "$f" "_$f"
+ ln -s vrunner "$f"
+ chmod a+x "$f"
+done
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 884d66920b6..87af7fd9ab8 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -411,2 +411,3 @@ ifeq ($(MAKELEVEL),0)
$(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep >>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
+ '$(abs_top_builddir)'/install-vrunner.sh '$(abs_top_builddir)' '$(abs_top_builddir)/tmp_install$(bindir)'
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 7f427e7d60e..e6115881f6b 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -620,12 +620,7 @@ wait_for_postmaster(pgpid_t pm_pid, bool do_checkpoint)
pmpid = atol(optlines[LOCK_FILE_LINE_PID - 1]);
pmstart = atol(optlines[LOCK_FILE_LINE_START_TIME - 1]);
if (pmstart >= start_time - 2 &&
-#ifndef WIN32
- pmpid == pm_pid
-#else
- /* Windows can only reject standalone-backend PIDs */
- pmpid > 0
-#endif
+ pmpid == pm_pid && kill(pmpid, 0) == 0
)
{
/*
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 03:30 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
2023-09-28 05:00 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Alexander Lakhin <exclusion@gmail.com>
@ 2023-09-28 07:07 ` Michael Paquier <michael@paquier.xyz>
2023-09-28 08:00 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2023-09-28 07:07 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Thu, Sep 28, 2023 at 08:00:00AM +0300, Alexander Lakhin wrote:
> Please try the following procedure (I've simplified my own):
> With the attached patch (for HEAD) applied
> CPPFLAGS="-DUSE_VALGRIND "
USE_VALGRIND is the difference here. In the past problems with
valgrind I've seen from you, I've never needed that, actually.
In my case, my process is much simpler: I just use installcheck with
an instance I set up locally using a command like the one I mentioned
above. No need to patch the tree with that and self-contained SQL
sequences.
Anyway, mystery solved.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 03:30 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
2023-09-28 05:00 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Alexander Lakhin <exclusion@gmail.com>
2023-09-28 07:07 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
@ 2023-09-28 08:00 ` Alexander Lakhin <exclusion@gmail.com>
0 siblings, 0 replies; 10+ messages in thread
From: Alexander Lakhin @ 2023-09-28 08:00 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org
28.09.2023 10:07, Michael Paquier wrote:
> In my case, my process is much simpler: I just use installcheck with
> an instance I set up locally using a command like the one I mentioned
> above. No need to patch the tree with that and self-contained SQL
> sequences.
Yes, I know that method, but unfortunately it doesn't cover TAP tests,
so I prefer to use such patch to perform whole check-world (and run also all
postgres binaries, e.g., pg_dump) under valgrind.
As to patching regress/sql/, in this case my intention was to minimize the
self-contained repro. I use separate scripts for running arbitrary SQL,
for sure.
Best regards,
Alexander
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
@ 2023-09-28 17:31 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-01 00:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 10+ messages in thread
From: Tom Lane @ 2023-09-28 17:31 UTC (permalink / raw)
To: exclusion@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> writes:
> The function CompareIndexInfo() contains the code:
> /* ignore expressions at this stage */
> if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
> (attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
> info1->ii_IndexAttrNumbers[i]))
> return false;
> where info1->ii_IndexAttrNumbers[i] is checked for InvalidAttrNumber
> (i. e. it's not an expression), but info2->ii_IndexAttrNumbers[i] is not.
Agreed, that's pretty broken, and it's not just that it risks an
invalid access. I don't think this reliably rejects cases where
one index has an expression and the other doesn't. Even if it does
work, it's way too complicated to convince oneself that that's
rejected. I think for clarity we should code it as attached.
regards, tom lane
Attachments:
fix-CompareIndexInfo.patch (text/x-diff, 1.7K) download
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 716de171a4..3d5adab2c5 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2559,7 +2559,7 @@ CompareIndexInfo(const IndexInfo *info1, const IndexInfo *info2,
/*
* and columns match through the attribute map (actual attribute numbers
- * might differ!) Note that this implies that index columns that are
+ * might differ!) Note that this checks that index columns that are
* expressions appear in the same positions. We will next compare the
* expressions themselves.
*/
@@ -2568,13 +2568,22 @@ CompareIndexInfo(const IndexInfo *info1, const IndexInfo *info2,
if (attmap->maplen < info2->ii_IndexAttrNumbers[i])
elog(ERROR, "incorrect attribute map");
- /* ignore expressions at this stage */
- if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
- (attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
- info1->ii_IndexAttrNumbers[i]))
- return false;
+ /* ignore expressions for now (but check their collation/opfamily) */
+ if (!(info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber &&
+ info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber))
+ {
+ /* fail if just one index has an expression in this column */
+ if (info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber ||
+ info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber)
+ return false;
+
+ /* both are columns, so check for match after mapping */
+ if (attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
+ info1->ii_IndexAttrNumbers[i])
+ return false;
+ }
- /* collation and opfamily is not valid for including columns */
+ /* collation and opfamily are not valid for included columns */
if (i >= info1->ii_NumIndexKeyAttrs)
continue;
=
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 17:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-10-01 00:31 ` Michael Paquier <michael@paquier.xyz>
2023-10-01 00:39 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2023-10-01 00:31 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Sep 28, 2023 at 01:31:39PM -0400, Tom Lane wrote:
> Agreed, that's pretty broken, and it's not just that it risks an
> invalid access. I don't think this reliably rejects cases where
> one index has an expression and the other doesn't. Even if it does
> work, it's way too complicated to convince oneself that that's
> rejected. I think for clarity we should code it as attached.
- /* ignore expressions at this stage */
- if ((info1->ii_IndexAttrNumbers[i] != InvalidAttrNumber) &&
- (attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=
- info1->ii_IndexAttrNumbers[i]))
- return false;
+ /* ignore expressions for now (but check their collation/opfamily) */
+ if (!(info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber &&
+ info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber))
+ {
+ /* fail if just one index has an expression in this column */
+ if (info1->ii_IndexAttrNumbers[i] == InvalidAttrNumber ||
+ info2->ii_IndexAttrNumbers[i] == InvalidAttrNumber)
+ return false;
I can see that this has already been committed as 9f71e10d65, but are
you sure that this is correct? I didn't take the time to reply back,
because I got the feeling that even what I proposed is not correct.
The previous code was careful enough to look at the information from
info2 only *through* the attribute map, which is not what this new
code is doing. Rather than looking directly at the elements in info2
at each iteration, shouldn't this stuff loop over the elements in
attmap->attnums for each info1->ii_IndexAttrNumbers[i]?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 17:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
2023-10-01 00:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
@ 2023-10-01 00:39 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-02 02:56 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2023-10-01 00:39 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> I can see that this has already been committed as 9f71e10d65, but are
> you sure that this is correct? I didn't take the time to reply back,
> because I got the feeling that even what I proposed is not correct.
> The previous code was careful enough to look at the information from
> info2 only *through* the attribute map, which is not what this new
> code is doing. Rather than looking directly at the elements in info2
> at each iteration, shouldn't this stuff loop over the elements in
> attmap->attnums for each info1->ii_IndexAttrNumbers[i]?
I think it's OK as is. What we are comparing in the modified logic
is whether index columns at the same column positions are expressions or
not, and that's not a matter for mapping. Once we've verified that
the current column is a non-expression in both indexes, then it's
appropriate to use the attmap to see whether the columns correspond.
(Or to put it another way: running "column zero" through the
attmap is always the wrong thing.)
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 17:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
2023-10-01 00:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
2023-10-01 00:39 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-10-02 02:56 ` Michael Paquier <michael@paquier.xyz>
2023-10-02 03:04 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2023-10-02 02:56 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Sat, Sep 30, 2023 at 08:39:37PM -0400, Tom Lane wrote:
> I think it's OK as is. What we are comparing in the modified logic
> is whether index columns at the same column positions are expressions or
> not, and that's not a matter for mapping. Once we've verified that
> the current column is a non-expression in both indexes, then it's
> appropriate to use the attmap to see whether the columns correspond.
FWIW, I was thinking about a case like that with 2 index attributes:
info1->attr[0] = 0, info1->attr[1] = 1
info2->attr[0] = 1, info2->attr[1] = 0
With a mapping from info2 to info1 like that:
attmap[0] = 1, attmap[1] = 0.
The code before 9f71e10d6 would have accessed an incorrect pointer.
The new code would return false, which would not be correct if the
expressions stored in ii_Expressions for info1/attr0 and info2/attr1
match?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 17:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
2023-10-01 00:31 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
2023-10-01 00:39 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Tom Lane <tgl@sss.pgh.pa.us>
2023-10-02 02:56 ` Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index Michael Paquier <michael@paquier.xyz>
@ 2023-10-02 03:04 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 10+ messages in thread
From: Tom Lane @ 2023-10-02 03:04 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> FWIW, I was thinking about a case like that with 2 index attributes:
> info1->attr[0] = 0, info1->attr[1] = 1
> info2->attr[0] = 1, info2->attr[1] = 0
> With a mapping from info2 to info1 like that:
> attmap[0] = 1, attmap[1] = 0.
> The code before 9f71e10d6 would have accessed an incorrect pointer.
> The new code would return false, which would not be correct if the
> expressions stored in ii_Expressions for info1/attr0 and info2/attr1
> match?
I think "false" is correct. Otherwise you're claiming that an
index on (expr, col1) is equivalent to an index on (col1, expr).
Maybe it is equivalent for some purposes, but I don't think this
function has any business assuming that it is so for all purposes.
Also, the pre-existing comment clearly intends that false is
the proper result here:
* might differ!) Note that this implies that index columns that are
* expressions appear in the same positions. We will next compare the
* expressions themselves.
The code just failed to enforce that fully.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2023-10-02 03:04 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-09-27 10:00 BUG #18135: Incorrect memory access occurs when attaching a partition with an index PG Bug reporting form <noreply@postgresql.org>
2023-09-28 03:30 ` Michael Paquier <michael@paquier.xyz>
2023-09-28 05:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-09-28 07:07 ` Michael Paquier <michael@paquier.xyz>
2023-09-28 08:00 ` Alexander Lakhin <exclusion@gmail.com>
2023-09-28 17:31 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-01 00:31 ` Michael Paquier <michael@paquier.xyz>
2023-10-01 00:39 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-10-02 02:56 ` Michael Paquier <michael@paquier.xyz>
2023-10-02 03:04 ` Tom Lane <tgl@sss.pgh.pa.us>
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