Обсуждение: BUG #18135: Incorrect memory access occurs when attaching a partition with an index

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

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

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

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

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

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

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;

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

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

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

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