schemadiff: validate views' referenced columns

[shlomi-noach]

Description

Up till now, schemadiff views validation and analysis was limited to:

• Obviously validate statement
• Infer tables (or views) reverenced by a view
• Validate those tables/views exist
• Validate no dependency loop

As of this PR, schemadiff also validates columns used and returned by views:

• Validate that columns in SELECT and in WHERE clause do in fact exist in tables/views referenced by the view. Error when an unknown column is encountered.
• Allow unqualified table name where there is no ambiguity. Error on unqualified name with ambiguity.
• Analyze view's column names, whether aliased or not.
• Support cascaded views.
• Support star expression, either qualified (create view v as select t.* from t, t2 ...) or unqualified (create view v as select * from t1,t2) ; infer list of columns ; support cascaded views based on star expression views.

Plenty tests added to validate behavior.

Joint with @dbussink and based on his preliminary work.

Related Issue(s)

• Tracking issue: schemadiff: tracking issue #10203

Checklist

• "Backport to:" labels have been added if this change should be back-ported
• Tests were added or are not required
• Documentation was added or is not required

Deployment Notes

[shlomi-noach]

In a followup PR: optimize such that we only analyze columns tables/views that are referenced by some view in the first place; there is no need to analyze e.g. a table that is never read by a view. This will allow us to save some memory & cpu.

[dbussink]

Is this check useful? We already check earlier that there's no error returned when schema2 is created?

[shlomi-noach]

removed

[dbussink]

What does this TODO reference to?

[shlomi-noach]

hehe, it references the fact that is is already done... removing...

[shlomi-noach]

fixed

[dbussink]

Usually use errs as a the name for this kinda thing to collect errors.

[shlomi-noach]

fixed

[dbussink]

Should this add to the error list instead? And then we continue through the rest of the entities? If this is a totally unexpected error, maybe good to add a comment about that and why it's safe to return immediately?

[shlomi-noach]

fixed

[dbussink]

Let's also update to 1.9.0 here since that's the latest?

[shlomi-noach]

done

[systay]

FYI - sqlparser.Walk will only return the error you return from inside the visitor function, and since you don't return any errors from that, no errors will ever make it out.

[shlomi-noach]

@systay ah, great! Thank you. I'll keep the check as it is, for safety, but good to know!

[dbussink]

@systay Right, but the errors are gathered here in errs? And those are at the end returned if the walker itself doesn't error (which is not expected).

[systay]

@dbussink yeah, that was my point. Not really necessary to catch the returned error from sqlparser.Walk since that is not how we are dealing with the errors. When I know it will never return an error, I usually write.

	_ = sqlparser.Walk(...)

OTOH - it's probably good defensive programming to do as @shlomi-noach is doing here and catching and checking the error anyway.

[dbussink]

@systay Right, guess as someone now knowing the details too much about how Walk is implemented, I wouldn't know that it can't return an error and would still write it defensively 😄.

[shlomi-noach]

Looking for a 2nd review/approval

[mattlord]

I think we can/should avoid importing the external multierr package but it otherwise LGTM.

Not adding a formal approval as we should, IMO, get @systay or @harshit-gangal to review as they are in the query serving team and working on the overall Vitess View support work. This way we can ensure that we're moving forward together and all the pieces will fit together nicely.

@systay or @harshit-gangal , could you please review? Please note if you cannot and I can add a formal approval here if needed. Thanks!

[mattlord]

Why not use the Vitess concurrency package here along with vterrors.Aggregate? You can see that used throughout the code base if you look for .AggrError(vterrors.Aggregate). Between the concurrency and vterrors packages we should have whatever related functionality you need here for error handling. For example:

vitess/go/vt/wrangler/traffic_switcher.go

Lines 146 to 161 in 390ddad

	func (ts *trafficSwitcher) ForAllSources(f func(source *workflow.MigrationSource) error) error {
	var wg sync.WaitGroup
	allErrors := &concurrency.AllErrorRecorder{}
	for _, source := range ts.sources {
	wg.Add(1)
	go func(source *workflow.MigrationSource) {
	defer wg.Done()
	if err := f(source); err != nil {
	allErrors.RecordError(err)
	}
	}(source)
	}
	wg.Wait()
	return allErrors.AggrError(vterrors.Aggregate)
	}

[dbussink]

It looks like it does use locking which is unnecessary here. Dunno if we care about the overhead of that here? That logic seems designed for concurrent error gathering which isn't what we're doing here.

Using multierr seems simpler here and it's already an indirect dependency? Not a really strong opinion though, we can also use this but it seems a bit off for what it was designed for.

I think this logic here is temporary anyway, since once golang/go#53435 is available with Go 1.20 we probably want to switch to that anyway.

[shlomi-noach]

cc @dbussink

[shlomi-noach]

heh, race condition

[shlomi-noach]

How strongly do people feel about this? Looking at the two implementation multierr does seem to be more fitting to our purpose, but I don't feel strongly.

[mattlord]

If you only need multiple errors you can use vterrors.Aggregate(), no need for the concurrency piece (as noted, geared toward N goroutines) like we do here e.g.:

vitess/go/vt/vttablet/tabletmanager/vreplication/vcopier.go

Lines 597 to 621 in 3f55e40

	var terrs []error
	for !empty {
	select {
	case result := <-resultCh:
	switch result.state {
	case vcopierCopyTaskCancel:
	// A task cancelation probably indicates an expired context due
	// to a PlannedReparentShard or elapsed copy phase duration,
	// neither of which are error conditions.
	case vcopierCopyTaskComplete:
	// Get the latest lastpk, purely for logging purposes.
	lastpk = result.args.lastpk
	case vcopierCopyTaskFail:
	// Aggregate non-nil errors.
	terrs = append(terrs, result.err)
	}
	default:
	empty = true
	}
	}
	if len(terrs) > 0 {
	terr := vterrors.Aggregate(terrs)
	log.Warningf("task error in workflow %s: %v", vc.vr.WorkflowName, terr)
	return fmt.Errorf("task error: %v", terr)
	}

I don't feel strongly about it though. Up to you.

[shlomi-noach]

Incorporated #12189 into this PR, as it deals with view analysis

[shlomi-noach]

I've switched away from multierr and into AllErrorsRecorder. I want to say multierr seems to have the better design over AllErrorsRecorder, and even over the new Go 1.20 error wrapping support.

[shlomi-noach]

Looking for another review/approval 🙏

[shlomi-noach]

ping @vitessio/query-serving for review

[shlomi-noach]

ping https://github.com/orgs/vitessio/teams/query-serving for review

[GuptaManan100]

Everything else LGTM!

[GuptaManan100]

Is there a specific reason for this downgrade?

[shlomi-noach]

It was upgraded in a previous PR, that was eventually not using multierr. I just ran go mod tidy and it auto-downgraded, so... I don't know?

[dbussink]

@shlomi-noach might be because the direct dependency was dropped? A go get -u go.uber.org/multierr probably would add this back correctly (it should be there, so good catch @GuptaManan100).

[shlomi-noach]

This is fixed.

[shlomi-noach]

Stalling this PR a bit as I'm to look into the go/vt/vtgate/semantics implementation of view dependency management.

[shlomi-noach]

Replaced by #12565 ; will close shortly.