> On 23 Aug 2023, at 08:58, Andres Freund <and...@anarazel.de> wrote:

> I'm hoping to push this fairly soon, as I'll be on vacation the last week of
> August. I'll be online intermittently though, if there are issues, I can react
> (very limited connectivity for middday Aug 29th - midday Aug 31th though). I'd
> appreciate a quick review or two.

I've been reading over these and the thread, and while not within my area of
expertise, nothing really sticks out.

I'll do another pass, but below are a few small comments so far.

I don't know Windows to know the implications, but should the below file have
some sort of warning about not doing that for production/shared systems, only
for dedicated test instances?

+++ b/src/tools/ci/windows_write_cache.ps1
@@ -0,0 +1,20 @@
+# Define the write cache to be power protected. This reduces the rate of cache
+# flushes, which seems to help metadata heavy workloads on NTFS. We're just
+# testing here anyway, so ...
+#
+# Let's do so for all disks, this could be useful beyond cirrus-ci.

One thing in 0010 caught my eye, and while not introduced in this patchset it
might be of interest here.  In the below hunks we loop X ticks around
system(psql), with the loop assuming the server can come up really quickly and
sleeping if it doesn't.  On my systems I always reach the pg_usleep after
failing the check, but if I reverse the check such it first sleeps and then
checks I only need to check once instead of twice.

@@ -2499,7 +2502,7 @@ regression_main(int argc, char *argv[],
                else
                        wait_seconds = 60;
 
-               for (i = 0; i < wait_seconds; i++)
+               for (i = 0; i < wait_seconds * WAITS_PER_SEC; i++)
                {
                        /* Done if psql succeeds */
                        fflush(NULL);
@@ -2519,7 +2522,7 @@ regression_main(int argc, char *argv[],
                                         outputdir);
                        }
 
-                       pg_usleep(1000000L);
+                       pg_usleep(1000000L / WAITS_PER_SEC);
                }
                if (i >= wait_seconds)
                {

It's a micro-optimization, but if we're changing things here to chase cycles it
might perhaps be worth doing?

--
Daniel Gustafsson



Reply via email to