Improve Xmin horizon check details #196

lfittl · 2023-10-25T07:03:58Z

Xmin horizon check: Show TXID with separate epoch, and assigned at time

Since the statistics views in Postgres utilize the 32-bit version of
transaction IDs, but we track 64-bit internally, it can be confusing
to show the raw 64-bit value (since you can't visually compare it to
queries you run on the database). To improve, instead format it with
a separate epoch, separated by a colon, like we do for freezing stats.

Additionally show the specific assigned at timestamp inline, to help
identify the problem origin better, e.g. to use for comparison with
xact_start in pg_stat_activity, when xmin horizon is held back by a
long-running transaction.

In passing introduce a component to handle xmin information consistently,
and fix typing for "heldBackInfo", which was incorrectly marked as a raw
number (its an object).

Xmin horizon check: Clarify that pg_terminate_backend is needed sometimes

To stop a long-running transaction that's idle, one needs to terminate
the connection, not just cancel (the non-existent) active query.

Additionally add a stronger disclaimer about the possible impact of such
an action.

Before:

After:

Since the statistics views in Postgres utilize the 32-bit version of transaction IDs, but we track 64-bit internally, it can be confusing to show the raw 64-bit value (since you can't visually compare it to queries you run on the database). To improve, instead format it with a separate epoch, separated by a colon, like we do for freezing stats. Additionally show the specific assigned at timestamp inline, to help identify the problem origin better, e.g. to use for comparison with xact_start in pg_stat_activity, when xmin horizon is held back by a long-running transaction. In passing introduce a component to handle xmin information consistently, and fix typing for "heldBackInfo", which was incorrectly marked as a raw number (its an object).

…imes To stop a long-running transaction that's idle, one needs to terminate the connection, not just cancel (the non-existent) active query. Additionally add a stronger disclaimer about the possible impact of such an action.

lfittl · 2023-10-25T07:25:15Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

+  return (
+    <>
+      A {type} is holding back the xmin horizon at{" "}
+      <code>{`${epoch}:${xid}`}</code> (assigned at {new Date(info["assigned_at"] * 1000).toISOString()})


On second thought, including the assigned at timestamp here might be unnecessary, since we already show that at the top of the issue page for Xmin Horizon issues. Happy to drop this part, since it also doesn't follow the user's selected timezone (it just uses the browser timezone).

No strong feelings. Though if you keep it, maybe this expression could be pulled up to a local in the component to keep the JSX focused on rendering.

Reading through this again, I think the way it composes with its JSX "callers" is kind of weird. I think this would work better as just something like

const XminHeldBackPoint: React.FunctionComponent<{ info: HeldBackInfoType; }> = ({ type, info }) => { const epoch = epochFromFullTransactionId(info["xmin"]); const xid = xidFromFullTransactionId(info["xmin"]) as number; const assignedAt = new Date(info["assigned_at"] * 1000).toISOString(); return ( <> <code>{`${epoch}:${xid}`}</code> (assigned at {assignedAt}) </> ) }

at the cost of a bit of duplication of the "holding back the xmin horizon" text. It'd be easier to tell what the actual messages would be. Not a super-strong feeling, though.

I was feeling a bit strange to see that "assigned at" in screenshot (thanks for adding them!), the format is not really something we use elsewhere, also as you mentioned, that info should be in the page already. I'd vote for dropping.

lfittl · 2023-10-25T07:26:46Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

+  epoch: number,
+): number {
+  return Number(BigInt(value) - (BigInt(epoch) << BigInt(32)));
+}


These functions are from app/javascript/utils/txid.ts in the main app, but I don't see a good way to share these (since this is also included in the public website code), thus copied them.

Given the limited amount of code and the fact that it won't change much, I think this is okay. Maybe we should add a comment referencing the source? If you think it's not appropriate to reference the private code in the public docs, I'm okay to leave as-is.

I was going to suggest to add xmin_txt or something as a new hash value so that xmin will keep xid8 whereas xmin_txt will have ${epoch}:${xid} (then we don't need to add a duplicated xid logic here).
Then, I sadly realized that we actaully don't have the logic to make xmin_txt in our backend handy (it's not so difficult to get xid part, we could do xmin_horizon_backend::text::xid during SELECT, but I don't think there is a handy way to get an epoch) so this code needs to be added somewhere newly :/
With that, I'd say that it's okay to add it here. Re: comment, how about adding it in the private repo side?

msakrejda

Some minor comments, but looks good.

msakrejda · 2023-10-25T15:59:13Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

+      <CodeBlock>
+        <SQL sql={`SELECT pg_terminate_backend('<query_pid>');`} />
+      </CodeBlock>
+      <p>Note this will roll back the transaction, and <strong>discard all data written to the database earlier within that transaction</strong>.</p>


Re: the <strong> part: I kinda feel like people know what rolling back a transaction means; this feels a bit patronizing. But I guess in the off chance you're not thinking about that, maybe this is a worthwhile reminder.

msakrejda · 2023-10-25T16:01:27Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

@@ -135,17 +173,22 @@ const GuidanceByBackend: React.FunctionComponent<{
                  ORDER BY greatest(age(backend_xmin), age(backend_xid)) DESC;`}
        />
      </CodeBlock>
-      <p>You can cancel it by running either of commands:</p>
+      <p>You can cancel it by running either <a href="https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADMIN-SIGNAL" target="_blank">pg_cancel_backend</a> if the transaction is running an active query:</p>


We have <PGDocsLink /> for linking to Postgres documentation; we should probably use that here.

msakrejda · 2023-10-25T16:05:44Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

+  epoch: number,
+): number {
+  return Number(BigInt(value) - (BigInt(epoch) << BigInt(32)));
+}


Given the limited amount of code and the fact that it won't change much, I think this is okay. Maybe we should add a comment referencing the source? If you think it's not appropriate to reference the private code in the public docs, I'm okay to leave as-is.

msakrejda · 2023-10-25T16:07:42Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

+  return (
+    <>
+      A {type} is holding back the xmin horizon at{" "}
+      <code>{`${epoch}:${xid}`}</code> (assigned at {new Date(info["assigned_at"] * 1000).toISOString()})


No strong feelings. Though if you keep it, maybe this expression could be pulled up to a local in the component to keep the JSX focused on rendering.

msakrejda · 2023-10-25T16:07:59Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

+  info: HeldBackInfoType;
+}> = ({ type, info }) => {
+  const epoch = epochFromFullTransactionId(info["xmin"]);
+  const xid = xidFromFullTransactionId(info["xmin"]) as number;


Why as number?

msakrejda · 2023-10-25T16:16:04Z

components/CheckDocumentation/vacuum/XminHorizon.tsx

+  return (
+    <>
+      A {type} is holding back the xmin horizon at{" "}
+      <code>{`${epoch}:${xid}`}</code> (assigned at {new Date(info["assigned_at"] * 1000).toISOString()})


Reading through this again, I think the way it composes with its JSX "callers" is kind of weird. I think this would work better as just something like

const XminHeldBackPoint: React.FunctionComponent<{ info: HeldBackInfoType; }> = ({ type, info }) => { const epoch = epochFromFullTransactionId(info["xmin"]); const xid = xidFromFullTransactionId(info["xmin"]) as number; const assignedAt = new Date(info["assigned_at"] * 1000).toISOString(); return ( <> <code>{`${epoch}:${xid}`}</code> (assigned at {assignedAt}) </> ) }

at the cost of a bit of duplication of the "holding back the xmin horizon" text. It'd be easier to tell what the actual messages would be. Not a super-strong feeling, though.

lfittl added 2 commits October 25, 2023 00:02

lfittl force-pushed the improve-xmin-horizon-details branch from 5b5206c to be2f82d Compare October 25, 2023 07:05

lfittl requested a review from a team October 25, 2023 07:06

lfittl commented Oct 25, 2023

View reviewed changes

msakrejda approved these changes Oct 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Xmin horizon check details #196

Improve Xmin horizon check details #196

lfittl commented Oct 25, 2023 •

edited

Loading

lfittl Oct 25, 2023 •

edited

Loading

msakrejda Oct 25, 2023

msakrejda Oct 25, 2023

keiko713 Oct 26, 2023

lfittl Oct 25, 2023

msakrejda Oct 25, 2023

keiko713 Oct 26, 2023

msakrejda left a comment

msakrejda Oct 25, 2023

msakrejda Oct 25, 2023

msakrejda Oct 25, 2023

msakrejda Oct 25, 2023

msakrejda Oct 25, 2023

msakrejda Oct 25, 2023

Improve Xmin horizon check details #196

Are you sure you want to change the base?

Improve Xmin horizon check details #196

Conversation

lfittl commented Oct 25, 2023 • edited Loading

lfittl Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msakrejda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lfittl commented Oct 25, 2023 •

edited

Loading

lfittl Oct 25, 2023 •

edited

Loading