Skip to content

Commit 7a81add

Browse files
jerrinotclaude
andcommitted
Fix stale datagram estimate on timestamp rename
TableOptionsImpl.designatedTimestamp() invalidated the cached base estimate only on the first name set. QwpTableBuffer permits renaming the designated timestamp while rowCount == 0, and the cache (keyed on column count alone) folds in the trailer size, which depends on the name length. After a flush kept the buffer's name and cache alive, a rename reused the shorter-trailer estimate, so a bounded UDP sender could emit datagrams above maxDatagramSize. The fix invalidates the cache on every accepted set/change; the committedDatagramEstimate bump stays under previousName == null because a rename requires rowCount == 0, where the committed estimate is already 0. The regression test asserts committedDatagramEstimate covers the actual datagram after a short-name flush and a long-name rename (fails at 43 vs 155 bytes without the fix). A new encoder test mirrors flushPendingRowsSplit: consecutive single-table messages on a reused encoder each carry the FLAG_TABLE_OPTIONS flag and trailer, and a table without options does not inherit them. The FLAG_TABLE_OPTIONS javadoc now records the forward-compat guarantee the ungated emission relies on: pre-table-options servers validate only magic, version and payload length, and iterate exactly the header-declared table count, so unknown flag bits and trailer bytes after the last table body are ignored (verified against the server decoder at 51b235b9ca~1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 80dabaa commit 7a81add

4 files changed

Lines changed: 133 additions & 9 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/qwp/client/QwpUdpSender.java

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1550,15 +1550,17 @@ public TableOptions designatedTimestamp(CharSequence columnName) {
15501550
}
15511551
String previousName = tableBuffer.getDesignatedTimestampName();
15521552
tableBuffer.setDesignatedTimestampName(columnName);
1553-
if (previousName == null) {
1554-
if (currentTableHeadroomState != null) {
1555-
currentTableHeadroomState.invalidateBaseEstimate();
1556-
}
1557-
if (trackDatagramEstimate && committedDatagramEstimate > 0) {
1558-
committedDatagramEstimate += QwpColumnWriter.getSingleTableOptionsTrailerSize(
1559-
tableBuffer.getDesignatedTimestampName()
1560-
);
1561-
}
1553+
// The cached base estimate folds in the trailer size, which depends on
1554+
// the name length, so every accepted set/change must invalidate it. The
1555+
// committedDatagramEstimate bump stays under previousName == null: a name
1556+
// change requires rowCount == 0, where the committed estimate is already 0.
1557+
if (currentTableHeadroomState != null) {
1558+
currentTableHeadroomState.invalidateBaseEstimate();
1559+
}
1560+
if (previousName == null && trackDatagramEstimate && committedDatagramEstimate > 0) {
1561+
committedDatagramEstimate += QwpColumnWriter.getSingleTableOptionsTrailerSize(
1562+
tableBuffer.getDesignatedTimestampName()
1563+
);
15621564
}
15631565
return this;
15641566
}

core/src/main/java/io/questdb/client/cutlass/qwp/protocol/QwpConstants.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,14 @@ public final class QwpConstants {
5252
public static final byte FLAG_GORILLA = 0x04;
5353
/**
5454
* Flag bit: per-table options trailer enabled.
55+
* <p>
56+
* The client sets this flag whenever a table option is declared, without
57+
* gating on the negotiated capability (UDP has no handshake, and
58+
* store-and-forward frames must replay to arbitrary servers). This relies
59+
* on the server-side forward-compat guarantee: pre-table-options servers
60+
* validate only magic, version and payload length (unknown flag bits are
61+
* ignored) and iterate exactly the header-declared table count, so trailer
62+
* bytes after the last table body are never read.
5563
*/
5664
public static final byte FLAG_TABLE_OPTIONS = 0x20;
5765
/**

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/QwpUdpSenderTest.java

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1745,6 +1745,40 @@ public void testTableOptionsOnlyRowIsDiscarded() throws Exception {
17451745
});
17461746
}
17471747

1748+
@Test
1749+
public void testTableOptionsRenameInvalidatesCachedDatagramEstimate() throws Exception {
1750+
assertMemoryLeak(() -> {
1751+
CapturingNetworkFacade nf = new CapturingNetworkFacade();
1752+
StringBuilder longName = new StringBuilder();
1753+
for (int i = 0; i < 120; i++) {
1754+
longName.append('t');
1755+
}
1756+
int maxDatagramSize = 1024;
1757+
try (QwpUdpSender sender = new QwpUdpSender(nf, 0, 0, 9000, 1, maxDatagramSize)) {
1758+
sender.table("t").tableOptions().designatedTimestamp("a");
1759+
sender.longColumn("x", 1).atNow();
1760+
sender.flush();
1761+
Assert.assertEquals(1, nf.packets.size());
1762+
1763+
// rowCount == 0 after the flush, so re-declaring a different (much
1764+
// longer) name is accepted; the cached base estimate sized for "a"
1765+
// must not survive the rename
1766+
sender.table("t").tableOptions().designatedTimestamp(longName);
1767+
sender.longColumn("x", 2).atNow();
1768+
long estimate = sender.committedDatagramEstimateForTest();
1769+
sender.flush();
1770+
1771+
Assert.assertEquals(2, nf.packets.size());
1772+
int actual = nf.packets.get(1).length;
1773+
Assert.assertTrue(
1774+
"committed estimate must cover the actual datagram [estimate=" + estimate + ", actual=" + actual + ']',
1775+
estimate >= actual
1776+
);
1777+
assertPacketsWithinLimit(new RunResult(nf.packets, nf.lengths, nf.sendCount), maxDatagramSize);
1778+
}
1779+
});
1780+
}
1781+
17481782
@Test
17491783
public void testTableOptionsRetainedReferenceRetargetsOnNextTableOptionsCall() throws Exception {
17501784
assertMemoryLeak(() -> {

core/src/test/java/io/questdb/client/test/cutlass/qwp/client/QwpWebSocketEncoderTest.java

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1402,6 +1402,86 @@ public void testTableOptionsTrailerEncodingWithMixedPresence() throws Exception
14021402
});
14031403
}
14041404

1405+
@Test
1406+
public void testTableOptionsTrailerOnConsecutiveSplitMessages() throws Exception {
1407+
assertMemoryLeak(() -> {
1408+
try (QwpWebSocketEncoder encoder = new QwpWebSocketEncoder();
1409+
QwpTableBuffer first = new QwpTableBuffer("first");
1410+
QwpTableBuffer second = new QwpTableBuffer("second");
1411+
QwpTableBuffer third = new QwpTableBuffer("third")) {
1412+
first.setDesignatedTimestampName("ts");
1413+
first.getOrCreateColumn("x", TYPE_LONG, false).addLong(1);
1414+
first.nextRow();
1415+
second.getOrCreateColumn("x", TYPE_LONG, false).addLong(2);
1416+
second.nextRow();
1417+
third.setDesignatedTimestampName("event_ts");
1418+
third.getOrCreateColumn("x", TYPE_LONG, false).addLong(3);
1419+
third.nextRow();
1420+
1421+
// mirrors flushPendingRowsSplit: one single-table message per
1422+
// table on the same reused encoder
1423+
GlobalSymbolDictionary dict = new GlobalSymbolDictionary();
1424+
1425+
encoder.beginMessage(1, dict, -1, -1);
1426+
encoder.addTable(first);
1427+
int size1 = encoder.finishMessage();
1428+
long address1 = encoder.getBuffer().getBufferPtr();
1429+
Assert.assertEquals(
1430+
FLAG_TABLE_OPTIONS,
1431+
(byte) (Unsafe.getUnsafe().getByte(address1 + HEADER_OFFSET_FLAGS) & FLAG_TABLE_OPTIONS)
1432+
);
1433+
Assert.assertEquals(size1 - HEADER_SIZE, Unsafe.getUnsafe().getInt(address1 + 8));
1434+
byte[] expectedTrailer1 = {
1435+
4,
1436+
TABLE_OPTION_TAG_DESIGNATED_TIMESTAMP_NAME,
1437+
2, 't', 's',
1438+
5, 0, 0, 0
1439+
};
1440+
long trailerAddress1 = address1 + size1 - expectedTrailer1.length;
1441+
for (int i = 0; i < expectedTrailer1.length; i++) {
1442+
Assert.assertEquals(
1443+
"message 1 trailer byte " + i,
1444+
expectedTrailer1[i],
1445+
Unsafe.getUnsafe().getByte(trailerAddress1 + i)
1446+
);
1447+
}
1448+
1449+
// a table without options must not inherit the previous
1450+
// message's flag or trailer
1451+
encoder.beginMessage(1, dict, -1, -1);
1452+
encoder.addTable(second);
1453+
int size2 = encoder.finishMessage();
1454+
long address2 = encoder.getBuffer().getBufferPtr();
1455+
Assert.assertEquals(0, Unsafe.getUnsafe().getByte(address2 + HEADER_OFFSET_FLAGS) & FLAG_TABLE_OPTIONS);
1456+
Assert.assertEquals(size2 - HEADER_SIZE, Unsafe.getUnsafe().getInt(address2 + 8));
1457+
1458+
encoder.beginMessage(1, dict, -1, -1);
1459+
encoder.addTable(third);
1460+
int size3 = encoder.finishMessage();
1461+
long address3 = encoder.getBuffer().getBufferPtr();
1462+
Assert.assertEquals(
1463+
FLAG_TABLE_OPTIONS,
1464+
(byte) (Unsafe.getUnsafe().getByte(address3 + HEADER_OFFSET_FLAGS) & FLAG_TABLE_OPTIONS)
1465+
);
1466+
Assert.assertEquals(size3 - HEADER_SIZE, Unsafe.getUnsafe().getInt(address3 + 8));
1467+
byte[] expectedTrailer3 = {
1468+
10,
1469+
TABLE_OPTION_TAG_DESIGNATED_TIMESTAMP_NAME,
1470+
8, 'e', 'v', 'e', 'n', 't', '_', 't', 's',
1471+
11, 0, 0, 0
1472+
};
1473+
long trailerAddress3 = address3 + size3 - expectedTrailer3.length;
1474+
for (int i = 0; i < expectedTrailer3.length; i++) {
1475+
Assert.assertEquals(
1476+
"message 3 trailer byte " + i,
1477+
expectedTrailer3[i],
1478+
Unsafe.getUnsafe().getByte(trailerAddress3 + i)
1479+
);
1480+
}
1481+
}
1482+
});
1483+
}
1484+
14051485
@Test
14061486
public void testVersionByteInHeader() throws Exception {
14071487
assertMemoryLeak(() -> {

0 commit comments

Comments
 (0)