Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getting setting the PIPES_AS_CONCAT sql_mode is unsupported #2034

Closed
marcustut opened this issue Aug 10, 2022 · 6 comments
Closed

Getting setting the PIPES_AS_CONCAT sql_mode is unsupported #2034

marcustut opened this issue Aug 10, 2022 · 6 comments

Comments

@marcustut
Copy link
Contributor

marcustut commented Aug 10, 2022

I'm using a planetscale database, which uses vitess under the hood. it was working fine for a past few days, but when I try to build my code today this error occured:

setting the PIPES_AS_CONCAT sql_mode is unsupported

    sqlx::query_as!(
        ExpenseQueryResponse,
        r#"
select
    e. `id`,
    e. `name`,
    e. `amount`,
    e. `date`,
    ec. `name` as `category`,
    e. `comment`,
    group_concat(t. `name` separator ', ') as `tags`
from
    `expenses` e
    inner join `expenses_categories` ec on e. `expenses_categories_id` = ec. `id`
    left join `tags_expenses` te on e. `id` = te. `expenses_id`
    left join `tags` t on t. `id` = te. `tags_id`
group by
    e. `id`,
    e. `name`,
    e. `amount`,
    e. `date`,
    ec. `name`,
    e. `comment`
order by
    e. `date` desc,
    e. `id` desc;
        "#,
    )
    .fetch_all(db)
    .await

I searched the code and I found that when sqlx connect to the db, it tries to set certain sql_mode on the connection. It can be found in sqlx-core/src/mysql/options/connect.rs

I queried for the current sql_mode set on the db, this is the result:

marcustut/|⚠ main ⚠|> show variables like '%sql_mode%';
+---------------+-----------------------------------------------------------------------------------------------------------------------+
| Variable_name | Value                                                                                                                 |
+---------------+-----------------------------------------------------------------------------------------------------------------------+
| sql_mode      | ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION |
+---------------+-----------------------------------------------------------------------------------------------------------------------+

It can be clearly see that PIPES_AS_CONCAT is not set. So I suspect that planetscale doesn't allow setting sql_mode but sqlx is trying to set the variable. Is there a way to override this behavior?

@abonander
Copy link
Collaborator

If this is something that only just started happening, it sounds like a regression in Planetside.

@launchbadge launchbadge deleted a comment from simonsmadsen Aug 10, 2022
@marcustut
Copy link
Contributor Author

If this is something that only just started happening, it sounds like a regression in Planetside.

Hmm that's weird. I've opened a discussion over there at the moment. planetscale/discussion#248

@fooforge
Copy link

fooforge commented Aug 10, 2022

Definitely us and not a problem with sqlx. I was able to reproduce this with a specific Vitess version and we're going to look into it. This issue can be closed. :-)

See below.

@dbussink
Copy link

dbussink commented Aug 10, 2022

Definitely us and not a problem with sqlx. I was able to reproduce this with a specific Vitess version and we're going to look into it. This issue can be closed. :-)

More details here from the PlanetScale side. The issue here is more nuanced than "this is a regression" because this breaking is something we actually consider a bug fix. The change was made specifically here:

https://github.com/vitessio/vitess/pull/10163/files#diff-9d2575386e98a47276e4d5c75e73cdefec7078a95ea4e159b50806a8c3099c1bR101

Vitess has the notion of certain SQL mode flags that are unsupported to be changed by the end user. PIPES_AS_CONCAT is included in that set of flags, but before that fix we didn't properly validate and error when clients end up setting unsupported flags.

Before this fix, what this can result in, is wrong results being returned. Vitess itself also parses the SQL queries and for example in the case of sharded setups has to aggregate and combine results from different MySQL instances. This means that we also have an SQL evaluation engine in Vitess.

That parser and evaluation engine only deal with the standard MySQL dialect. Options like PIPES_AS_CONCAT or ANSI_QUOTES change the actual parsing of SQL queries which is not something Vitess supports. This means that before when we didn't explicitly error on this, you would get silently potentially wrong results returned whenever an || was used in a query (depending on details like sharding and other factors of where this query was evaluated, in Vitess, MySQL or in both).

In order the avoid this silent broken behavior, what we needed to do was explicitly blocking these changes since we don't have any current plans to support modes like ANSI_QUOTES or PIPES_AS_CONCAT.

Is it possible that sqlx is willing to consider not setting / needing PIPES_AS_CONCAT? And instead for example use the CONCAT() function when two strings need to be concatenated?

@abonander
Copy link
Collaborator

We chose to enable PIPES_AS_CONCAT by default as an ergonomic choice, and to get MySQL to behave at least somewhat more closely to SQLite and PostgreSQL. While || is generally understood to be the boolean OR operator, in the SQL standard it represents string concatenation, which makes this an annoying deviation from the standard from our perspective.

Similarly, setting PIPES_AS_CONCAT by default gives more consistent behavior when using the runtime-polymorphic Any driver to execute queries for applications that want to be database-agnostic.

I'd support a toggle in MySqlConnectOptions to turn it on or off, but changing the default would be a breaking behavior change for SQLx.

@marcustut
Copy link
Contributor Author

We chose to enable PIPES_AS_CONCAT by default as an ergonomic choice, and to get MySQL to behave at least somewhat more closely to SQLite and PostgreSQL. While || is generally understood to be the boolean OR operator, in the SQL standard it represents string concatenation, which makes this an annoying deviation from the standard from our perspective.

Similarly, setting PIPES_AS_CONCAT by default gives more consistent behavior when using the runtime-polymorphic Any driver to execute queries for applications that want to be database-agnostic.

I'd support a toggle in MySqlConnectOptions to turn it on or off, but changing the default would be a breaking behavior change for SQLx.

Sounds great! I do agree on not disabling PIPES_AS_CONCAT as default since there might be application out there using it with sqlx at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants