Adding a column to the base table should invalidate prepared statements for views

[nyh]

In https://github.com/scylladb/scylla-enterprise/issues/3493#issuecomment-1850936534, @tgrabiec noticed that:

"I've noticed that adding a column to the base table does not invalidate prepared statements for the views. So select * query doesn't see the new column. Only after scylla restarts queries start to see it. So it can happen that depending on which host you contact, you may see the new column or not."

[mykaul]

This sounds quite severe, no? I think it should be prioritized. @nyh - any rough estimates on the complexity of a fix for this?

[tzach]

@nyh I understand the issue is visable when creating a MV with "SELECT *" and using "SELECT *" from MV.
Is it the case?

This remind me of https://stackoverflow.com/questions/3639861/why-is-select-considered-harmful

[mykaul]

@nyh - ping - do we have any thoughts on this? Just an estimate would be great - assigning it to you (temporarily) for consideration.

[nyh]

This sounds quite severe, no? I think it should be prioritized. @nyh - any rough estimates on the complexity of a fix for this?

As I said I just quoted @tgrabiec's statement about this being a bug - I didn't reproduce it myself and I don't know, if this bug exists, how severe it is. I'm not really an expert in prepared statement invalidation and why was important to have it for non-views - I assume the same reasons why we need prepared-statement-invalidation would apply to views (@mykaul maybe you know who the expert on this might be).

[mykaul]

@eliransin - is the above in your team? Who can look at this?

[eliransin]

First step IMO is to reproduce the issue (if even exists):
IIUC this should reproduce it quite easily:

1. Start a 3 node cluster
2. Create a materialized view that "select *" the rows
3. Create a prepared statement on the view that "select *" the row
4. Write a row
5. Issue the prepared statement from 3 and validate that we get 1 row and that it is correct
6. Add a column to the base table
7. Write another row with the new column
8. Issue the prepared statement from 3 and validate that we get 2 row and that they have the new value set where it should.

Will make an attempt on this.

[eliransin]

Reproduced with cql-pytest:

def test_mv_prepared_statement_with_altered_base(scylla_only, cql, test_keyspace):
    with new_test_table(cql, test_keyspace, 'id int PRIMARY KEY, v1 int') as base:
        with new_materialized_view(cql, table=base, select='*', pk='id', where='id IS NOT NULL') as view:
            base_query = cql.prepare(f"SELECT * FROM {base} WHERE id=?")
            view_query = cql.prepare(f"SELECT * FROM {view} WHERE id=?")
            cql.execute(f"INSERT INTO {base} (id,v1) VALUES (0,0)")
            assert cql.execute(base_query,[0]) == cql.execute(view_query,[0])
            cql.execute(f"ALTER TABLE {base} ADD (v2 int)")
            cql.execute(f"INSERT INTO {base} (id,v1,v2) VALUES (1,1,1)")
            assert list(cql.execute(base_query,[1])) == list(cql.execute(view_query,[1]))

This fails with:

>               assert list(cql.execute(base_query,[1])) == list(cql.execute(view_query,[1]))
E               assert [Row(id=1, v1=1, v2=1)] == [Row(id=1, v1=1)]
E                 At index 0 diff: Row(id=1, v1=1, v2=1) != Row(id=1, v1=1)

Working on a fix

[eliransin]

Fix submitted 🙂

[eliransin]

@nyh had very good questions about why didn't this happen automatically and I think he is right. This fix in #16830 will work. However, it is not the correct point for fixing this so I will try to figure it out using the correct point this time.
@mykaul you asked for an estimation, I think it will take a day or two to debug and lock in on the actual fix.

[eliransin]

This bug exists since very long ago, so all live versions needs a backport 🙂 (I checked 5.1 and above).
It looks like the fix itself is going to merge cleanly since this code looks untouched on older versions.
The test is not guarantied to merge cleanly since there was a lot of development around test.py during the past months.

[mykaul]

@scylladb/scylla-maint please backport

[nyh]

@scylladb/scylla-maint please backport

I'll do it now.

[nyh]

Backported to next-5.4 (df18433) - both the code and test backported cleanly (but this took ages to do).
Backported to next-5.2 (351d6d6) - both the code and test backported with only minor conflicts needed to be fixed in the test.