bschoening commented on code in PR #4596:
URL: https://github.com/apache/cassandra/pull/4596#discussion_r2755326401
##########
src/java/org/apache/cassandra/db/guardrails/GuardrailsConfig.java:
##########
@@ -406,6 +406,40 @@ public interface GuardrailsConfig
*/
boolean getVectorTypeEnabled();
+ /**
+ * <p>
+ * A statement is considered "mis-prepared" if it contains hardcoded
literal values
Review Comment:
I think this should be "if it contains only hardcoded liter values and
without any bind markers"
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -1886,4 +1914,12 @@ private static long
minimumTimestampAsRelativeMicros(@Nullable DurationSpec.Long
? Long.MIN_VALUE
: (ClientState.getLastTimestampMicros() -
duration.toMicroseconds());
}
+
+ public static void onMisprepared(ClientState state)
+ {
+ mispreparedStatementsEnabled.ensureEnabled(state);
+
+ // only warn if user ins't super user or a external call, these checks
are handled by guardrail framework
Review Comment:
typo on **ins't** -- but why is super user relevant to the warning?
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -54,6 +54,9 @@ public final class Guardrails implements GuardrailsMBean
public static final String MBEAN_NAME =
"org.apache.cassandra.db:type=Guardrails";
public static final GuardrailsConfigProvider CONFIG_PROVIDER =
GuardrailsConfigProvider.instance;
+
+ public static final String MISPREPARED_STATEMENT_WARN_MESSAGE =
"Performance Tip: This query contains literal values in the WHERE clause. " +
"Using '?' placeholders (bind markers) is much more efficient. " + "It allows
the server to cache this query once and reuse it for different values, " +
"reducing memory usage and improving speed.";
Review Comment:
```suggestion
public static final String MISPREPARED_STATEMENT_WARN_MESSAGE = "This
query contains only literal values in the WHERE clause. " + "Using one or more
'?' placeholder values (bind markers) allow a prepared statement to be reused.";
```
##########
test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java:
##########
@@ -0,0 +1,388 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.cql3;
+
+import java.net.InetSocketAddress;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.auth.AuthenticatedUser;
+import org.apache.cassandra.auth.CassandraAuthorizer;
+import org.apache.cassandra.auth.PasswordAuthenticator;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.db.guardrails.GuardrailViolatedException;
+import org.apache.cassandra.db.guardrails.Guardrails;
+import org.apache.cassandra.service.ClientState;
+import org.apache.cassandra.service.ClientWarn;
+
+import static org.junit.Assert.assertTrue;
+
+public class MispreparedStatementsTest extends CQLTester
+{
+ private static final ClientState state = ClientState.forExternalCalls(new
InetSocketAddress("127.0.0.1", 9042));
+
+ @BeforeClass
+ public static void setupGlobalConfig()
+ {
+ DatabaseDescriptor.setAuthenticator(new PasswordAuthenticator());
+ DatabaseDescriptor.setAuthorizer(new CassandraAuthorizer());
+ DatabaseDescriptor.setMispreparedStatementsEnabled(false);
+ }
+
+ @Before
+ public void setUp()
+ {
+ AuthenticatedUser nonSuperUser = new AuthenticatedUser("regular-user")
+ {
+ @Override
+ public boolean isSuper()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isSystem()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isAnonymous()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean canLogin()
+ {
+ return true;
+ }
+ };
+
+ state.login(nonSuperUser);
+ state.setKeyspace(KEYSPACE);
+ ClientWarn.instance.captureWarnings();
+ createTable("CREATE TABLE %s (id int, description text, age int, name
text, PRIMARY KEY (id, name, age))");
+ }
+
+ @After
+ public void tearDown()
+ {
+ DatabaseDescriptor.setMispreparedStatementsEnabled(false);
+ }
+
+ @Test
+ public void testSelectWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name =
'v1' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithWhereNonPrimaryKeyColumn()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE
description = 'foo' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+
+ @Test
+ public void testSelectWithCompositeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s WHERE
(name, age) = ('a', 1) ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithFunction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s where
name = 'v1' ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithRangeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name > 'a'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3)", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name IN
('a', 'b') ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3) AND name in ('a', 'b')", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDDLStatementsBypass()
+ {
+ assertGuardrailPassed("CREATE TABLE IF NOT EXISTS test_table (id INT
PRIMARY KEY)");
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testWhereLiteralWithLike()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name
LIKE 'prefix%%' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testInsertJsonGuardrail()
+ {
+ assertGuardrailViolated(String.format("INSERT INTO %s JSON '{\"id\":
1, \"name\": \"v1\"}'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'new' WHERE id = 1 AND name = 'name' AND age = 1", KEYSPACE + '.' +
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithIfCondition()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'v2' WHERE id = 1 AND name = 'v1' AND age = 1 IF description = 'v0'",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDeleteWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("DELETE FROM %s WHERE id = 1 AND
name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testMultiTableBatchGuardrailAllLiteral()
+ {
+ String table1 = currentTable();
+ String table2 = createTable("CREATE TABLE %s (id int PRIMARY KEY, val
text)");
+ String query = String.format("BEGIN BATCH " +
+ "UPDATE %s.%s SET description = 'v1'
WHERE id = 1 AND name = 'n1' AND age = 1; " +
+ "INSERT INTO %s.%s (id, val) VALUES (2,
'v2'); " +
+ "APPLY BATCH",
+ KEYSPACE, table1, KEYSPACE, table2);
+
+ assertGuardrailViolated(query);
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testProperlyPreparedWithBindMarkers()
+ {
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND
name = ?", currentTable()));
+ assertNoWarnings();
+ }
Review Comment:
add one for WHERE id = 5 AND name = ?
a prepared statement should be valid with one or more bind markers, not all
args have to be bind markers
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -613,6 +616,17 @@ public final class Guardrails implements GuardrailsMBean
format("The keyspace %s has a replication factor of %s,
above the %s threshold of %s.",
what, value, isWarning ? "warning" : "failure",
threshold));
+ /**
+ * Prevents heap exhaustion caused by the anti-pattern of preparing
queries with
+ * hardcoded literals instead of bind markers. This prevents filling the
statement cache with non-reusable entries.
+ */
+
+ public static final EnableFlag mispreparedStatementsEnabled =
+ new EnableFlag("misprepared_statements_enabled",
+ "misprepared statements cause server-side memory exhaustion
and high GC pressure by flooding the statement cache with non-reusable query
entries",
Review Comment:
"misprepared statements create non-reusable query entries and cause cache
overflow"
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -613,6 +616,17 @@ public final class Guardrails implements GuardrailsMBean
format("The keyspace %s has a replication factor of %s,
above the %s threshold of %s.",
what, value, isWarning ? "warning" : "failure",
threshold));
+ /**
+ * Prevents heap exhaustion caused by the anti-pattern of preparing
queries with
Review Comment:
not heap exhaustion, but cache eviction
_Prevents cache overflow and eviction caused by ..._
##########
.python-version:
##########
@@ -0,0 +1 @@
+3.11.9
Review Comment:
was this added by accident?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
