cli/zip: avoid collecting crdb_internal.jobs in clusters prior to v20.2

[knz]

Fixes #48486

The crdb_internal.jobs vtables materializes in RAM.
When there are many rows due to frequent backup jobs, this can
cause the node where the data is collected to fail with OOM errors.

In v20.2, this failure mode is eliminated by preventing RAM
materialization of vtables. However this optimization is not available
in prior versions.

This patch changes zip to detect the version of the node performing
the collection, and skips over crdb_internal.jobs when v19.x or
v20.1 is detected. The table system.jobs is still collected as
its data is properly streamed and not materialized in RAM.

It's not possible to add automated testing of this functionality in
the code, because all test clusters run under fictional version
"v999.0.0". However I manually tested using v19.1 and v20.1 and
confirmed the table is skipped.

Release note (bug fix): Running cockroach debug zip does not any
more run the risk of growing memory usage of the remote
node to dangerous levels if there are many job entries currently
in system.jobs. This bug has always been present since debug zip
was first introduced.

[cockroach-teamcity]

This change is 

[tbg]

Do we even support using a cli that doesn't match the server version? Seems like another mixed-version story that we don't need.

LGTM though

[knz]

Do we even support using a cli that doesn't match the server version?

Yes it's supported, in both directions even: v20.1 CLI client with v19.2 server, and v19.2 client with v20.1 server.

This is not enforced by unit tests, but we fix bugs when they are encountered. Also all the client-server RPCs are designed to ensure this works.

[knz]

@dt would you be comfortable with only retrieving system.jobs and skipping over crdb_internal.jobs in all cases? This would simplify the solution altogether.

[tbg]

Doesn't system.jobs have proto-encoded blobs in it? I'd rather not have to deal with that. I routinely check the jobs just to get an idea of what's going on.

[dt]

yeah, it is, and AFAIK, the cli's printing of those binary fields in 19.x is actually unparsable so system.jobs in 19.x debug zips is unusable.

[knz]

the cli's printing of those binary fields in 19.x is actually unparsable so system.jobs in 19.x debug zips is unusable.

I thought we backported spas' fix to dump the field in hex. If not I will do it.

[knz]

I'd rather not have to deal with that.

My PR as is makes you have to deal with it (it removes the vtable from the dump).

If you want to have the vtable, I'd need to extend the PR to first query the table's size server-side, and only if it is known to be under some reasonable row count limit request it. Would that work?

[tbg]

I'm fine if the table is missing when the cli version does not match the server, on the assumption that it will be relatively rare.

[knz]

I'm fine if the table is missing when the cli version does not match the
server,

That's not what this does. The table gets skipped from any cluster running v19.x or v20.1. I intend to backport this change. This means we won't get it from any cluster running those versions from now on.

[dt]

I feel like this is pessimistic -- we're assuming that nodes have insufficient memory to serve this query, but this seems like a big hammer to bring in when we don't even know if we have a nail.

Also, if this is OOM'ing the node, so would SHOW JOBS or other inspection of the vtable. I feel like the "fix" here is to add memory accounting to vtable (which I thought @jordanlewis already did) and then make zip gracefully handle getting an error back rather than avoid the vtable unconditionally in debugging data collection. Just removing this entirely from our debugging collection will seriously hamper actual debugging.

[knz]

@jordanlewis do you confirm that the SHOW JOBS / crdb_internal.jobs memory accounting has been added and backported both to 20.1, 19.2 and 19.1?

If it has, then this PR is not necessary (and I can close it).
If it has not, maybe we can backport it? Or I can massage this PR further.

[knz]

I traced the root cause to a SQL issue that hasn't been fixed yet: #48595

I think that unless we prioritize a fix for that issue, this PR (or something similar) should be merged. The stability risk is real.

[knz]

suspending work on this as #49148 seems to be making progress.