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

switch postgresql plugin driver - fixes poor handling of system column types. #1617

Conversation

james-lawrence
Copy link
Contributor

currently lib/pq doesn't handle name and oid types consistently making adding custom queries for other tables difficult. switching the driver resolves the issue.

I also just copied the helper code from lib/pq it was a dependency whose logic I didn't wish to change for this pr.

@james-lawrence james-lawrence force-pushed the handle-postgres-name-type-cleanly branch from 5c1fe87 to bccef14 Compare August 11, 2016 23:13
@sparrc
Copy link
Contributor

sparrc commented Sep 5, 2016

@menardorama could you review?

@james-lawrence
Copy link
Contributor Author

Just fyi, might be able to get away with just the pq driver, seems they fixed at least one of the datatypes that was a blocker.

@james-lawrence
Copy link
Contributor Author

I just havent had the time to check

@sparrc
Copy link
Contributor

sparrc commented Sep 5, 2016

@james-lawrence I would strongly prefer to keep the pq driver, does it only require updating the dependency?

@james-lawrence
Copy link
Contributor Author

Depends. Not sure, assuming updating the dependency does resolve all the unsupported types then could remove some of the utility functions i had to add both for postgresql and the tests. But we'd want to keep the removal of the special cased columns.

@james-lawrence
Copy link
Contributor Author

Willing to check next weekend.

@sparrc
Copy link
Contributor

sparrc commented Sep 5, 2016

OK, I'm going to close the PR for the time being then while keeping the pq library is reviewed, feel free to re-open a new PR whenever you need

@sparrc sparrc closed this Sep 5, 2016
@james-lawrence
Copy link
Contributor Author

james-lawrence commented Sep 10, 2016

@sparrc did some investigating this morning.

so libpq still doesn't handle datname properly. (its still returning a byte array instead of a string by default) but it does properly handle the column if you cast it in the query. (which it didn't in the past)
eg)
with libpq:

SELECT datid, datname::text, numbackends, xact_commit, xact_rollback, blks_read, blks_hit, tup_returned, tup_fetched, tup_inserted, tup_updated, tup_deleted, conflicts, temp_files, temp_bytes, deadlocks, blk_read_time, blk_write_time FROM pg_stat_database

with pgx:

SELECT * FROM pg_stat_database

pgx just handles datname and other special postgresql typed columns correctly which makes for a much nicer experience on the end user side.

I think its worth using pgx.

@sparrc
Copy link
Contributor

sparrc commented Sep 10, 2016

@james-lawrence is this is a bug in the pq library? Have you looked into submitting a PR to fix this behavior in the pq library?

@james-lawrence
Copy link
Contributor Author

james-lawrence commented Sep 10, 2016

I consider it a bug unsure if the maintainers of pq will. I'm not interested in putting in the time with pq to see if it will get fixed, in the past I've had trouble getting them to make changes that could impact end users codes. changing the return type for a postgresql column type would fall into that category.

@james-lawrence
Copy link
Contributor Author

james-lawrence commented Sep 10, 2016

examples of pq unwilling to change returned types:
lib/pq#300
but feel free to report it to them and try to get them to change it =)

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.

2 participants