refact(session connection): remove session connection state table

[irenarindos]

WIP

This PR removes the session_connection_state table, replacing it with the new field connected_time_range on session_connection

[mgaffney]

Why not add the column and migrate the data from session_connection_state?

[irenarindos]

Do you think it's worth migrating? I'm assuming that the control plane would go down for maintenance, and if we regularly remove terminated sessions from the db it seems like we should be fine to drop that table because sessions should be in a terminated state.

[tmessi]

I think we should migrate the data. We only delete the sessions after they have been terminated for an hour. So once the controllers are back up there will be an hour before the previous sessions are deleted, and I think we should report an accurate connection state for that time period.

[tmessi]

I still need to review the go code, but I have a few comments/requests on the sql.

[tmessi]

Is this comment block for the function? If so, we should remove this black line between the comment block and the function.

[tmessi]

Is this a new function? If so we can just use create function not create or replace. If it is not new, we should document which migration this is replacing, and preferably would perform a drop then create instead of create or replace.

I have this same question for the other functions that are defined in this migration.

[tmessi]

Could this just be an update with the necessary where clause?

Suggested change

	perform from
	session_connection cs
	where
	cs.public_id = new.public_id and
	-- If the connection is already closed, there's no need to update the connected_time_range
	upper(cs.connected_time_range) <= now();
	if not found then
	update session_connection
	set
	connected_time_range = tstzrange(lower(connected_time_range), now())
	where
	public_id = new.public_id;
	update session_connection
	set connected_time_range = tstzrange(lower(connected_time_range), now())
	where public_id = new.public_id
	and cs.connected_time_range = tstzrange(lower(cs.connected_time_range), 'infinity'::timestamptz);

[tmessi]

We should check if this is equal to 'infinity'::timestamptz.

[tmessi]

I think this can be done without the subquery and without using not in:

perform
   from session_connection
  where session_id                  = new.public_id
    and upper(connected_time_range) = 'infinity'::timestamptz;
if found then
  raise 'session %s has open connections', new.public_id;
end if;

[tmessi]

Here, and elsewhere can you use river formatting, i.e.:

Suggested change

	with
	authorized_timestamp (date_dim_key, time_dim_key, ts) as (
	select wh_date_key(create_time), wh_time_key(create_time), create_time
	from session_connection
	where public_id = new.public_id
	and connected_time_range is null
	),
	session_dimension (host_dim_key, user_dim_key, credential_group_dim_key) as (
	select host_key, user_key, credential_group_key
	from wh_session_accumulating_fact
	where session_id = new.session_id
	)
	insert into wh_session_connection_accumulating_fact (
	connection_id,
	session_id,
	host_key,
	user_key,
	credential_group_key,
	connection_authorized_date_key,
	connection_authorized_time_key,
	connection_authorized_time,
	client_tcp_address,
	client_tcp_port_number,
	endpoint_tcp_address,
	endpoint_tcp_port_number,
	bytes_up,
	bytes_down
	)
	select new.public_id,
	new.session_id,
	session_dimension.host_dim_key,
	session_dimension.user_dim_key,
	session_dimension.credential_group_dim_key,
	authorized_timestamp.date_dim_key,
	authorized_timestamp.time_dim_key,
	authorized_timestamp.ts,
	new.client_tcp_address,
	new.client_tcp_port,
	new.endpoint_tcp_address,
	new.endpoint_tcp_port,
	new.bytes_up,
	new.bytes_down
	from authorized_timestamp,
	session_dimension
	returning * into strict new_row;
	return null;
	with
	authorized_timestamp (date_dim_key, time_dim_key, ts) as (
	select wh_date_key(create_time), wh_time_key(create_time), create_time
	from session_connection
	where public_id = new.public_id
	and connected_time_range is null
	),
	session_dimension (host_dim_key, user_dim_key, credential_group_dim_key) as (
	select host_key, user_key, credential_group_key
	from wh_session_accumulating_fact
	where session_id = new.session_id
	)
	insert into wh_session_connection_accumulating_fact (
	connection_id,
	session_id,
	host_key,
	user_key,
	credential_group_key,
	connection_authorized_date_key,
	connection_authorized_time_key,
	connection_authorized_time,
	client_tcp_address,
	client_tcp_port_number,
	endpoint_tcp_address,
	endpoint_tcp_port_number,
	bytes_up,
	bytes_down
	)
	select new.public_id,
	new.session_id,
	session_dimension.host_dim_key,
	session_dimension.user_dim_key,
	session_dimension.credential_group_dim_key,
	authorized_timestamp.date_dim_key,
	authorized_timestamp.time_dim_key,
	authorized_timestamp.ts,
	new.client_tcp_address,
	new.client_tcp_port,
	new.endpoint_tcp_address,
	new.endpoint_tcp_port,
	new.bytes_up,
	new.bytes_down
	from authorized_timestamp,
	session_dimension
	returning * into strict new_row;
	return null;

[tmessi]

I think this should check if = 'infinity'::timestamptz

[tmessi]

Can we just do these updates within the two separate cases of the if above and avoid the use of format and execute?

[tmessi]

Likewise, can you format blocks like these to form a river?:

Suggested change

	update session_connection
	set connected_time_range=tstzrange(now(),'infinity')
	where public_id = 's1c1___clare';
	update session_connection
	set connected_time_range = tstzrange(now(), 'infinity')
	where public_id = 's1c1___clare';