Date: Sun, 29 Dec 2019 02:02:21 -0500
From: Noah Misch <noah@leadboat.com>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Consolidate 'unique array values' logic into a reusable function?
Message-ID: <20191229070221.GA13873@gust.leadboat.com>
References: 
 <CAEepm=2vmFTNpAmwbGGD2WaryM6T3hSDVKQPfUwjdD_5XY6vAA@mail.gmail.com>
 <22244.1470585142@sss.pgh.pa.us>
 <CA+hUKGK_kwiS+3VCeMMGAKg=27T1v17ABzt+xDa1qeW7W7wruA@mail.gmail.com>
 <CA+hUKGLz=6eDPewLb6ToPLcg1R3brH+5eBiWW0WVyn7+fh5y=Q@mail.gmail.com>
 <CA+hUKG+7kZ0nVrL8HeVyP+a2K64oLq921K4S4LCubGuw0JWuHw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="AhhlLboLdkugWU4S"
Content-Disposition: inline
In-Reply-To: 
 <CA+hUKG+7kZ0nVrL8HeVyP+a2K64oLq921K4S4LCubGuw0JWuHw@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--AhhlLboLdkugWU4S
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Nov 04, 2019 at 12:02:21PM +1300, Thomas Munro wrote:
> Rebased.  I'm planning to commit this soon.

In each installcheck-parallel run under valgrind-3.14.0, I now see ~1200
reports like this:

==00:00:00:28.322 1527557== Source and destination overlap in memcpy(0x1000104, 0x1000104, 4)
==00:00:00:28.322 1527557==    at 0x4C2E74D: memcpy@@GLIBC_2.14 (vg_replace_strmem.c:1035)
==00:00:00:28.322 1527557==    by 0xA9A57B: qunique (qunique.h:34)
==00:00:00:28.322 1527557==    by 0xA9A843: InitCatalogCache (syscache.c:1056)
==00:00:00:28.322 1527557==    by 0xAB6B18: InitPostgres (postinit.c:682)
==00:00:00:28.322 1527557==    by 0x91F98E: PostgresMain (postgres.c:3909)
==00:00:00:28.322 1527557==    by 0x872DE9: BackendRun (postmaster.c:4498)
==00:00:00:28.322 1527557==    by 0x8725B3: BackendStartup (postmaster.c:4189)
==00:00:00:28.322 1527557==    by 0x86E7F4: ServerLoop (postmaster.c:1727)
==00:00:00:28.322 1527557==    by 0x86E0AA: PostmasterMain (postmaster.c:1400)
==00:00:00:28.322 1527557==    by 0x77CB56: main (main.c:210)
==00:00:00:28.322 1527557== 
{
   <insert_a_suppression_name_here>
   Memcheck:Overlap
   fun:memcpy@@GLIBC_2.14
   fun:qunique
   fun:InitCatalogCache
   fun:InitPostgres
   fun:PostgresMain
   fun:BackendRun
   fun:BackendStartup
   fun:ServerLoop
   fun:PostmasterMain
   fun:main
}

This is like the problem fixed in 9a9473f; the precedent from there would be
to test src!=dst before calling mempcy(), e.g. as attached.  I suppose the
alternative would be to add a suppression like the one 9a9473f removed.

I do wonder why the Valgrind buildfarm animals haven't noticed.

--AhhlLboLdkugWU4S
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="qunique-valgrind-v1.patch"

diff --git a/src/include/lib/qunique.h b/src/include/lib/qunique.h
index 4d620f8..fc539ca 100644
--- a/src/include/lib/qunique.h
+++ b/src/include/lib/qunique.h
@@ -30,8 +30,9 @@ qunique(void *array, size_t elements, size_t width,
 
 	for (i = 1, j = 0; i < elements; ++i)
 	{
-		if (compare(bytes + i * width, bytes + j * width) != 0)
-			memcpy(bytes + ++j * width, bytes + i * width, width);
+		if (compare(bytes + i * width, bytes + j * width) != 0 &&
+			++j != i)
+			memcpy(bytes + j * width, bytes + i * width, width);
 	}
 
 	return j + 1;
@@ -55,8 +56,9 @@ qunique_arg(void *array, size_t elements, size_t width,
 
 	for (i = 1, j = 0; i < elements; ++i)
 	{
-		if (compare(bytes + i * width, bytes + j * width, arg) != 0)
-			memcpy(bytes + ++j * width, bytes + i * width, width);
+		if (compare(bytes + i * width, bytes + j * width, arg) != 0 &&
+			++j != i)
+			memcpy(bytes + j * width, bytes + i * width, width);
 	}
 
 	return j + 1;

--AhhlLboLdkugWU4S--