Avoid race condition for event_triggers regress test

Started by Mikhail Gribkovabout 2 years ago2 messages

youzhick@gmail.com

about 2 years ago

1 attachment(s)

Hi hackers,

After committing the on-login trigger
(e83d1b0c40ccda8955f1245087f0697652c4df86) the event_trigger regress test
became sensible to any other parallel tests, not only DDL. Thus it should
be placed in a separate parallel schedule group.

The current problem is that a race condition may occur on some systems,
when oidjoins test starts a moment later than normally and affects logins
count for on-login trigger test. The problem is quite a rare one and I only
faced it once. But rare or not - the problem is a problem and it should be
addressed.

Such race condition can be simulated by adding "select pg_sleep(2);" and
"\c" at the very beginning of oidjoins.sql and adding "select pg_sleep(5);"
after creation of the login trigger in event_trigger.sql.
The resulting symptoms are quite recognizable: regression.diffs file will
contain unexpected welcome message for oidjoins test and unexpectedly
increased result of "SELECT COUNT(*) FROM user_logins;" for event_triggers
test. (These are accompanied with the expected responses to the newly added
commands of course)

To get rid of the unexpected results the oidjoins and event_triggers tests
should be splitted into separate paralell schedule groups. This is exactly
what the proposed (attached) patch is doing.

What do you think?
--
best regards,
Mikhail A. Gribkov

e-mail: youzhick@gmail.com

Attachments:

v001_avoid_race_condition_for_event_trigger_test.patchapplication/octet-stream; name=v001_avoid_race_condition_for_event_trigger_test.patchDownload

diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 4df9d8503b..5677ae3cac 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -121,10 +121,12 @@ test: plancache limit plpgsql copy2 temp domain rangefuncs prepare conversion tr
 # ----------
 test: partition_join partition_prune reloptions hash_part indexing partition_aggregate partition_info tuplesort explain compression memoize stats
 
-# event_trigger depends on create_am and cannot run concurrently with
-# any test that runs DDL
-# oidjoins is read-only, though, and should run late for best coverage
-test: event_trigger oidjoins
+# event_trigger cannot run concurrently with any other tests because
+# of on-login event handling
+test: event_trigger
+
+# oidjoins should run late for best coverage
+test: oidjoins
 
 # this test also uses event triggers, so likewise run it by itself
 test: fast_default

Aleksander Alekseev

aleksander@timescale.com

about 2 years ago

In reply to: Mikhail Gribkov (#1)

1 attachment(s)

Re: Avoid race condition for event_triggers regress test

Hi,

The current problem is that a race condition may occur on some systems, when oidjoins test starts a moment later than normally and affects logins count for on-login trigger test. The problem is quite a rare one and I only faced it once. But rare or not - the problem is a problem and it should be addressed.

Thanks for the patch and the steps to reproduce.

I tested the patch and it does what is claimed. Including the steps to
reproduce as a separate patch with .txt extension so cfbot will ignore
it.

I think it's a good find and a good fix.

--
Best regards,
Aleksander Alekseev

Attachments:

reproduce.txttext/plain; charset=US-ASCII; name=reproduce.txtDownload

commit 80df7cf26476a3ede42310354715e972fa40a8cf
Author: Aleksander Alekseev <aleksander@timescale.com>
Date:   Thu Oct 19 16:30:27 2023 +0300

    reproduce

diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index eaaff6ba6f..741099747c 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -648,6 +648,13 @@ BEGIN
 END;
 $$ LANGUAGE plpgsql;
 CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE on_login_proc();
+-- AALEKSEEV DEBUG
+select pg_sleep(5);
+ pg_sleep 
+----------
+ 
+(1 row)
+
 ALTER EVENT TRIGGER on_login_trigger ENABLE ALWAYS;
 \c
 NOTICE:  You are welcome!
diff --git a/src/test/regress/expected/oidjoins.out b/src/test/regress/expected/oidjoins.out
index 215eb899be..01f4959f21 100644
--- a/src/test/regress/expected/oidjoins.out
+++ b/src/test/regress/expected/oidjoins.out
@@ -1,3 +1,11 @@
+-- AALEKSEEV DEBUG
+select pg_sleep(2);
+ pg_sleep 
+----------
+ 
+(1 row)
+
+\c
 --
 -- Verify system catalog foreign key relationships
 --
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 9c2b7903fb..84b0b7fac8 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -506,6 +506,10 @@ BEGIN
 END;
 $$ LANGUAGE plpgsql;
 CREATE EVENT TRIGGER on_login_trigger ON login EXECUTE PROCEDURE on_login_proc();
+
+-- AALEKSEEV DEBUG
+select pg_sleep(5);
+
 ALTER EVENT TRIGGER on_login_trigger ENABLE ALWAYS;
 \c
 SELECT COUNT(*) FROM user_logins;
diff --git a/src/test/regress/sql/oidjoins.sql b/src/test/regress/sql/oidjoins.sql
index 8b22e6d10c..576d5f03a9 100644
--- a/src/test/regress/sql/oidjoins.sql
+++ b/src/test/regress/sql/oidjoins.sql
@@ -1,3 +1,7 @@
+-- AALEKSEEV DEBUG
+select pg_sleep(2);
+\c
+
 --
 -- Verify system catalog foreign key relationships
 --