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

rust test changes #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

kevinkarch88
Copy link

@kevinkarch88 kevinkarch88 commented Aug 22, 2023

I added one test that does the basic SQL functions: Create a table, insert data, update it, and delete.

The main obstacle so far has been prepared statements. It would take a lot of work to edit the driver and get them functional. Postgres uses $1, $2, etc for parameters while Vertica uses a ‘?’, which is why the query_prepared() test fails.

There were some small changes I had to make in a lot of the tests you can see in the diff. Some include: Vertica uses INT8 (64bit) instead of INT4 (32bit) for INT type, temporary tables can’t have SERIAL PRIMARY KEY, can’t insert DEFAULT values (which is NULL for INT).

Some tests were ignored because they have Postgres functionality that Vertica doesn’t have. These are:
Transaction_drop_immediate_rollback - no ON CONFLICT in Vertica
Binary_copy_in - no BINARY option for COPY
Copy_out and binary_copy_out -no COPY to STDOUT
Portal - I changed this test to get it to pass but it could be ignored. Portals work by processing data in chunks, while Vertica gets all the data at once. The test was changed to reflect that.
Notifications_iter, notifications_blocking_iter, and notifications_timeout_iter - no LISTEN

The COPY commands work with a little tweaking. Postgres uses the TEXT type and the UDx creates an alias for it with CREATE TYPE IF NOT EXISTS pg_catalog.text(x=varchar);
However, when trying to copy with TEXT type, I get an error that user created types are not allowed in COPY commands. Something to consider in the future.

Further, with COPY, there is no COPY FROM LOCAL so with a text file, you just have to feed it into STDIN.

The portal() test I had to change from a multi-row insert statement to separate insert statements. I noticed in the ADO.NET driver there’s nowhere that does multi-row inserts. Do we know why this doesn’t work?

TLS is not provided by the driver itself but external libraries postgres-openssl and postgres-native tls. The connection string can have the parameter “sslmode” with three options: disable, prefer, and require.

There’s likely other functionality that exists in Vertica and Postgres that we want to make sure works. If you have any idea what I should add tests for, let me know.

Edit: If you want to try out this branch for yourself, make sure you run the 'install.sql' script in server\udx\supported\pgcompat\ddl.
Here are some useful links for understanding pgcompt and rust-postgres:
https://confluence.verticacorp.com/pages/viewpage.action?spaceKey=~akalinin&title=PG+ODBC+Investigation
https://confluence.verticacorp.com/pages/viewpage.action?spaceKey=~jslaunwhite&title=Potential+Paths+forward+for+Postgres+Compatibility
https://confluence.verticacorp.com/pages/viewpage.action?spaceKey=DEV&title=Comparison+of+Vertica+and+Postgres+Protocols
https://confluence.verticacorp.com/pages/viewpage.action?spaceKey=~akalinin&title=Postgres+Compatibility%3A+What+we+have+got+here+is+failure+to+communicate

.batch_execute(
"CREATE TEMPORARY TABLE foo(
id INT,
name TEXT,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to replace TEXT with VARCHAR here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the postgres UDx that Alex is working on makes VARCHAR an alias for TEXT, so this works. You can find the UDx in server/udx/supported/pgcompat. I could not get the generateseries() function in it to work, but I ran the 'install.sql' script in the 'ddl' folder.

assert_eq!(rows.len(), 2);
assert_eq!(rows[0].get::<_, i32>(0), 1);
assert_eq!(rows[1].get::<_, i32>(0), 2);
let rows = transaction.query_portal(&portal, 3).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the meaning of parameters in transaction.query_portal()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The integer is the number of rows. In postgres, a portal can return fewer than the whole set of rows but in Vertica we can only query all at once. So really, it's kind of pointless here.

assert_eq!(rows.len(), 3);
assert_eq!(rows[0].get::<_, i64>(0), 1);
assert_eq!(rows[1].get::<_, i64>(0), 2);
assert_eq!(rows[1].get::<_, i64>(0), 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this line be the following?

Suggested change
assert_eq!(rows[1].get::<_, i64>(0), 2);
assert_eq!(rows[2].get::<_, i64>(0), 3);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll fix that, thanks.

assert_eq!(rows.len(), 1);
assert_eq!(rows[0].get::<_, i32>(0), 3);
assert_eq!(rows.len(), 3);
assert_eq!(rows[0].get::<_, i64>(0), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the second call to transaction.query_portal(&portal, 2).unwrap() return 0 row?

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.

5 participants