sql: delete temporary tables when session exits gracefully #42742
Conversation
|
This change is |
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
2ae9685
to
f676fc1
Compare
There was a problem hiding this comment.
Left some thoughts. The conn_executor-level stuff is fairly unfamiliar to me so we might want to pull someone else in to take a look at that.
Reviewed 95 of 95 files at r1, 5 of 23 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @solongordon)
pkg/jobs/jobspb/jobs.proto, line 307 at r2 (raw file):
CREATE_STATS = 6 [(gogoproto.enumvalue_customname) = "TypeCreateStats"]; AUTO_CREATE_STATS = 7 [(gogoproto.enumvalue_customname) = "TypeAutoCreateStats"]; DELETE_TEMP_TABLES = 8 [(gogoproto.enumvalue_customname) = "TypeDeleteTempTables"];
Unrelated (as you already mentioned)
pkg/sql/conn_executor.go, line 747 at r2 (raw file):
return err } return nil
Nit: Just return cleanupSessionTempObjects(ctx, &ex.planner, ex.sessionID)
pkg/sql/conn_executor.go, line 1202 at r2 (raw file):
ex.sessionID = ex.generateID() sessionID := ex.sessionID
Why reassign this?
pkg/sql/temporary_schema.go, line 50 at r2 (raw file):
} func TemporarySchemaName(sessionID ClusterWideID) string {
Does this need to be exported?
pkg/sql/temporary_schema.go, line 56 at r2 (raw file):
func cleanupSessionTempObjects(ctx context.Context, p *planner, sessionID ClusterWideID) error { tempSchemaName := TemporarySchemaName(sessionID) if p.sessionDataMutator != nil && p.sessionDataMutator.data.SearchPath.GetTemporarySchemaName() != tempSchemaName {
This is a roundabout way of checking if any temp schemas were created, right? Maybe nicer to make an explicit method on SearchPath for this.
pkg/sql/temporary_schema.go, line 63 at r2 (raw file):
return err } for _, id := range dbIDs {
Did you take a look at the DROP DATABASE implementation to see if any code can be shared with that?
pkg/sql/temporary_schema.go, line 74 at r2 (raw file):
return err } _, err = p.dropTableImpl(ctx, tbDesc)
Does this also work for views and sequences?
pkg/sql/sqlbase/structured.proto, line 508 at r2 (raw file):
(gogoproto.customname) = "ParentID", (gogoproto.casttype) = "ID"]; // ID of the parentSchema. Before this field being introduced goes hand in hand // with CRDB supporting additional physical schemas (other than public). If
This sentence doesn't make sense to me.
pkg/sql/sqlbase/structured.proto, line 512 at r2 (raw file):
// table was scoped under the public physical schema, so this field has the // implied value of 29 (when unset). optional uint32 parent__schema_id = 40 [(gogoproto.nullable) = false,
Extra underscore?
pkg/sql/sqlbase/structured.proto, line 692 at r2 (raw file):
(gogoproto.customname) = "ParentID", (gogoproto.casttype) = "ID"]; // The schemaID of the schema the table belongs to before the rename/drop. // Required to reclaim the old table name back.
Not clear to me what the last sentence means.
|
You may want to ask Andrei for the conn executor change.
Also let me know if you want me to give another look too.
--
Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.
|
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
f676fc1
to
052ae7b
Compare
There was a problem hiding this comment.
TFTR. I talked to Andrei offline last week and discussed/showed him my approach around the conn_executor stuff. I'll add him as a reviewer here.
@knz feel free to have a look over this stuff! :)
Reviewable status:
pkg/jobs/jobspb/jobs.proto, line 307 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Unrelated (as you already mentioned)
Done.
pkg/sql/conn_executor.go, line 1202 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Why reassign this?
Leftover from some debugging around this area. Removed.
pkg/sql/temporary_schema.go, line 50 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Does this need to be exported?
Nup, leftover from a previous approach.
pkg/sql/temporary_schema.go, line 56 at r2 (raw file):
Previously, solongordon (Solon) wrote…
This is a roundabout way of checking if any temp schemas were created, right? Maybe nicer to make an explicit method on SearchPath for this.
Done.
pkg/sql/temporary_schema.go, line 63 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Did you take a look at the DROP DATABASE implementation to see if any code can be shared with that?
Refactored the actual dropping of the descriptor to reuse some code.
Ideally, this code, and the database code, should go through a dropSchema/dropSchemaImpl pattern code path. I can work on that after Im done the schema cache stuff in a PR. I've left a TODO here.
drop database is a bit trickier, because it happens in two steps. My first impression is that it will require implementing the bulk of DROP SCHEMA.
pkg/sql/temporary_schema.go, line 74 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Does this also work for views and sequences?
Fixed.
pkg/sql/sqlbase/structured.proto, line 508 at r2 (raw file):
Previously, solongordon (Solon) wrote…
This sentence doesn't make sense to me.
Done.
pkg/sql/sqlbase/structured.proto, line 512 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Extra underscore?
Done.
pkg/sql/sqlbase/structured.proto, line 692 at r2 (raw file):
Previously, solongordon (Solon) wrote…
Not clear to me what the last sentence means.
Changed to be more explicit. Basically, drops/renames involve reclaiming names in the drain name process. To identify which name to reclaim, we need the parentSchemaID.
|
@andreimatei I got to all the changes you suggested in the |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 743 at r4 (raw file):
} func (ex *connExecutor) maybeCleanupTempObjects() {
this method deserves a comment
pkg/sql/conn_executor.go, line 743 at r4 (raw file):
} func (ex *connExecutor) maybeCleanupTempObjects() {
nit: I don't like the maybe prefix (but I also don't really know what this function does). Is the maybe referring to the possibility of there being no temp objects? If so, I'd drop it. If it's referring to something else, spell it out in the name.
pkg/sql/conn_executor.go, line 744 at r4 (raw file):
func (ex *connExecutor) maybeCleanupTempObjects() { // Has to happen in a new context, because the context associated with the
double space
pkg/sql/conn_executor.go, line 744 at r4 (raw file):
func (ex *connExecutor) maybeCleanupTempObjects() { // Has to happen in a new context, because the context associated with the
This sentence is missing a subject and also it's unclear. What does connection closing have to do with this?
Simply say "Don't inherit context cancelation. This cleanup happens after the connection is closed.". And see below.
pkg/sql/conn_executor.go, line 746 at r4 (raw file):
// Has to happen in a new context, because the context associated with the // session is cancelled when the connection is closed. ctx := context.Background()
take in a context and do logtags.WithTags(context.Background(), logtags.FromContext(ctx)to get the log tags ([n1,...])
And do all this in the caller and pass in the proper context. The comments about connection closing will actually make sense.
pkg/sql/conn_executor.go, line 748 at r4 (raw file):
ctx := context.Background() err := ex.planner.ExtendedEvalContext().DB.Txn(ctx, func(ctx context.Context, txn *client.Txn) error { ex.resetPlanner(ctx, &ex.planner, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */, 0)
pls comment inline on the 0 constant as per the style guide (' /* numAnnotations */').
Same below.
pkg/sql/conn_executor.go, line 749 at r4 (raw file):
err := ex.planner.ExtendedEvalContext().DB.Txn(ctx, func(ctx context.Context, txn *client.Txn) error { ex.resetPlanner(ctx, &ex.planner, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */, 0) return cleanupSessionTempObjects(ctx, &ex.planner, ex.sessionID)
how about having this guy return ex.planner.extendedEvalCtx.SchemaChangers ?
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
}) if err != nil { log.Error(ctx, err)
cleanupSessionTempObjects() doesn't seem to do much error wrapping. Make sure there's a good error message that talks about dropping temp tables, one way or another.
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
}) if err != nil { log.Error(ctx, err)
remember the error, return it later, and swallow it in the caller. (There's doesn't appear to be a reason why this method can't return an error, and if it can, it should.)
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
}) if err != nil { log.Error(ctx, err)
is there a point in continuing after an error? If so, please put some comments to this effect for the return values of cleanupSessionTempObjects() - and hint that schema changes are queued up.
pkg/sql/conn_executor.go, line 754 at r4 (raw file):
log.Error(ctx, err) } err = ex.planner.ExtendedEvalContext().DB.Txn(ctx, func(ctx context.Context, txn *client.Txn) error {
can you give a nod about why this needs to be a different txn than the above?
pkg/sql/conn_executor.go, line 756 at r4 (raw file):
err = ex.planner.ExtendedEvalContext().DB.Txn(ctx, func(ctx context.Context, txn *client.Txn) error { ieFactory := func(ctx context.Context, sd *sessiondata.SessionData) sqlutil.InternalExecutor { ex.resetPlanner(ctx, &ex.planner, txn, ex.server.cfg.Clock.PhysicalTime() /* stmtTS */, 0)
Is this resetPlanner call necessary? It scares me because it suggests that the sbie below uses ex.planner, but that kind of sharing seems against the spirit of passing a factory to execSchemaChanges() - which might use the factory multiple times, concurrently?
pkg/sql/conn_executor.go, line 766 at r4 (raw file):
return ie } err = ex.planner.extendedEvalCtx.SchemaChangers.execSchemaChanges(ctx, ex.server.cfg, &ex.sessionTracing, ieFactory)
line over 100 chars
pkg/sql/conn_executor.go, line 767 at r4 (raw file):
} err = ex.planner.extendedEvalCtx.SchemaChangers.execSchemaChanges(ctx, ex.server.cfg, &ex.sessionTracing, ieFactory) if err != nil {
return ex.planner.... instead of all this error checking. The linter should also yell at you (are you running make lintshort?)
pkg/sql/conn_executor.go, line 773 at r4 (raw file):
}) if err != nil { log.Error(ctx, err)
ditto - return the error
pkg/sql/drop_table.go, line 605 at r4 (raw file):
// Drop a descriptor based its type. func (p *planner) dropAppropriateDesc(
"appropriate"? This word doesn't seem to add anything to the name. dropDesc seems fine.
pkg/sql/drop_table.go, line 605 at r4 (raw file):
// Drop a descriptor based its type. func (p *planner) dropAppropriateDesc(
what is this returning? comment
pkg/sql/drop_table.go, line 609 at r4 (raw file):
) ([]string, error) { if desc.IsView() { // TODO(knz): dependent dropped views should be qualified here.
can you put more words in this TODO? (or ask Rafa). I don't know what this is telling me, and I can't judge if it's appropriate in the new location. Perhaps take the opportunity to do whatever this wants.
pkg/sql/table.go, line 140 at r3 (raw file):
allDatabaseDescriptors []*sqlbase.DatabaseDescriptor // settings is a pointer to the current cluster settings.
remove the comment, it's not useful.
pkg/sql/temporary_schema.go, line 55 at r4 (raw file):
} func cleanupSessionTempObjects(ctx context.Context, p *planner, sessionID ClusterWideID) error {
this function deserves a comment
pkg/sql/temporary_schema.go, line 69 at r4 (raw file):
return err } // TODO(arul): This should probably go through a dropSchemaImpl function
unless this is imminent, I'd get rid of this TODO
pkg/sql/temporary_schema.go, line 70 at r4 (raw file):
} // TODO(arul): This should probably go through a dropSchemaImpl function // once we have user defined schemas/support for dropping schemas implemented.
nit: is this comment wrapper at 80 cols?
pkg/sql/temporary_schema.go, line 71 at r4 (raw file):
// TODO(arul): This should probably go through a dropSchemaImpl function // once we have user defined schemas/support for dropping schemas implemented. tbNames, err := GetObjectNames(ctx, p.txn, p, dbDesc, tempSchemaName, true /*explicitPrefix*/)
nit: space before explicit
pkg/sql/temporary_schema.go, line 73 at r4 (raw file):
tbNames, err := GetObjectNames(ctx, p.txn, p, dbDesc, tempSchemaName, true /*explicitPrefix*/) for i := range tbNames { tbDesc, err := p.ResolveMutableTableDescriptor(ctx, &tbNames[i], true /* true */, ResolveAnyDescType)
fix the comment
pkg/sql/temporary_schema.go, line 77 at r4 (raw file):
return err } if _, err := p.dropAppropriateDesc(ctx, tbDesc, tree.DropCascade); err != nil {
assert that the first return value is empty. (right?)
pkg/sql/temporary_schema.go, line 81 at r4 (raw file):
} } // TODO(arul): When there is a schema cache, this should probably go through
unless this schema cache is imminent, I'd get rid of this todo.
arulajmani
changed the title
There was a problem hiding this comment.
I'm replying to the question addressed to me below.
I'm willing to also review the PR but not before the other one is merged and this one rebased on top. It's a bit too unwieldy to review otherwise.
Reviewed 1 of 95 files at r1, 1 of 34 files at r3, 22 of 23 files at r4.
Reviewable status:
pkg/sql/drop_table.go, line 609 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
can you put more words in this TODO? (or ask Rafa). I don't know what this is telling me, and I can't judge if it's appropriate in the new location. Perhaps take the opportunity to do whatever this wants.
Have you copy-copied this comment from drop_database.go? The comment pertains to the object names accumulated in the tbNameStrings slice. I don't see you using tbNameStrings here so I don't think the comment is relevant/warranted.
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
052ae7b
to
82d349b
Compare
There was a problem hiding this comment.
@knz that should be merged soon, will comment here when this branch is rebased and more "review friendly"
Reviewable status:
pkg/sql/conn_executor.go, line 743 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
this method deserves a comment
Done.
pkg/sql/conn_executor.go, line 743 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: I don't like the
maybeprefix (but I also don't really know what this function does). Is the maybe referring to the possibility of there being no temp objects? If so, I'd drop it. If it's referring to something else, spell it out in the name.
Yup, it refers to the possibility of there being no temp objects. Dropping the maybe.
pkg/sql/conn_executor.go, line 744 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
double space
Done.
pkg/sql/conn_executor.go, line 744 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
This sentence is missing a subject and also it's unclear. What does connection closing have to do with this?
Simply say "Don't inherit context cancelation. This cleanup happens after the connection is closed.". And see below.
Done.
pkg/sql/conn_executor.go, line 746 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
take in a context and do
logtags.WithTags(context.Background(), logtags.FromContext(ctx)to get the log tags ([n1,...])
And do all this in the caller and pass in the proper context. The comments about connection closing will actually make sense.
Done.
pkg/sql/conn_executor.go, line 748 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
pls comment inline on the 0 constant as per the style guide (' /* numAnnotations */').
Same below.
Done.
pkg/sql/conn_executor.go, line 749 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
how about having this guy return
ex.planner.extendedEvalCtx.SchemaChangers?
Makes sense, done.
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
cleanupSessionTempObjects()doesn't seem to do much error wrapping. Make sure there's a good error message that talks about dropping temp tables, one way or another.
Done.
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
remember the error, return it later, and swallow it in the caller. (There's doesn't appear to be a reason why this method can't return an error, and if it can, it should.)
I've implemented this for now, but I'm finding this remembering error and swallowing in the caller sort of ugly.
What if there is an error in both cleanupSessionTempObjects and the partial schema changes that have been queued up? Currently, I'm sticking with logging the second error and returning the first. Not sure thats the best option here.
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
is there a point in continuing after an error? If so, please put some comments to this effect for the return values of
cleanupSessionTempObjects()- and hint that schema changes are queued up.
Yup, there could be schema changes for some tables queued up. Added a comment in cleanupSessionTempObjects.
pkg/sql/conn_executor.go, line 754 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
can you give a nod about why this needs to be a different txn than the above?
Done.
pkg/sql/conn_executor.go, line 756 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Is this
resetPlannercall necessary? It scares me because it suggests that thesbiebelow usesex.planner, but that kind of sharing seems against the spirit of passing a factory toexecSchemaChanges()- which might use the factory multiple times, concurrently?
No it isn't. Removed.
pkg/sql/conn_executor.go, line 766 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
line over 100 chars
Done.
pkg/sql/conn_executor.go, line 767 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
return ex.planner....instead of all this error checking. The linter should also yell at you (are you runningmake lintshort?)
Done.
pkg/sql/conn_executor.go, line 773 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
ditto - return the error
Done.
pkg/sql/drop_table.go, line 605 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
"appropriate"? This word doesn't seem to add anything to the name.
dropDescseems fine.
Done.
pkg/sql/drop_table.go, line 605 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
what is this returning? comment
Done.
pkg/sql/drop_table.go, line 609 at r4 (raw file):
Previously, knz (kena) wrote…
Have you copy-copied this comment from
drop_database.go? The comment pertains to the object names accumulated in thetbNameStringsslice. I don't see you usingtbNameStringshere so I don't think the comment is relevant/warranted.
I have, but I've also changed drop_database.go to use this codepath, and concatenate the return value of this function to tbNameStrings. Should I delete this comment completely/move it back to drop_database.go?
pkg/sql/table.go, line 140 at r3 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
remove the comment, it's not useful.
Modified in the original PR this PR is built off of.
pkg/sql/temporary_schema.go, line 55 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
this function deserves a comment
Done.
pkg/sql/temporary_schema.go, line 69 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
unless this is imminent, I'd get rid of this TODO
Done.
pkg/sql/temporary_schema.go, line 73 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
fix the comment
Done.
pkg/sql/temporary_schema.go, line 77 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
assert that the first return value is empty. (right?)
For now, but not when support for temporary views is turned on.
pkg/sql/temporary_schema.go, line 81 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
unless this schema cache is imminent, I'd get rid of this todo.
Should be done before I leave.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 743 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Done.
can you qualify "objects" a bit pls? Tables and what else?
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I've implemented this for now, but I'm finding this remembering error and swallowing in the caller sort of ugly.
What if there is an error in both
cleanupSessionTempObjectsand the partial schema changes that have been queued up? Currently, I'm sticking with logging the second error and returning the first. Not sure thats the best option here.
use errors.CombineErrors()
pkg/sql/conn_executor.go, line 767 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Done.
Doesn't look done :)
pkg/sql/conn_executor.go, line 773 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Done.
you almost never want to both log an error and return it. Just return it.
pkg/sql/conn_executor.go, line 755 at r5 (raw file):
return err }) // schema changes run after the transaction that queued them has committed, so
nit: s/schema/Schema
pkg/sql/conn_executor.go, line 757 at r5 (raw file):
// schema changes run after the transaction that queued them has committed, so // they must be run in a new transaction. err := ex.planner.ExtendedEvalContext().DB.Txn(ctx,
return errors.CombineErrors(cleanupError, ex.planner....)
pkg/sql/conn_executor.go, line 794 at r5 (raw file):
err := ex.cleanupTempObjects(cleanupCtx) if err != nil { log.Errorf(ctx, "error deleting temporary objects %v", err)
s/%v/%s (because you know that err is not nil)
And put a colon after "objects" pls.
pkg/sql/temporary_schema.go, line 77 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
For now, but not when support for temporary views is turned on.
Well, at that point, the assertion will fail. The assertion serves as insurance that we won't forget to change this.
Also, it will serve as clarifying what the return value of of dropDesc is, and as a implicit comment about why we're ignoring it.
pkg/sql/temporary_schema.go, line 81 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should be done before I leave.
It's not very clear to me from this comment what "this" and "there" mean. Please improve it or... just remove it :).
If you intend to do it, then the TODO is not necessary. If you don't do it, then your name on it is wrong (ticking clock and all).
pkg/sql/temporary_schema.go, line 88 at r5 (raw file):
// there, so that the cache entry is removed as well. // // Finally, also remove the temporary schema from the namespace table
nit: period at the end of the sentence pls
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
2 times, most recently
from
aa84bde
to
2dcfdb7
Compare
There was a problem hiding this comment.
Switched to a new approach to use an internal planner, instead of the session planner, as discussed with @andreimatei offline. I also rebased off master after the namespace PR landed, so this should be easier to review now :)
Reviewable status:
pkg/sql/conn_executor.go, line 743 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
can you qualify "objects" a bit pls? Tables and what else?
I don't need this function anymore, but objects was referring to tables, views, and sequences here. Added a nod to this in temporary_schema.go.
pkg/sql/conn_executor.go, line 752 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
use
errors.CombineErrors()
Done.
pkg/sql/conn_executor.go, line 767 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Doesn't look done :)
I don't think it applied without errors.CombineErrors. Should be good now though :)
pkg/sql/conn_executor.go, line 773 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
you almost never want to both log an error and return it. Just return it.
Done.
pkg/sql/conn_executor.go, line 757 at r5 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
return errors.CombineErrors(cleanupError, ex.planner....)
Done.
pkg/sql/conn_executor.go, line 794 at r5 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
s/%v/%s (because you know that err is not nil)
And put a colon after "objects" pls.
Done.
pkg/sql/temporary_schema.go, line 81 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
It's not very clear to me from this comment what "this" and "there" mean. Please improve it or... just remove it :).
If you intend to do it, then the TODO is not necessary. If you don't do it, then your name on it is wrong (ticking clock and all).
Removed.
pkg/sql/temporary_schema.go, line 88 at r5 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: period at the end of the sentence pls
Removed the comment completely.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 748 at r6 (raw file):
if ex.planner.HasCreatedTemporarySchema(ex.sessionID) { // Don't inherit context cancellation. This cleanup happens after the
the part about happening after something is closed doesn't make sense any more now that you're in close(). Also removing the cancelation seems good for the contexts passed to all callees in this method. Let's take the opportunity to move it to closeWrapper() and just say "// Closing is not cancelable."
There was a problem hiding this comment.
Reviewable status:
pkg/sql/drop_table.go, line 605 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Done.
Actually let's name it dropObject. Putting desc in the name makes it sound like it's a low level function that just deletes a descriptor, whereas this is anything but.
pkg/sql/drop_table.go, line 615 at r6 (raw file):
} // Drops a descriptor based its type. Returns the names of any additional views
"dropDesc drops". Even though the linter does enforce this for non-exported methods, we keep the convention.
pkg/sql/planner.go, line 354 at r6 (raw file):
} func (p *planner) HasCreatedTemporarySchema(sessionID ClusterWideID) bool {
Get rid of this method. The caller has access to the session data. It doesn't need to go through the planner.
pkg/sql/planner.go, line 360 at r6 (raw file):
// can be called by session bound internal executors, this nil check is // required. return p.sessionDataMutator != nil && p.sessionDataMutator.data.SearchPath.HasCreatedTemporarySchema(tempSchemaName)
long line
pkg/sql/sequence.go, line 462 at r6 (raw file):
// When a table/column is dropped that owns sequences, the sequences must be dropped // as well. func dropSequencesOwnedByCol(ctx context.Context, col *sqlbase.ColumnDescriptor, p *planner) error {
this seems to not need a planner, just a txn
pkg/sql/sequence.go, line 482 at r6 (raw file):
func removeSequenceDependencies( ctx context.Context, p *planner,
nit: put the planner last. The other args are more interesting.
And in fact it looks like just the txn is needed, not a planner.
pkg/sql/temporary_schema.go, line 59 at r6 (raw file):
} // Removes all temporary objects (tables, sequences, views) created by the
"cleanupSession... removes"
pkg/sql/temporary_schema.go, line 71 at r6 (raw file):
) defer cleanup() // The temporary schema needs to be set on the new internal planner, as
nit:s/new internal planner/planner
pkg/sql/temporary_schema.go, line 75 at r6 (raw file):
// only be accessed by the session that created them. p.SetTemporarySchemaName(tempSchemaName) dbIDs, err := GetAllDatabaseDescriptorIDs(ctx, p.txn)
please describe around how we're gonna do everything, since it's not obvious.
We're going to read all the database descriptor ids, then for each database we're going
to list the tables in the temporary schema. We're going to then drop the one by one, and then run the schema changes produced by the dropping.
pkg/sql/temporary_schema.go, line 81 at r6 (raw file):
for _, id := range dbIDs { dbDesc, err := MustGetDatabaseDescByID(ctx, p.txn, id) if err != nil {
Since we're making such a big deal about continuing after errors below, why don't we continue accumulating temp tables after errors here or when we handle one table at a time below? We should continue the other databases / tables, shouldn't we?
Or alternatively don't run the schema changes after errors at all. As it stands, we do it half way if I read correctly.
If you do want to tolerate errors as much as possible, consider making another function that returns ids of temp tables, and also an error if the collection might be incomplete.
pkg/sql/temporary_schema.go, line 84 at r6 (raw file):
return err } tbNames, err := GetObjectNames(ctx, p.txn, p, dbDesc, tempSchemaName, true /* explicitPrefix*/)
over 100 chars?
pkg/sql/temporary_schema.go, line 84 at r6 (raw file):
return err } tbNames, err := GetObjectNames(ctx, p.txn, p, dbDesc, tempSchemaName, true /* explicitPrefix*/)
Isn't there a way to get ids instead of names?
Alternatively, since it seems that we're doing a bunch of work to get what we want anyway, can't we simply run DROP TABLE statements and not any custom code?
pkg/sql/temporary_schema.go, line 87 at r6 (raw file):
for i := range tbNames { tbDesc, err := p.ResolveMutableTableDescriptor( ctx, &tbNames[i], true /* required */, ResolveAnyDescType,
Don't we want ResolveRequireTableDesc here? If we're expecting all of these to be tables, my vote is to be strict about it, not tolerant. If nothing else, for documentation purposes.
pkg/sql/temporary_schema.go, line 90 at r6 (raw file):
) if err != nil { queuedSchemaChanges = p.ExtendedEvalContext().SchemaChangers
can't you just assign queuedSchemaChanges once, immediately after the planner was constructed?
pkg/sql/temporary_schema.go, line 93 at r6 (raw file):
return err } if droppedViews, err := p.dropDesc(ctx, tbDesc, tree.DropCascade); err != nil {
Is this going to leave the data around for the TTL period? Don't we want to truncate?
pkg/sql/temporary_schema.go, line 94 at r6 (raw file):
} if droppedViews, err := p.dropDesc(ctx, tbDesc, tree.DropCascade); err != nil { if len(droppedViews) != 0 {
you want the assertion in the case where err == nil, not the reverse
pkg/sql/temporary_schema.go, line 95 at r6 (raw file):
if droppedViews, err := p.dropDesc(ctx, tbDesc, tree.DropCascade); err != nil { if len(droppedViews) != 0 { return errors.AssertionFailedf("only temp views can refer to temp tables, " +
nit: is there an AssertionFailed() (no f)? Cause you don't need it here. But actually:
"unexpected views referencing temp table: %s (%d views), , "
Definitely remove the "until something is supported" part.
pkg/sql/temporary_schema.go, line 125 at r6 (raw file):
return ie } err := queuedSchemaChanges.execSchemaChanges(ctx, server.cfg, &SessionTracing{}, ieFactory)
return queuedSchemaChange...
pkg/sql/logictest/testdata/logic_test/pg_catalog, line 647 at r6 (raw file):
4225994721 13 2 true true false true false true false false true false 1 7 0 0 0 0 2 2 NULL NULL
spurious diff?
pkg/sql/sessiondata/search_path.go, line 156 at r6 (raw file):
// Returns true if a temporary schema has been created by the session. func (s SearchPath) HasCreatedTemporarySchema(expectedTempSchemaName string) bool {
What is this "expected" arg? Needs a comment. Or rather, this method doesn't seem to belong on a SearchPath. The SearchPath doesn't know shit about whether anybody has created a temp schema or not. In fact, it's not even clear who that somebody is. Naming the method like this sounds like it's the SearchPath that might have created a temp schema, which is nonsensical. Get rid of it.
pkg/sql/sqlbase/structured.proto, line 514 at r6 (raw file):
// this field is not set, it implies the descriptor was created when every // table was scoped under the public physical schema. So this field has the // implied value of 29 when unset.
By making the field not nullable, you're taking away the ability to discern whether it was set or not, so s/not set/zero.
I don't really know what the "goes hand in hand" part is telling me. Just say "For backwards compatibility, 0 means the table is part of the public schema (id 29)."
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
2dcfdb7
to
f95d2cc
Compare
There was a problem hiding this comment.
Switched to executing a DROP TABLE statement instead of running custom code to drop descriptors and then run schema changes.
Reviewable status:
pkg/sql/conn_executor.go, line 748 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
the part about happening after something is closed doesn't make sense any more now that you're in
close(). Also removing the cancelation seems good for the contexts passed to all callees in this method. Let's take the opportunity to move it tocloseWrapper()and just say "// Closing is not cancelable."
Done.
pkg/sql/drop_table.go, line 615 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
"dropDesc drops". Even though the linter does enforce this for non-exported methods, we keep the convention.
Done.
pkg/sql/planner.go, line 354 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Get rid of this method. The caller has access to the session data. It doesn't need to go through the planner.
I've changed the method (and the name) slightly, but I think this should be a method call instead of constructing the temp schema name and comparing against the temp schema set on the SearchPath (in the conn_executor).
I thought of putting this method on SessionData, instead of the planner, but that doesn't work because sessionID is in the sql package and that causes a dependency cycle when included in the sessiondata package. The next best alternative imo is to have this method on the planner instead.
pkg/sql/planner.go, line 360 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
long line
Done.
pkg/sql/sequence.go, line 462 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
this seems to not need a planner, just a txn
It accesses the table collection/calls dropSequenceImpl which is a method on the planner.
pkg/sql/sequence.go, line 482 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
nit: put the planner last. The other args are more interesting.
And in fact it looks like just the txn is needed, not a planner.
Put the planner last.
This accesses some methods on the planner as well (WriteSchemaChange, Table() etc.).
pkg/sql/temporary_schema.go, line 59 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
"cleanupSession... removes"
Comment was outdated. Fixed that.
pkg/sql/temporary_schema.go, line 75 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
please describe around how we're gonna do everything, since it's not obvious.
We're going to read all the database descriptor ids, then for each database we're going to list the tables in the temporary schema. We're going to then drop the one by one, and then run the schema changes produced by the dropping.
Added a comment, though the new approach is slightly easier to understand than what was there before. Let me know if the comment isn't required anymore.
pkg/sql/temporary_schema.go, line 81 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Since we're making such a big deal about continuing after errors below, why don't we continue accumulating temp tables after errors here or when we handle one table at a time below? We should continue the other databases / tables, shouldn't we?
Or alternatively don't run the schema changes after errors at all. As it stands, we do it half way if I read correctly.If you do want to tolerate errors as much as possible, consider making another function that returns ids of temp tables, and also an error if the collection might be incomplete.
comment no longer applicable as we don't queue/run schema changes manually anymore.
pkg/sql/temporary_schema.go, line 84 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
over 100 chars?
Fixed.
pkg/sql/temporary_schema.go, line 84 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Isn't there a way to get ids instead of names?
Alternatively, since it seems that we're doing a bunch of work to get what we want anyway, can't we simply runDROP TABLEstatements and not any custom code?
Currently there isn't a way to get all table descriptor IDs under a particular (database, schema). We have functions to get all database IDs, or the ID given a single table name.
Switched the approach to use a DROP TABLE statement, as we discussed offline.
pkg/sql/temporary_schema.go, line 87 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Don't we want
ResolveRequireTableDeschere? If we're expecting all of these to be tables, my vote is to be strict about it, not tolerant. If nothing else, for documentation purposes.
For now, we are yes. This will have to change once views/sequences are allowed.
pkg/sql/temporary_schema.go, line 90 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
can't you just assign
queuedSchemaChangesonce, immediately after the planner was constructed?
Done.
pkg/sql/temporary_schema.go, line 93 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Is this going to leave the data around for the TTL period? Don't we want to truncate?
As we discussed offline, truncate doesn't actually do the deletion. It simply drops the old table (without draining the names), and creates a new table descriptor and updates the namespace table with the new ID.
pkg/sql/temporary_schema.go, line 94 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
you want the assertion in the case where
err == nil, not the reverse
Done.
pkg/sql/temporary_schema.go, line 125 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
return queuedSchemaChange...
Done.
pkg/sql/logictest/testdata/logic_test/pg_catalog, line 647 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
spurious diff?
Extra new line (probably because of my rebase). Removed.
pkg/sql/sessiondata/search_path.go, line 156 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
What is this "expected" arg? Needs a comment. Or rather, this method doesn't seem to belong on a
SearchPath. TheSearchPathdoesn't know shit about whether anybody has created a temp schema or not. In fact, it's not even clear who that somebody is. Naming the method like this sounds like it's theSearchPaththat might have created a temp schema, which is nonsensical. Get rid of it.
Done.
pkg/sql/sqlbase/structured.proto, line 514 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
By making the field not nullable, you're taking away the ability to discern whether it was set or not, so s/not set/zero.
I don't really know what the "goes hand in hand" part is telling me. Just say "For backwards compatibility, 0 means the table is part of the public schema (id 29)."
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 752 at r7 (raw file):
err := cleanupSessionTempObjects(ctx, ex.server, ex.sessionID) if err != nil { log.Errorf(ctx, "error deleting temporary objects: %s", err)
please leave a comment here with a hint about the async retry process
pkg/sql/planner.go, line 354 at r6 (raw file):
but I think this should be a method call instead of constructing the temp schema name and comparing against the temp schema set on the SearchPath (in the conn_executor).
Why? Make it a method on the connExecutor if you insist. The planner doesn't have much to do with a session.
I thought of putting ...
Move ClusterWideID, or whatever type you need, to sqlbase if it helps you
pkg/sql/planner.go, line 358 at r7 (raw file):
func (p *planner) HasSessionCreatedTemporarySchema(sessionID ClusterWideID) bool { tempSchemaName := temporarySchemaName(sessionID) return p.SessionData().SearchPath.GetTemporarySchemaName() == tempSchemaName
Can you find a good place to explain what's going on here? Why is the temporary schema name not a constant? Why is there a function called temporarySchemaName that returns something that's not the same as GetTemporarySchemaName()?
I don't know what I'm talking about, but it seems to me that this name should just be a well known constant.
pkg/sql/planner.go, line 399 at r7 (raw file):
} func (p *planner) SetTemporarySchemaName(scName string) {
Does this method need to be on the planner/exist? I think all callers have access to the sessionDataMutator. If it doesn't need to exist, I'd rather untangle the planner from performing mutations to the session state as much as possible.
pkg/sql/sequence.go, line 462 at r6 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
It accesses the table collection/calls
dropSequenceImplwhich is a method on the planner.
ok
pkg/sql/sequence.go, line 460 at r7 (raw file):
} // When a table/column is dropped that owns sequences, the sequences must be dropped
this comment should start with the function name and say what the function does, not abstractly what must happen. After explaining what it does, you can give a hint about when it's called, or even when it must be called.
dropSequencesOwnedByCol drops all the sequences from col.OwnsSequenceIDs. Called when the respective column (or the whole table) is being dropped.
pkg/sql/sequence.go, line 462 at r7 (raw file):
// When a table/column is dropped that owns sequences, the sequences must be dropped // as well. func dropSequencesOwnedByCol(ctx context.Context, col *sqlbase.ColumnDescriptor, p *planner) error {
One of the callers of this unnecessarily gates the call on if len(col.OwnsSequenceIds) > 0. The other caller doesn't. Get rid of the gating.
pkg/sql/sequence.go, line 462 at r7 (raw file):
// When a table/column is dropped that owns sequences, the sequences must be dropped // as well. func dropSequencesOwnedByCol(ctx context.Context, col *sqlbase.ColumnDescriptor, p *planner) error {
This function doesn't really need to take a col, just a list of sequence ids -> dropSequences(). Being in sequence.go, tying it to column is not great. If you move it to another place that has to do with columns more, the objection might go away.
pkg/sql/temporary_schema.go, line 93 at r6 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
As we discussed offline, truncate doesn't actually do the deletion. It simply drops the old table (without draining the names), and creates a new table descriptor and updates the namespace table with the new ID.
mind leaving a TODO(andrei) or a NOTE saying that we might want to accelerate the deletion of this data
pkg/sql/temporary_schema.go, line 52 at r7 (raw file):
func temporarySchemaName(sessionID ClusterWideID) string { return fmt.Sprintf("pg_temp_%v%v", sessionID.Hi, sessionID.Lo)
s/%v/%d/g
And please explain what this is and how it relates to the other GetTemporarySchemaName().
pkg/sql/temporary_schema.go, line 65 at r7 (raw file):
) defer cleanup() // The temporary schema needs to be set on the planner, as the planner is
I don't understand what this comment is telling me. If you explain it more to me, perhaps I'll be able to suggest a better one.
pkg/sql/temporary_schema.go, line 66 at r7 (raw file):
defer cleanup() // The temporary schema needs to be set on the planner, as the planner is // responsible for ensuring temporary table descriptors can only be
double space after "can"
pkg/sql/temporary_schema.go, line 66 at r7 (raw file):
defer cleanup() // The temporary schema needs to be set on the planner, as the planner is // responsible for ensuring temporary table descriptors can only be
nit: ensuring that ...
pkg/sql/temporary_schema.go, line 70 at r7 (raw file):
p.SetTemporarySchemaName(tempSchemaName) queuedSchemaChanges = p.ExtendedEvalContext().SchemaChangers dbIDs, err := GetAllDatabaseDescriptorIDs(ctx, p.txn)
Can we go fully the SQL route and not read any descriptors, etc? Can we get the names of the tables that we want to delete from system.namespace or from crdb_internal.tables?
Cause if we can, I think it'd be a lot more readable.
pkg/sql/temporary_schema.go, line 76 at r7 (raw file):
// We are going to read all database descriptor IDs, then for each database // we are going to list all the tables under the temporary schema and drop // them using a single DROP TABLE command. By dropping all the tables in
s/command/statement
Delete the last sentence; it begets more questions than it answers.
pkg/sql/temporary_schema.go, line 97 at r7 (raw file):
} else { query = fmt.Sprintf( "%s, %s.%s.%s", query, tbName.CatalogName, tbName.SchemaName, tbName.Table(),
This is an unusual way to append to a string. Do query += fmt.Sprintf(...) or strings.Builder. With the latter you can get rid of the duplication of format args.
pkg/sql/temporary_schema.go, line 101 at r7 (raw file):
} } if query != "" {
if len(tbNames) > 0 is better. query is more incidental.
pkg/sql/temporary_schema.go, line 113 at r7 (raw file):
} // Even if no query was constructed, the temporary schema may exist (eg. // a temporary table was created and then dropped). So we still try to
s/try to/
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
f95d2cc
to
37ad7bb
Compare
There was a problem hiding this comment.
Switched to not using an internalPlanner and added a session variable HasCreatedTempTable.
Reviewable status:
pkg/sql/conn_executor.go, line 752 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
please leave a comment here with a hint about the async retry process
Done.
pkg/sql/planner.go, line 354 at r6 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
but I think this should be a method call instead of constructing the temp schema name and comparing against the temp schema set on the SearchPath (in the conn_executor).
Why? Make it a method on the
connExecutorif you insist. The planner doesn't have much to do with a session.I thought of putting ...
Move
ClusterWideID, or whatever type you need, tosqlbaseif it helps you
As discussed offline, I added a new session variable HasCreatedTemporarySchema, and I perform that check here now.
pkg/sql/planner.go, line 358 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Can you find a good place to explain what's going on here? Why is the temporary schema name not a constant? Why is there a function called
temporarySchemaNamethat returns something that's not the same asGetTemporarySchemaName()?
I don't know what I'm talking about, but it seems to me that this name should just be a well known constant.
As discussed offline, I've added a session variable HasCreatedTemporarySchema, instead of relying on the temporary schema being set on the SearchPath.
pkg/sql/planner.go, line 399 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Does this method need to be on the planner/exist? I think all callers have access to the
sessionDataMutator. If it doesn't need to exist, I'd rather untangle the planner from performing mutations to the session state as much as possible.
Removed.
pkg/sql/sequence.go, line 460 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
this comment should start with the function name and say what the function does, not abstractly what must happen. After explaining what it does, you can give a hint about when it's called, or even when it must be called.
dropSequencesOwnedByCol drops all the sequences from col.OwnsSequenceIDs. Called when the respective column (or the whole table) is being dropped.
Done.
pkg/sql/sequence.go, line 462 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
One of the callers of this unnecessarily gates the call on
if len(col.OwnsSequenceIds) > 0. The other caller doesn't. Get rid of the gating.
Done.
pkg/sql/sequence.go, line 462 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
This function doesn't really need to take a
col, just a list of sequence ids ->dropSequences(). Being insequence.go, tying it to column is not great. If you move it to another place that has to do with columns more, the objection might go away.
Sure, but we miss the opportunity to implicitly document why these sequenceIDs are being dropped if we just drop a list of ids without any context.
Seems like a lot of functions in this file take a column descriptor to work on a col/sequence relationship. I couldn't find a place that has to do with columns more, but if you'd like me to move this someplace specific, I can do that.
pkg/sql/temporary_schema.go, line 52 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
s/%v/%d/g
And please explain what this is and how it relates to the other
GetTemporarySchemaName().
Done. Added a comment describing the purpose of this function so it is a bit less confusing.
pkg/sql/temporary_schema.go, line 65 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I don't understand what this comment is telling me. If you explain it more to me, perhaps I'll be able to suggest a better one.
No need for this comment, as we aren't constructing an internal planner anymore.
pkg/sql/temporary_schema.go, line 66 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
double space after "can"
ditto
pkg/sql/temporary_schema.go, line 70 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Can we go fully the SQL route and not read any descriptors, etc? Can we get the names of the tables that we want to delete from
system.namespaceor fromcrdb_internal.tables?
Cause if we can, I think it'd be a lot more readable.
Then we won't be able to distinguish if the object is a view/sequence/table, which is required for the drop query.
pkg/sql/temporary_schema.go, line 76 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
s/command/statement
Delete the last sentence; it begets more questions than it answers.
Done.
pkg/sql/temporary_schema.go, line 97 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
This is an unusual way to append to a string. Do
query += fmt.Sprintf(...)orstrings.Builder. With the latter you can get rid of the duplication of format args.
Done.
pkg/sql/temporary_schema.go, line 101 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
if len(tbNames) > 0is better.queryis more incidental.
Done.
pkg/sql/temporary_schema.go, line 113 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
s/try to/
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor.go, line 752 at r7 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Done.
I meant a comment that hints to who/when will retry. Like "// The such and such manager will retry periodically."
pkg/sql/create_table.go, line 1033 at r8 (raw file):
columnDefaultExprs := make([]tree.TypedExpr, len(n.Defs)) desc := InitTableDescriptor(id, parentID, parentSchemaID, n.Table.Table(), creationTime, privileges, temporary)
long line
pkg/sql/create_view.go, line 160 at r8 (raw file):
semaCtx *tree.SemaContext, ) (sqlbase.MutableTableDescriptor, error) { desc := InitTableDescriptor(id, parentID, keys.PublicSchemaID, viewName, creationTime, privileges, false /* temporary */)
long line (the fact that it was already long is no excuse :) )
pkg/sql/drop_table.go, line 609 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I have, but I've also changed
drop_database.goto use this codepath, and concatenate the return value of this function totbNameStrings. Should I delete this comment completely/move it back todrop_database.go?
Rafa what's the right thing to do here?
pkg/sql/opt_catalog.go, line 112 at r8 (raw file):
os.name.Schema(), true, /* explicitPrefix */ false, /* required */
required used to be implicitly true. Did you mean to change it?
pkg/sql/resolver.go, line 70 at r8 (raw file):
scName string, explicitPrefix bool, required bool,
The new argument needs a comment (and add one for explicitPrefix too if you can).
Consider changing this bool to an enum, such that the values are self-documenting. Bool args are discouraged by the style guide.
Feel free to ignore, but better yet, would it be appropriate for this to take DatabaseListFlags? Perhaps a flavor of that with the caching flags stripped out if caching is not a reasonable concern for the callers.
pkg/sql/sequence.go, line 462 at r7 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Sure, but we miss the opportunity to implicitly document why these sequenceIDs are being dropped if we just drop a list of ids without any context.
Seems like a lot of functions in this file take a column descriptor to work on a col/sequence relationship. I couldn't find a place that has to do with columns more, but if you'd like me to move this someplace specific, I can do that.
ok
pkg/sql/temporary_schema.go, line 72 at r8 (raw file):
a := UncachedPhysicalAccessor{} return a.GetObjectNames( ctx, txn, dbDesc, tempSchemaName, tree.DatabaseListFlags{CommonLookupFlags: tree.CommonLookupFlags{Required: false}},
long line
pkg/sql/temporary_schema.go, line 76 at r8 (raw file):
} // cleanupSessionTempObjects removes all temporary objects (tables, sequences,
this function looks better, thanks
pkg/sql/temporary_schema.go, line 120 at r8 (raw file):
} } // Even if no objects were found under the temporary schema, it may still
nit: s/it/the schema itself
pkg/sql/vars.go, line 832 at r8 (raw file):
}, `has_created_temporary_schema`: {
let's move this state to the connEx as discussed
pkg/sql/sessiondata/search_path.go, line 28 at r7 (raw file):
// schema if/when CRDB supports them. In PG, schema names starting with `pg_` // are "reserved", so this can never clash with an actual physical schema. const DefaultTemporarySchemaName = "pg_no_temp_schema"
I thought this was going away?
There was a problem hiding this comment.
Looks generally good, although I'd like a last look after Andrei's remaining comments have been addressed.
Reviewed 1 of 23 files at r4, 7 of 55 files at r6, 8 of 15 files at r7, 9 of 9 files at r8.
Reviewable status:
pkg/sql/drop_database.go, line 144 at r8 (raw file):
for _, toDel := range n.td { cascadedViews, err := p.dropObject(ctx, toDel.desc, tree.DropCascade)
rename cascadedViews to cascadedObjects
pkg/sql/drop_table.go, line 609 at r4 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Rafa what's the right thing to do here?
With the new code structure, the comment is now relevant. Nothing else to change here.
pkg/sql/opt_catalog.go, line 112 at r8 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
required used to be implicitly
true. Did you mean to change it?
same question, this sounds like this should be required here
pkg/sql/sequence.go, line 462 at r8 (raw file):
// dropSequencesOwnedByCol drops all the sequences from col.OwnsSequenceIDs. // Called when the respective column (or the whole table) is being dropped. func dropSequencesOwnedByCol(ctx context.Context, col *sqlbase.ColumnDescriptor, p *planner) error {
Make this a method on planner instead of passing the planner as argument.
pkg/sql/sequence.go, line 484 at r8 (raw file):
tableDesc *sqlbase.MutableTableDescriptor, col *sqlbase.ColumnDescriptor, p *planner,
ditto
pkg/sql/temporary_schema.go, line 115 at r8 (raw file):
// nil transaction ensures that the internal executor runs the schema // change, which drains names and deletes the data. _, err = ie.Exec(ctx, "delete-temp_tables", nil /* txn */, query.String())
delete-temp-tables
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
37ad7bb
to
53b297d
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/create_table.go, line 1033 at r8 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
long line
Done.
pkg/sql/create_view.go, line 160 at r8 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
long line (the fact that it was already long is no excuse :) )
Done.
pkg/sql/drop_database.go, line 144 at r8 (raw file):
Previously, knz (kena) wrote…
rename
cascadedViewstocascadedObjects
Done.
pkg/sql/opt_catalog.go, line 112 at r8 (raw file):
Previously, knz (kena) wrote…
same question, this sounds like this should be required here
My bad. Fixed.
pkg/sql/resolver.go, line 70 at r8 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
The new argument needs a comment (and add one for
explicitPrefixtoo if you can).
Consider changing thisboolto an enum, such that the values are self-documenting. Bool args are discouraged by the style guide.Feel free to ignore, but better yet, would it be appropriate for this to take
DatabaseListFlags? Perhaps a flavor of that with the caching flags stripped out if caching is not a reasonable concern for the callers.
Changed back, added a comment for explicitPrefix.
pkg/sql/sequence.go, line 462 at r8 (raw file):
Previously, knz (kena) wrote…
Make this a method on
plannerinstead of passing the planner as argument.
Done.
pkg/sql/sequence.go, line 484 at r8 (raw file):
Previously, knz (kena) wrote…
ditto
Done.
pkg/sql/temporary_schema.go, line 72 at r8 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
long line
Done.
pkg/sql/temporary_schema.go, line 76 at r8 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
this function looks better, thanks
🙌
pkg/sql/temporary_schema.go, line 115 at r8 (raw file):
Previously, knz (kena) wrote…
delete-temp-tables
Done.
pkg/sql/vars.go, line 832 at r8 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
let's move this state to the
connExas discussed
Done.
pkg/sql/sessiondata/search_path.go, line 28 at r7 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I thought this was going away?
Done.
There was a problem hiding this comment.
You'll need to update the expected output in the show_trace test with the new parent schema ID.
LGTM beyond that. Thank you!
Reviewed 22 of 22 files at r9.
Reviewable status:
There was a problem hiding this comment.
LGTM
Not for this PR, but there's 6 remaining TODO(arul) in the codebase. Please update them to different names.
Reviewed 1 of 23 files at r4, 1 of 22 files at r9.
Reviewable status:
pkg/ccl/importccl/load.go, line 169 at r9 (raw file):
// At this point the CREATE statements in the loaded SQL do not // use the SERIAL type so we need not process SERIAL types here. desc, err := sql.MakeTableDesc(ctx, txn, nil /* vt */, st, s, dbDesc.ID, keys.PublicSchemaID, 0, /* table ID */
long line
Please put a visual cue in your editor.
pkg/sql/conn_executor.go, line 988 at r9 (raw file):
executorType executorType // hasCreatedTemporarySchema is set to true if the executor has created a
nit: s/set to true/set
pkg/sql/create_sequence.go, line 147 at r9 (raw file):
params *runParams, ) (sqlbase.MutableTableDescriptor, error) { desc := InitTableDescriptor(id, parentID, keys.PublicSchemaID, sequenceName, creationTime, privileges, false /* temporary */)
long line
pkg/sql/create_table.go, line 1021 at r9 (raw file):
st *cluster.Settings, n *tree.CreateTable, parentID, parentSchemaID, id sqlbase.ID,
parentID and parentSchemaID need comments now. Should parentID be renamed to parentDatabaseID?
pkg/sql/drop_table.go, line 375 at r9 (raw file):
if drainName { parentSchemaID := tableDesc.GetParentSchemaID() if parentSchemaID == sqlbase.InvalidID {
Don't use the getter if there's no particular reason; proto fields are generally accessed directly. As written, that auto-generated getter should not even exist - except someone had the bright idea to use the gogoproto.goproto_getters option on this particular proto so that the generated struct implements some interface. They needed like 2 getters and we got 100.
More importantly, I don't like it that two callers need to do this if parentSchemaID == sqlbase.InvalidID { business. This should all be centralized. One way or another, the getter should do it (and then having a getter would actually make sense). Unfortunately the getter name GetParentSchemaID() is taken.
Some options:
- Make the field nullable and give it a
default = 29(since this file is proto2 you can have defaults). Then the getter does the right thing. But setting the field is a bit more annoying cause one way or another you have to create pointers from the values you want to set it to (seeproto.Int()). - Implement another getter (say,
GetParentSchemaIDOrDefault()) and comment everywhere that that one should be used. If you're filling hot, there's precedent to adding a linter to disallow the default getter (see descriptor_marshall.go) - Get rid of the
gogoproto.goproto_getters. Copy all the getters that are already generated and used from structured.pb.go to structured.go, so they stay around. And thenGetParentSchemaID()is under our control.
pkg/sql/exec_util.go, line 1756 at r9 (raw file):
// setHasCreatedTemporarySchema is called when the temporary schema is set // on the search path. setHasCreatedTemporarySchema func(val bool)
Name this according to when it's called, not what it does. Think of this as an event listener; the caller doesn't care what it does. In the case of setCurTxnReadOnly above, the name sounds like it describes what it does, but it actually describes when its called (on a set statement).
// onTempSchemaCreation is called when the temporary schema is created (for the first time? there can be multiple temp schemas, right?)
onTempSchemaCreation
pkg/sql/exec_util.go, line 1796 at r9 (raw file):
func (m *sessionDataMutator) SetTemporarySchemaName(scName string) { m.setHasCreatedTemporarySchema(true)
if you rename m.setHasCreatedTemporarySchema, extract this out of this method. And at that point the method can go away if you want.
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
53b297d
to
80e93d5
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/importccl/load.go, line 169 at r9 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
long line
Please put a visual cue in your editor.
Done.
pkg/sql/create_sequence.go, line 147 at r9 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
long line
Done.
pkg/sql/create_table.go, line 1021 at r9 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
parentID and parentSchemaID need comments now. Should
parentIDbe renamed toparentDatabaseID?
Added the comments.
Could do that, if we make rename the field on the table descriptor. Would require accounting for all descriptors created before 20.1 though, and the change will have to be made in a lot more places. I'd say that should be considered as a separate PR?
pkg/sql/drop_table.go, line 375 at r9 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Don't use the getter if there's no particular reason; proto fields are generally accessed directly. As written, that auto-generated getter should not even exist - except someone had the bright idea to use the
gogoproto.goproto_gettersoption on this particular proto so that the generated struct implements some interface. They needed like 2 getters and we got 100.More importantly, I don't like it that two callers need to do this
if parentSchemaID == sqlbase.InvalidID {business. This should all be centralized. One way or another, the getter should do it (and then having a getter would actually make sense). Unfortunately the getter nameGetParentSchemaID()is taken.Some options:
- Make the field nullable and give it a
default = 29(since this file is proto2 you can have defaults). Then the getter does the right thing. But setting the field is a bit more annoying cause one way or another you have to create pointers from the values you want to set it to (seeproto.Int()).- Implement another getter (say,
GetParentSchemaIDOrDefault()) and comment everywhere that that one should be used. If you're filling hot, there's precedent to adding a linter to disallow the default getter (see descriptor_marshall.go)- Get rid of the
gogoproto.goproto_getters. Copy all the getters that are already generated and used from structured.pb.go to structured.go, so they stay around. And thenGetParentSchemaID()is under our control.
Discussed offline, changed the field to UnexposedParentSchemaID and added a getter GetParentSchemaID() in structured.go
pkg/sql/exec_util.go, line 1756 at r9 (raw file):
there can be multiple temp schemas, right?
Just one per session (executor). Done.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/create_table.go, line 1021 at r9 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Added the comments.
Could do that, if we make rename the field on the table descriptor. Would require accounting for all descriptors created before 20.1 though, and the change will have to be made in a lot more places. I'd say that should be considered as a separate PR?
(FYI Renaming the field does not affect existing descriptors. No comment further about when to do it though.)
There was a problem hiding this comment.
Reviewable status:
pkg/sql/create_table.go, line 1021 at r9 (raw file):
Previously, knz (kena) wrote…
(FYI Renaming the field does not affect existing descriptors. No comment further about when to do it though.)
I had missed that this goes directly into the descriptor. I'd leave it alone then, and consider renaming it in a different commit. And then I'd drop the "parent" prefix from both the database and the schema fields.
For now just please make sure that the descriptor has a comment telling me that one of them is a database id.
Previously, temporary tables and the temporary schema created by a session were not cleaned up when the session ended. As these objects are session scoped, this needs to change. This PR: - Addresses the cleanup of temporary objects when the session exits gracefully. - Adds a SchemaID field to table descriptors. If this field is not set, it is assumed to be 29, which is the schemaID of `public` schema. Release note (sql change): When a session that has created temporary tables exits gracefully, the tables and temporary schema are deleted automatically.
arulajmani
force-pushed
the
temp-tables-session-deletion
branch
from
80e93d5
to
6cd0e02
Compare
There was a problem hiding this comment.
Fixed a failing test, should be good to merge now.
Reviewable status:
pkg/sql/create_table.go, line 1021 at r9 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I had missed that this goes directly into the descriptor. I'd leave it alone then, and consider renaming it in a different commit. And then I'd drop the "parent" prefix from both the database and the schema fields.
For now just please make sure that the descriptor has a comment telling me that one of them is a database id.
I think the descriptor already has a comment saying the parentID refers to the parent database.
I'd be in favor of renaming it in a different commit too.
|
bors r+ |
42742: sql: delete temporary tables when session exits gracefully r=arulajmani a=arulajmani Previously, temporary tables and the temporary schema created by a session were not cleaned up when the session ended. As these objects are session scoped, this needs to change. This PR: - Addresses the cleanup of temporary objects when the session exits gracefully. - Adds a SchemaID field to table descriptors. If this field is not set, it is assumed to be 29, which is the schemaID of `public` schema. Release note (sql change): When a session that has created temporary tables exits gracefully, the tables and temporary schema are deleted automatically. Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
Build succeeded |
Previously, temporary tables and the temporary schema created by a
session were not cleaned up when the session ended. As these objects
are session scoped, this needs to change.
This PR:
Addresses the cleanup of temporary objects when the session
exits gracefully.
Adds a SchemaID field to table descriptors. If this field is not
set, it is assumed to be 29, which is the schemaID of
publicschema.Release note (sql change): When a session that has created temporary tables
exits gracefully, the tables and temporary schema are deleted
automatically.