Added failure test for create index concurrently #2210

mtuncer · 2018-06-12T06:34:53Z

No description provided.

onderkalaci · 2018-06-12T10:24:40Z

src/test/regress/expected/failure_create_index_concurrently.out

+
+(1 row)
+
+CREATE INDEX CONCURRENTLY idx_index_test ON index_test(id, value_1);


we could maybe add a cancellation test as well?

codecov · 2018-07-06T19:22:22Z

Codecov Report

Merging #2210 into master will increase coverage by 0.49%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2210      +/-   ##
==========================================
+ Coverage   93.27%   93.77%   +0.49%     
==========================================
  Files         110      110              
  Lines       27215    27477     +262     
==========================================
+ Hits        25385    25766     +381     
+ Misses       1830     1711     -119

onderkalaci · 2018-07-11T14:02:27Z

src/test/regress/sql/failure_create_index_concurrently.sql

@@ -0,0 +1,65 @@
+--


Considering this comment and thinking about this PR as the main one for bare DDLs, I could suggest three more tests:

Test with replication factor 1 as well (I think the default in the regression test suite is two, but, please double check that)

Add tests for DROP INDEX CONCURRENTLY as well.

Since the CREATE/DROP INDEX CONCURRENTLY uses sequential execution mode, we could kill/cancel the second (or later) commands over a single connection. For example, if you create 8 shards, we'd end up with 4 shards per worker and 4 shards are modified over the same connection per node. So, say if you have whatevercase().after(2).kill(), it'd kill the connection after seeing the same command 2 times. An example can be found here

onderkalaci · 2018-07-11T14:04:54Z

src/test/regress/expected/failure_create_index_concurrently.out

+HINT:  Use DROP INDEX CONCURRENTLY IF EXISTS to remove the invalid index, then retry the original command.
+SELECT citus.dump_network_traffic();
+                                                                                                                                                                                                                                                                       dump_network_traffic                                                                                                                                                                                                                                                                        
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


This is a discussion we've made with @lithp as well, but we haven't concluded the discussion.

I think this dump_network_traffic () is super useful by itself both during development and debugging. But, do you see necessary to add to regression tests? I'm OK with keeping it, but I don't add them to the PRs that I open because they seem too verbose to add to the output. I even hit few cases where the output isn't consistent such as here. (I don't expect inconsistencies for this case, but, just wanted to note)

I think you're right that they're too verbose to add to every line. A style that I think makes sense is to show the dump just once at the top, run a happy-case so future readers can see exactly what the rest of the test is mucking with. These two dumps don't really show me what is supposed to happen, they just show the same truncated log twice.

I don't want to impose that style on anyone though if they don't agree with it.

lithp · 2018-07-12T01:53:23Z

src/test/regress/expected/failure_create_index_concurrently.out

+
+(1 row)
+
+SELECT citus.clear_network_traffic();


There's no need to call clear_network_traffic() if you're not going to dump afterwards. All it does it empty the buffer of captured packets.

mtuncer · 2018-07-12T12:58:18Z

removed clear/dump network traffic calls
created tables in a schema to allow parallel execution
added drop index concurrently test
added after(1) calls to allow some commands to succeed, rest to fail

mtuncer · 2018-07-12T13:00:15Z

@lithp @onderkalaci I am not really enjoying worker_2_port + 2 to describe nodeport Can we define something like worker_2_proxy or worker_2_proxy_port in the test infrastructure

onderkalaci

I think you can simply add that in this PR:

diff --git a/src/test/regress/pg_regress_multi.pl b/src/test/regress/pg_regress_multi.pl
index 6274a83..3389728 100755
--- a/src/test/regress/pg_regress_multi.pl
+++ b/src/test/regress/pg_regress_multi.pl
@@ -262,6 +262,8 @@ for (my $workerIndex = 1; $workerIndex <= $workerCount; $workerIndex++) {
     push(@workerPorts, $workerPort);
 }
 
+my $workerBehindProxyPort = $workerPorts[1] + 2;
+
 my $followerCoordPort = 9070;
 my @followerWorkerPorts = ();
 for (my $workerIndex = 1; $workerIndex <= $workerCount; $workerIndex++) {
@@ -417,6 +419,7 @@ if ($usingWindows)
 }
 print $fh catfile($bindir, "psql")." ";
 print $fh "--variable=master_port=$masterPort ";
+print $fh "--variable=worker_2_proxy_port=$workerBehindProxyPort ";
 print $fh "--variable=follower_master_port=$followerCoordPort ";
 print $fh "--variable=default_user=$user ";
 print $fh "--variable=SHOW_CONTEXT=always ";
@@ -425,6 +428,9 @@ for my $workeroff (0 .. $#workerPorts)
     my $port = $workerPorts[$workeroff];
     print $fh "--variable=worker_".($workeroff+1)."_port=$port ";
 }

or maybe with a very minor syntax difference. If you are to add the port to the regression tests, consider the minor comment as well.

onderkalaci · 2018-07-12T14:39:09Z

src/test/regress/sql/failure_create_index_concurrently.sql

+SELECT * FROM run_command_on_workers($$SELECT count(*) FROM pg_indexes WHERE indexname LIKE 'idx_index_test%' $$)
+WHERE nodeport = :worker_2_proxy_port;
+
+


Pretty minor: extra new lines

lithp · 2018-07-24T22:09:18Z

src/test/regress/expected/failure_create_index_concurrently.out

+
+-- verify index is not created
+SELECT * FROM run_command_on_workers($$SELECT count(*) FROM pg_indexes WHERE indexname LIKE 'idx_index_test%' $$)
+WHERE nodeport = :worker_2_proxy_port;


Another way would have been to write WHERE nodeport != :worker_1_port

lithp · 2018-07-24T22:17:01Z

src/test/regress/pg_regress_multi.pl

@@ -417,6 +419,7 @@ sub revert_replace_postgres
 }
 print $fh catfile($bindir, "psql")." ";
 print $fh "--variable=master_port=$masterPort ";
+print $fh "--variable=worker_2_proxy_port=$workerBehindProxyPort ";


We could also change :worker_2_port and have it always point at the proxy, that sounds like a cleaner option.

mtuncer requested a review from lithp June 12, 2018 06:35

onderkalaci reviewed Jun 12, 2018

View reviewed changes

lithp force-pushed the mitmproxy-failure-testing branch 3 times, most recently from 8eea61c to 9133912 Compare June 21, 2018 20:48

onderkalaci mentioned this pull request Jun 26, 2018

Mitmproxy-based automated failure testing #2119

Merged

16 tasks

lithp force-pushed the mitmproxy-failure-testing branch 3 times, most recently from 3121431 to 6c01440 Compare June 29, 2018 02:13

lithp force-pushed the mitmproxy-failure-testing branch from 6c01440 to 3e309e3 Compare July 6, 2018 18:51

lithp force-pushed the mt_failure_test branch from 8645def to 94d0020 Compare July 6, 2018 19:22

metdos mentioned this pull request Jul 10, 2018

Automate Remaining Failure/Cancellation Tests #2262

Open

5 tasks

mtuncer force-pushed the mt_failure_test branch 2 times, most recently from 05af0df to 6749b10 Compare July 11, 2018 08:33

mtuncer changed the base branch from mitmproxy-failure-testing to master July 11, 2018 08:34

mtuncer force-pushed the mt_failure_test branch 4 times, most recently from 35ea52f to 7d24e08 Compare July 11, 2018 13:56

onderkalaci reviewed Jul 11, 2018

View reviewed changes

lithp reviewed Jul 12, 2018

View reviewed changes

mtuncer force-pushed the mt_failure_test branch from 4fa1291 to 7b56157 Compare July 12, 2018 13:03

onderkalaci approved these changes Jul 12, 2018

View reviewed changes

Added failure test for create index concurrently

2795494

mtuncer force-pushed the mt_failure_test branch from 7b56157 to 2795494 Compare July 13, 2018 08:54

mtuncer merged commit b51d252 into master Jul 13, 2018

mtuncer deleted the mt_failure_test branch July 16, 2018 06:42

lithp reviewed Jul 24, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added failure test for create index concurrently #2210

Added failure test for create index concurrently #2210

mtuncer commented Jun 12, 2018

onderkalaci Jun 12, 2018

codecov bot commented Jul 6, 2018 •

edited

Loading

onderkalaci Jul 11, 2018

onderkalaci Jul 11, 2018

lithp Jul 12, 2018

lithp Jul 12, 2018

mtuncer commented Jul 12, 2018

mtuncer commented Jul 12, 2018

onderkalaci left a comment

onderkalaci Jul 12, 2018

lithp Jul 24, 2018

lithp Jul 24, 2018


		(1 row)

		CREATE INDEX CONCURRENTLY idx_index_test ON index_test(id, value_1);

		SELECT * FROM run_command_on_workers($$SELECT count(*) FROM pg_indexes WHERE indexname LIKE 'idx_index_test%' $$)
		WHERE nodeport = :worker_2_proxy_port;

Added failure test for create index concurrently #2210

Added failure test for create index concurrently #2210

Conversation

mtuncer commented Jun 12, 2018

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtuncer commented Jul 12, 2018

mtuncer commented Jul 12, 2018

onderkalaci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2018 •

edited

Loading