Git Product home page Git Product logo

Comments (69)

franciscojunior avatar franciscojunior commented on June 8, 2024

When this error occurs, are you able to continue connecting and use the server ?

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

We need better handling of those cases.
Until now, the only reported problem was about the lc_monetary. Maybe we need to find another way to handle monetary values instead of relying on changing runtime configuration value.

Stackoverflow also has a question about that: http://stackoverflow.com/questions/20969365/permission-denied-to-set-parameter-lc-monetary-to-c

from npgsql.

griffoff avatar griffoff commented on June 8, 2024

No. The error stops the connection as it throws an error within the open method.

from npgsql.

griffoff avatar griffoff commented on June 8, 2024

I agree. An inability to set a parameter should not stop the connection.
But also this is an expected parameter in postgres so it is entirely
reasonable to expect it to be there.

I do appreciate that Redshift is an edge case and could potentially become
a branch to the driver rather than being accommodated in a over size fits
all driver.
On 12 Feb 2014 20:54, "Francisco Figueiredo Jr." [email protected]
wrote:

We need better handling of those cases.
Until now, the only reported problem was about the lc_monetary. Maybe we
need to find another way to handle monetary values instead of relying on
changing runtime configuration value.

Stackoverflow also has a question about that:
http://stackoverflow.com/questions/20969365/permission-denied-to-set-parameter-lc-monetary-to-c

Reply to this email directly or view it on GitHubhttps://github.com//issues/163#issuecomment-34916622
.

from npgsql.

mateusaubin avatar mateusaubin commented on June 8, 2024

I wrote some custom code to avoid two issues with Redshift. You can see it on my fork.

I could create a pull request if you guys find it usefull.

The issues I've found were:

  • LC_MONETARY;
  • varchar fields prefixed with E (i.e: E'string');

from npgsql.

khalidsalomao avatar khalidsalomao commented on June 8, 2024

This issue was introduced in the versions +2.1.
Last version that worked fine is 2.0.14.3.

from npgsql.

Heucles avatar Heucles commented on June 8, 2024

Thanks @khalidsalomao for the tip. I am looking forward to have this bug fixed. Regardless, @franciscojunior great job with the lib, I'm starting to use, but I can already consider myself a big fan.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Thank you for your feedback, @Heucles ! We are working in a series of changes in Npgsql. I hope Amazon create a new version of Redshift based on Postgresql 9.x. This would ease very much our lives :)

from npgsql.

roji avatar roji commented on June 8, 2024

@franciscojunior, so if I understand correctly, RedShift is based on Postgresql 8.0.2, which doesn't yet support the LC_MONETARY parameter, right?

8.0.2 is a truly ancient version: released in 2005 and reached EOL in 2010. I'm really surprised Amazon based Redshift on it, and a quick search doesn't show any plans on supporting newer Postgresql features. Npgsql supports the same versions Postgresql officially supports, so 9.0 and up. However, since Redshift is an important service it's tempting to try and maintain support if it's not too hard.

Are we aware of any other Npgsql issues with Redshift apart from LC_MONETARY? If that's all we can definitely try to check for Redshift (or simply the Postgresql backend version) and not send this parameter on startup.

@franciscojunior, what do you think? If so I will also undo the legacy cleanup I performed in #329.

from npgsql.

khalidsalomao avatar khalidsalomao commented on June 8, 2024

Hi Guys,
I have been using Redshift for the last 6 months and the version before the
legacy cleanup works fine!

On Saturday, August 30, 2014, Shay Rojansky [email protected]
wrote:

@franciscojunior https://github.com/franciscojunior, so if I understand
correctly, RedShift is (based on Postgresql 8.0.2)[
http://docs.aws.amazon.com/redshift/latest/dg/c_redshift-and-postgres-sql.html],
which doesn't yet support the LC_MONETARY parameter, right?

8.0.2 is a truly ancient version: released in 2005 and reached EOL in
2010. I'm really surprised Amazon based Redshift on it, and a quick search
doesn't show any plans on supporting newer Postgresql features. Npgsql
supports the same versions Postgresql officially supports, so 9.0 and up.
However, since Redshift is an important service it's tempting to try and
maintain support if it's not too hard.

Are we aware of any other Npgsql issues with Redshift apart from
LC_MONETARY? If that's all we can definitely try to check for Redshift (or
simply the Postgresql backend version) and not send this parameter on
startup.

@franciscojunior https://github.com/franciscojunior, what do you think?
If so I will also undo the legacy cleanup I performed in #329
#329.


Reply to this email directly or view it on GitHub
#163 (comment).

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Hi, @khalidsalomao . Which version before the legacy cleanup is working for you? Are you talking about the 2.0.14, 2.1.x or some custom built from git?

Are we aware of any other Npgsql issues with Redshift apart from LC_MONETARY? If that's all we can definitely try to check for Redshift (or simply the Postgresql backend version) and not send this parameter on startup.

I think there is the E' ' construction support which redshift doesn't support, according to @mateusaubin

from npgsql.

roji avatar roji commented on June 8, 2024

@franciscojunior, I think we already have a check for E' ' compatibility, I hope it means everything is OK with Redshift.

Like you say, once @khalidsalomao confirms which version was the last to work well we can check what happened. If it's only a matter of adding a version condition around the LC_MONETARY startup parameter, it's pretty trivial.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

@franciscojunior, I think we already have a check for E' ' compatibility, I hope it means everything is OK with Redshift.

Whooops, you are right! I was basing my comment in the @mateusaubin 's fork. But the code has already been modified. Thanks for the heads up! :)

Like you say, once @khalidsalomao confirms which version was the last to work well we can check what happened. If it's only a matter of adding a version condition around the LC_MONETARY startup parameter, it's pretty trivial.

Yes. But we will need to remove its initialization from the startup package because the startup package is sent before any query can be made.

Another option is to study a way of handling monetary type without needing to change the lc_monetary startup parameter. This change is similar to the extra_float_digits. I use it in order to get a consistent result from the server regarding negative values and decimal point. I noticed that in some locales, the negative monetary value is returned inside parenthesis while in others, it is returned with a leading minus signal.

from npgsql.

khalidsalomao avatar khalidsalomao commented on June 8, 2024

@franciscojunior the version that we're using is the 2.0.14. We've been
using it heavily and so far it has been working fine.

On Sat, Aug 30, 2014 at 3:27 PM, Francisco Figueiredo Jr. <
[email protected]> wrote:

@franciscojunior https://github.com/franciscojunior, I think we already
have a check for E' ' compatibility, I hope it means everything is OK with
Redshift.

Whooops, you are right! I was basing my comment in the @mateusaubin
https://github.com/mateusaubin 's fork. But the code has already been
modified. Thanks for the heads up! :)

Like you say, once @khalidsalomao https://github.com/khalidsalomao
confirms which version was the last to work well we can check what
happened. If it's only a matter of adding a version condition around the
LC_MONETARY startup parameter, it's pretty trivial.

Yes. But we will need to remove its initialization from the startup
package because the startup package is sent before any query can be made.

Another option is to study a way of handling monetary type without needing
to change the lc_monetary startup parameter. This change is similar to the
extra_float_digits. I use it in order to get a consistent result from the
server regarding negative values and decimal point. I noticed that in some
locales, the negative monetary value is returned inside parenthesis while
in others, it is returned with a leading minus signal.


Reply to this email directly or view it on GitHub
#163 (comment).

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

@franciscojunior the version that we're using is the 2.0.14. We've been using it heavily and so far it has been working fine.

Thanks for the information, @khalidsalomao ! We will check how to make it work again on latest versions.

from npgsql.

InsidiousForce avatar InsidiousForce commented on June 8, 2024

More people using redshift I guess. The version I pulled from nuget today has this issue still. (2.2.1) - solution is to still manually patch this? thanks

from npgsql.

roji avatar roji commented on June 8, 2024

Yes, 2.2.1 still has this issue.

We do intend to fix this, but since it introduces breakage it probably won't happen before 3.0...

from npgsql.

InsidiousForce avatar InsidiousForce commented on June 8, 2024

Thanks. mateusaubin's fork is working well for now

from npgsql.

jeffgabhart avatar jeffgabhart commented on June 8, 2024

I get the same error connecting through pg_bouncer, but it sounds like I can handle it through ignore_startup_parameters configuration based on @franciscojunior post

from npgsql.

roji avatar roji commented on June 8, 2024

Thanks for letting us know. I'm still planning to get around to this in Npgsql 3.0 but there's a lot of work to be done there.

from npgsql.

roji avatar roji commented on June 8, 2024

With the move to full-binary in version 3.0, this bug will naturally go away, no more need to set lc_monetary.

Being such a major version it'll still be some time before we release it, hang in there.

from npgsql.

roji avatar roji commented on June 8, 2024

Can anyone out there with an Amazon Redshift instance help us test the workaround for the LC_MONETARY issue in #476? We'd like to release it soon to make everyone's life simpler but need testing...

from npgsql.

khalidsalomao avatar khalidsalomao commented on June 8, 2024

Just tested but now there is another error whenever a connection is open.

ERROR: 42704: unrecognized configuration parameter "ssl_renegotiation_limit"

at Npgsql.NpgsqlConnector.DoReadSingleMessage(DataRowLoadingMode dataRowLoadingMode, Boolean ignoreNotifications) in ...\npgsql\Npgsql\NpgsqlConnector.cs:line 770
at Npgsql.NpgsqlConnector.ReadSingleMessage(DataRowLoadingMode dataRowLoadingMode, Boolean ignoreNotifications) in ...\npgsql\Npgsql\NpgsqlConnector.cs:line 733
at Npgsql.NpgsqlConnector.SkipUntil(BackendMessageCode[] stopAt) in ...\npgsql\Npgsql\NpgsqlConnector.cs:line 880
at Npgsql.NpgsqlConnector.ExecuteBlind(String query) in ...\npgsql\Npgsql\NpgsqlConnector.cs:line 1423
at Npgsql.NpgsqlConnector.Open() in ...\npgsql\Npgsql\NpgsqlConnector.cs:line 401
at Npgsql.NpgsqlConnection.Open() in ...\npgsql\Npgsql\NpgsqlConnection.cs:line 178
at ...

@roji, I tested using the updated master.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Hi, @khalidsalomao !

Thank you very much for your feedback!

Can you post the result of the select version() query from your redshift install? This would help me check how to identify redshift install so we can add support to not use the ssl_renegotiation_limit.

Thanks in advance.

from npgsql.

Emill avatar Emill commented on June 8, 2024

I think we should have a better SSL/TLS implementation that doesn't have all different problems.. such as bad async/sync efficiency, no renegotiation support, no DataAvailable flag, bad cipher suites (no forward secrecy)

from npgsql.

khalidsalomao avatar khalidsalomao commented on June 8, 2024

Hi @franciscojunior ,

Thank you guys for this amazing driver!

select version()
version
PostgreSQL 8.0.2 on i686-pc-linux-gnu, compiled by GCC gcc (GCC) 3.4.2 20041017 (Red Hat 3.4.2-6.fc3), Redshift 1.0.880

from npgsql.

roji avatar roji commented on June 8, 2024

Geez, Redshift is based on an old PostgreSQL...

@Emill, even if you're right we're looking for a quick hack for 2.2.5 rather than a complete solution...

According to the PostgreSQL docs ssl_renegotiation_limit is supported all the way back to 8.1, so it seems OK to simply disable SSL renegotiation for server version < 8.1? But since under mono this will not work (ssl_renegotiation_limit is required), we may want to bomb when SSL is enabled, server version < 8.1 and on mono?

from npgsql.

roji avatar roji commented on June 8, 2024

Interestingly, the PG docs state that ssl_renegotiation_limit is supported even for 8.0...!

I guess this is a Redshift-specific thing? In this case maybe we need to make it depend on the connection-string parameter for now?

For 3.0, where LC_MONETARY isn't sent at all, we can try to detect Redshift when connecting and disable ssl_renegotiation_limit ...

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Interestingly, the PG docs state that ssl_renegotiation_limit is supported even for 8.0...!

I noticed that by checking our code where support for it is configured:

this._supportsSslRenegotiationLimit = ((ServerVersion >= new Version(8, 4, 3)) ||
                     (ServerVersion >= new Version(8, 3, 10) && ServerVersion < new Version(8, 4, 0)) ||
                     (ServerVersion >= new Version(8, 2, 16) && ServerVersion < new Version(8, 3, 0)) ||
                     (ServerVersion >= new Version(8, 1, 20) && ServerVersion < new Version(8, 2, 0)) ||
                     (ServerVersion >= new Version(8, 0, 24) && ServerVersion < new Version(8, 1, 0)) ||
                     (ServerVersion >= new Version(7, 4, 28) && ServerVersion < new Version(8, 0, 0)) );

If I recall correctly, they backported it to give options to workaround bugs in the ssl host library.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

I guess this is a Redshift-specific thing? In this case maybe we need to make it depend on the connection-string parameter for now?

Agreed!

I'm working on it right now.

from npgsql.

roji avatar roji commented on June 8, 2024

@franciscojunior, but then our check should understand that SSL renegotiation isn't supported (because it's 8.0.2, which is below 8.0.24)? Do we have a bug there somewhere?

Regardless, I think we should add a check that bombs if we're on mono, SSL renegotation is not supported (based on the server version), and the connection is SSL... Otherwise people will have mysteriously failing connections later.

Finally, one last thought: may we can simply send LC_MONETARY a bit later, once we know which server version we're connected to? This would remove the need for a special redshift connection parameter? What do you think?

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

@franciscojunior, but then our check should understand that SSL renegotiation isn't supported (because it's 8.0.2, which is below 8.0.24)? Do we have a bug there somewhere?

I also noticed that too. I'll check that.

Regardless, I think we should add a check that bombs if we're on mono, SSL renegotation is not supported (based on the server version), and the connection is SSL... Otherwise people will have mysteriously failing connections later.

Agreed. But based on the fact that we don't receive reports about this failing, I think very few people use this setup combination.

Finally, one last thought: may we can simply send LC_MONETARY a bit later, once we know which server version we're connected to? This would remove the need for a special redshift connection parameter? What do you think?

It used to be this way but we received a patch which moved a lot of parameters to startup packet in order to avoid needing to reset it every time the connection was returned from the pool.
Based on the fact that we may face more issues like that where we would like to enable/disable features based on the host we are connecting to, I think we should go back to use this approach. There is even support for it already in the form of the initQueries which today is used only to the ssl_renegotiation_limit. This query is executed whenever the connection is returned from the pool, after the connection is reset with the discard all.

from npgsql.

roji avatar roji commented on June 8, 2024

OK. Don't forget this is mostly temporary - in v3.0 there is no more LC_MONETARY at all.

In 2.2, if we can move the LC_MONETARY to the initQueries and support Redshift with no parameter, I think that's the ideal solution. This way nobody has to change their connection strings etc...

Let me know if you want me to help implement @franciscojunior.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

In 2.2, if we can move the LC_MONETARY to the initQueries and support Redshift with no parameter, I think that's the ideal solution. This way nobody has to change their connection strings etc...

Ok. I'll take care of that.

Let me know if you want me to help implement @franciscojunior.

Thank you very much, Shay! I'll let you know.
I also don't want to bother you too much as you already are working in a lot of stuff in the master branch :)

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

@franciscojunior, but then our check should understand that SSL renegotiation isn't supported (because it's 8.0.2, which is below 8.0.24)? Do we have a bug there somewhere?

Found it.

@khalidsalomao said he was using master branch. In the master branch we have the following:
https://github.com/npgsql/npgsql/blob/master/Npgsql/NpgsqlConnector.cs#L397

// Some connection parameters for protocol 3 had been sent in the startup packet.
// The rest will be setted here.
sbInitQueries.WriteLine("SET ssl_renegotiation_limit=0;");

There is no checking for the SupportsSslRenegotiationLimit as we have in the 2.2 branch:

if (SupportsSslRenegotiationLimit)
            {
                sbInitQueries.WriteLine("SET ssl_renegotiation_limit=0;");
            }

@khalidsalomao , would you mind testing Npgsql from the 2.2 branch?

You can get compiled binaries from our build server here: https://build.npgsql.org/viewLog.html?buildId=13960&tab=artifacts&buildTypeId=npgsql_All22

I think that with those builds you won't have problems with ssl_renegotiation_limit.
Also, remember to add amazonredshift=true in your connection string.
I'm working in the support to not need this connection string change.

Thanks in advance.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

In 2.2, if we can move the LC_MONETARY to the initQueries and support Redshift with no parameter, I think that's the ideal solution. This way nobody has to change their connection strings etc...

While doing some tests, I noticed that in order to be able to check if we are connected to redshift, we'll need to send a query select version(). Parameterstatus doesn't give me information other than the server value 8.0.2.

Other than that, it will be a simple patch. We parse the value returned from select version() and if we find the redshift signature, we skip the lc_monetary setting.

What do you think?

from npgsql.

roji avatar roji commented on June 8, 2024

Sounds OK to me...!

from npgsql.

griffoff avatar griffoff commented on June 8, 2024

Hi guys, great work on this driver!

I was wondering if you should consider forking this code specifically for redshift - the evolution path for redshift and PostgreSQL are divergent as far as I know, and it might make it simpler to have a specific redshift version. Also, because it's Ssas there should only ever be one version of redshift available at any one time, meaning that backwards compatibility and multi version support are unnecessary. It could make the whole code base smaller and more efficient for redshift.

Just a thought. Keep up the great work!

from npgsql.

Emill avatar Emill commented on June 8, 2024

We'll see how v3.0 will be compatible with redshift. Many non-compatible things were to the text format we are now not using anymore...

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Sounds OK to me...!

It is done. As I don't have a redshift server, I tested it with the osx fingerprint apple-darwin and it worked ok!

I'll add more commits to remove the code which add support for connection string parameter AmazonRedshift. In fact, I think I'll try the revert support of github. :)

But I think we can create a build right now in the build server so @khalidsalomao can give it a try.

from npgsql.

mateusaubin avatar mateusaubin commented on June 8, 2024

Once the binaries are available I can try them out as well ;)

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Hi guys, great work on this driver!

Hi, @griffoff. Thank you very much. Your words are very rewarding.

I was wondering if you should consider forking this code specifically for redshift - the evolution path for redshift and PostgreSQL are divergent as far as I know, and it might make it simpler to have a specific redshift version. Also, because it's Ssas there should only ever be one version of redshift available at any one time, meaning that backwards compatibility and multi version support are unnecessary. It could make the whole code base smaller and more efficient for redshift. Just a thought. Keep up the great work!

We thought about that in the past. Today the main differences are those startup problems (which we are fixing only for the 2.2 branch as the 3.0 branch won't have this problem) and EntityFramework support which uses a lot of sql features present only in postgresql 9.0 and above. Other than that, Npgsql can be used without many problems with Amazon Redshift.

As @Emill said, I think 3.0 will enable people to use redshift without many hiccups. If we check it isn't enough, I think your idea is the way to go. This way we won't need to maintain a lot of exceptions for redshift.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Once the binaries are available I can try them out as well ;)

I'm building them right now in our build server. :)

from npgsql.

roji avatar roji commented on June 8, 2024

@griffoff, I agree with @Emill and @franciscojunior above - we're currently aware of very little incompatibilities with Redshift. If and when that changes a fork may make sense, but as @Emill said our next major version (3.0) will be even less Redshift-sensitive.

One more note: I've gotten in touch with AWS to see if they can help us out by donating a Redshift instance, and possibly even more. This would allow us to regularly check for Redshift breakage in our continuous integration process.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

One more note: I've gotten in touch with AWS to see if they can help us out by donating a Redshift instance, and possibly even more. This would allow us to regularly check for Redshift breakage in our continuous integration process.

That's excellent news! This will help us very much!

from npgsql.

griffoff avatar griffoff commented on June 8, 2024

I'm all for pragmatism! Your approach makes sense. Thanks for the response.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

I'm building them right now in our build server. :)

We are having some timeout issues when running tests, but I got binaries built:

For .net 4.5:
https://build.npgsql.org/repository/download/npgsql_net45/13984:id/Npgsql/bin/Release-net45/Npgsql.dll

For .net 4.0:
https://build.npgsql.org/repository/download/npgsql_net40/13983:id/Npgsql/bin/Release-net40/Npgsql.dll

For 3.5:
https://build.npgsql.org/repository/download/npgsql_net35/13970:id/Npgsql/bin/Release-net35/Npgsql.dll

For 2.0:
https://build.npgsql.org/repository/download/npgsql_net20/13981:id/Npgsql/bin/Release-net20/Npgsql.dll

Please, give it a try and let me know if you find any issues.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

still getting exceptions on connection.Open() due to LC_MONETARY

Hmmmm, can you post the result of select version(), please?

I'm looking for the redshift fingerprint and if it is found, Npgsql shouldn't send the command.

from npgsql.

mateusaubin avatar mateusaubin commented on June 8, 2024

trying to use "Redshift=True;" in the connectionstring results in a ArgumentException.

the unmodified connectionstring causes a NpgsqlException but still works fine with my forked code.

version is:

PostgreSQL 8.0.2 on i686-pc-linux-gnu, compiled by GCC gcc (GCC) 3.4.2 20041017 (Red Hat 3.4.2-6.fc3), Redshift 1.0.880

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

trying to use "Redshift=True;" in the connectionstring results in a ArgumentException.

the unmodified connectionstring causes a NpgsqlException but still works fine with my forked code.

Hmmmmm, this is strange. Because I didn't remove the Redshift connection string parameter support.
Do you think it is possible that you are using Npgsql from GAC or some other version?

I'll check here to see if I did something wrong in the builds.

from npgsql.

mateusaubin avatar mateusaubin commented on June 8, 2024

There's nothing related to Npgsql in my GAC, I'm referencing the dll directly.
I ran my test using DbProviderFactories but now I've also run them by directly instantiating NpgsqlConnection and got the same results.

Where can I find the code with your patches so I can build it locally?

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

You can find it here:
https://github.com/npgsql/npgsql/tree/2.2

And patch here:
#487

Thanks for testing.

from npgsql.

mateusaubin avatar mateusaubin commented on June 8, 2024

I think i've sorted it out.

the problem was:

  • I hadn't set AmazonRedshift=true;
  • NpgsqlConnector attempts to initialize the state sending a NpgsqlStartpPacket which relies on the connectionstring parameter
  • Exception is throw due to LC_MONETARY being set before even asking the db version and verifying whether it's running redshift or not

also I think that relying on version detection and also defining a connectionstring parameter is a little too much. if the user is explicitly defining a parameter saying it's redshift the library should simply comply.
or the library could completely automate detection through version(), either one or the other way, not both.

I'd prefer the connectionstring parameter and avoid any detection, which could have to change in future versions due to changes in redshift. since redshift is sold like a service and has frequent (and automatic) updates such change would be a major concern for anyone relying on Npgsql.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

also I think that relying on version detection and also defining a connectionstring parameter is a little too much.

Ahh, sorry about that. There are 2 ways of handling the problem of the lc_monetary. One was through the connection string parameter which is already committed in the 2.2 branch. The other one was checking the return value of select version(). I didn't remove the connection string parameter support yet. :( Sorry for not making this clear.

But on the other side, did it work to connect to Amazon redshift ? Did you find any other problem?
Another side effect of this change is that money support is broken in Amazon redshift. Are you using this datatype in your projects?

I'd prefer the connectionstring parameter and avoid any detection, which could have to change in future versions due to changes in redshift. since redshift is sold like a service and has frequent (and automatic) updates such change would be a major concern for anyone relying on Npgsql.

Hmmmm, I see. I think both ways have pro and cons. The connection string parameter has the con of asking users to change the connection string parameter and the dynamic checking has the cons you said also the performance penalty everyone else would be paying.

@roji , what do you think?

from npgsql.

mateusaubin avatar mateusaubin commented on June 8, 2024

But on the other side, did it work to connect to Amazon redshift ? Did you find any other problem?
Another side effect of this change is that money support is broken in Amazon redshift. Are you using this datatype in your projects?

Yes, it worked just fine.
Regarding the money datatype we haven't used it since it is explicitly unsupported by redshift, see Unsupported PostgreSQL Data Types and Data Types.

Attempting to create a table with a money column leads to an error:

ERROR: 0A000: Column "#testtable.lotsof" has unsupported type "money".

from npgsql.

roji avatar roji commented on June 8, 2024

@franciscojunior, I'd implement SupportsSetLcMonetary as a check for server_version >= 8.1.0. Then I'd check this property in NpgsqlConnector.Open() and add LC_MONETARY='C' to sbInitQueries just like we currently do with SupportsExtraFloatDigits3.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

@franciscojunior, I'd implement SupportsSetLcMonetary as a check for server_version >= 8.1.0. Then I'd check this property in NpgsqlConnector.Open() and add LC_MONETARY='C' to sbInitQueries just like we currently do with SupportsExtraFloatDigits3.

Done! Sorry for taking so long.

I just created a pull request with those changes. #489

I'll now revert the change which add the connection string parameter support.

from npgsql.

roji avatar roji commented on June 8, 2024

Great! I've triggered the 2.2. build for your PR, hopefully it doesn't do any more slowdown issues.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Great! I've triggered the 2.2. build for your PR, hopefully it doesn't do any more slowdown issues.

I triggered the net 2.0 build just to make a test and the slowdown doesn't occur there.
But in the 487 it still occurs.
Right now I can see the builds happening in the 4 and a half minutes time frame. If we trigger the 487 pull request build, we can see that it occurs in the 17 minutes timeframe. :(

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

I just reverted the pull request (#477) which added amazon redshift connection string parameter. Github interface worked like a charm! (#490) :)

I'll rebase #489 with this new 2.2.

from npgsql.

roji avatar roji commented on June 8, 2024

Great. I think we can forget about #487 and merge #489, that should do it.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Yes. There is only a missing line in #489 . I'm committing it now....

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Done! Now #489 is 100%.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

All set!
@mateusaubin , @khalidsalomao you can download latest 2.2 binaries with latest amazon redshift support here: https://build.npgsql.org/viewLog.html?buildId=14051&tab=artifacts&buildTypeId=npgsql_All22

Please, give it a try and let us know if you find any issues. Thanks in advance.

from npgsql.

khalidsalomao avatar khalidsalomao commented on June 8, 2024

Hi @franciscojunior, @roji ,

It worked! 😄

Tested several operations and all is fine!

Thanks guys!

from npgsql.

roji avatar roji commented on June 8, 2024

Great. @franciscojunior, go ahead and merge #489 into 2.2. Then we can see if there's anything else we want to do for 2.2.5.

from npgsql.

franciscojunior avatar franciscojunior commented on June 8, 2024

Great. @franciscojunior, go ahead and merge #489 into 2.2. Then we can see if there's anything else we want to do for 2.2.5.

Done!

from npgsql.

ReinsBrain avatar ReinsBrain commented on June 8, 2024

This issue comes back again when you use pgbouncer ( which cannot support startup parameters for various reasons ). If I may weigh in on the matter, I don't think the driver should be setting parameters - please let the users do that :)

from npgsql.

roji avatar roji commented on June 8, 2024

@ReinsBrain this issue is 5 years old, which means I've been maintaining Npgsql for too long :)

More importantly though, Npgsql hasn't been sending LC_MONETARY for several years now - which exact parameter incompatibility are you seeing with pgbouncer? For reference, here's the code which sends the parameters on startup.

from npgsql.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.