Do not skip metadata when quering `SELECT * FROM ...` #275

sylwiaszunejko · 2024-09-25T13:53:09Z

This PR makes skipping metadata optimization disabled for SELECT * FROM ... queries due to potential correctness issues when altering the table. This may cause a decrease in performance in comparison to always skipping metadata. To avoid that it is recommended not to use SELECT * FROM ... and provide specific columns names.

Fixes: #261

session.go

avikivity · 2024-09-25T16:42:25Z

I think this logic belongs in the server. All drivers are susceptible to it.

avikivity · 2024-09-25T16:44:24Z

I think this logic belongs in the server. All drivers are susceptible to it.

Although, we have to check:

whether the protocol allows returning metadata even though the driver requested not to
whether drivers look at metadata even if they asked to skip metadata, but the server returned it regardless

sylwiaszunejko · 2024-09-26T06:50:08Z

I think this logic belongs in the server. All drivers are susceptible to it.

True, @dkropachev was supposed to create a core issue for that. This PR is meant as a temporary solution to help a customer with correctness problems when they run this type of queries as solving it in the core would probably take some time. AFAIK there is no way right now to invalidate prepared statements when schema changes. In cql v5 that is solved and we could have something analogical, am I right @Lorak-mmk ?

This commit makes skipping metadata optimization disabled for `SELECT * FROM ...` queries due to potential correctness issues when altering the table. This may cause a decrease in performance in comparison to always skipping metadata. To avoid that it is recommended not to use `SELECT * FROM ...` and provide specific columns names.

sylwiaszunejko · 2024-09-26T07:25:46Z

I think this logic belongs in the server. All drivers are susceptible to it.

Although, we have to check:
* whether the protocol allows returning metadata even though the driver requested not to

* whether drivers look at metadata even if they asked to skip metadata, but the server returned it regardless

If we set disableSkipMetadata for a specific query to true as in this PR, the driver will request the server to return the metadata even if in the general cluster settings we have skipping metadata enabled https://github.com/scylladb/gocql/blob/master/conn.go#L1440

wprzytula · 2024-09-26T08:31:18Z

I think this logic belongs in the server. All drivers are susceptible to it.

Although, we have to check:
* whether the protocol allows returning metadata even though the driver requested not to

Having read the protocol spec, I'm undecided.

on one hand, the flag turning on the optimisation is described as:
0x02: Skip_metadata. If set, the Result Set returned as a response the query (if any) will have the NO_METADATA flag (see 4.2.5.2)., which suggests implication skip_metadata => no_metadata (but I interpret this implication as logical protocol requirement, not protocol format requirement).
on the other hand, drivers deserialize result frames wrt metadata based on no_metadata flag's value
0x0004 No_metadata: if set, the <metadata> is only composed of [...]. This will only ever be the case if this was requested during the query (see QUERY and RESULT messages).

So I lean towards interpretation that the protocol does allow returning metadata even when skip_metadata was requested.

* whether drivers look at metadata even if they asked to skip metadata, but the server returned it regardless

Rust driver's case is that if cached metadata is present, it is used unconditionally. However, it would be a trivial change to compare cached metadata with the recent result metadata and invalidate the prepared statement in case of a mismatch; and also, to use result metadata (if present) instead of cached metadata.

avikivity · 2024-09-26T08:51:53Z

I agree with your analysis. However, the Rust case proves that a server-side solution is insufficient and we must audit and possibly fix all the drivers anyway. So the question isn't whether to apply a server-side or client-side solution, it's whether to apply a (more accurate) server-side solution and audit/fix all the drivers, or just patch all the drivers.

I think it's worthwhile (and easy) to do it in the server, so why not, even though it doesn't save us work on the drivers.

mykaul · 2024-09-26T08:52:39Z

@wprzytula - sounds like the classic case of "Be liberal in what you accept, and conservative in what you send.", no? You can ask for no metadata, but be prepared to process it if your request is ignored. Ignoring it seems like a bug on the server side though.

avikivity · 2024-09-26T08:54:35Z

Here, I'm proposing for the server to be less conservative: even though the driver requested to skip metadata, I'm sending it anyway. I'm relying on the driver to be liberal and accept it anyway. We already know the Rust driver will accept it but throw it away.

Lorak-mmk · 2024-09-26T09:11:45Z

I agree with your analysis. However, the Rust case proves that a server-side solution is insufficient and we must audit and possibly fix all the drivers anyway. So the question isn't whether to apply a server-side or client-side solution, it's whether to apply a (more accurate) server-side solution and audit/fix all the drivers, or just patch all the drivers.

I think it's worthwhile (and easy) to do it in the server, so why not, even though it doesn't save us work on the drivers.

If we do it on the server I think it would be good to use the solution from cql v5. In cql v5 when preparing a statement there are 2 IDs returned:

statement id, the same as it is now
result metadata id

During EXECUTE driver sends both IDs. If the server detects that result metadata id is incorrect (because schema changed) it sends new result metadata id (+ of course new result metadata) in the RESULT (so no additional round trip required). Driver can then handle the response correctly, and update its local data to reflect new result metadata.

It solves the issue with SELECT * (and similar problems with UDT), without sacrificing performance (because there is no need to send metadata on each request, just once after schema changed).

wprzytula · 2024-09-26T09:12:36Z

A side note: Rust driver's PreparedStatement holds cached metadata as immutable. This means that once a patched server sends new metadata after schema changes, the driver can use that new metadata but not update the cached version. Hence, we will have to either:

allow updates there,
return an error if metadata changed, requesting manual repreparation by the user (i.e. creating a new PreparedStatement by issuing Session::prepare()).

An argument for the second solution is that if a schema change affects the table queried by the particular PreparedStatement, then chances are the user's struct reflecting the table no longer fits, so a type check error would be thrown later anyway.
OTOH, in cases that a schema change does not affect the keyspace/table the statement operates on at all, then it would be a needless overhead to reprepare such statement manually (as does not truly require any user action to fix it).

Can we somehow differentiate between schema changes that affect a particular statement and those that don't? Preferably on server side, so that new metadata is only sent when the statement is affected.

Lorak-mmk · 2024-09-26T09:14:48Z

A side note: Rust driver's PreparedStatement holds cached metadata as immutable. This means that once a patched server sends new metadata after schema changes, the driver can use that new metadata but not update the cached version. Hence, we will have to either:
* allow updates there,

* return an error if metadata changed, requesting manual repreparation by the user (i.e. creating a new PreparedStatement by issuing Session::prepare()).
An argument for the second solution is that if a schema change affects the table queried by the particular PreparedStatement, then chances are the user's struct reflecting the table no longer fits, so a type check error would be thrown later anyway. OTOH, in cases that a schema change does not affect the keyspace/table the statement operates on at all, then it would be a needless overhead to reprepare such statement manually (as does not truly require any user action to fix it).

Can we somehow differentiate between schema changes that affect a particular statement and those that don't? Preferably on server side, so that new metadata is only sent when the statement is affected.

Solution from CQL v5 that I mentioned in the comment above does that.

mykaul · 2024-09-26T09:17:46Z

Ref scylladb/scylladb#17788

sylwiaszunejko · 2024-09-27T07:53:17Z

@dkropachev @avikivity @Lorak-mmk I think we agree that the changes to the server side are needed, but what do you recommend to do with this PR, is it okay to have it as a temporary fix or should we wait until it is fixed on server side?

dkropachev · 2024-09-27T10:34:17Z

@dkropachev @avikivity @Lorak-mmk I think we agree that the changes to the server side are needed, but what do you recommend to do with this PR, is it okay to have it as a temporary fix or should we wait until it is fixed on server side?

@sylwiaszunejko , I think we need to merge it in, let me quickly test other cases, i will come back with approval in couple of hours.

dkropachev · 2024-09-27T14:11:53Z

@sylwiaszunejko , I see that exactly same problem we have with UDTs, let's brain storm if there is a way to cover all the cases.

sylwiaszunejko · 2024-09-27T14:44:34Z

@sylwiaszunejko , I see that exactly same problem we have with UDTs, let's brain storm if there is a way to cover all the cases.

Could you provide the reproducer for UDTs?

dkropachev · 2024-09-27T14:51:01Z

@sylwiaszunejko , I see that exactly same problem we have with UDTs, let's brain storm if there is a way to cover all the cases.

Could you provide the reproducer for UDTs?

package gocql

import (
	"github.com/google/go-cmp/cmp"
	"sort"
	"strings"
	"testing"
	"time"
)

func TestPreparedStatementInvalidation(t *testing.T) {
	t.Run("select * from table", func(t *testing.T) {
		session := createSession(t)
		defer session.Close()
		if err := createTable(session, `CREATE TABLE gocql_test.prepared_stmt_test (pkey text,value text, PRIMARY KEY (pkey, value));`); err != nil {
			t.Fatal("create table:", err)
		}

		err := session.Query(`INSERT INTO gocql_test.prepared_stmt_test (pkey, value) values (?,?)`,
			"key", "value").Exec()
		if err != nil {
			t.Fatal("failed to insert record:", err)
		}

		ret := map[string]interface{}{}
		query := session.Query(`SELECT * FROM gocql_test.prepared_stmt_test where pkey = ?`, "key")
		err = query.MapScan(ret)
		if err != nil {
			t.Fatal("session 1 query failed:", err)
		}

		otherSession := createSession(t)
		otherSessionQuery := otherSession.Query(`SELECT * FROM gocql_test.prepared_stmt_test where pkey = ?`, "key")

		otherSessionRet := map[string]interface{}{}
		// Obtain prepared statement id
		err = otherSessionQuery.MapScan(otherSessionRet)
		if err != nil {
			t.Fatal("session 2 query failed:", err)
		}

		if diff := cmp.Diff(ret, otherSessionRet); diff != "" {
			t.Fatalf("expected the results to be different: %s", diff)
		}

		err = otherSession.Query(`ALTER TABLE gocql_test.prepared_stmt_test ADD value2 text`).Exec()
		if err != nil {
			t.Fatalf("failed to alter table: %v", err)
		}

		time.Sleep(100 * time.Millisecond)

		// Reset query prepared statement cache on the server after it is being invalidated
		iter := otherSessionQuery.Iter()
		rows, err := iter.RowData()
		if err != nil {
			t.Fatal("session 2 query failed:", err)
		}
		if len(rows.Columns) != 3 {
			t.Fatalf("expected the results to have 3 columns, but it has %d: %v", len(rows.Columns), rows.Columns)
		}

		iter = query.Iter()
		rows, err = iter.RowData()
		if err != nil {
			t.Fatal("session 1 query failed:", err)
		}
		if len(rows.Columns) != 3 {
			t.Errorf("session 1 expected results to have 3 columns, but it has %d: %v", len(rows.Columns), rows.Columns)
		}
	})

	t.Run("udt", func(t *testing.T) {
		session := createSession(t)
		defer session.Close()

		err := session.Query(`CREATE TYPE gocql_test.phone (country_code int, number text);`).Exec()
		if err != nil {
			t.Fatal("failed to create udt type:", err)
		}

		if err := createTable(session, `CREATE TABLE gocql_test.subscription_by_user (pkey text, value frozen<phone>, PRIMARY KEY (pkey));`); err != nil {
			t.Fatal("create table:", err)
		}

		udtvalue := map[string]interface{}{
			"country_code": 10,
			"number":       "value",
		}

		err = session.Query(`INSERT INTO gocql_test.subscription_by_user (pkey, value) values (?,?)`,
			"testtext", udtvalue).Exec()
		if err != nil {
			t.Fatal("insert:", err)
		}

		originalResult := map[string]interface{}{}
		query := session.Query(`SELECT value FROM gocql_test.subscription_by_user where pkey = ?`, "testtext")
		err = query.MapScan(originalResult)
		if err != nil {
			t.Fatal("session 1 query failed:", err)
		}

		otherSession := createSession(t)
		otherSessionQuery := otherSession.Query(`SELECT value FROM gocql_test.subscription_by_user where pkey = ?`, "testtext")

		otherSessionResult := map[string]interface{}{}
		// Obtain prepared statement id
		err = otherSessionQuery.MapScan(otherSessionResult)
		if err != nil {
			t.Fatal("session 2 query failed:", err)
		}

		if diff := cmp.Diff(originalResult, otherSessionResult); diff != "" {
			t.Fatalf("expected the results to be different: %s", diff)
		}

		err = session.Query(`ALTER TYPE gocql_test.phone ADD number2 text;`).Exec()
		if err != nil {
			t.Fatal("failed to create udt type:", err)
		}

		iter := otherSessionQuery.Iter()
		otherSessionColumns := iter.Columns()
		err = iter.Close()
		if err != nil {
			t.Fatal("session 2 query failed:", err)
		}
		if len(otherSessionColumns[0].TypeInfo.(UDTTypeInfo).Elements) != 3 {
			t.Fatalf("session 2 has old metadata")
		}

		iter = query.Iter()
		originalSessionColumns := iter.Columns()
		err = iter.Close()
		if err != nil {
			t.Fatal("session 1 query failed:", err)
		}
		if len(originalSessionColumns[0].TypeInfo.(UDTTypeInfo).Elements) != 3 {
			t.Fatalf("session 1 has old metadata")
		}
	})
}

func getKeys(val map[string]interface{}) string {
	keys := make([]string, 0, len(val))
	for k := range val {
		keys = append(keys, k)
	}
	sort.Strings(keys)
	return strings.Join(keys, ",")
}

UPDATED

dkropachev · 2024-09-27T15:06:05Z

@sylwiaszunejko ,
if any column type in the select has udt in it, it is also affected by this issue.
and now, once it is in the picture, this solution does not look so good, because driver is going to be throwing this warnings on all sort of queries, and also slow down effect is going to be much wider.
And documenting this behavior as good enogh solution, does not look so grim now)

sylwiaszunejko · 2024-09-27T15:09:41Z

@sylwiaszunejko ,
if any column type in the select has udt in it, it is also affected by this issue.
and now, once it is in the picture, this solution does not look so good, because driver is going to be throwing this warnings on all sort of queries, and also slow down effect is going to be much wider.

Maybe we should reconsider going with the rust driver approach and disabling skipping metadata by default if there are so many cases when we are at risk of incorrect results

dkropachev · 2024-09-27T15:15:58Z

@sylwiaszunejko ,
if any column type in the select has udt in it, it is also affected by this issue.
and now, once it is in the picture, this solution does not look so good, because driver is going to be throwing this warnings on all sort of queries, and also slow down effect is going to be much wider.

Maybe we should reconsider going with the rust driver approach and disabling skipping metadata by default if there are so many cases when we are at risk of incorrect results

And create a core issue to address it on server side, and when it is merged we could return default to true again, or have some feature flag to let driver know it is safe to skip metadata now.

dkropachev · 2024-09-27T15:35:56Z

core issue

avikivity · 2024-09-29T15:31:02Z

@dkropachev @avikivity @Lorak-mmk I think we agree that the changes to the server side are needed, but what do you recommend to do with this PR, is it okay to have it as a temporary fix or should we wait until it is fixed on server side?

We shouldn't block this series, server side changes are much slower to propagate.

avikivity · 2024-09-29T15:33:45Z

@sylwiaszunejko , if any column type in the select has udt in it, it is also affected by this issue. and now, once it is in the picture, this solution does not look so good, because driver is going to be throwing this warnings on all sort of queries, and also slow down effect is going to be much wider. And documenting this behavior as good enogh solution, does not look so grim now)

Are UDTs really affected? Yes, if a UDT has types added, the server will serialize a larger tuple, but the driver can/should just ignore the new fields (are they always added at the end? must be). It can't ignore new fields in SELECT * because fields aren't ordered wrt addition time.

dkropachev · 2024-09-30T12:03:10Z

@sylwiaszunejko , @avikivity and another case with SELECT col FROM table ALTER TABLE table RENAME col TO old_col ALTER TABLE table ADD col <new_type>

sylwiaszunejko · 2024-09-30T12:11:09Z

As there are many other cases when the behavior is broken I am closing this PR, I will open another one with changing default behavior to disable skipping metadata until this is fixed on the server side

sylwiaszunejko requested review from dkropachev and Lorak-mmk September 25, 2024 13:53

sylwiaszunejko self-assigned this Sep 25, 2024

mykaul reviewed Sep 25, 2024

View reviewed changes

session.go Show resolved Hide resolved

dkropachev requested changes Sep 25, 2024

View reviewed changes

session.go Outdated Show resolved Hide resolved

session.go Show resolved Hide resolved

sylwiaszunejko force-pushed the wildcard_select branch from bf55aaa to c8f8c58 Compare September 26, 2024 06:58

sylwiaszunejko requested a review from dkropachev September 26, 2024 06:58

sylwiaszunejko closed this Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not skip metadata when quering `SELECT * FROM ...` #275

Do not skip metadata when quering `SELECT * FROM ...` #275

sylwiaszunejko commented Sep 25, 2024

avikivity commented Sep 25, 2024

avikivity commented Sep 25, 2024

sylwiaszunejko commented Sep 26, 2024

sylwiaszunejko commented Sep 26, 2024

wprzytula commented Sep 26, 2024

avikivity commented Sep 26, 2024

mykaul commented Sep 26, 2024

avikivity commented Sep 26, 2024

Lorak-mmk commented Sep 26, 2024

wprzytula commented Sep 26, 2024

Lorak-mmk commented Sep 26, 2024

mykaul commented Sep 26, 2024

sylwiaszunejko commented Sep 27, 2024

dkropachev commented Sep 27, 2024

dkropachev commented Sep 27, 2024

sylwiaszunejko commented Sep 27, 2024

dkropachev commented Sep 27, 2024 •

edited

Loading

dkropachev commented Sep 27, 2024 •

edited

Loading

sylwiaszunejko commented Sep 27, 2024

dkropachev commented Sep 27, 2024 •

edited

Loading

dkropachev commented Sep 27, 2024

avikivity commented Sep 29, 2024

avikivity commented Sep 29, 2024

dkropachev commented Sep 30, 2024

sylwiaszunejko commented Sep 30, 2024

Do not skip metadata when quering SELECT * FROM ... #275

Do not skip metadata when quering SELECT * FROM ... #275

Conversation

sylwiaszunejko commented Sep 25, 2024

avikivity commented Sep 25, 2024

avikivity commented Sep 25, 2024

sylwiaszunejko commented Sep 26, 2024

sylwiaszunejko commented Sep 26, 2024

wprzytula commented Sep 26, 2024

avikivity commented Sep 26, 2024

mykaul commented Sep 26, 2024

avikivity commented Sep 26, 2024

Lorak-mmk commented Sep 26, 2024

wprzytula commented Sep 26, 2024

Lorak-mmk commented Sep 26, 2024

mykaul commented Sep 26, 2024

sylwiaszunejko commented Sep 27, 2024

dkropachev commented Sep 27, 2024

dkropachev commented Sep 27, 2024

sylwiaszunejko commented Sep 27, 2024

dkropachev commented Sep 27, 2024 • edited Loading

UPDATED

dkropachev commented Sep 27, 2024 • edited Loading

sylwiaszunejko commented Sep 27, 2024

dkropachev commented Sep 27, 2024 • edited Loading

dkropachev commented Sep 27, 2024

avikivity commented Sep 29, 2024

avikivity commented Sep 29, 2024

dkropachev commented Sep 30, 2024

sylwiaszunejko commented Sep 30, 2024

Do not skip metadata when quering `SELECT * FROM ...` #275

Do not skip metadata when quering `SELECT * FROM ...` #275

dkropachev commented Sep 27, 2024 •

edited

Loading

dkropachev commented Sep 27, 2024 •

edited

Loading

dkropachev commented Sep 27, 2024 •

edited

Loading