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

Add DPB properties for time zone bind and decfloat configuration [CORE6032] #6282

Closed
firebird-automations opened this issue Mar 21, 2019 · 33 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: @mrotteveel

As previously discussed in "[Firebird-devel] Setting time zone bind through DPB?", please add DPB properties for the time zone bind and decfloat configuration. This simplifies configuration and setup for clients (see also the discussion in the thread).

Specifically for:

- set time zone bind
- set decfloat round
- set decfloat traps to
- set decfloat bind

My suggestion would be to use plain string values corresponding to the allowed option config (eg isc_dpb_time_zone_bind with possible values native and legacy).

The value set through the DPB should survive an `alter session reset` (config set using `set time zone bind` or `set decfloat` options do not survive session reset). Ensuring these properties survive a session reset allows a stable configuration for connection pools that want to be able use `alter session reset`.

Commits: 2a9f8fa FirebirdSQL/jaybird@c5ff78f

====== Test Details ======

Currently test can not be implemented: no way to use these parameters in FDB

@firebird-automations
Copy link
Collaborator Author

Modified by: @mrotteveel

description: As previously discussed in "[Firebird-devel] Setting time zone bind through DPB?", please add DPB properties for the time zone bind and decfloat configuration. This simplifies configuration and setup for clients (see also the discussion in the thread).

The value set through the DPB should survive an `alter session reset` (config set using `set time zone bind` or `set decfloat` options do not survive session reset). Ensuring these properties survive a session reset allows a stable configuration for connection pools that want to be able use `alter session reset`.

=>

As previously discussed in "[Firebird-devel] Setting time zone bind through DPB?", please add DPB properties for the time zone bind and decfloat configuration. This simplifies configuration and setup for clients (see also the discussion in the thread).

Specifically for:

- set time zone bind
- set decfloat round
- set decfloat traps to
- set decfloat bind

My suggestion would be to use plain string values.

The value set through the DPB should survive an `alter session reset` (config set using `set time zone bind` or `set decfloat` options do not survive session reset). Ensuring these properties survive a session reset allows a stable configuration for connection pools that want to be able use `alter session reset`.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

I'm against introducing things that is not reset when session is reset.
And there was no agreement in fb-devel about it, afair.

@firebird-automations
Copy link
Collaborator Author

Commented by: @mrotteveel

To quote from the thread:

"""
> To me you suggestion looks reasonable - but I'm not sure is not it too late to add new features to FB4 after beta1.

Not late at all. We should finalize new features before the release and this is just an extension to the already presented feature.
"""

And as to reset behavior: if I create a connection with a specific configuration, then I expect alter session reset to reset the connection to that initial configuration (it should only revert any configuration done **after** establishing the connection to that initial configuration on connect). If it doesn't do that, then the "session reset" feature is not really generally useful for client-side connection pools, and it could lead to unexpected behavior or hard to diagnose bugs.

As an example, if time zone bind would be set to legacy through a DPB, then alter session reset should revert to legacy and not to native, because that was the initial config on connect.

@firebird-automations
Copy link
Collaborator Author

Commented by: @dyemanov

I agree with Mark re. DPB vs session reset.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

I'm strongly against the second part of propostion which change values for the some session properties to the values set at DPB level.

Is complicates enigne code as it forces engine to remember that values and witn every new kind of session property it will add more work
to do and require more memory for mostly unused thigns.

It also complicates things for an end user as he will have no idea what SESSION RESET will do exactly.

Instead, i offer to enhance ALTER SESSION RESET statement with ability to set new session properties after reset.
I.e. something like:

ALTER SESSION RESET
SET TIME ZONE BIND LEGACY
SET DECFLOAT BIND LEGACY
...

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

The mark proposal is correct. If that session properties could be inserted as DPB, then ALTER SESSION RESET should reset properties to that values.

A different solution than that does not make sense and turns ALTER SESSION RESET a useless thing.

@firebird-automations
Copy link
Collaborator Author

Modified by: @asfernandes

assignee: Adriano dos Santos Fernandes [ asfernandes ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Adriano,

your level of argumentation is so high, so i can't reach it

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

Vlad, we're in the same situation, as I don't see any sense on your argumentation against what Mark proposed that I have no argument other than say "Vlad, you certainly didn't understood, look again".

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

I don't remember the final implementation of ALTER SESSION RESET.

I known there were discussions about triggers (ON CONNECT / ON RESET) that may be not taken completely in account.

Certainly, if you relly on ON CONNECT trigger to set that attributes and RESET just reset to another thing, things may broke. But that would be a ALTER SESSION RESET design problem that must be fixed, if is the case.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Adriano,

i already explained why i'm against the proposition.

You give no agruments. It is very easy to say "you not undertstood" on every argument you not like.
Do you want me to play the same game ?
It is not productive, but i can, if you wish.

@firebird-automations
Copy link
Collaborator Author

Commented by: @mrotteveel

Given any part of an application (or a trigger, or a stored procedure, etc) can issue an ALTER SESSION RESET, it is not a viable alternative to explicitly set the configuration there again. To me, ALTER SESSION RESET already implies it should reset to the initial configuration specified on connect, and if it doesn't reset the session to the initial configuration then it is sub-optimal for where I think this feature would be most useful.

But if that is problematic or requires more discussion, then I'm fine with reducing the scope of this ticket.

However, I think we need to discuss what ALTER SESSION RESET does and doesn't do so we can come to a consensus.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Mark,

i re-read thread in fb-devel and i see you arguments why you want to use old bindings with new software and that Alex agreed with it (as you cite above).

But i see no reasons why you don't want to run simple single SQL statement to setup session for [re]use.

As i could imagine part of logic of client side conneciton pool:

- get connection parameters from user (db name, user\password, etc)
- find idle connection with same set of params at pool, or
create a new one if not found

Here in "create a new one" part you want to set additional params at DPB.
And in dependence of Java version used in implementation you going to set different values.
OK.

How it differs if you execute single statement which will set required session properties after
you get connection (from pool or new) ?

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

> Given any part of an application (or a trigger, or a stored procedure, etc) can issue an ALTER SESSION RESET

No, it can't. ALTER SESSION RESET can not be used within PSQL block.
It will be documented and corresponding changes (check) will be implemented in the engine soon.

The technical reason is that PSQL executor code not ready to work with transaction changed by ALTER SESSION RESET.
Savepoints stack is broken and there is no simple way to avoid it.

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

Your argument about saving the DPB values to reuse latter will consume precious memory and complicate things is non-sense.

It's the obvious solution and so far only you didn't see it.

Time zone has no DPB yet (it's part of this ticket) but original time zone is already set, saved and restored to the current time zone with RESET. The only change for it is that original time zone will be the DPB value instead of the OS setting.

I will complement a specific thing about original time zone specified in the standard in another ticket.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

> To me, ALTER SESSION RESET already implies it should reset to the initial configuration specified on connect

To me, it should work same way not depending on anything else.
I already wrote that users have no idea what properties session will have after reset within your proposition.
It will force users to set session properties on re-use to be sure its have required values.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Adriano,

you fixed on time zone only, while i speak about all possible session's properties.
I will not wonder if in the near feature there will be tens of them.
And you offer to duplicate its values (default and effective) - for what ?

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

For me it's clear what ALTER SESSION RESET is or should be.

It should be equivalent to disconnect + connect with the same connection properties (including DPB) [1].

Anything not working as that is bugs. So, no confusion for users.

[1] Excluding environment changes, like a change in the time zone that should not be updated but reverted to original time zone of connection.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

> It should be equivalent to disconnect + connect with the same connection properties (including DPB) [1].

Good point.
But what about properties that set by driver, not user ?
What if user set some property to the value that driver overrides ?

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

Then the initial connection will have that overrides too, so reseted session should too.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

No confusion for users, you said ?

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

I don't understand you, really.

You are entering on a totally different and speculative subject: that Jaybird driver may be doing changes to connection attributes passed by user.

So what? What is the relation with RESET?

If that happens, it always happened without RESET! I don't see RESET as a way to user fix possible Jaybird mistakes.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

I speak about a Jaybird because this ticket was born as part of Jaybird support.

I show you the case when feature you defend confuse users (and finally nobody will rely on it).
It is 100% safe to explicitly set session properties on connection reuse.
And is not safe to rely on DPB items set at connection time, at least without ability to query current values of session properties.

BTW, read this: https://stackoverflow.com/questions/596365/what-does-sp-reset-connection-do.
Unfortunately, i can't find more details about other DBMS

@firebird-automations
Copy link
Collaborator Author

Commented by: @mrotteveel

My requirements and expectations of ALTER SESSION RESET are simple:

- the connection configuration should be identical to the configuration immediately after attach (that is: the config set through the DPB).
- Client properties (RDB$SET/GET_CONTEXT) should be cleared
- GTTs should be cleared

If especially the first one is not met, it is just not usable for the things I'd like to use ALTER SESSION RESET for.

And no, Jaybird wouldn't always be able set the config back to the original, because in Java you normally use third-party connection pools (Jaybird doesn't even contain its own connection pool anymore). And those third-party connection pools - usually - have an option to execute a statement before every lease, making it ideal to execute ALTER SESSION RESET. However, that will only be useful if it resets to the configuration of the connection pool, and that is the configuration that was used in the DPB on connect.

Furthermore, if applications or users execute ALTER SESSION RESET in the case of the TIME ZONE BIND or DECFLOAT config, it could simply **break** driver behavior or application expectations (eg because all of a sudden WITH TIME ZONE types are returned instead of WITHOUT TIME ZONE types).

@firebird-automations
Copy link
Collaborator Author

Commented by: @mrotteveel

And really, this is not about fixing Jaybird "mistakes", this is about fulfilling what I think are reasonable expectations of a feature in the simplest way possible **for the user**.

I want ALTER SESSION RESET to allow a connection to behave the same as when it was just created. That way this feature is useful for connection pool management to reset anything 'messed up' in the previous lease, that way a connection checked out from the pool will behave identical whether it was just created or recycled.

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

Well, for ALTER SESSION RESET I was clear in devel that to have no confusion it should reset the connection to the stated produced after (the initial) ON CONNECT trigger.

Vlad said it is diffcult because GTT PRESERVE ROWS. I understand it, so an implementation detail (big) defeact feature design. There is always tradeoffs.

But Vlad, what is requested here is exactly a way for Jaybird does not need to execute commands after initial connection. So it sets DPB before that initial connection, and RESET always reset to that desired state.

@firebird-automations
Copy link
Collaborator Author

Modified by: @mrotteveel

summary: Add DPB properties for time zone bind and decfloat configruation => Add DPB properties for time zone bind and decfloat configuration

@firebird-automations
Copy link
Collaborator Author

Modified by: @mrotteveel

description: As previously discussed in "[Firebird-devel] Setting time zone bind through DPB?", please add DPB properties for the time zone bind and decfloat configuration. This simplifies configuration and setup for clients (see also the discussion in the thread).

Specifically for:

- set time zone bind
- set decfloat round
- set decfloat traps to
- set decfloat bind

My suggestion would be to use plain string values.

The value set through the DPB should survive an `alter session reset` (config set using `set time zone bind` or `set decfloat` options do not survive session reset). Ensuring these properties survive a session reset allows a stable configuration for connection pools that want to be able use `alter session reset`.

=>

As previously discussed in "[Firebird-devel] Setting time zone bind through DPB?", please add DPB properties for the time zone bind and decfloat configuration. This simplifies configuration and setup for clients (see also the discussion in the thread).

Specifically for:

- set time zone bind
- set decfloat round
- set decfloat traps to
- set decfloat bind

My suggestion would be to use plain string values corresponding to the allowed option config (eg isc_dpb_time_zone_bind with possible values native and legacy).

The value set through the DPB should survive an `alter session reset` (config set using `set time zone bind` or `set decfloat` options do not survive session reset). Ensuring these properties survive a session reset allows a stable configuration for connection pools that want to be able use `alter session reset`.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

So far nobody read and consider what i wrote above:

> It complicates engine code as it forces engine to remember that values and witn every new kind of session property it will add more work
> to do and require more memory for mostly unused thigns.
>
> It also complicates things for an end user as he will have no idea what SESSION RESET will do exactly.

So, finally, i can live with reset to DPB-specified values, but only if following will be implemented:
- all session properties should be managed in the same way, i.e. there should be no separate fields in Attachment for current\default value of every
possible property. Probably something like Config should be introduced with ability to iterate thru set of properties and its values.
- there should be a way for end user to query current value of every session property.

It allows to add new properties not breaking session reset code, prevents appearance of spaghetty of settings\properties
and allows to user know how it session works currently.

@firebird-automations
Copy link
Collaborator Author

Modified by: @asfernandes

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

Fix Version: 4.0 Beta 2 [ 10888 ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Adriano,

i don't see how user could query current values of decfloat\timezone settings.
Are you going to implement it ?

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

status: Resolved [ 5 ] => Resolved [ 5 ]

QA Status: No test => Deferred

Test Details: Currently test can not be implemented: no way to use these parameters in FDB

@firebird-automations
Copy link
Collaborator Author

Commented by: @mrotteveel

Implemented and tested the time zone bind DPB item in Jaybird as part of JDBC540, it is working as I expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment