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

sql: add pg_backend_pid column to crdb_internal.{node,cluster}_sessions #116673

Merged
merged 1 commit into from Dec 19, 2023

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Dec 18, 2023

Fixes #98581.
Informs #95771.

This commit adds a new pg_backend_pid column to the crdb_internal.node_sessions and crdb_internal.cluster_sessions tables.

Release note (sql change): A pg_backend_pid column was added to crdb_internal.node_sessions and crdb_internal.cluster_sessions. This value corresponds to the numerical ID returned from pg_backend_pid().

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nvanbenschoten nvanbenschoten changed the title sql: add pg_backend_pid column to crdb_internal.{node, cluster}_sessions sql: add pg_backend_pid column to crdb_internal.{node, cluster}_sessions Dec 18, 2023
@nvanbenschoten nvanbenschoten changed the title sql: add pg_backend_pid column to crdb_internal.{node, cluster}_sessions sql: add pg_backend_pid column to crdb_internal.{node,cluster}_sessions Dec 18, 2023
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/sessionPID branch 3 times, most recently from 8ae76a3 to 8a95753 Compare December 18, 2023 18:33
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! just had suggestions for the comments

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/server/serverpb/status.proto line 1052 at r1 (raw file):

    (gogoproto.stdduration) = true];

  // The Postgres backend PID of the session.

it might be useful to document the ways this differs from the Postgres PID. The following comments have more info, which might be useful to include here too so that people don't think this is actually a PID.

Info: "Returns a numerical ID attached to this session. This ID is " +
"part of the query cancellation key used by the wire protocol. This " +
"function was only added for compatibility, and unlike in Postgres, the " +
"returned value does not correspond to a real process ID.",

// BackendKeyData is a 64-bit identifier used by the pgwire protocol to cancel
// queries. It is created at the time of session initialization. It contains the
// SQLInstanceID of the node. SQLInstanceID is an alias of int32, but we use a
// variable number of bits here. The leading bit will only be set if the
// SQLInstanceID is greater than or equal to 2^11. If it is set, the other 31
// bits are used for the ID; otherwise 11 bits are used for the ID. This is safe
// because SQLInstanceID is always a non-negative integer. The remaining bits
// (either 32 bits or 52 bits) are random, and are used to uniquely identify a
// session on that SQL node.


pkg/sql/crdb_internal.go line 2813 at r1 (raw file):

  status             STRING,         -- the status of the session (open, closed)
  session_end        TIMESTAMPTZ,    -- the time when the session was closed
  pg_backend_pid     INT             -- the postgres backend PID of the session

same for the comment here; it feels misleading to just say this is the same as the postgres backend PID.

Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not going to add this column to node_queries, cluster_queries, node_transactions, or cluster_transactions. It has such a specialized role and including it more widely might promote confusion or abuse.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)


pkg/server/serverpb/status.proto line 1052 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it might be useful to document the ways this differs from the Postgres PID. The following comments have more info, which might be useful to include here too so that people don't think this is actually a PID.

Info: "Returns a numerical ID attached to this session. This ID is " +
"part of the query cancellation key used by the wire protocol. This " +
"function was only added for compatibility, and unlike in Postgres, the " +
"returned value does not correspond to a real process ID.",

// BackendKeyData is a 64-bit identifier used by the pgwire protocol to cancel
// queries. It is created at the time of session initialization. It contains the
// SQLInstanceID of the node. SQLInstanceID is an alias of int32, but we use a
// variable number of bits here. The leading bit will only be set if the
// SQLInstanceID is greater than or equal to 2^11. If it is set, the other 31
// bits are used for the ID; otherwise 11 bits are used for the ID. This is safe
// because SQLInstanceID is always a non-negative integer. The remaining bits
// (either 32 bits or 52 bits) are random, and are used to uniquely identify a
// session on that SQL node.

Done.


pkg/sql/crdb_internal.go line 2813 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

same for the comment here; it feels misleading to just say this is the same as the postgres backend PID.

Done.

@nvanbenschoten nvanbenschoten marked this pull request as ready for review December 18, 2023 20:31
@nvanbenschoten nvanbenschoten requested review from a team as code owners December 18, 2023 20:31
@nvanbenschoten nvanbenschoten requested review from dhartunian and removed request for a team December 18, 2023 20:31
@nvanbenschoten
Copy link
Member Author

TFTR!

Fixes cockroachdb#98581.
Informs cockroachdb#95771.

This commit adds a new pg_backend_pid column to the
crdb_internal.node_sessions and crdb_internal.cluster_sessions tables.

Release note (sql change): A pg_backend_pid column was added to
crdb_internal.node_sessions and crdb_internal.cluster_sessions. This
value corresponds to the numerical ID returned from pg_backend_pid().
@nvanbenschoten
Copy link
Member Author

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Dec 19, 2023

Build succeeded:

@craig craig bot merged commit 95eb706 into cockroachdb:master Dec 19, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: add pg_backend_pid column to crdb_internal.{node, cluster}_sessions
3 participants