From: Tom Lane <tgl@sss.pgh.pa.us>
To: Justin Pryzby <pryzby@telsasoft.com>
cc: Michael Paquier <michael@paquier.xyz>,
 Robert Haas <robertmhaas@gmail.com>,
        Tomas Vondra <tomas.vondra@enterprisedb.com>,
        pgsql-hackers@postgresql.org
Subject: Re: should check interrupts in BuildRelationExtStatistics ?
In-reply-to: <20220617222549.GI29853@telsasoft.com>
References: <20220509000108.GQ28830@telsasoft.com>
 <YniLBN/x8N97ZDzf@paquier.xyz> <3592964.1652067393@sss.pgh.pa.us>
 <CA+TgmoY=Q9C9n=P=e8gTqLFTLza89t-cz+X-16LPaxq2+=qY2g@mail.gmail.com>
 <20220603152837.GO29853@telsasoft.com> <20220605014233.GR29853@telsasoft.com>
 <Yp2rdhPtSHDJxPAd@paquier.xyz> <20220617222549.GI29853@telsasoft.com>
Comments: In-reply-to Justin Pryzby <pryzby@telsasoft.com>
	message dated "Fri, 17 Jun 2022 17:25:49 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <154856.1656717534.0@sss.pgh.pa.us>
Date: Fri, 01 Jul 2022 19:19:11 -0400
Message-ID: <154878.1656717551@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/154878.1656717551%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <154856.1656717534.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

Justin Pryzby <pryzby@telsasoft.com> writes:
> On Mon, Jun 06, 2022 at 04:23:34PM +0900, Michael Paquier wrote:
>> Hmm.  I have to admit that adding a CFI() in multi_sort_compare()
>> stresses me a bit as it is dependent on the number of rows involved,
>> and it can be used as a qsort routine.

> That's exactly the problem for which I showed a backtrace - it took 10s =
of
> seconds to do qsort, which is (uh) a human timescale and too long to be
> unresponsive, even if I create on a table with many rows a stats object =
with a
> lot of columns and a high stats target.

Hmm.  On my machine, the example last shown upthread takes about 9
seconds, which I agree is a mighty long time to be unresponsive
--- but it appears that fully half of that elapses before we
reach multi_sort_compare for the first time.  The first half of
the ANALYZE run does seem to contain some CFI calls, but they
are not exactly thick on the ground there either.  So I'm feeling
like this patch isn't ambitious enough.

I tried interrupting at a random point and then stepping, and
look what I hit after just a couple of steps:

(gdb) s
qsort_arg (data=3Ddata@entry=3D0x13161410, n=3D<optimized out>, n@entry=3D=
1679616, =

    element_size=3Delement_size@entry=3D16, =

    compare=3Dcompare@entry=3D0x649450 <compare_scalars>, =

    arg=3Darg@entry=3D0x7ffec539c0f0) at ../../src/include/lib/sort_templa=
te.h:353
353                             if (r =3D=3D 0)
(gdb) =

358                             pc -=3D ST_POINTER_STEP;
(gdb) =

359                             DO_CHECK_FOR_INTERRUPTS();

That, um, piqued my interest.  After a bit of digging,
I modestly propose the attached.  I'm not sure if it's
okay to back-patch this, because maybe someone out there
is relying on qsort() to be incapable of throwing an error
--- but it'd solve the problem at hand and a bunch of other
issues of the same ilk.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="enable-check-for-interrupts-in-qsort.patch";
	charset="us-ascii"
Content-ID: <154856.1656717534.2@sss.pgh.pa.us>
Content-Description: enable-check-for-interrupts-in-qsort.patch

diff --git a/src/port/qsort.c b/src/port/qsort.c
index 7879e6cd56..34b99aac08 100644
--- a/src/port/qsort.c
+++ b/src/port/qsort.c
@@ -2,7 +2,12 @@
  *	qsort.c: standard quicksort algorithm
  */
 
-#include "c.h"
+#ifndef FRONTEND
+#include "postgres.h"
+#include "miscadmin.h"
+#else
+#include "postgres_fe.h"
+#endif
 
 #define ST_SORT pg_qsort
 #define ST_ELEMENT_TYPE_VOID
@@ -10,6 +15,11 @@
 #define ST_SCOPE
 #define ST_DECLARE
 #define ST_DEFINE
+
+#ifndef FRONTEND
+#define ST_CHECK_FOR_INTERRUPTS
+#endif
+
 #include "lib/sort_template.h"
 
 /*
diff --git a/src/port/qsort_arg.c b/src/port/qsort_arg.c
index fa7e11a3b8..685ab32913 100644
--- a/src/port/qsort_arg.c
+++ b/src/port/qsort_arg.c
@@ -2,7 +2,12 @@
  *	qsort_arg.c: qsort with a passthrough "void *" argument
  */
 
-#include "c.h"
+#ifndef FRONTEND
+#include "postgres.h"
+#include "miscadmin.h"
+#else
+#include "postgres_fe.h"
+#endif
 
 #define ST_SORT qsort_arg
 #define ST_ELEMENT_TYPE_VOID
@@ -11,4 +16,9 @@
 #define ST_COMPARE_ARG_TYPE void
 #define ST_SCOPE
 #define ST_DEFINE
+
+#ifndef FRONTEND
+#define ST_CHECK_FOR_INTERRUPTS
+#endif
+
 #include "lib/sort_template.h"

------- =_aaaaaaaaaa0--