Thread: Reducing opr_sanity test's runtime under CLOBBER_CACHE_ALWAYS

In a nearby thread I bemoaned the fact that the core regression tests
seem to have gotten significantly slower in the last couple of months,
at least with CCA enabled: hyrax reports completing them in 12:52:44
on 18 March, while its most recent run on 1 May took 14:08:18.

Trying to diagnose the cause overall seemed a bit daunting, but
I thought I'd dig into the opr_sanity test in particular, as it
is one of the slower tests under CCA to start with and had also
slowed down noticeably (from 3701581 ms to 4761183 ms, or 28%).
I was able to complete a bisection using just that test, and
got an unexpected result: most of the slowdown appeared at
ab596105b (BRIN minmax-multi indexes).  Apparently the additional
time is simply from having to check the additional pg_amop and
pg_amproc entries, which that patch added quite a few of.

I noticed that all of the slowest queries in that test were dependent
on the binary_coercible() plpgsql function that it uses.  Now, that
function has always been a rather lame attempt to approximate the
behavior of the parser's IsBinaryCoercible() function, so I've been
thinking for some time that we ought to get rid of it in favor of
actually using IsBinaryCoercible().  I tried that, by adding a
shim function to regress.c, and got a most gratifying result:
on my machine opr_sanity's runtime with
debug_invalidate_system_caches_always = 1 drops from
29m9s to 3m19s.  Without CCA the speedup is far less impressive,
360ms to 305ms, but that's still useful.  Especially since this
makes the test strictly more accurate.

(I am thinking that this suggests that plpgsql may be hurt more
by cache clobbers than it really needs to be; but doing anything
about that would require some research.)

Anyway, I propose that we ought to sneak this into HEAD, since
it's only touching test code and not anything production-critical.

The patch is a bit more invasive than I would have liked, because
adding the SQL definition of binary_coercible() to create_function_1
(where the other regress.c functions are declared) didn't work:
that runs after opr_sanity, and just moving it up to before
opr_sanity causes the latter to complain about some of the functions
in it.  So I ended up splitting the create_function_1 test into
create_function_0 and create_function_1.   It's annoying from a
parallelism standpoint that create_function_0 runs by itself, but
the two parallel groups ahead of it are already full.  Maybe we
should rebalance that by moving a few of those tests to run in
parallel with create_function_0, but I didn't do that here.

Thoughts?

            regards, tom lane

diff --git a/src/test/regress/expected/.gitignore b/src/test/regress/expected/.gitignore
index 93c56c85a0..b99caf5f40 100644
--- a/src/test/regress/expected/.gitignore
+++ b/src/test/regress/expected/.gitignore
@@ -1,5 +1,6 @@
 /constraints.out
 /copy.out
+/create_function_0.out
 /create_function_1.out
 /create_function_2.out
 /largeobject.out
diff --git a/src/test/regress/expected/conversion.out b/src/test/regress/expected/conversion.out
index e34ab20974..04fdcba496 100644
--- a/src/test/regress/expected/conversion.out
+++ b/src/test/regress/expected/conversion.out
@@ -41,7 +41,7 @@ DROP USER regress_conversion_user;
 -- Test built-in conversion functions.
 --
 -- Helper function to test a conversion. Uses the test_enc_conversion function
--- that was created in the create_function_1 test.
+-- that was created in the create_function_0 test.
 create or replace function test_conv(
   input IN bytea,
   src_encoding IN text,
diff --git a/src/test/regress/expected/opr_sanity.out b/src/test/regress/expected/opr_sanity.out
index 7a0d345b60..562b586d8e 100644
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -16,61 +16,6 @@
 --
 -- NB: run this test earlier than the create_operator test, because
 -- that test creates some bogus operators...
--- Helper functions to deal with cases where binary-coercible matches are
--- allowed.
--- This should match IsBinaryCoercible() in parse_coerce.c.
--- It doesn't currently know about some cases, notably domains, anyelement,
--- anynonarray, anyenum, or record, but it doesn't need to (yet).
-create function binary_coercible(oid, oid) returns bool as $$
-begin
-  if $1 = $2 then return true; end if;
-  if EXISTS(select 1 from pg_catalog.pg_cast where
-            castsource = $1 and casttarget = $2 and
-            castmethod = 'b' and castcontext = 'i')
-  then return true; end if;
-  if $2 = 'pg_catalog.any'::pg_catalog.regtype then return true; end if;
-  if $2 = 'pg_catalog.anyarray'::pg_catalog.regtype then
-    if EXISTS(select 1 from pg_catalog.pg_type where
-              oid = $1 and typelem != 0 and
-              typsubscript = 'pg_catalog.array_subscript_handler'::pg_catalog.regproc)
-    then return true; end if;
-  end if;
-  if $2 = 'pg_catalog.anyrange'::pg_catalog.regtype then
-    if (select typtype from pg_catalog.pg_type where oid = $1) = 'r'
-    then return true; end if;
-  end if;
-  if $2 = 'pg_catalog.anymultirange'::pg_catalog.regtype then
-    if (select typtype from pg_catalog.pg_type where oid = $1) = 'm'
-    then return true; end if;
-  end if;
-  return false;
-end
-$$ language plpgsql strict stable;
--- This one ignores castcontext, so it will allow cases where an explicit
--- (but still binary) cast would be required to convert the input type.
--- We don't currently use this for any tests in this file, but it is a
--- reasonable alternative definition for some scenarios.
-create function explicitly_binary_coercible(oid, oid) returns bool as $$
-begin
-  if $1 = $2 then return true; end if;
-  if EXISTS(select 1 from pg_catalog.pg_cast where
-            castsource = $1 and casttarget = $2 and
-            castmethod = 'b')
-  then return true; end if;
-  if $2 = 'pg_catalog.any'::pg_catalog.regtype then return true; end if;
-  if $2 = 'pg_catalog.anyarray'::pg_catalog.regtype then
-    if EXISTS(select 1 from pg_catalog.pg_type where
-              oid = $1 and typelem != 0 and
-              typsubscript = 'pg_catalog.array_subscript_handler'::pg_catalog.regproc)
-    then return true; end if;
-  end if;
-  if $2 = 'pg_catalog.anyrange'::pg_catalog.regtype then
-    if (select typtype from pg_catalog.pg_type where oid = $1) = 'r'
-    then return true; end if;
-  end if;
-  return false;
-end
-$$ language plpgsql strict stable;
 -- **************** pg_proc ****************
 -- Look for illegal values in pg_proc fields.
 SELECT p1.oid, p1.proname
diff --git a/src/test/regress/expected/type_sanity.out b/src/test/regress/expected/type_sanity.out
index 5480f979c6..f567fd378e 100644
--- a/src/test/regress/expected/type_sanity.out
+++ b/src/test/regress/expected/type_sanity.out
@@ -635,7 +635,7 @@ WHERE (rngcollation = 0) != (typcollation = 0);
 (0 rows)

 -- opclass had better be a btree opclass accepting the subtype.
--- We must allow anyarray matches, cf opr_sanity's binary_coercible()
+-- We must allow anyarray matches, cf IsBinaryCoercible()
 SELECT p1.rngtypid, p1.rngsubtype, o.opcmethod, o.opcname
 FROM pg_range p1 JOIN pg_opclass o ON o.oid = p1.rngsubopc
 WHERE o.opcmethod != 403 OR
diff --git a/src/test/regress/input/create_function_0.source b/src/test/regress/input/create_function_0.source
new file mode 100644
index 0000000000..28fe373c7d
--- /dev/null
+++ b/src/test/regress/input/create_function_0.source
@@ -0,0 +1,42 @@
+--
+-- CREATE_FUNCTION_0
+--
+-- This makes available various test support functions that are in
+-- src/test/regress/regress.c
+-- (although a few more are created in create_function_1)
+--
+
+CREATE FUNCTION make_tuple_indirect (record)
+    RETURNS record
+    AS '@libdir@/regress@DLSUFFIX@'
+    LANGUAGE C STRICT;
+
+CREATE FUNCTION test_atomic_ops()
+    RETURNS bool
+    AS '@libdir@/regress@DLSUFFIX@'
+    LANGUAGE C;
+
+CREATE FUNCTION test_fdw_handler()
+    RETURNS fdw_handler
+    AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'
+    LANGUAGE C;
+
+CREATE FUNCTION test_support_func(internal)
+    RETURNS internal
+    AS '@libdir@/regress@DLSUFFIX@', 'test_support_func'
+    LANGUAGE C STRICT;
+
+CREATE FUNCTION test_opclass_options_func(internal)
+    RETURNS void
+    AS '@libdir@/regress@DLSUFFIX@', 'test_opclass_options_func'
+    LANGUAGE C;
+
+CREATE FUNCTION test_enc_conversion(bytea, name, name, bool,
+                                    validlen OUT int, result OUT bytea)
+    AS '@libdir@/regress@DLSUFFIX@', 'test_enc_conversion'
+    LANGUAGE C STRICT;
+
+CREATE FUNCTION binary_coercible(oid, oid)
+    RETURNS bool
+    AS '@libdir@/regress@DLSUFFIX@', 'binary_coercible'
+    LANGUAGE C STRICT STABLE PARALLEL SAFE;
diff --git a/src/test/regress/input/create_function_1.source b/src/test/regress/input/create_function_1.source
index 6c69b7fe6c..8b58860245 100644
--- a/src/test/regress/input/create_function_1.source
+++ b/src/test/regress/input/create_function_1.source
@@ -52,36 +52,6 @@ CREATE FUNCTION set_ttdummy (int4)
         AS '@libdir@/regress@DLSUFFIX@'
         LANGUAGE C STRICT;

-CREATE FUNCTION make_tuple_indirect (record)
-        RETURNS record
-        AS '@libdir@/regress@DLSUFFIX@'
-        LANGUAGE C STRICT;
-
-CREATE FUNCTION test_atomic_ops()
-    RETURNS bool
-    AS '@libdir@/regress@DLSUFFIX@'
-    LANGUAGE C;
-
--- Tests creating a FDW handler
-CREATE FUNCTION test_fdw_handler()
-    RETURNS fdw_handler
-    AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'
-    LANGUAGE C;
-
-CREATE FUNCTION test_support_func(internal)
-    RETURNS internal
-    AS '@libdir@/regress@DLSUFFIX@', 'test_support_func'
-    LANGUAGE C STRICT;
-
-CREATE FUNCTION test_opclass_options_func(internal)
-    RETURNS void
-    AS '@libdir@/regress@DLSUFFIX@', 'test_opclass_options_func'
-    LANGUAGE C;
-
-CREATE FUNCTION test_enc_conversion(bytea, name, name, bool, validlen OUT int, result OUT bytea)
-    AS '@libdir@/regress@DLSUFFIX@', 'test_enc_conversion'
-    LANGUAGE C STRICT;
-
 -- Things that shouldn't work:

 CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
diff --git a/src/test/regress/output/create_function_0.source b/src/test/regress/output/create_function_0.source
new file mode 100644
index 0000000000..a90049a760
--- /dev/null
+++ b/src/test/regress/output/create_function_0.source
@@ -0,0 +1,35 @@
+--
+-- CREATE_FUNCTION_0
+--
+-- This makes available various test support functions that are in
+-- src/test/regress/regress.c
+-- (although a few more are created in create_function_1)
+--
+CREATE FUNCTION make_tuple_indirect (record)
+    RETURNS record
+    AS '@libdir@/regress@DLSUFFIX@'
+    LANGUAGE C STRICT;
+CREATE FUNCTION test_atomic_ops()
+    RETURNS bool
+    AS '@libdir@/regress@DLSUFFIX@'
+    LANGUAGE C;
+CREATE FUNCTION test_fdw_handler()
+    RETURNS fdw_handler
+    AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'
+    LANGUAGE C;
+CREATE FUNCTION test_support_func(internal)
+    RETURNS internal
+    AS '@libdir@/regress@DLSUFFIX@', 'test_support_func'
+    LANGUAGE C STRICT;
+CREATE FUNCTION test_opclass_options_func(internal)
+    RETURNS void
+    AS '@libdir@/regress@DLSUFFIX@', 'test_opclass_options_func'
+    LANGUAGE C;
+CREATE FUNCTION test_enc_conversion(bytea, name, name, bool,
+                                    validlen OUT int, result OUT bytea)
+    AS '@libdir@/regress@DLSUFFIX@', 'test_enc_conversion'
+    LANGUAGE C STRICT;
+CREATE FUNCTION binary_coercible(oid, oid)
+    RETURNS bool
+    AS '@libdir@/regress@DLSUFFIX@', 'binary_coercible'
+    LANGUAGE C STRICT STABLE PARALLEL SAFE;
diff --git a/src/test/regress/output/create_function_1.source b/src/test/regress/output/create_function_1.source
index c66146db9d..88265779f7 100644
--- a/src/test/regress/output/create_function_1.source
+++ b/src/test/regress/output/create_function_1.source
@@ -47,30 +47,6 @@ CREATE FUNCTION set_ttdummy (int4)
         RETURNS int4
         AS '@libdir@/regress@DLSUFFIX@'
         LANGUAGE C STRICT;
-CREATE FUNCTION make_tuple_indirect (record)
-        RETURNS record
-        AS '@libdir@/regress@DLSUFFIX@'
-        LANGUAGE C STRICT;
-CREATE FUNCTION test_atomic_ops()
-    RETURNS bool
-    AS '@libdir@/regress@DLSUFFIX@'
-    LANGUAGE C;
--- Tests creating a FDW handler
-CREATE FUNCTION test_fdw_handler()
-    RETURNS fdw_handler
-    AS '@libdir@/regress@DLSUFFIX@', 'test_fdw_handler'
-    LANGUAGE C;
-CREATE FUNCTION test_support_func(internal)
-    RETURNS internal
-    AS '@libdir@/regress@DLSUFFIX@', 'test_support_func'
-    LANGUAGE C STRICT;
-CREATE FUNCTION test_opclass_options_func(internal)
-    RETURNS void
-    AS '@libdir@/regress@DLSUFFIX@', 'test_opclass_options_func'
-    LANGUAGE C;
-CREATE FUNCTION test_enc_conversion(bytea, name, name, bool, validlen OUT int, result OUT bytea)
-    AS '@libdir@/regress@DLSUFFIX@', 'test_enc_conversion'
-    LANGUAGE C STRICT;
 -- Things that shouldn't work:
 CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
     AS 'SELECT ''not an integer'';';
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index a091300857..45696031a7 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -24,6 +24,9 @@ test: boolean char name varchar text int2 int4 int8 oid float4 float8 bit numeri
 # ----------
 test: strings numerology point lseg line box path polygon circle date time timetz timestamp timestamptz interval inet
macaddrmacaddr8 tstypes multirangetypes 

+# opr_sanity depends on this
+test: create_function_0
+
 # ----------
 # Another group of parallel tests
 # geometry depends on point, lseg, box, path, polygon and circle
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 1990cbb6a1..d8756e5ba0 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -36,6 +36,7 @@
 #include "nodes/supportnodes.h"
 #include "optimizer/optimizer.h"
 #include "optimizer/plancat.h"
+#include "parser/parse_coerce.h"
 #include "port/atomics.h"
 #include "storage/spin.h"
 #include "utils/builtins.h"
@@ -1194,3 +1195,14 @@ test_enc_conversion(PG_FUNCTION_ARGS)

     PG_RETURN_DATUM(HeapTupleGetDatum(tuple));
 }
+
+/* Provide SQL access to IsBinaryCoercible() */
+PG_FUNCTION_INFO_V1(binary_coercible);
+Datum
+binary_coercible(PG_FUNCTION_ARGS)
+{
+    Oid            srctype = PG_GETARG_OID(0);
+    Oid            targettype = PG_GETARG_OID(1);
+
+    PG_RETURN_BOOL(IsBinaryCoercible(srctype, targettype));
+}
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 5644847601..bdadb1db29 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -43,6 +43,7 @@ test: inet
 test: macaddr
 test: macaddr8
 test: tstypes
+test: create_function_0
 test: geometry
 test: horology
 test: regex
diff --git a/src/test/regress/sql/.gitignore b/src/test/regress/sql/.gitignore
index 46c8112094..fe14af6ae7 100644
--- a/src/test/regress/sql/.gitignore
+++ b/src/test/regress/sql/.gitignore
@@ -1,5 +1,6 @@
 /constraints.sql
 /copy.sql
+/create_function_0.sql
 /create_function_1.sql
 /create_function_2.sql
 /largeobject.sql
diff --git a/src/test/regress/sql/conversion.sql b/src/test/regress/sql/conversion.sql
index ea85f20ed8..8358682432 100644
--- a/src/test/regress/sql/conversion.sql
+++ b/src/test/regress/sql/conversion.sql
@@ -40,7 +40,7 @@ DROP USER regress_conversion_user;
 --

 -- Helper function to test a conversion. Uses the test_enc_conversion function
--- that was created in the create_function_1 test.
+-- that was created in the create_function_0 test.
 create or replace function test_conv(
   input IN bytea,
   src_encoding IN text,
diff --git a/src/test/regress/sql/opr_sanity.sql b/src/test/regress/sql/opr_sanity.sql
index 393acdf8c3..5a9c479692 100644
--- a/src/test/regress/sql/opr_sanity.sql
+++ b/src/test/regress/sql/opr_sanity.sql
@@ -18,65 +18,6 @@
 -- that test creates some bogus operators...


--- Helper functions to deal with cases where binary-coercible matches are
--- allowed.
-
--- This should match IsBinaryCoercible() in parse_coerce.c.
--- It doesn't currently know about some cases, notably domains, anyelement,
--- anynonarray, anyenum, or record, but it doesn't need to (yet).
-create function binary_coercible(oid, oid) returns bool as $$
-begin
-  if $1 = $2 then return true; end if;
-  if EXISTS(select 1 from pg_catalog.pg_cast where
-            castsource = $1 and casttarget = $2 and
-            castmethod = 'b' and castcontext = 'i')
-  then return true; end if;
-  if $2 = 'pg_catalog.any'::pg_catalog.regtype then return true; end if;
-  if $2 = 'pg_catalog.anyarray'::pg_catalog.regtype then
-    if EXISTS(select 1 from pg_catalog.pg_type where
-              oid = $1 and typelem != 0 and
-              typsubscript = 'pg_catalog.array_subscript_handler'::pg_catalog.regproc)
-    then return true; end if;
-  end if;
-  if $2 = 'pg_catalog.anyrange'::pg_catalog.regtype then
-    if (select typtype from pg_catalog.pg_type where oid = $1) = 'r'
-    then return true; end if;
-  end if;
-  if $2 = 'pg_catalog.anymultirange'::pg_catalog.regtype then
-    if (select typtype from pg_catalog.pg_type where oid = $1) = 'm'
-    then return true; end if;
-  end if;
-  return false;
-end
-$$ language plpgsql strict stable;
-
--- This one ignores castcontext, so it will allow cases where an explicit
--- (but still binary) cast would be required to convert the input type.
--- We don't currently use this for any tests in this file, but it is a
--- reasonable alternative definition for some scenarios.
-create function explicitly_binary_coercible(oid, oid) returns bool as $$
-begin
-  if $1 = $2 then return true; end if;
-  if EXISTS(select 1 from pg_catalog.pg_cast where
-            castsource = $1 and casttarget = $2 and
-            castmethod = 'b')
-  then return true; end if;
-  if $2 = 'pg_catalog.any'::pg_catalog.regtype then return true; end if;
-  if $2 = 'pg_catalog.anyarray'::pg_catalog.regtype then
-    if EXISTS(select 1 from pg_catalog.pg_type where
-              oid = $1 and typelem != 0 and
-              typsubscript = 'pg_catalog.array_subscript_handler'::pg_catalog.regproc)
-    then return true; end if;
-  end if;
-  if $2 = 'pg_catalog.anyrange'::pg_catalog.regtype then
-    if (select typtype from pg_catalog.pg_type where oid = $1) = 'r'
-    then return true; end if;
-  end if;
-  return false;
-end
-$$ language plpgsql strict stable;
-
-
 -- **************** pg_proc ****************

 -- Look for illegal values in pg_proc fields.
diff --git a/src/test/regress/sql/type_sanity.sql b/src/test/regress/sql/type_sanity.sql
index 4739aca84a..404c3a2043 100644
--- a/src/test/regress/sql/type_sanity.sql
+++ b/src/test/regress/sql/type_sanity.sql
@@ -465,7 +465,7 @@ FROM pg_range p1 JOIN pg_type t ON t.oid = p1.rngsubtype
 WHERE (rngcollation = 0) != (typcollation = 0);

 -- opclass had better be a btree opclass accepting the subtype.
--- We must allow anyarray matches, cf opr_sanity's binary_coercible()
+-- We must allow anyarray matches, cf IsBinaryCoercible()

 SELECT p1.rngtypid, p1.rngsubtype, o.opcmethod, o.opcname
 FROM pg_range p1 JOIN pg_opclass o ON o.oid = p1.rngsubopc

On Sat, May 08, 2021 at 03:44:57PM -0400, Tom Lane wrote:
> I tried that, by adding a
> shim function to regress.c, and got a most gratifying result:
> on my machine opr_sanity's runtime with
> debug_invalidate_system_caches_always = 1 drops from
> 29m9s to 3m19s.  Without CCA the speedup is far less impressive,
> 360ms to 305ms, but that's still useful.  Especially since this
> makes the test strictly more accurate.

The speedup is quite welcome and still impressive in both cases.

> Anyway, I propose that we ought to sneak this into HEAD, since
> it's only touching test code and not anything production-critical.

+1 for pushing it in HEAD.

Looking at the patch, explicitly_binary_coercible wasn't used since
e9f42d529f990f94e1b7bdcec4a1111465c85326 (and was renamed there too).  Just to
be sure, is it ok to remove it, as it was described as

> --- We don't currently use this for any tests in this file, but it is a
> --- reasonable alternative definition for some scenarios.

It would still be in the git history in needed, so I'm not objecting.

On 5/8/21 3:44 PM, Tom Lane wrote:
> Anyway, I propose that we ought to sneak this into HEAD, since
> it's only touching test code and not anything production-critical.
>
> The patch is a bit more invasive than I would have liked, because
> adding the SQL definition of binary_coercible() to create_function_1
> (where the other regress.c functions are declared) didn't work:
> that runs after opr_sanity, and just moving it up to before
> opr_sanity causes the latter to complain about some of the functions
> in it.  So I ended up splitting the create_function_1 test into
> create_function_0 and create_function_1.   It's annoying from a
> parallelism standpoint that create_function_0 runs by itself, but
> the two parallel groups ahead of it are already full.  Maybe we
> should rebalance that by moving a few of those tests to run in
> parallel with create_function_0, but I didn't do that here.
>
> Thoughts?


+1 for doing it now.


You could possibly just move "inet macaddr macaddr8 " to the following
group and so have room for create_function_0. I just tried that and it
seemed happy.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Julien Rouhaud <rjuju123@gmail.com> writes:
> Looking at the patch, explicitly_binary_coercible wasn't used since
> e9f42d529f990f94e1b7bdcec4a1111465c85326 (and was renamed there too).  Just to
> be sure, is it ok to remove it, as it was described as

>> --- We don't currently use this for any tests in this file, but it is a
>> --- reasonable alternative definition for some scenarios.

> It would still be in the git history in needed, so I'm not objecting.

It's my own comment, so it doesn't scare me particularly ;-).
I think that

(a) it's unlikely we'll ever again need that old physically-coercible
check.  That was a hangover from Berkeley-era type cheats, and I think
our standards are higher now.  If somebody submits a patch that would
depend on such a cheat, I think our response would be "fix the patch",
not "it's okay to weaken the type-matching checks".

(b) if we did need it, we'd probably want an implementation like this
one (ie invoke some C code), both for speed and because it's hard to
make a plpgsql function's behavior match the C code's exactly.

            regards, tom lane

On Sun, May 09, 2021 at 01:01:38PM -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > Looking at the patch, explicitly_binary_coercible wasn't used since
> > e9f42d529f990f94e1b7bdcec4a1111465c85326 (and was renamed there too).  Just to
> > be sure, is it ok to remove it, as it was described as
> 
> >> --- We don't currently use this for any tests in this file, but it is a
> >> --- reasonable alternative definition for some scenarios.
> 
> > It would still be in the git history in needed, so I'm not objecting.
> 
> It's my own comment, so it doesn't scare me particularly ;-).

Yes, I saw that when digging in git history :)

> I think that
> 
> (a) it's unlikely we'll ever again need that old physically-coercible
> check.  That was a hangover from Berkeley-era type cheats, and I think
> our standards are higher now.  If somebody submits a patch that would
> depend on such a cheat, I think our response would be "fix the patch",
> not "it's okay to weaken the type-matching checks".
> 
> (b) if we did need it, we'd probably want an implementation like this
> one (ie invoke some C code), both for speed and because it's hard to
> make a plpgsql function's behavior match the C code's exactly.

I quite agree with both. As I said I just wanted to mention it for extra
safety.

Hi,

On 2021-05-08 15:44:57 -0400, Tom Lane wrote:
> In a nearby thread I bemoaned the fact that the core regression tests
> seem to have gotten significantly slower in the last couple of months,
> at least with CCA enabled: hyrax reports completing them in 12:52:44
> on 18 March, while its most recent run on 1 May took 14:08:18.
>
> Trying to diagnose the cause overall seemed a bit daunting, but
> I thought I'd dig into the opr_sanity test in particular, as it
> is one of the slower tests under CCA to start with and had also
> slowed down noticeably (from 3701581 ms to 4761183 ms, or 28%).
> I was able to complete a bisection using just that test, and
> got an unexpected result: most of the slowdown appeared at
> ab596105b (BRIN minmax-multi indexes).  Apparently the additional
> time is simply from having to check the additional pg_amop and
> pg_amproc entries, which that patch added quite a few of.

I suspect that it might be not just that. From a quick profile it looks
like debug_invalidate_system_caches_always spends a good chunk of its
time in ResetCatalogCache() and hash_seq_search(). Those cost linearly
with the size of the underlying hash tables.

Wo what what might be happening is that the additional catalog entries
pushed some of the catcache hash tables into growing
(RehashCatCache()). Which then makes all subsequent ResetCatalogCache()
scans slower.

Not that that changes much - your proposed fix still seems reasonable.


> I noticed that all of the slowest queries in that test were dependent
> on the binary_coercible() plpgsql function that it uses.  Now, that
> function has always been a rather lame attempt to approximate the
> behavior of the parser's IsBinaryCoercible() function, so I've been
> thinking for some time that we ought to get rid of it in favor of
> actually using IsBinaryCoercible().  I tried that, by adding a
> shim function to regress.c, and got a most gratifying result:
> on my machine opr_sanity's runtime with
> debug_invalidate_system_caches_always = 1 drops from
> 29m9s to 3m19s.  Without CCA the speedup is far less impressive,
> 360ms to 305ms, but that's still useful.  Especially since this
> makes the test strictly more accurate.

Cool!


> Anyway, I propose that we ought to sneak this into HEAD, since
> it's only touching test code and not anything production-critical.

+1

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2021-05-08 15:44:57 -0400, Tom Lane wrote:
>> I was able to complete a bisection using just that test, and
>> got an unexpected result: most of the slowdown appeared at
>> ab596105b (BRIN minmax-multi indexes).  Apparently the additional
>> time is simply from having to check the additional pg_amop and
>> pg_amproc entries, which that patch added quite a few of.

> I suspect that it might be not just that. From a quick profile it looks
> like debug_invalidate_system_caches_always spends a good chunk of its
> time in ResetCatalogCache() and hash_seq_search(). Those cost linearly
> with the size of the underlying hash tables.
> Wo what what might be happening is that the additional catalog entries
> pushed some of the catcache hash tables into growing
> (RehashCatCache()). Which then makes all subsequent ResetCatalogCache()
> scans slower.

Hm.  But constantly flushing the caches should mean that they're never
populated with very many entries at one time, which ought to forestall
that, at least to some extent.

I wonder if there's anything we could do to make ResetCatalogCache
faster?  It wouldn't help much for normal execution of course,
but it might do something to bring CCA testing time down out of
the stratosphere.

            regards, tom lane

Hi,

On 2021-05-10 14:06:16 -0400, Tom Lane wrote:
> Hm.  But constantly flushing the caches should mean that they're never
> populated with very many entries at one time, which ought to forestall
> that, at least to some extent.

That's probably true...


> I wonder if there's anything we could do to make ResetCatalogCache
> faster?  It wouldn't help much for normal execution of course,
> but it might do something to bring CCA testing time down out of
> the stratosphere.

We could make the hashtables shrink, not just grow...

There's also the issue that most people, I assume, run CCA tests with -O0. In
a quick test that does make a big difference in e.g. ResetCatalogCache(). I
just added a function specific annotation to optimize just that function and
the overall time in my test shrank 10% or so.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2021-05-10 14:06:16 -0400, Tom Lane wrote:
>> I wonder if there's anything we could do to make ResetCatalogCache
>> faster?  It wouldn't help much for normal execution of course,
>> but it might do something to bring CCA testing time down out of
>> the stratosphere.

> We could make the hashtables shrink, not just grow...

Maybe ...

> There's also the issue that most people, I assume, run CCA tests with -O0. In
> a quick test that does make a big difference in e.g. ResetCatalogCache(). I
> just added a function specific annotation to optimize just that function and
> the overall time in my test shrank 10% or so.

If they do I think they're nuts ;-).  CCA is slow enough already without
hobbling it.

hyrax appears to use the usual -O2, as does/did avocet.  Not sure
if we have any other CCA buildfarm members right now.

            regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> On 2021-05-10 14:06:16 -0400, Tom Lane wrote:
>> I wonder if there's anything we could do to make ResetCatalogCache
>> faster?  It wouldn't help much for normal execution of course,
>> but it might do something to bring CCA testing time down out of
>> the stratosphere.

> We could make the hashtables shrink, not just grow...

I noticed that we already have counters that can tell whether a
catcache or dynahash table is empty, so I experimented with the
attached patch.  Testing one of the slow queries from privileges.sql
(which might not be very representative of the overall issue),
I see:

HEAD:
Time: 536429.715 ms (08:56.430)

with ResetCatalogCache hack:
Time: 488938.597 ms (08:08.939)

plus hash_seq_search hack:
Time: 475400.634 ms (07:55.401)

Of course, the issue with these patches is that they change these
counters from things that (I think) we only trust for statistical
purposes into things that had better be right or you're going to
have impossible-to-track-down bugs with sometimes failing to
invalidate cache entries.  My gut feeling is that the risk-to-reward
ratio is worth it for changing ResetCatalogCache, but not for
hash_seq_search.  This is partly because of the greater absolute
payback and partly because ResetCatalogCache doesn't deal with
shared data structures, reducing the risk of counting issues.

            regards, tom lane

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 4fbdc62d8c..df5f219254 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -644,6 +644,10 @@ ResetCatalogCache(CatCache *cache)
     dlist_mutable_iter iter;
     int            i;

+    /* If the cache is entirely empty, there's nothing to do */
+    if (cache->cc_ntup == 0)
+        return;
+
     /* Remove each list in this cache, or at least mark it dead */
     dlist_foreach_modify(iter, &cache->cc_lists)
     {
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 6546e3c7c7..1ac87725c0 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -1455,11 +1455,23 @@ hash_seq_search(HASH_SEQ_STATUS *status)
     }

     /*
-     * Search for next nonempty bucket starting at curBucket.
+     * Nothing to do if hash table is entirely empty.  (For a partitioned
+     * hash table, we'd have to check all the freelists, which is unlikely
+     * to be a win.)
      */
-    curBucket = status->curBucket;
     hashp = status->hashp;
     hctl = hashp->hctl;
+    if (hctl->freeList[0].nentries == 0 &&
+        !IS_PARTITIONED(hctl))
+    {
+        hash_seq_term(status);
+        return NULL;            /* table is empty */
+    }
+
+    /*
+     * Search for next nonempty bucket starting at curBucket.
+     */
+    curBucket = status->curBucket;
     ssize = hashp->ssize;
     max_bucket = hctl->max_bucket;

Hi,

On 2021-05-10 16:17:18 -0400, Tom Lane wrote:
> I noticed that we already have counters that can tell whether a
> catcache or dynahash table is empty, so I experimented with the
> attached patch.  Testing one of the slow queries from privileges.sql
> (which might not be very representative of the overall issue),
> I see:
> 
> HEAD:
> Time: 536429.715 ms (08:56.430)
> 
> with ResetCatalogCache hack:
> Time: 488938.597 ms (08:08.939)
> 
> plus hash_seq_search hack:
> Time: 475400.634 ms (07:55.401)

Oh, nice.

Perhaps we generally ought to lower the initial sycache sizes further?
20cb18db4668 did that, but we still have things like PROCNAMEARGNSP,
PROCOID, RELNAMENSP, RELOID, STATRELATTINH, ... using 128 as the initial
size. Not hard to imagine that some of these are larger than what simple
workloads or CCA encounter.


> Of course, the issue with these patches is that they change these
> counters from things that (I think) we only trust for statistical
> purposes into things that had better be right or you're going to
> have impossible-to-track-down bugs with sometimes failing to
> invalidate cache entries.  My gut feeling is that the risk-to-reward
> ratio is worth it for changing ResetCatalogCache, but not for
> hash_seq_search.  This is partly because of the greater absolute
> payback and partly because ResetCatalogCache doesn't deal with
> shared data structures, reducing the risk of counting issues.

That sounds reasonable. We could mitigate the risk for dynahash by
testing HASH_SHARED_MEM (which we don't store right now), but it's not
clear it's worth it here. But I wonder if there's other cases where it
could help? If we did make the check support shared memory *and*
partitioned tables, I could easily see it be a win for things like
LockReleaseAll().

Greetings,

Andres Freund

On Tue, May 11, 2021 at 8:52 AM Andres Freund <andres@anarazel.de> wrote:
> ... If we did make the check support shared memory *and*
> partitioned tables, I could easily see it be a win for things like
> LockReleaseAll().

For that case, has the idea of maintaining a dlist of local locks been
considered?

Hi,

On 2021-05-11 10:57:03 +1200, Thomas Munro wrote:
> On Tue, May 11, 2021 at 8:52 AM Andres Freund <andres@anarazel.de> wrote:
> > ... If we did make the check support shared memory *and*
> > partitioned tables, I could easily see it be a win for things like
> > LockReleaseAll().

Errr, that's not even a shared hashtable... So it would help even if we
just excluded shared memory hashtables.


> For that case, has the idea of maintaining a dlist of local locks been
> considered?

Yea, there's been a long discussion on that for
LockReleaseAll(). Combined with alternatives around shrinking the hashtable...

Greetings,

Andres Freund

Andrew Dunstan <andrew@dunslane.net> writes:
> +1 for doing it now.

Pushed.

> You could possibly just move "inet macaddr macaddr8 " to the following
> group and so have room for create_function_0. I just tried that and it
> seemed happy.

I decided that the minimum change would be to push tstypes to the
following group, so I did it like that.

            regards, tom lane