pgwire: add telemetry for fetch limits #31637
Conversation
|
This change is |
pkg/sql/pgwire/command_result.go
Outdated
| // We have to do the telemetry explicitly here because automatic | ||
| // telemetry for unimplemented features is only done in the | ||
| // connExecutor. | ||
| telemetry.Count("errorcodes." + pgerror.CodeFeatureNotSupportedError) |
There was a problem hiding this comment.
I'm missing something -- why can't we let the regular unimplemented code counting in conn exec get this?
There was a problem hiding this comment.
Like I said, I'm probably missing something. Usually we let the error bubble up to the conn executor and it inspects it as a goes back to the client, so I was wondering why that didn't work for this case?
There was a problem hiding this comment.
This was actually pretty painless. I had forgotten that I had dropped the dependency on the executor in a previous PR. Done.
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/sql/pgwire/command_result.go, line 134 at r1 (raw file):
Previously, dt (David Taylor) wrote…
I'm missing something -- why can't we let the regular unimplemented code counting in conn exec get this?
how would you propose to do that?
|
the conn executor sits above pgwire. This error is not originating in conn executor, it originates in pgwire. |
There was a problem hiding this comment.
LGTM
Reviewable status:
pkg/sql/pgwire/command_result.go, line 134 at r1 (raw file):
// telemetry for unimplemented features is only done in the // connExecutor. telemetry.Count("errorcodes." + pgerror.CodeFeatureNotSupportedError)
I'd use r.err.code instead of repeating the code here
|
I'd have a slight preference to library-sizing |
|
lemme look if I can factor this code then |
|
Thanks, and again, that's a slight preference, so if it turns out to be annoying or if there's no good home for such a function, still LGTM as-is. |
|
aye |
knz
force-pushed
the
20181019-limit-telemetry
branch
from
58f1abb
to
80c7afe
Compare
|
TFYRs! bors r+ |
Merge conflict (retrying...) |
The JDBC driver and perhaps others commonly try to use the "fetch limit" parameter, which is yet unsupported in CockroachDB (cockroachdb#4035). This patch adds telemetry to gauge demand. Release note (sql change): attempts by client apps to use the unsupported "fetch limit" parameter (e.g. via JDBC) will now be captured in telemetry if statistics reporting is enabled, to gauge support for this feature.
knz
force-pushed
the
20181019-limit-telemetry
branch
from
80c7afe
to
552aabd
Compare
Canceled |
|
bors r+ |
31637: pgwire: add telemetry for fetch limits r=knz a=knz Requested by @awoods187 The JDBC driver and perhaps others commonly try to use the "fetch limit" parameter, which is yet unsupported in CockroachDB (#4035). This patch adds telemetry to gauge demand. Release note (sql change): attempts by client apps to use the unsupported "fetch limit" parameter (e.g. via JDBC) will now be captured in telemetry if statistics reporting is enabled, to gauge support for this feature. 31725: sql/parser: re-allow FAMILY, MINVALUE, MAXVALUE, NOTHING and INDEX in table names r=knz a=knz Fixes #31589. CockroachDB introduced non-standard extensions to its SQL dialect very early in its history, before concerns of compatibility with existing PostgreSQL clients became a priority. When these features were added, new keywords were liberally marked as "reserved", so as to "make the grammar work", and without noticing / care for the fact that this change would make existing valid SQL queries/clients encounter new errors. An example of this: 1. let's make "column families" a thing 2. the syntax `create table(..., constraint xxx family(a,b,c))` is not good enough (although this would not require reserved keywords), we really want also `create table (..., family (a,b,c))` to be possible. 3. oh, the grammar won't let us because "family" is a possible column name? No matter! let's mark "FAMILY" as a reserved name for column/function names. - No concern for the fact that "family" is a perfectly valid English name for things that people want to make an attribute of in inventory / classification tables. - No concern for the fact that reserved column/function names are also reserved for table names. 4. (much later) Clients complaining about the fact they can't call their columns or tables `family` without quoting. Ditto "INDEX", "MINVALUE", "MAXVALUE", and perhaps others. Moral of the story: DO NOT MAKE NEW RESERVED KEYWORDS UNLESS YOU'RE VERY VERY VERY SURE THAT THERE IS NO LEGITIMATE USE FOR THEM IN CLIENT APPS EVER. (An example perhaps: the word "NOTHING" was also marked as reserved, but it's much more unlikely this word will ever be used for something useful.) This patch restores the use of FAMILY, INDEX, NOTHING, MINVALUE and MAXVALUE in table and function names, by introducing an awkward dance in the grammar of keyword non-terminals and database object names. They remain reserved as colum names because of the non-standard CockroachDB extensions. Release note (sql change): It is now again possible to use the keywords FAMILY, MINVALUE, MAXVALUE, INDEX or NOTHING as table names, for compatibility with PostgreSQL. 31731: sql/parser: unreserve INDEX and NOTHING from the RHS of SET statements r=knz a=knz First commit from #31725. The SET statement in the pg dialect is special because it auto-converts identifiers on its RHS to symbolic values or strings. In particular it is meant to support a diversity of special keywords as pseudo-values. This patch ensures that INDEX and NOTHING are accepted on the RHS. Release note (sql change): the names "index" and "nothing" are again accepted in the right-hand-side of the assignment in SET statements, for compatibility with PostgreSQL. Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Build succeeded |
Requested by @awoods187
The JDBC driver and perhaps others commonly try to use the "fetch
limit" parameter, which is yet unsupported in
CockroachDB (#4035). This patch adds telemetry to gauge demand.
Release note (sql change): attempts by client apps to use the
unsupported "fetch limit" parameter (e.g. via JDBC) will now be
captured in telemetry if statistics reporting is enabled, to gauge
support for this feature.