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

feat(db) add pg_timeout configuration parameter to postgres strategy #3808

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@bungle
Copy link
Member

commented Sep 27, 2018

Summary

Adds support for pg_timeout configuration parameter. Also fixes major_minor_version of Postgres according to Postgres versioning policy: https://www.postgresql.org/support/versioning/.

@thibaultcha
Copy link
Member

left a comment

Awesome! Mind addressing the concern on version numbers?

@bungle bungle force-pushed the feat/pg-timeout branch from 2b78ad8 to 88d61cc Sep 27, 2018

@bungle bungle force-pushed the feat/pg-timeout branch from 88d61cc to 6dc0229 Sep 28, 2018

Show resolved Hide resolved kong/db/strategies/postgres/connector.lua
res[1].server_version .. "'"
local major = floor(ver / 10000)
if major < 10 then
self.major_version = tonumber(fmt("%u.%u", major, floor(ver / 100 % 100)))

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Sep 28, 2018

Member

I think this should be a string, to align with the self.major_minor_version attribute. If we want a number to be able to do branching on postgres versions, we should save the number returned by SHOW server_version_num;, and itsd attribute should be suffixed with _num, like major_version_num, as previously done in the Cassandra strategy iirc.

This comment has been minimized.

Copy link
@bungle

bungle Sep 29, 2018

Author Member

I followed Cassandra strategy here, where self.major_version is number and self.major_minor_version is string

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 1, 2018

Member

I recalled wrong then :) Yes, the C* connector needs to know if the major version is 2 or 3 in order to make some queries slightly differently... Well, let's say both variables are due for a rename then!

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 1, 2018

Member

(Outside of the scope of this PR, of course)

local connector = require "kong.db.strategies.cassandra.connector"


describe("kong.db [#postgres] connector", function()

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Sep 28, 2018

Member

Wrong tag here, I think you meant #cassandra

Show resolved Hide resolved spec/01-unit/000-new-dao/06-postgres_spec.lua

@bungle bungle force-pushed the feat/pg-timeout branch from 6dc0229 to 7dcf5ed Sep 29, 2018

@thibaultcha

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

Merged!

@thibaultcha thibaultcha closed this Oct 1, 2018

@thibaultcha thibaultcha deleted the feat/pg-timeout branch Oct 1, 2018

thibaultcha added a commit that referenced this pull request Oct 1, 2018

thibaultcha added a commit that referenced this pull request Oct 1, 2018

feat(conf) add 'pg_timeout' property
From #3808

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha added a commit that referenced this pull request Oct 1, 2018

feat(core) use 'pg_timeout' as router rebuild semaphore timeout
Complementary commit for:

* 11d69be feat(conf) add 'pg_timeout' property
* 85946ba perf(router) wrap router rebuilds in a worker mutex

We now use the configured timeout for PostgreSQL as the timeout to our
router rebuilt semaphore. Note that the default value of `60` is
preserved to be friendly to any fork out there that may support other
configuration backends.

See #3794
See #3808

thibaultcha added a commit that referenced this pull request Oct 1, 2018

feat(core) use 'pg_timeout' as router rebuild semaphore timeout
Complementary commit for:

* 11d69be feat(conf) add 'pg_timeout' property
* 85946ba perf(router) wrap router rebuilds in a worker mutex

We now use the configured timeout for PostgreSQL as the timeout to our
router rebuild semaphore. Note that the default value of `60` is
preserved to be friendly to any fork out there that may support other
configuration backends.

See #3794
See #3808

thibaultcha added a commit that referenced this pull request Oct 1, 2018

feat(core) use 'pg_timeout' as router rebuild semaphore timeout
Complementary commit for:

* 11d69be feat(conf) add 'pg_timeout' property
* 85946ba perf(router) wrap router rebuilds in a worker mutex

We now use the configured timeout for PostgreSQL as the timeout to our
router rebuild semaphore. Note that the default value of `60` is
preserved to be friendly to any fork out there that may support other
configuration backends.

See #3794
See #3808 
From #3820
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.