Should this case also verify that the state isn't actually disconnected either via timeout or disonnected_at?

I extended the condition.

Frankly speaking,, I'm thinking about refactoring that part of the code and creating an extra "runtime" column for agent status. I'm not quite sure if it's possible to use SQL clauses to reflect this logic. I really wouldn't like to create functions...

I guess that you can take a look at the CASE logic implementation one more time as I've rewritten it to look similar to the Go code.

@mtojek I think the case logic is exactly what you want. But you would have to add that case logic to all reads. So either remove all GetWorkspaceAgentsByXXX and make 1 with a filter (like workspaces/template/etc). Or you could also make a view. I looked into this before with sqlc in an issue here:

#2201

Hey @Emyrk, thanks for jumping in!

I had a chat about this issue with @mafredri to try WITH expression first. I guess that I can transform it into a VIEW if there are performance/clean-sql concerns. Otherwise, I am happy to do this in a follow-up.

The only reason I mentioned a view is because sqlc creates our model types from the sql.

So you have GetWorkspaceAgentByAuthToken, GetWorkspaceAgentByID, GetWorkspaceAgentByInstanceID, GetWorkspaceAgentsByResourceIDs, GetWorkspaceAgentsCreatedAfter. If you update 1 to select this extra "status" column dynamically, sqlc creates a new model type for this. It will not be type WorkspaceAgent. I think it has the name of the function, eg GetWorkspaceAgentByXXX (iirc?). So you will need to add this new column to all the calls, and I think each call gets a unique model type. A way around this is to consolidate all these calls too.

A "view" is a way to repackage the extra dynamic column in a way sqlc still sees it as a single type. So all the above queries go from ... FROM workspace_agents ... to ... FROM workspace_agents_view .... The sqlc type will be type WorkspaceAgentsView.

So tl;dr the view is just a way to keep the sqlc clean. You'll see when you run a make gen with a dynamic column (SELECT *, "unknown" as status FROM workspace_agents).

So either remove all GetWorkspaceAgentsByXXX and make 1 with a filter (like workspaces/template/etc)

Fortunately, at moment we only need it to search workspaces using filtering. I guess that we don't need to hurry up and inject the case logic everywhere.

The only reason I mentioned a view is because sqlc creates our model types from the sql.

In terms of clean code and the benefits of sqlc, I totally agree with you. I admit that I wouldn't like to get stuck in this pull request with too many changes and I'm wondering if I can split it into multiple PRs.

Follow-up ideas:

1. Replace the WITH expression with a view. Adjust the rest of the codebase to use it.
2. Make agent status dynamically set in SQL. Remove the agent status logic from Go code - we don't need it in two places, except for databasefake.

Frankly speaking, if there aren't big performance or clean code concerns around this PR, I would leave it as is, and focus on improvements once the requested feature is delivered.

Let me know what you think.

@Emyrk @mafredri After further research I noticed a possible blocker for a view. The CASE logic
requires agent_inactive_disconnect_timeout_seconds, which is a coderd option, and it's currently provided via query.

Unless we can change it to a static value, then I suppose we can't use views here.

That's a good point @mtojek, it definitely hampers the use case. Ideally this value would come from either the terraform provider or be stored in the database by some other means, at which point it can be part of the query.

I don't find it unthinkable that we would serialize certain coder server runtime properties in the database, either as a temp table or one that is (re)written on startup. The inactivity seconds could then be stored there and joined.

If you also CTE'd the latest_build query, like this:

WITH latest_build AS (
	SELECT ...
), workspace_builds_agents AS (
	SELECT ...
)

You could join latest_builds in workspace_builds_agents to reduce the agent results to the latest build only. This should make the query more performant (reduced rows).

This is a good suggestion, but won't it be a problem that latest_build goes over all workspaces?

SELECT
	workspaces.*, COUNT(*) OVER () as count
FROM
	workspaces
LEFT JOIN LATERAL (
	SELECT

...

	WHERE
		workspace_builds.workspace_id = workspaces.id
...
) latest_build ON TRUE

I presume that we will need latest_builds (plural) for every workspace, but I'm not sure if it isn't the same complexity.

Ok, I investigated a few concepts we talked about offline with @mafredri:

1. Replace latest_build with workspace_latest_builds CTE.

Unfortunately, I wasn't able to select only the last build per workspace.

2. Replace workspace_build_agent CTE with JOIN LATERAL.

Workspaces with multiple agents will return multiple records, which is unexpected.

3. Place the CASE logic in the WHERE clause

It looks like the conditional logic will fire only if has-agent is specified and it shouldn't affect other GetWorkspaces queries.

Let me know your thoughts.

I haven't tested if this works, but I would suggest this final change:

You'll need to add provisioner_jobs.id AS provisioner_job_id to the select for latest_build though (renaming via AS would be optional, doesn't collide with any other names).

This should be more performant than re-joining these tables here. (It also eliminates the need for the build_number check in WHERE.)

I'm afraid that it would work if latest_build is CTE. Otherwise, it's: relation "latest_build" does not exist.