Issue Details (XML | Word | Printable)

Key: CORE-6032
Type: Improvement Improvement
Status: Resolved Resolved
Resolution: Fixed
Priority: Major Major
Assignee: Adriano dos Santos Fernandes
Reporter: Mark Rotteveel
Votes: 0
Watchers: 2
Operations

If you were logged in you would be able to see more operations.
Firebird Core

Add DPB properties for time zone bind and decfloat configuration

Created: 21/Mar/19 12:35 PM   Updated: 13/Apr/19 09:08 AM
Component/s: API / Client Library, Engine
Affects Version/s: 4.0 Beta 1
Fix Version/s: 4.0 Beta 2

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


 Description  « Hide
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`.

 All   Comments   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Vlad Khorsun added a comment - 21/Mar/19 12:51 PM
I'm against introducing things that is not reset when session is reset.
And there was no agreement in fb-devel about it, afair.

Mark Rotteveel added a comment - 21/Mar/19 01:01 PM - edited
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.

Dmitry Yemanov added a comment - 21/Mar/19 01:09 PM
I agree with Mark re. DPB vs session reset.

Vlad Khorsun added a comment - 21/Mar/19 01:13 PM
  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
...

Adriano dos Santos Fernandes added a comment - 21/Mar/19 01:37 PM
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.

Vlad Khorsun added a comment - 21/Mar/19 01:39 PM
Adriano,

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

Adriano dos Santos Fernandes added a comment - 21/Mar/19 01:50 PM
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".

Adriano dos Santos Fernandes added a comment - 21/Mar/19 01:53 PM
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.

Vlad Khorsun added a comment - 21/Mar/19 01:54 PM
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.

Mark Rotteveel added a comment - 21/Mar/19 02:00 PM
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.

Vlad Khorsun added a comment - 21/Mar/19 02:01 PM
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) ?

Vlad Khorsun added a comment - 21/Mar/19 02:04 PM - edited
> 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.

Adriano dos Santos Fernandes added a comment - 21/Mar/19 02:05 PM
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.

Vlad Khorsun added a comment - 21/Mar/19 02:08 PM
> 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.

Vlad Khorsun added a comment - 21/Mar/19 02:11 PM
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 ?

Adriano dos Santos Fernandes added a comment - 21/Mar/19 02:12 PM - edited
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.

Vlad Khorsun added a comment - 21/Mar/19 02:17 PM
> 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 ?

Adriano dos Santos Fernandes added a comment - 21/Mar/19 02:25 PM
Then the initial connection will have that overrides too, so reseted session should too.

Vlad Khorsun added a comment - 21/Mar/19 02:30 PM
No confusion for users, you said ?

Adriano dos Santos Fernandes added a comment - 21/Mar/19 02:38 PM - edited
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.

Vlad Khorsun added a comment - 21/Mar/19 02:53 PM
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

Mark Rotteveel added a comment - 21/Mar/19 03:10 PM
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).

Mark Rotteveel added a comment - 21/Mar/19 03:19 PM
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.

Adriano dos Santos Fernandes added a comment - 21/Mar/19 03:20 PM
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.

Vlad Khorsun added a comment - 21/Mar/19 05:54 PM
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.

Vlad Khorsun added a comment - 07/Apr/19 05:54 PM
Adriano,

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

Mark Rotteveel added a comment - 13/Apr/19 09:08 AM
Implemented and tested the time zone bind DPB item in Jaybird as part of JDBC-540, it is working as I expected.