Configure with thread sanitizer fails the thread test

On 2021-Jul-23, Mikhail Matrosov wrote:

I am not sure what is the proper fix for the issue, but I at least suggest
to disable this test when building with thread sanitizer:
https://github.com/conan-io/conan-center-index/pull/6472/files#diff-b8639f81e30f36c8ba29a0878f1ef4d9f1552293bc9098ebb9b429ddb1f0935f

Here's the proposed patch. Patches posted to the mailing list by their
authors proposed for inclusion are considered to be under the PostgreSQL
license, but this patch hasn't been posted by the author so let's assume
they're not authorizing them to use it. (Otherwise, why wouldn't they
just post it here instead of doing the absurdly convoluted dance of a
github PR?)

diff --git a/src/test/thread/thread_test.c b/src/test/thread/thread_test.c
index e1bec01..e4ffd78 100644
--- a/src/test/thread/thread_test.c
+++ b/src/test/thread/thread_test.c
@@ -61,6 +61,14 @@ main(int argc, char *argv[])
 	fprintf(stderr, "Perhaps rerun 'configure' using '--enable-thread-safety'.\n");
 	return 1;
 }
+
+#elif __has_feature(thread_sanitizer)
+int
+main(int argc, char *argv[])
+{
+	printf("Thread safety check is skipped since it does not work with thread sanitizer.\n");
+	return 0;
+}
 #else

/* This must be down here because this is the code that uses threads. */

--
Álvaro Herrera

On 2021-Jul-23, Alvaro Herrera wrote:

On 2021-Jul-23, Mikhail Matrosov wrote:

I am not sure what is the proper fix for the issue, but I at least suggest
to disable this test when building with thread sanitizer:
https://github.com/conan-io/conan-center-index/pull/6472/files#diff-b8639f81e30f36c8ba29a0878f1ef4d9f1552293bc9098ebb9b429ddb1f0935f

Here's the proposed patch. Patches posted to the mailing list by their
authors proposed for inclusion are considered to be under the PostgreSQL
license, but this patch hasn't been posted by the author so let's assume
they're not authorizing them to use it. (Otherwise, why wouldn't they
just post it here instead of doing the absurdly convoluted dance of a
github PR?)

... that said, I wonder why would we do this in the thread_test program
rather than in configure itself. Wouldn't it make more sense for the
configure test to be skipped altogether (marking the result as
thread-safe) when running under thread sanitizer, if there's a way to
detect that?

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto. No es cierto, y si fuera cierto,
no me acuerdo." (Augusto Pinochet a una corte de justicia)

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

... that said, I wonder why would we do this in the thread_test program
rather than in configure itself. Wouldn't it make more sense for the
configure test to be skipped altogether (marking the result as
thread-safe) when running under thread sanitizer, if there's a way to
detect that?

TBH, I wonder why we don't just nuke thread_test.c altogether.
Is it still useful in 2021? Machines that still need
--disable-thread-safety can doubtless be counted without running
out of fingers, and I think their owners can be expected to know
that they need that.

regards, tom lane

On Fri, Jul 23, 2021 at 01:42:41PM -0400, Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

... that said, I wonder why would we do this in the thread_test program
rather than in configure itself. Wouldn't it make more sense for the
configure test to be skipped altogether (marking the result as
thread-safe) when running under thread sanitizer, if there's a way to
detect that?

TBH, I wonder why we don't just nuke thread_test.c altogether.
Is it still useful in 2021? Machines that still need
--disable-thread-safety can doubtless be counted without running
out of fingers, and I think their owners can be expected to know
that they need that.

I think it is fine to remove it. It was really designed to just try a
lot of flags to see what the compiler required, but things have become
much more stable since it was written.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

Hi,

On 2021-07-23 13:42:41 -0400, Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

... that said, I wonder why would we do this in the thread_test program
rather than in configure itself. Wouldn't it make more sense for the
configure test to be skipped altogether (marking the result as
thread-safe) when running under thread sanitizer, if there's a way to
detect that?

TBH, I wonder why we don't just nuke thread_test.c altogether.
Is it still useful in 2021? Machines that still need
--disable-thread-safety can doubtless be counted without running
out of fingers, and I think their owners can be expected to know
that they need that.

+1. And before long it might be time to remove support for systems
without threads...

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2021-07-23 13:42:41 -0400, Tom Lane wrote:

TBH, I wonder why we don't just nuke thread_test.c altogether.

+1. And before long it might be time to remove support for systems
without threads...

I'm not prepared to go that far just yet; but certainly we can stop
expending configure cycles on the case.

Should we back-patch this, or just do it in HEAD? Maybe HEAD+v14?

regards, tom lane

Hi, Alvaro and all,

this patch hasn't been posted by the author so let's assume
they're not authorizing them to use it.

Not sure what you mean. I am the author and I authorize anyone to do
whatever they want with it.

Otherwise, why wouldn't they just post it here instead of doing the

absurdly convoluted dance of a github PR?

Well, for me submitting a github PR is a well-established and easy-to-do
procedure which is the same for all libraries. Posting to a new mailing
list definitely is not. I've spent around 15 minutes trying to do a proper
reply to the thread and did not succeed.

Another reason to post a PR is that we consume libpq via conan, and
releasing a new revision of the conan recipe for the same library version
is a relatively fast and well-defined process. While waiting for a new
version of a library with a patch depends heavily on a particular library.
I am not aware of the release cadence of libpq.

Anyway, I am very glad there is swift feedback from you guys and I am
looking forward to your comments on the proper way to fix the issue!

-----
Best regards, Mikhail Matrosov

Hi,

On 2021-07-23 17:18:37 -0400, Tom Lane wrote:

I'm not prepared to go that far just yet; but certainly we can stop
expending configure cycles on the case.

Should we back-patch this, or just do it in HEAD? Maybe HEAD+v14?

I'm ok with all, with a preference for HEAD+v14, followed by HEAD, and
all branches.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2021-07-23 17:18:37 -0400, Tom Lane wrote:

Should we back-patch this, or just do it in HEAD? Maybe HEAD+v14?

I'm ok with all, with a preference for HEAD+v14, followed by HEAD, and
all branches.

After a bit more thought, HEAD+v14 is also my preference. I'm not
eager to change this in stable branches, but it doesn't seem too
late for v14.

regards, tom lane

I wrote:

After a bit more thought, HEAD+v14 is also my preference. I'm not
eager to change this in stable branches, but it doesn't seem too
late for v14.

Perusing the commit log, I realized that there's another reason why
v14 is a good cutoff: thread_test.c was in a different place with
an allegedly different raison d'etre before 8a2121185. So done
that way.

regards, tom lane