Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql/pgwire: set options based on "options" url parameter #59404

Closed
rafiss opened this issue Jan 25, 2021 · 2 comments · Fixed by #59621
Closed

sql/pgwire: set options based on "options" url parameter #59404

rafiss opened this issue Jan 25, 2021 · 2 comments · Fixed by #59621
Labels
A-sql-pgwire pgwire protocol issues. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue

Comments

@rafiss
Copy link
Collaborator

rafiss commented Jan 25, 2021

The PGJDBC docs describe how one can include a URL parameter of options in order to set session variables: https://jdbc.postgresql.org/documentation/head/connect.html#connection-parameters

This is important because PGJDBC does not allow one to set session variables directly in the URL. That is, you need to specify like this:

postgresql://user@host:port/database?options=-c%20serial_normalization=virtual_sequence

and not this

postgresql://user@host:port/database?serial_normalization=virtual_sequence

However, CockroachDB only recognizes the latter right now. The connection initialization in parseClientProvidedSessionParameters
needs to be updated to look for the options key.


here's an example that can be used to repro and write a test. the test for this can go in pkg/sql/pgwire/conn_test.go. The test TestConnMessageTooBig has better examples that show how to use pgx and the test/assertion libraries.

    config, err := pgx.ParseConnectionString("postgresql://root@localhost:26257/defaultdb?sslmode=disable&options=-c%20serial_normalization=virtual_sequence")
    if err != nil {
        log.Fatal("error configuring the database: ", err)
    }

    conn, err := pgx.Connect(config)
    if err != nil {
        log.Fatal("error connecting to the database: ", err)
    }
    defer conn.Close(context.Background())

    row, err := conn.QueryRow(context.Background(), "SHOW serial_normalization")
    if err != nil {
        log.Fatal(err)
    }

    var s string
    if err := rows.Scan(&s); err != nil {
        log.Fatal(err)
    }
    // assert s == "virtual_sequence" here
    fmt.Printf("%s \n", s)
@rafiss rafiss added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-pgwire pgwire protocol issues. labels Jan 25, 2021
@rafiss rafiss added E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue labels Jan 28, 2021
@mneverov
Copy link
Contributor

hi, I'd like to work on this.

@rafiss
Copy link
Collaborator Author

rafiss commented Jan 29, 2021

@mneverov thanks for your interest! please go ahead. the issue description should have enough pointers to get started :)

mneverov added a commit to mneverov/cockroach that referenced this issue Jan 31, 2021
Previously, CRDB ignored "options" URL parameter. User session parameters should
have been set via URL parameters directly:
`postgres://user@host:port/database?serial_normalization=virtual_sequence`

CRDB can now parse "options" URL parameter and set corresponding session
parameters (in compliance with Postgres jdbc connection parameters):
`postgres://user@host:port/database?options=-c%20serial_normalization=virtual_sequence`

Fixes cockroachdb#59404

Release note (sql change): CockroachDB now recognizes "options" URL parameter.
mneverov added a commit to mneverov/cockroach that referenced this issue Feb 1, 2021
Previously, CRDB ignored "options" URL parameter. User session parameters should
have been set via URL parameters directly:
`postgres://user@host:port/database?serial_normalization=virtual_sequence`

CRDB can now parse "options" URL parameter and set corresponding session
parameters (in compliance with Postgres jdbc connection parameters):
`postgres://user@host:port/database?options=-c%20serial_normalization=virtual_sequence`

Fixes cockroachdb#59404

Release note (sql change): CockroachDB now recognizes "options" URL parameter.
mneverov added a commit to mneverov/cockroach that referenced this issue Feb 6, 2021
Previously, CRDB ignored "options" URL parameter. User session parameters should
have been set via URL parameters directly:
`postgres://user@host:port/database?serial_normalization=virtual_sequence`

CRDB can now parse "options" URL parameter and set corresponding session
parameters (in compliance with Postgres jdbc connection parameters):
`postgres://user@host:port/database?options=-c%20serial_normalization=virtual_sequence`

Fixes cockroachdb#59404

Release note (sql change): CockroachDB now recognizes "options" URL parameter. The
"options" parameter specifies session variables to set at connection start. This
is treated the same as defined in the PostgreSQL docs: https://www.postgresql.org/docs/13/libpq-connect.html#LIBPQ-PARAMKEYWORDS
craig bot pushed a commit that referenced this issue Feb 9, 2021
59441: streamingccl: improvements to the random stream test client r=pbardea a=adityamaru

This change improves on the random stream client to allow for better
testing of the various components of the stream ingestion job.
Specifically:

- Adds support for specifying number of partitions. For simplicity,
  a partition generates KVs for a particular table span.

- Generates system KVs (descriptor and namespace) KVs, as the first two
  KVs on the partition stream. I played around with the idea of having a
separate "system" and "table data" partition, but the code and tests
became more convoluted, compared to the current approach.

- Hookup the CDC orderValidator to the random stream client's output.
  This gives us some guarantees that the data being generated is
semantically correct.

- Maintain an in-memory copy of all the streamed events, that can be
  efficiently queried. This allows us to compare the ingested KVs to the
streamed KVs and gain more confidence in our pipeline.

Infroms: #59175

Release note: None

59621: pgwire: set options based on "options" URL parameter r=rafiss a=mneverov

pgwire: set options based on "options" URL parameter

Previously, CRDB ignored "options" URL parameter. User session parameters should
have been set via URL parameters directly:
`postgres://user@host:port/database?serial_normalization=virtual_sequence`

CRDB can now parse "options" URL parameter and set corresponding session
parameters (in compliance with Postgres jdbc connection parameters):
`postgres://user@host:port/database?options=-c%20serial_normalization=virtual_sequence`

Fixes #59404

Release note (sql change): CockroachDB now recognizes "options" URL parameter.

59781: sql,metrics: do not increment ROLLBACK counter if in CommitWait r=arulajmani a=rafiss

fixes #50780 

Release note (bug fix): Previously if `RELEASE SAVEPOINT cockroach_restart`
was followed by `ROLLBACK`, the `sql.txn.rollback.count`
metric would be incremented. This was incorrect, since the txn had already
committed. Now that metric is not incremented in this case.

Co-authored-by: Aditya Maru <adityamaru@gmail.com>
Co-authored-by: Max Neverov <neverov.max@gmail.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
@craig craig bot closed this as completed in dd2512c Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgwire pgwire protocol issues. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants