Vschema DDL: add column, drop column, show columns etc.

[inexplicable]

ALTER VSCHEMA ON {table} ADD COLUMN {col} {type}
ALTER VSCHEMA ON {table} DROP COLUMN {col}
ALTER VSCHEMA ON {table} SET authoritative={TRUE|FALSE}
ALTER VSCHEMA ON {table} SET column_list_authoritative={TRUE|FALSE}
SHOW VSCHEMA COLUMNS ON {table}

Fixes #4641

[inexplicable]

per dweitzman
Nit: columns in a vindex is not necessarily authoritative. Depends whether the table is marked as having authoritative columns. Maybe something like "AddVschemaColStr" would be a more accurate name than "AddAuthColumnStr"

[sougou]

One question: why do we have two ways of specifying that column list is authoritative? Currently, in the vschema there is only one. We should match only one construct to that.

I vaguely remember a discussion with @demmer. And I think we're reserving the more generic is_authoritative for using when we fetch the schema from vttablet. We should wait till we implement that feature before introducing the new construct.

[inexplicable]

per dweitzman
It seems like aligning the name here with the name in the vschema itself ("column_list_authoritative") would keep things more consistent.

[demmer]

I agree and don’t think we should support two types for the same setting.

Being verbose with column_list_authoritative is clearest.

[inexplicable]

per dweitzman
For completeness it would probably make sense to have add column first and add column after , similar to table alters.

i don't see a usage of column ordering in vschema columns, and the ROI might be low since the long term goal is to let vtgate figure out the right schema itself.

[demmer]

Since the only element we care about from column_definition is the type, I’d lean towards just parsing that and not the rest of the syntax.

[inexplicable]

per dweitzman
Looks like today vitess doesn't need to know how to parse column information from alter ddls.
A question to consider: when adding support for parsing some detail from a vschema alter statement, is it worthwhile to try to align the AST with the direction it would go in if trying to support parsing mysql schema alter statements?
Here's the alter AST in the mysql 8 code, for reference: https://github.com/mysql/mysql-server/blob/8e797a5d6eb3a87f16498edcb7261a75897babae/sql/sql_alter.h#L168.

not seeing a column order use case in vschema columns, and since the longer term solution is to let vtgate figure out the correct schema cross shards themselves, i'm not going to implement that in this PR.

[inexplicable]

@sougou i started off using authoritative, then @dweitzman suggested it to be aligned with the struct field naming column_list_authoritative, i guess i found that too wordy, and wanna keep a shorthand value. if we're going to land on is_authoritative, that should be fine, will just wait for this to be confirmed. and i'll make it a single option.

[sougou]

In that case, we should definitely not overload is_authoritative because the name is reserved for future use.

[demmer]

Overall this looks good to me pending the minor naming feedback above

[demmer]

Minor nit — this isn’t a column name so something like setting would be clearer than colName

[dweitzman]

Would also be good to have a test here with invalid params, like a non-boolean value and an unrecognized parameter name.

[inexplicable]

👍

[inexplicable]

done

[dweitzman]

This is not a comment on your PR, but something I'm noticing about sqltypes.Result in general while looking at this line:

It seems worthwhile to clarify at some point in sqltypes.Result's comments exactly when RowsAffected should and shouldn't be set. There seems to be an inconsistency in the codebase today about whether it's set for non-mutating queries like select and show.

My sense is that RowsAffected probably should not be set for show, select, explain, etc. No rows were "affected", and the row count is already available by looking at the rows themselves. It'd be kind of like having struct{rows []string; numRows int}, which is redundant since a slice already knows its own length.

In the vitess mysql protocol implementation the RowsAffected value is sometimes discarded in StreamExecute mode. I haven't looked at the mysql protocol yet to see what it returns in OK packets for select and show, though.

[sougou]

The current behavior mimics the behavior of the mysql clients, which has two modes: use_results and store_results. One is streaming and the other is all-at-once. The all-at-once case does set RowsAffected even for selects, but the all-at-once case doesn't. Since this is a client side behavior (which doesn't parse queries), it's likely that it doesn't differentiate between select, show, etc.

Right now, I think this is relevant only for grpc. There's probably no way to send this info when returning query results.

[dweitzman]

This helper is modeled after some existing helpers, but these similar-looking helpers look like they're multiplying. Could be cleaner to replace all the similar-ish wait logic with a single, more generic wait helper (not necessarily in this PR, though). Maybe something like this:

vschemaWaiter := makeVschemaWaiter(t, executor.serv, "aa")
_, _ := executor.Execute(... some vschema change ...)
vschema := vschemaWaiter()
// validate that the vschema looks as it should
// run another execute that changes the vschema
vschema = vschemaWaiter()
// validate again
// etc.

Where makeVschemaWaiter() might look something like:

// makeVschemaWaiter returns a function that gets the current vschema.
func makeVschemaWaiter(t testing.T, s svtopo.Server, cell string) func() *vschemapb.SrvVSchema {
    t.Helper()
	c := make(chan *vschemapb.SrvVSchema, 20)
    sentinel := &vschemapb.SrvVSchema{}
	s.WatchSrvVSchema(context.Background(), cell, func(v *vschemapb.SrvVSchema, err error) {
		c <- v
	})
    return func() *vschemapb.SrvVSchema {
       c <- sentinel
       var lastFound *vschemapb.SrvVSchema
       for {
          select {
            case found := <-c:
                if found == sentinel {
                  return lastFound
                }
                lastFound = found
            case <-time.After(100 * time.Millisecond):
                t.Error("Timeout waiting for vschema update")
                return nil
            }
       }
    }
}

[inexplicable]

make sense to me

[inexplicable]

hmm, 2nd thought, as the vschema ddl stuff is to be replaced, i don't want to spend too much renovation on the test case.

[dweitzman]

I would prefer if this were squashed into a single commit or commit(s) with clear descriptions. For example, something in the spirit of:

vtgate executor: support vschema DDLs to add/remove/show columns and set is_column_authoritative

New syntax:
  ALTER VSCHEMA ON {table} ADD COLUMN {col} {type}
  ALTER VSCHEMA ON {table} DROP COLUMN {col}
  ALTER VSCHEMA ON {table} SET column_list_authoritative={TRUE|FALSE}
  SHOW VSCHEMA COLUMNS ON {table}

Fixes #4641

[dweitzman]

Would probably be good to check err == nil || err.Error() != "the error string" here and in the next case to double-check the error text returned

[inexplicable]

👍

[inexplicable]

done & squashed into one commit

[sougou]

Is this ready? If so, @dweitzman: can you do a final once-over and merge?

[inexplicable]

Is this ready? If so, @dweitzman: can you do a final once-over and merge?

yes, i believe the comments were all addressed (or reject 🤣 )

[sougou]

@dweitzman: waiting on your final approval.

[dkhenry]

May I request we update the title of this PR before its merged. Right now its not very descriptive

[sougou]

@inexplicable conflicts need merging, and @dkhenry requests a better title.

[morgo]

@inexplicable followup ping :-)