sqlccl: merge IMPORT and RESTORE jobs

[maddyblue]

Previously the IMPORT statement would create both an IMPORT and a
RESTORE job (unless transform_only was specified). This created some
problems: it forced a temp directory to be specified and prevented
IMPORT from being a real job due to how the job code currently works.

Change IMPORT to instead directly ingest sstables as soon as they
are created, thus removing the need for the RESTORE job. This new
functionality is only available in distributed mode. Make distributed
the default since it is now well tested enough that we have high
enough confidence in it to flip that switch. Add a local option to
use local import instead. (If a user uses local, and thus must also
use transform, they will need to use a normal RESTORE job in order
to import the produced backups, which would require a CCL license.)

This change allows for some optimizations during sst generation,
like preallocating the table ID and generating data with it, thus
removing the need for any key rewriting, which has historically been
limiting speed factor during restores.

Remove transform_only and temp, but merge them into a new transform
option that performs only a transform.

This commit does not change IMPORT jobs to be handled by the registry,
which means that a failure could leave orphaned data.

While here, fix up subtests in TestImportStmt to use correct indexes
for jobs, and database and directory names.

Release note (enterprise): IMPORT CSV has had its required temp
parameter removed. In addition, the transform_only option has been
renamed to transform and now takes an argument specifying where to
store transformed CSV data (but keeps its behavior that, if specified,
only the transform part of the IMPORT will be performed). Finally,
IMPORT no longer creates a RESTORE job, but instead directly restores
data itself. For most uses of IMPORT, simply remove the temp option
from the query will achieve the same result as before.

[cockroach-teamcity]

This change is 

[maddyblue]

@tschottdorf Adding you to review my usage of AddSSTable to make sure it's correct since Dan is out.

[maddyblue]

Benchmark: 4 node geo-distributed cluster doing a 8-way split SF-10 TPCH IMPORT
old: 47 minutes
new: 35 minutes
single node load csv conversion: 84 minutes

[maddyblue]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/ccl/sqlccl/csv.go, line 1543 at r1 (raw file):

					log.Errorf(ctx, "failed to scatter span %s: %s", roachpb.PrettyPrintKey(nil, span.End), pErr)
				}
				if err := sp.db.AddSSTable(ctx, firstKey, lastKey, data); err != nil {

The restore code has a thing that computes stats after this. Do we need that here? I'm not familiar enough with this to know.

---

Comments from Reviewable

[dt]

Reviewed 1 of 3 files at r1, 1 of 2 files at r2.
Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/ccl/sqlccl/csv.go, line 1543 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

The restore code has a thing that computes stats after this. Do we need that here? I'm not familiar enough with this to know.

As far as correctness, I think evalAddSStable does the MVCC stat work that has to be done though, so this is more a nice-to-have, not a must-have.

I can certainly see wanting to know how many rows I IMPORTed, and wouldn't mind the other numbers on index entries and byte size either. We could probably accumulate these during the aggregation / SST creation step above using a BulkOpSummary? Anyway, don't think it needs to block this change.

---

pkg/ccl/sqlccl/csv.go, line 1540 at r2 (raw file):

				}
				if _, pErr := client.SendWrapped(ctx, sp.db.GetSender(), scatterReq); pErr != nil {
					// TODO(dan): Unfortunately, Scatter is still too unreliable to

Comment is stale here (i.e. not in RESTORE). Could maybe just remove it.

We could mention something about in the future, trying to split and scatter on the coordinator so that the distsql collector can be scheduled on the actual ingesting node, instead of adding another network trip for the AddSSTable, but that's an optimization for later either way.

---

Comments from Reviewable

[maddyblue]

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/ccl/sqlccl/csv.go, line 1543 at r1 (raw file):

Previously, dt (David Taylor) wrote…

As far as correctness, I think evalAddSStable does the MVCC stat work that has to be done though, so this is more a nice-to-have, not a must-have.

I can certainly see wanting to know how many rows I IMPORTed, and wouldn't mind the other numbers on index entries and byte size either. We could probably accumulate these during the aggregation / SST creation step above using a BulkOpSummary? Anyway, don't think it needs to block this change.

Yes, I'd like to do this in a followup commit.

---

pkg/ccl/sqlccl/csv.go, line 1540 at r2 (raw file):

Previously, dt (David Taylor) wrote…

Comment is stale here (i.e. not in RESTORE). Could maybe just remove it.

We could mention something about in the future, trying to split and scatter on the coordinator so that the distsql collector can be scheduled on the actual ingesting node, instead of adding another network trip for the AddSSTable, but that's an optimization for later either way.

I've changed RESTORE to IMPORT, but I've otherwise kept the comment the same because afaik it's still true, or at least untested. Yes, we could pre-split everything in the coordinator, but that would then take up 10s of minutes just splitting, because we'd have to do it all up front. Your proposal would also require the distsql plan to know which nodes are in the correct zone to be able to store the table, whereas the current code allows the scatter implementation to determine that.

---

Comments from Reviewable

[dt]

---

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[maddyblue]

Running a SF-100 import had an error:

pq: ADDSSTable [/Table/51/1/10190243/2/0, /Table/51/1/10462464/1/0/NULL]: result is ambiguous (removing replica)

[maddyblue]

Current error:

pq: addsstable [/Table/51/1/4938598/4/0,/Table/51/1/6192549/6/0/NULL): command is too large: 75242286 bytes (max: 67108864)

[maddyblue]

Ok all bugs fixed I think. Changed some stuff around AddSSTables to retry if there was a split and to split up commands larger than max raft size. Tested with a 100GB SF IMPORT and it worked correctly over 8 hours.

RFAL

[tbg]

Sorry to let this sit, Matt. I'll take a look first thing tomorrow.

On Sat, Jan 20, 2018, 7:18 PM Matt Jibson ***@***.***> wrote: Ok all bugs fixed I think. Changed some stuff around AddSSTables to retry if there was a split and to split up commands larger than max raft size. Tested with a 100GB SF IMPORT and it worked correctly over 8 hours. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21490 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE135JLdPbBMw2dDy-28xxW2-C9__PBaks5tMoJwgaJpZM4Rgb9R> .

--

…

-- Tobias

[tbg]

on the AddSSTable part mod a comment.

---

Reviewed 1 of 2 files at r2, 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/ccl/sqlccl/csv.go, line 1559 at r3 (raw file):

						log.Errorf(ctx, "failed to scatter span %s: %s", roachpb.PrettyPrintKey(nil, end), pErr)
					}
					if err := storageccl.AddSSTable(ctx, sp.db, firstKey, lastKey, data); err != nil {

lastKey is exclusive, right? It can't be a key that's actually in the SSTable. Just double checking. If lastKey is already actualLastKey.Next() I think it's worth renaming the arguments to avoid confusion.

---

Comments from Reviewable

[danhhz]

Heads up that I added a use of transform_only in #21373, so you'll want to pull past that

[maddyblue]

Rebased for #21373 fix.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/ccl/sqlccl/csv.go, line 1559 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

lastKey is exclusive, right? It can't be a key that's actually in the SSTable. Just double checking. If lastKey is already actualLastKey.Next() I think it's worth renaming the arguments to avoid confusion.

lastKey was exclusive, yes. However I've modified the code to make it more obvious what's going on. As a side effect we now only call .Next once per SSTable instead of on every KV, which was certainly a large amount of wasted CPU and memory copying.

---

Comments from Reviewable

[maddyblue]

@danhhz @tschottdorf Do I need to worry about MVCC stats after calling addsstable? I have no idea what those are and saw some code in restore dealing with them. Just want to make sure this is safe to merge if the tests pass.

[danhhz]

AddSSTable handles updating them for you, so shouldn't be anything to worry about at the RESTORE/IMPORT level