Alternator: evaluate the need to validate table name on each request

[nuivall]

Currently we call static void validate_table_name(const std::string& name) on each request, in theory we should only check if table name is correct when creating it and after that don't need to validate it in executor::find_table calls.

This issue was brough up in https://github.com/scylladb/scylladb/pull/12445/files/0d89b21a1582e2da58083a8eccb2c25a37637ed2#r1062322506

[nyh]

The link above doesn't work for me, so here is one that does seem to work (I don't know if it's the same thing you wanted to link to): #12445 (comment)

[nuivall]

Yes, looks like links to comments in diff view don't persist well

[nyh]

Unfortunately, it turns out that DynamoDB does check the validity the given table name in all requests, not just CreateTable. For example, consider the following test for PutItem:

def test_item_operations_improper_named_table(dynamodb):
    with pytest.raises(ClientError, match='ValidationException'):
        dynamodb.meta.client.put_item(TableName='non!existent!table',
            Item={'a':{'S':'b'}})
    with pytest.raises(ClientError, match='ValidationException'):
        dynamodb.meta.client.put_item(TableName='xx',
            Item={'a':{'S':'b'}})

This test attempts a PutItem operation with the table names non!existent!table and xx, both forbidden by the naming rules. Both DynamoDB and Alternator respond with a ValidationException - that the name is invalid, and not ResourceNotFoundException as happens when the table "just" doesn't exist.

It is arguable whether Alternator needs to be 100% compatible with the error handling of DynamoDB in esoteric error cases (this has no effect on successful requests). So if this extra check causes a significant slowdown of requests, by all means we should consider dropping the extra check. But if the slowdown caused by the extra check is tiny, maybe it's better not to break even this minor error-case compatibility with Cassandra, and leave the extra checking as is?

@nuivall do you have an estimate how much of the request time in the worst case (e.g., small read request from cache) can be attributed to this extra check?

[nyh]

In #12608 (comment) @nuivall had a good idea: we can do the table-name validation only after (and if) we realize it doesn't exist, so in the usual case where the table exists, it isn't done. If we do that, we remain error-compatible with DynamoDB, but don't lose any performance (not even a tiny bit) in the usual successful case.

Basically, we can remove the validate_table_name() call from find_table_name(), and move it into its callers - create_table_on_shard0() will call validate_table_name() after get_table_name(), and other callers use find_table() or get_table() and those should call validate_table_name() (we need to check we covered all the calls, though...).

[nuivall]

Currently validate_table_name disappears from the profile, meaning probably that it's ~100 instructions or less (so ~0% of req time). Whole alternator::get_table_or_view is 5% of alternator::executor::query (and mostly due to needed work it seems and couple less needed allocations).

This is nothing compared to calculate_bounds_condition_expression which takes 27% of alternator::executor::query time. And to my current understanding especially antlr3 part is "just" parsing dozen of characters and needs 7k instructions to do that.

[avikivity]

Let's tackle the big ones first, then move back to this.

I'm hopeful caching the query parse result will yield large improvements.

[avikivity]

btw, I see alternator grew its own expression language (primitive_condition/condition_expression).

We should consider migrating to cql3 expressions (perhaps moved out to a top-level module), so that any improvements there (say, JIT compilation) are carried over.

[nyh]

Let's tackle the big ones first, then move back to this.

Yes, this is always a good approach in optimization, but sometimes if you already know how to easily shave off something small, there is no reason not to do it too. Of course it shouldn't have high priority.

I'm hopeful caching the query parse result will yield large improvements.

Indeed. This is is #5023.
It will definitely help with "new-style" DynamoDB requests which use one or more expressions in every request, and normally most of them will reuse the same expression.
Separately it will be interesting to understand why the expression parsing is slow. It shouldn't have been - the whole idea of parser generators is (or at least used to be...) to generate optimal code. Maybe it is something silly like we had in the http parser, where somehow the way we used ANTLR (or ANTLR itself) causes us to do a lot of work per byte or something.

[nyh]

btw, I see alternator grew its own expression language (primitive_condition/condition_expression).

Right. We had this before CQL had its ;-)

We should consider migrating to cql3 expressions (perhaps moved out to a top-level module), so that any improvements there (say, JIT compilation) are carried over.

This is easier said than done. You already saw how many changes you needed to do just to change the CQL expressions to support two variants of "= NULL". The DynamoDB expressions are different in almost every respect from CQL expressions. The terminals (constants, columns and nested pieces of these columns) and their types are different, the error handling is different, the "bound variable" support is different. See alternator/expression_types.hh for a commented introduction to the parsed expression tree that Alternator uses.

Maybe it's something we can and should consider in the future, but I don't think it's urgent. One downside of Alternator's different expressions is that today we can only use these expressions (e.g., filtering) on the coordinator - we can't push these expressions to replicas, and so on. Today it's not a big problem because CQL doesn't do this either, but if one day it would - it would be good to have just one expression class, not two.

[harsh020]

Hi @nuivall @nyh @avikivity, I am a new contributor to scylladb and would love to work on this.

The summary that I gathered from all the above comments is:

1. Dynamodb validates table name for all request, so does alternator
2. To optimise this we would want to only validate table name on
   - create table request
   - other request iff the table does not exists (so as to keep compatibility with the error that we raise is same as that of Dynamodb)

So, the changes need to be:

1. validate table before creating
2. in other request intercept any ResourceNotFoundException or anything that suggests table does not exists and validate the table name. If the table name is invalid, raise ValidationError, else re-raise the catch exception.

That's an high level implementation that I understood. Please let me know if I am missing something. Meanwhile, I will dig into code for more details!

[nuivall]

@harsh020 yes, your summary seems correct. Exception is in most (all?) places called no_such_column_family.

[harsh020]

@nuivall, Thank you for the info! Please let me have a dig at the code and get back to you!