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

Create separate metrics for BEGIN, COMMIT, and ROLLBACK statements in YSQL (#6486) #6729

Merged
merged 4 commits into from
Dec 30, 2020

Conversation

VishnuK007
Copy link
Contributor

This patch separates out the metrics for transaction begin/commit/rollback statements.

[Note: It doesn't break out PREPARE statements yet. The handling for that might require different logic, which I haven't been able to figure out yet. Might be best to separate the PREPARE handling into its own issue.]

Testing:

  • I created a local YugabyteDB cluster using yb-ctl
  • Used ysqlsh to create a sample table and perform of combination of begin, insert, commit, and rollback statements
  • Visited the metrics page [http://127.0.0.1:13000/metrics] to ensure that these new metrics showed up and that their counters were incremented

Sample output from page shown below:

[
    {
        "type": "server",
        "id": "yb.ysqlserver",
        "metrics": [
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_SelectStmt",
                "count": 0,
                "sum": 0
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_InsertStmt",
                "count": 1,
                "sum": 9579
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_DeleteStmt",
                "count": 0,
                "sum": 0
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_UpdateStmt",
                "count": 0,
                "sum": 0
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_BeginStmt",
                "count": 1,
                "sum": 62
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_CommitStmt",
                "count": 1,
                "sum": 14
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_RollbackStmt",
                "count": 1,
                "sum": 41
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_OtherStmts",
                "count": 0,
                "sum": 0
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_Transactions",
                "count": 1,
                "sum": 9579
            },
            {
                "name": "handler_latency_yb_ysqlserver_SQLProcessor_AggregatePushdowns",
                "count": 0,
                "sum": 0
            }
        ]
    }
]

@CLAassistant
Copy link

CLAassistant commented Dec 22, 2020

CLA assistant check
All committers have signed the CLA.

@jaki
Copy link
Contributor

jaki commented Dec 23, 2020

Hi, @VishnuK007. Looks like your commits don't use your proper email, so you aren't able to sign the CLA. You should set user.email in your git config and set the same email to your GitHub account. You might also have to amend your two commits to use that email. Let me know if you need help with that. You can reach out on our community slack yugabyte-db.slack.com as well.

@VishnuK007
Copy link
Contributor Author

Thanks @jaki. I have made the suggested changes. Can you please take a look again?

Copy link
Contributor

@jaki jaki left a comment

Choose a reason for hiding this comment

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

Thanks, @VishnuK007. I'm not too familiar with prepare. I don't see a problem with deferring it for later.

Notable issue is START TRANSACTION is regarded as OtherStmt.

src/postgres/contrib/yb_pg_metrics/yb_pg_metrics.c Outdated Show resolved Hide resolved
src/postgres/contrib/yb_pg_metrics/yb_pg_metrics.c Outdated Show resolved Hide resolved
Copy link
Contributor Author

@VishnuK007 VishnuK007 left a comment

Choose a reason for hiding this comment

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

@jaki - Could you please take another look? Thanks.

Copy link
Contributor

@jaki jaki left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait. Added one small comment. I'll run this through internal testing.

src/postgres/contrib/yb_pg_metrics/yb_pg_metrics.c Outdated Show resolved Hide resolved
Co-authored-by: Jason Kim <git@jasonk.me>
Copy link
Contributor

@jaki jaki left a comment

Choose a reason for hiding this comment

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

This looks good. One questionable thing is that a COMMIT on a transaction that went bad and actually rollbacks will register in metrics as a commit statement. That may or may not be desired, but I think it doesn't need to be decided in the scope of this PR.

BEGIN;
abc; -- syntax error
COMMIT; -- rollback

@jaki
Copy link
Contributor

jaki commented Dec 30, 2020

@VishnuK007

It doesn't break out PREPARE statements yet

is this outdated now? Also, I don't think we support PREPARE TRANSACTION statements anyway (issue #1125).

yugabyte=# prepare transaction 'abc';
ERROR:  PREPARE TRANSACTION not supported yet
LINE 1: prepare transaction 'abc';
        ^
HINT:  See https://github.com/YugaByte/yugabyte-db/issues/1125. Click '+' on the description to raise its priority

@kmuthukk
Copy link
Collaborator

This looks good. One questionable thing is that a COMMIT on a transaction that went bad and actually rollbacks will register in metrics as a commit statement. That may or may not be desired, but I think it doesn't need to be decided in the scope of this PR.

BEGIN;
abc; -- syntax error
COMMIT; -- rollback

Yes - agreed. The metric is measuring the user level operation initiated-- rather than what might happen underneath. For example, this may happen implicitly on a DML statement also if the txn conflicts with a higher priority txn.

@jaki -- assuming/when your internal test runs also pass, can you squash & merge as well.

@jaki
Copy link
Contributor

jaki commented Dec 30, 2020

I think I got it. Since "prepare" is mentioned by @kmuthukk in issue #6486, it probably means https://www.postgresql.org/docs/11/sql-prepare.html rather than https://www.postgresql.org/docs/11/sql-prepare-transaction.html.

@jaki
Copy link
Contributor

jaki commented Dec 30, 2020

And internal testing looks fine. Merging. Leaving issue #6486 open because of PREPARE statements.

@jaki jaki merged commit e297413 into yugabyte:master Dec 30, 2020
@VishnuK007
Copy link
Contributor Author

And internal testing looks fine. Merging. Leaving issue #6486 open because of PREPARE statements.

Thanks @jaki for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants