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

DBZ-558 Return correct array OID #412

Closed
wants to merge 1 commit into from
Closed

Conversation

jpechane
Copy link
Contributor

@rcoup
Copy link
Contributor

rcoup commented Jan 22, 2018

Seems like jdbcColumnToOid() always returned component types when passed an OID=ARRAY. But now it depends on whether jdbcType=ARRAY and typeName=null? Maybe it should be split into two functions?

Array element types are in pg_type, as are links to the array-version of a regular type. Is there a reason we don't always have that info?

@jpechane
Copy link
Contributor Author

I think I've faound the root cause - we are identifying array based on _ prefix. But geometry array type is named "postgis"."_geometry" thus all code that depends on _ is wrong.
Try grep -r 'substring(1' and grep -r '"_"' to see affected parts of code.
It is necessary to extract array type detection and handling into a separate class and extend it with support of postgis types. The class should be able to detect whether the type is array and get element type for array too.

@rcoup
Copy link
Contributor

rcoup commented Jan 23, 2018

For the unit tests, the Geometry type happens to be "postgis"."_geometry" because I installed it into the postgis schema I created. In a different DB it will be somewhere else (or in public, so unprefixed). But there's only one actual _geometry type per database, AFAIK you can't have postgis._geometry and other._geometry.

Though we shouldn't be relying on the _ prefix either ideally:

Before PostgreSQL version 8.3, the name of a generated array type was always exactly the element type's name with one underscore character (_) prepended. (Type names were therefore restricted in length to one less character than other names.) While this is still usually the case, the array type name may vary from this in case of maximum-length names or collisions with user type names that begin with underscore. Writing code that depends on this convention is therefore deprecated. Instead, use pg_type.typarray to locate the array type associated with a given type.

It may be advisable to avoid using type and table names that begin with underscore. While the server will change generated array type names to avoid collisions with user-given names, there is still risk of confusion, particularly with old client software that may assume that type names beginning with underscores always represent arrays.

@gunnarmorling
Copy link
Member

@rcoup To double-check, is this PR ok from your POV, once you've disabled support for Geo arrays in your PR?

@rcoup
Copy link
Contributor

rcoup commented Jan 23, 2018

Yes. I’ll attempt to get to it, but feel free make that change if you want it quicker

@gunnarmorling
Copy link
Member

Thanks for confirming, @rcoup. I'd prefer to leave the Geo PR to you :) We can push out the release to Thursday, if that helps.

@gunnarmorling
Copy link
Member

gunnarmorling commented Jan 23, 2018

Hey @jpechane, this still gives me one test failure with wal2json:

RecordsStreamProducerIT.shouldReceiveChangesForTypeConstraints:363

Is this passing for you?

@jpechane
Copy link
Contributor Author

You have an old version on Postgres image with old plugin probably

@gunnarmorling
Copy link
Member

gunnarmorling commented Jan 23, 2018 via email

@gunnarmorling
Copy link
Member

Rebased and applied. Thanks, @jpechane!

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