-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
switch postgresql drivers to support more column types out of the box. #1750
switch postgresql drivers to support more column types out of the box. #1750
Conversation
@@ -40,8 +40,12 @@ should now look like: | |||
evaluated at every flush interval, rather than once at startup. This makes it | |||
consistent with the behavior of `collection_jitter`. | |||
|
|||
- postgresql plugins switched drivers to better handle oid and name data types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit of an implementation detail that doesn't need to be part of the changelog. What does this mean for the end-user of the postgres plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I should change the wording to actually allow oid/name types =)
what does this change do for the postgres plugin user? can you update the readme(s)? |
60d12c0
to
85dda55
Compare
will look into the failures tomorrow night. |
I'll fix the conflicts saturday, any timeline for this getting merged into mainline? |
I'll set this to the 1.2 milestone, though I can't guarantee we'll have time to get it reviewed in time. |
@james-lawrence can you describe how you've gone about testing change? how have you verified that this won't break current users of the postgres & postgres_extensible plugins? |
Honestly I havent done a full on regression test.
I will do a smoke test when I fix the merge conflict of running a configuration using master and then upgrading to this version. |
I'll fix the conflict this weekend. |
36d0ccf
to
5a370dc
Compare
I compared the output of master and this branch only 1 line changed and it looks to be a simple reordering of the columns:
here they are reordered, they match exactly:
This was the configuration I used for both master and this branch.
|
that handles them properly
5a370dc
to
6fa2daf
Compare
thanks for testing that out @james-lawrence, I'll merge this now and hopefully give it some soak time before the 1.3 release |
…nfluxdata#1750) that handles them properly
…nfluxdata#1750) that handles them properly
Can we connect to redshift using the postgresql_extensible plugin? |
…1750) that handles them properly
previous pr/discussion: #1617
currently lib/pq doesn't handle name, oid types, and I suspect other builtin postgersql types consistently making adding custom queries for some tables difficult.
It is doubtful pq will change the current output of these types to be friendlier to the end user as the maintainer is pretty hardline about not changing behaviour, even when its for the better.
switching to pgx allows support for all the currently ignored columns except stats_reset, and that is a problem with writing output not the db driver. it likely could be fixed in a follow up pr.
copied the ParseURL from lib/pq as it was a dependency whose logic I didn't wish to change, but at the same time didn't want a dependency on the driver just for that function.