refactor: Replace lib/pq with pgx in a PostgreSQL scaler #4705
Conversation
|
Thank you for your contribution! 🙏 We will review your PR as soon as possible. While you are waiting, make sure to:
Learn more about: |
a-narsudinov
changed the title
a-narsudinov
force-pushed
the
refactor/pg-scaler/replace-libpq
branch
from
beffda5
to
6bc73e4
Compare
There was a problem hiding this comment.
@a-narsudinov nice, thanks for the contribution. Could you please add a test to cover the multihost scenario which is discussed in the issue?
|
@zroubalik I believe I can add test for this scenario, but could you please help me figure out how to implement it better? I guess the proper way to test it is to modify the current end-to-end |
Yeah, that's what I was thinking about https://github.com/kedacore/keda/blob/main/tests/scalers/postgresql/postgresql_test.go Also, do you think it makes sense to add unit test for parsing the multihost setup? |
|
To be honest, I didn't even test the case when we parse the data from separate fields and use it to build a connection string via strings joiner. By the way it should work nicely right now, but I suppose we should add a unit test for this scenario just so we won't forget about this very nice feature during the next refactoring and won't break it. This is not so much work compared to the e2e test, so we just may do it, even if it isn't essential now. |
|
I'm really interested in merging this PR into the mainline until the upcoming release, how much time do I have to do the tests? :D |
😄 We plan to do release later this week (Wed-Thu). The best would be to include the test by Tuesday EOD. |
a-narsudinov
force-pushed
the
refactor/pg-scaler/replace-libpq
branch
2 times, most recently
from
8c64786
to
3fb4e21
Compare
|
Hi @zroubalik! I think that this PR looks good now with a new tests. Could you please have a look? :) |
|
/run-e2e postgresql |
a-narsudinov
force-pushed
the
refactor/pg-scaler/replace-libpq
branch
from
3fb4e21
to
07d6cf2
Compare
|
I'm sorry, I made a typo in my email and I had to re-push one more time :( |
The development of `lib/pq` is over and the library doesn't receive any fixes or new features. This is sad because nowadays `lib/pq` lacks a of lot of features we are get used to in the C version of `libpq` and its derivatives. I've experienced troubles with this unimplemented stuff in `lib/pq`: - Multi-host connection, which is important for a high availability - Support of parsing CA certificate of PostgreSQL server stored in a `~/.postgresql` folder that led to a negative user experience I believe there are more missing features. These things will be never fixed in `lib/pq`, so in this patch, we replace the abandoned `lib/pq` with an actively developing `pgx`. Signed-off-by: Alexander Narsudinov <[email protected]>
Signed-off-by: Alexander Narsudinov <[email protected]>
We are going to introduce a new test for HA PostgreSQL, so this patch moves all reusable code from the test code to the helper. In the next patch we'll use this helper to build a new test case. Signed-off-by: Alexander Narsudinov <[email protected]>
This patch allows to check if the KEDA operator able to use multihost connection to PostgreSQL HA setup. Signed-off-by: Alexander Narsudinov <[email protected]>
Signed-off-by: Alexander Narsudinov <[email protected]>
a-narsudinov
force-pushed
the
refactor/pg-scaler/replace-libpq
branch
from
07d6cf2
to
3749348
Compare
|
/run-e2e postgresql |
Signed-off-by: Alexander Narsudinov <[email protected]>
The development of
lib/pqis over and the library doesn't receive any fixes or new features. This is sad because nowadayslib/pqlacks a of lot of features we are get used to in the C version oflibpqand its derivatives.I've experienced troubles with this unimplemented stuff in
lib/pq:~/.postgresqlfolder that led to a negative user experienceI believe there are more missing features.
These things will be never fixed in
lib/pq, so in this patch, we replace the abandonedlib/pqwith an actively developingpgx.I tested this patch in my environment and got that I needed: high avaliabillity with help of multihost connection which just works in
pgxdriver.I think that it mustn't have a breaking changes and may be merged easily.
Checklist
Fixes #4704