From: Alexander Lakhin <exclusion@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
To: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
Date: Thu, 28 Sep 2023 08:00:00 +0300
Message-ID: <9e3acda6-76b8-d6b4-c013-f78f84d7c5ab@gmail.com> (raw)
In-Reply-To: <ZRTzX0h68fxZ5Dth@paquier.xyz>
References: <18135-532f4a755e71e4d2@postgresql.org>
<ZRTzX0h68fxZ5Dth@paquier.xyz>
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) downloaddiff --git a/install-vrunner.sh b/install-vrunner.shnew file mode 100755index 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"+donediff --git a/src/Makefile.global.in b/src/Makefile.global.inindex 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.cindex 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
)
{
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: exclusion@gmail.com, michael@paquier.xyz, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18135: Incorrect memory access occurs when attaching a partition with an index
In-Reply-To: <9e3acda6-76b8-d6b4-c013-f78f84d7c5ab@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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