Skip to content

Commit 6c31ff6

Browse files
committed
[yugabyte#27062] YSQL: Fix ConcurrentTablespaceTest.testAlterIndexSetTablespace
Summary: Currently, `ConcurrentTablespaceTest.testAlterIndexSetTablespace` has a concurrency bug due to the way `CyclicBarrier` is used. The test starts two threads, one that does DDL and one that does DML, and has them each execute 12 statements. Before executing each statement, each thread calls `CyclicBarrier.await` to ensure they are executing the statements at the same time. ### The problem The problem is when the DML thread can runs into retriable errors (which is expected)—if this happens, the statement is retried without incrementing the statement counter, but the thread calls `CyclicBarrier.await` before retrying the statement. This means that if thread A runs into an error on statement 10, it'll retry statement 10 but this retry will be synchronized with other threads' statement 11. This causes an issue at the end of the test when the threads run into uneven numbers of errors; since `CyclicBarrier.await` waits until all threads call `await`, it's possible that one thread finishes executing, so it never calls `await`, but another thread is stuck waiting at `await`. ### The fix We fix this by swapping out the `CyclicBarrier` for a `Phaser`--`CyclicBarrier` requires a fixed number of threads, while `Phaser` allows for a dynamic number of threads. Using the `Phaser`, we de-register threads that are done executing, allowing the remaining threads to continue execution afterwards. Jira: DB-16543 Test Plan: ``` ./yb_build.sh release --java-test org.yb.pgsql.ConcurrentTablespaceTest#testAlterIndexSetTablespace ``` Reviewers: sanketh, myang Reviewed By: myang Differential Revision: https://phorge.dev.yugabyte.com/D43992
1 parent 363fe44 commit 6c31ff6

File tree

1 file changed

+24
-18
lines changed

1 file changed

+24
-18
lines changed

java/yb-pgsql/src/test/java/org/yb/pgsql/ConcurrentTablespaceTest.java

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ private Tablespace[] generateTestTablespaces() {
121121

122122
private List<Thread> setupConcurrentDdlDmlThreads(String ddlTemplate) {
123123
final int totalThreads = numDmlThreads + numDdlThreads;
124-
final CyclicBarrier barrier = new CyclicBarrier(totalThreads);
124+
final Phaser phaser = new Phaser(totalThreads);
125125
final List<Thread> threads = new ArrayList<>();
126126

127127
// Add the DDL thread.
@@ -131,14 +131,14 @@ private List<Thread> setupConcurrentDdlDmlThreads(String ddlTemplate) {
131131
connections[i],
132132
ddlTemplate,
133133
errorsDetected,
134-
barrier,
134+
phaser,
135135
numStmtsPerThread,
136136
tablespaces));
137137
}
138138

139139
// Add the DML threads.
140140
for (int i = numDdlThreads; i < totalThreads; ++i) {
141-
threads.add(new DMLRunner(connections[i], errorsDetected, barrier, numStmtsPerThread, i));
141+
threads.add(new DMLRunner(connections[i], errorsDetected, phaser, numStmtsPerThread, i));
142142
}
143143
return threads;
144144
}
@@ -223,7 +223,7 @@ public void testTableCreationFailure() throws Exception {
223223
YBClient client = miniCluster.getClient();
224224
connections = setupConnections();
225225
final int totalThreads = numDmlThreads + numDdlThreads;
226-
final CyclicBarrier barrier = new CyclicBarrier(totalThreads);
226+
final Phaser phaser = new Phaser(totalThreads);
227227
final List<Thread> threads = new ArrayList<>();
228228
AtomicBoolean invalidPlacementError = new AtomicBoolean(false);
229229

@@ -236,7 +236,7 @@ public void testTableCreationFailure() throws Exception {
236236
connections[0],
237237
"CREATE TABLE validplacementtable (a int) TABLESPACE %s",
238238
errorsDetected,
239-
barrier,
239+
phaser,
240240
1,
241241
new Tablespace[] {valid_ts}));
242242

@@ -248,7 +248,7 @@ public void testTableCreationFailure() throws Exception {
248248
connections[1],
249249
"CREATE TABLE invalid_placementtable (a int) TABLESPACE %s",
250250
invalidPlacementError,
251-
barrier,
251+
phaser,
252252
1,
253253
new Tablespace[] {invalid_ts}));
254254

@@ -268,19 +268,19 @@ public void testTableCreationFailure() throws Exception {
268268
public abstract class SQLRunner extends Thread {
269269
protected final Connection conn;
270270
protected final AtomicBoolean errorsDetected;
271-
protected final CyclicBarrier barrier;
271+
protected final Phaser phaser;
272272
protected final int numStmtsPerThread;
273273
protected int idx; // Only used by DMLRunner
274274

275275
public SQLRunner(
276276
Connection conn,
277277
AtomicBoolean errorsDetected,
278-
CyclicBarrier barrier,
278+
Phaser phaser,
279279
int numStmtsPerThread,
280280
int idx) {
281281
this.conn = conn;
282282
this.errorsDetected = errorsDetected;
283-
this.barrier = barrier;
283+
this.phaser = phaser;
284284
this.numStmtsPerThread = numStmtsPerThread;
285285
this.idx = idx; // This field is not used in DDLRunner
286286
}
@@ -289,16 +289,22 @@ public SQLRunner(
289289
public void run() {
290290
int item_idx = 0;
291291
while (item_idx < numStmtsPerThread && !errorsDetected.get()) {
292-
try (Statement lstmt = conn.createStatement()) {
293-
barrier.await();
294-
executeStatement(lstmt, item_idx);
292+
try {
293+
phaser.arriveAndAwaitAdvance();
294+
295+
try (Statement lstmt = conn.createStatement()) {
296+
executeStatement(lstmt, item_idx);
297+
}
298+
295299
item_idx++;
296300
} catch (PSQLException e) {
297301
handlePSQLException(e);
298-
} catch (SQLException | InterruptedException | BrokenBarrierException e) {
302+
} catch (SQLException e) {
299303
logAndSetError(e);
300304
}
301305
}
306+
// Thread finished: shrink the party count.
307+
phaser.arriveAndDeregister();
302308
}
303309

304310
protected abstract void executeStatement(Statement lstmt, int item_idx) throws SQLException;
@@ -324,7 +330,7 @@ private void handlePSQLException(PSQLException e) {
324330
protected void logAndSetError(Exception e) {
325331
LOG.info("SQL thread: Unexpected error: ", e);
326332
errorsDetected.set(true);
327-
barrier.reset();
333+
phaser.forceTermination();
328334
}
329335
}
330336

@@ -337,10 +343,10 @@ public class DMLRunner extends SQLRunner {
337343
public DMLRunner(
338344
Connection conn,
339345
AtomicBoolean errorsDetected,
340-
CyclicBarrier barrier,
346+
Phaser phaser,
341347
int numStmtsPerThread,
342348
int idx) {
343-
super(conn, errorsDetected, barrier, numStmtsPerThread, idx);
349+
super(conn, errorsDetected, phaser, numStmtsPerThread, idx);
344350
}
345351

346352
@Override
@@ -372,10 +378,10 @@ public DDLRunner(
372378
Connection conn,
373379
String sql,
374380
AtomicBoolean errorsDetected,
375-
CyclicBarrier barrier,
381+
Phaser phaser,
376382
int numStmtsPerThread,
377383
Tablespace[] tablespaces) {
378-
super(conn, errorsDetected, barrier, numStmtsPerThread, 0); // idx is not used here
384+
super(conn, errorsDetected, phaser, numStmtsPerThread, 0); // idx is not used here
379385
this.sql = sql;
380386
this.tablespaces = tablespaces;
381387
}

0 commit comments

Comments
 (0)