Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

@magnitudo
Copy link

Hi!
I've make some improoves to Npgsql.

First and second commits are universal and works good.

Third may break backward compatibility. But it's correct for actual versions of PostgreSQL. If you make me advice how to do this more correct I can rewrite my code.

@franciscojunior
Copy link
Member

Hi, Andrey!

Thank you very much for your patches. I'll apply them to our cvs repository and then I'll merge it to git.

Can you provide a more detailed explanation of the fix number 1 so I can add in the changelog and in the release notes?

About the fix number 3, I think we should provide a connection string switch to enable the developer to use the current behavior. This way I think people who can't change their programs to adapt to new quoting rules still can use newer Npgsql versions.

@franciscojunior
Copy link
Member

I just sent a mail to Josh Cooley. He is the developer who coded Entity Framework support so he can also have a look and review it.

@magnitudo
Copy link
Author

Hi. It's a bit hard to provide extended explain for 1. I think it's the bug. I've try to find UnitTests for EntityFramework support, but I can't understand how to run it. Can you help me? Then I can totally check my fix in correct way.

Backslash quoting problem is more complex. My fix breaks executing of test Bug1010557BackslashGetDoubled and InsertStringWithBackslashes in CommandsTests.
This happens bacause at performing direct queries all strings with slashes are marked with E. But with EntityFramework it not be done.

@magnitudo
Copy link
Author

I want to explore slash problem more closer. I need to know how PostgreSQL return slashes from server to client: there can I find this in Npgsql code.

@magnitudo magnitudo closed this Jan 20, 2013
@magnitudo magnitudo reopened this Jan 20, 2013
@franciscojunior
Copy link
Member

I can help you run the test suite we have for Npgsql. What steps have you already done to set up Npgsql nunit tests?

Because of this backward compatibility problems I suggest we create a connection string parameter to control this backslash behavior. This way, clients expecting previous behavior would only need to change the connection string.

@franciscojunior
Copy link
Member

Npgsql receive data from the server here: https://github.com/franciscojunior/Npgsql2/blob/master/src/Npgsql/NpgsqlState.cs#L828

Another tool you can use to inspect postgresql data transmission is WireShark, (previous know as Ethereal). It already recognizes postgresql protocol and it very useful. I hope it helps.

@magnitudo
Copy link
Author

I run all test from NpgsqlTests2010 projects, but this solutions doesn't contain any EntityFramework test. Also this solution contains xmlModel folder wich might be a base for EF tests, but I don't understand how it's been used.

I suggest that Npgsql already contains such connetion string parameter NpgsqlConnector.CheckStringConformanceRequirements() but it's uncomplete proccessed at every part of the code. I'll try to discover entire logic and create correct fix.

Thanks for advice about code and WireShark.

@franciscojunior
Copy link
Member

Hmmm, these files were created by Josh Cooley but I think they aren't automated tests. I'll check with him how to run those tests.

@franciscojunior
Copy link
Member

Hi, Andrey.

About your backslash patch, I also received another report of problems with backslashes when running Npgsql with entity framework. http://pgfoundry.org/forum/forum.php?thread_id=15533&forum_id=518&group_id=1000140

I think we should check how to get the preceding 'E' in front of queries created by Npgsql entity framework support. This way we wouldn't need to change current code which already provides support for current and previous postgresql.

What do you think?

@magnitudo
Copy link
Author

Hi.
I think it's a nice idea.

@magnitudo
Copy link
Author

Hi.
Look at last commit.
We explore backslash bug and fix it. Fix is quite simple but work good in all use cases.
We also add test to check it.

@franciscojunior
Copy link
Member

That's great, Andrey! I'll commit it to cvs and let you know.

@magnitudo
Copy link
Author

Agree

@franciscojunior
Copy link
Member

Great! Will commit this revised version to cvs as soon as I get back home. Here in my job I can't access cvs. :(

@franciscojunior
Copy link
Member

Hi, while preparing to commit, I notice a little problem: Npgsql supports postgresql since 7.x and this version doesn't support the E'string' syntax. We would need to check how to send the information if the E'string' syntax is supported or not so that the
Today Npgsql has a property in the NpgsqlConnector which says if it is supported or not. It is called NpgsqlConnector.CheckStringConformanceRequirements() It is used by the NpgsqlCommand when generating the sql from parameters. I still couldn't figure out a way to send this information.

@franciscojunior
Copy link
Member

I think we would need to make a better handling of those strings. I think we need to change the ConvertToBackend method which would be used by both EF queries and NpgsqlCommand queries.

@franciscojunior
Copy link
Member

I'm still trying to figure out a way to handle that. When doing the output from EF in the ConstantExpression class, there is no connection/context information available. Or at least, I couldn't find it.

I sent a mail to Josh Cooley who coded this part asking him if he has any idea about how to handle that.

@franciscojunior
Copy link
Member

Yesterday I was playing with this issue and I think we could use some methods of NpgsqlServices class to get the information about the ConformantStrings so the writeSql can create the string literal with the proper escape syntax.

Inside NpgsqlServices there are the methods called GetDbProviderManifestToken and GetDbProviderManifest. I think we could use them to retrieve the status of the ConformantStrings and pass down the chain of sql generators in the TranslateCommandTree method.

@franciscojunior
Copy link
Member

After getting feedback from Josh, I think we can try the following idea:

The current code already gets the server version. With this information, we can do the following: For server versions 7.x we write the string without the escape syntax. With versions greater than 8.0, we write the string with the escape syntax (E'string value').

What do you think?

@franciscojunior
Copy link
Member

After asking more questions to Josh. he told me there are a lot of metadata information used by EF that he isn't sure if they are available to Npgsql in the Postgresql 7.x. We would need to do more tests. For while, I'm applying your patch which adds the escape syntax to EF code as, currently, EF support is only available in postgresql 8.x. As escape syntax is also supported on postgresql versions 8.x and above, we can say that this problem right now is fixed. If we find a way to support EF with 7.x, we will add a condition to this escape syntax.

@FransBouma
Copy link
Contributor

If it's fixed it might be good to close this PR, so it doesn't look like the project won't merge PRs ;)

@franciscojunior
Copy link
Member

Agreed! I'll check the current state of those patches and write them here and will close this PR if everything is already applied.

Thanks for the heads up, @FransBouma !

@roji roji force-pushed the master branch 2 times, most recently from 8bcb1ca to 1bed81a Compare February 28, 2015 17:11
@roji roji force-pushed the master branch 4 times, most recently from 1236d1f to e8ae2e0 Compare April 12, 2015 07:50
@roji roji force-pushed the master branch 12 times, most recently from 2fc8944 to 3eb6fa4 Compare April 29, 2015 14:47
@roji roji force-pushed the master branch 4 times, most recently from c3f6238 to e680736 Compare May 2, 2015 12:56
@roji
Copy link
Member

roji commented Aug 9, 2015

@Emill, @franciscojunior, can any of you take a look if this PR contains salvageable stuff for 3.0? Otherwise I'll close it...

@Emill Emill closed this Aug 9, 2015
roji pushed a commit to roji/Npgsql that referenced this pull request Jul 18, 2023
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.

6 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.