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

Support options connection parameter #2216

Merged
merged 1 commit into from Jul 8, 2020
Merged

Conversation

rafiss
Copy link
Contributor

@rafiss rafiss commented May 13, 2020

This supports the connection parameter documented here:
https://www.postgresql.org/docs/9.1/libpq-connect.html#LIBPQ-CONNECT-OPTIONS

fixes #2214

@brianc
Copy link
Owner

brianc commented May 13, 2020

wow that was fast! Do you know of a param I could send to the backend & then query the backend to verify it's taken effect? I'd like to add a test that does something like...

const client = new Client({ options: '--foo=bar' })
await client.connect()
const { rows } = await client.query('SELECT session_options')
assert(rows[0].key === 'foo')
assert(rows[0].value === 'bar')

or something like that. The specifics aren't really ironed out there I just always like to test "round tripping" the data when I can. I'll write this test if you know of how it could work?

@sehrope
Copy link
Contributor

sehrope commented May 13, 2020

@brianc How about default_transaction_isolation? I have a feeling that one won't be going away:

$ psql \
    -c 'SHOW default_transaction_isolation'
 default_transaction_isolation 
-------------------------------
 read committed
(1 row)

$ PGOPTIONS='--default_transaction_isolation=serializable' psql \
    -c 'SHOW default_transaction_isolation'
 default_transaction_isolation 
-------------------------------
 serializable
(1 row)

@boromisp
Copy link
Contributor

With recent postgres versions you can use custom namespaced variables:

PGOPTIONS="-c example.foo=bar" psql -c "SHOW example.foo"
 example.foo
-------------
 bar
(1 row)

psql -c "SHOW example.foo"
ERROR:  unrecognized configuration parameter "example.foo"

@rafiss
Copy link
Contributor Author

rafiss commented May 13, 2020

Doing a round-trip test definitely sounds good to me. And thanks for taking on the task of writing it!

I'd agree with using a custom namespaced variable like how @boromisp posted. I think the -c name=value syntax is needed to set it.

@boromisp
Copy link
Contributor

I think you will have to parse these parameters and include them in the startup message for the JavaScript client.

@rafiss
Copy link
Contributor Author

rafiss commented May 13, 2020

I think you will have to parse these parameters and include them in the startup message for the JavaScript client.

Ah thanks, I'd missed that earlier. Just updated.

@boromisp
Copy link
Contributor

The #2103 issue could be fixed by setting the statement_timeout for the native driver in the options parameter:

var escapeOptionValue = function (value) {
  return ('' + value).replace(/\\/g, '\\\\').replace(/ /g, '\\ ')
}

var addOption = function (options, config, optionName) {
  var value = config[optionName]
  if (value !== undefined && value !== null) {
    options.push('-c ' + optionName + '=' + escapeOptionValue(value))
  }
}

ConnectionParameters.prototype.getLibpqConnectionString = function (cb) {
  // ...
  var options = []
  if (this.options) {
    options.push(this.options)
  }
  addOption(options, this, 'statement_timeout')
  if (options.length > 0) {
    params.push('options=' + quoteParamValue(options.join(' ')))
  }
  // ...
}

An other (undocumented) config parameter that doesn't work with the native driver is idle_in_transaction_session_timeout. This could also be fixed the same way.

(The unsupported startup parameter: statement_timeout error in #2103 is probably related to this: https://www.pgbouncer.org/config.html#ignore_startup_parameters)

@brianc
Copy link
Owner

brianc commented Jun 3, 2020

Hey sorry for the delay! I was out on vacation & just getting back...I'll take a look at this tomorrow! ❤️

@brianc
Copy link
Owner

brianc commented Jul 8, 2020

@rafiss I have included a patch file for an integration test

https://cloudup.com/cmGDTMNSW9f

If you apply that patch to your branch I can merge this & release a new minor version w/ the support added. Thanks for your work here & sorry for the delay!

@brianc
Copy link
Owner

brianc commented Jul 8, 2020

or actually i can just merge this & then add the test myself. :)

@brianc brianc merged commit da5d4ef into brianc:master Jul 8, 2020
brianc added a commit that referenced this pull request Jul 8, 2020
brianc added a commit that referenced this pull request Jul 8, 2020
@brianc
Copy link
Owner

brianc commented Jul 8, 2020

Thanks for the PR! I'll release this tomorrow morning so I can make sure it doesn't cause weird regressions in anyone's systems. (it shouldn't...but you never know)

@rafiss rafiss deleted the pgoptions branch July 8, 2020 22:32
@rafiss
Copy link
Contributor Author

rafiss commented Jul 8, 2020

Thank you @brianc! Let me know if anything comes up.

@brianc
Copy link
Owner

brianc commented Jul 9, 2020

Released as pg@8.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support options/PGOPTIONS connection parameter/environment variable
4 participants