Implement fingerprinting version 3.1 #155
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Consider lists that have a single NIL element as being present for determining whether the parent field should be included in the fingerprint. In particular, this matters for fingerprinting "SELECT DISTINCT" without columns specified. Additionally, ignore more DDL attributes to avoid a lot of unique fingerprints: (ListenStmt | UnlistenStmt | NotifyStmt).conditionname => This groups all LISTEN, UNLISTEN and NOTIFY statements together. Channels are often made unique to notify about different per end user objects, causing a lot of unique condition names. CreateFunctionStmt.options and FunctionParameter.name => Some ORMs, like Sequelize.js, have a pattern where they repeatedely create functions with the exact name and types, but different function bodies -- this groups all these creations together. We may be more sophisticated in the future and introduce a "PL/pgSQL fingerprint". DoStmt.args => This groups all anonymous DO code blocks together, since we don't have a good way to separate them otherwise, for now.
This normalizes arguments to CreateFunctionStmt, DoStmt, CreateSubscriptionStmt, AlterSubscriptionStmt, CreateUserMapping and AlterUserMapping. Note that this is different from pg_stat_statements itself, which does not normalize utility statements at all today. In passing, add a helper function to output the JSON of individual parse nodes, used for debugging only.
msakrejda
reviewed
Nov 2, 2022
| sprintf(buffer, "%d", node->relationOid); | ||
| _fingerprintString(ctx, "relationOid"); | ||
| _fingerprintString(ctx, buffer); | ||
| } |
There was a problem hiding this comment.
This adds the relation oid to the fingerprint if non-zero? Is this part of one of the changes in the description?
There was a problem hiding this comment.
Hm, interesting. I wonder how that got in there. That seems more by accident than intentional. Will review (and likely remove). Thanks for noticing!
There was a problem hiding this comment.
Upon further review, this looks like it was missed in a prior update. The relationOid is always 0 in the raw parser, so this is effectively a no-op.
seanlinsley
approved these changes
Nov 2, 2022
lfittl
deleted the
13-latest-fingerprinting-normalization-improvements
branch
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Consider lists that have a single NIL element as being present for determining
whether the parent field should be included in the fingerprint. In particular,
this matters for fingerprinting "SELECT DISTINCT" without columns specified.
Additionally, ignore more DDL attributes to avoid a lot of unique fingerprints:
(ListenStmt | UnlistenStmt | NotifyStmt).conditionname
=> This groups all LISTEN, UNLISTEN and NOTIFY statements together.
Channels are often made unique to notify about different per end user
objects, causing a lot of unique condition names.
CreateFunctionStmt.options and FunctionParameter.name
=> Some ORMs, like Sequelize.js, have a pattern where they repeatedely
create functions with the exact name and types, but different function
bodies -- this groups all these creations together. We may be more
sophisticated in the future and introduce a "PL/pgSQL fingerprint".
DoStmt.args
=> This groups all anonymous DO code blocks together, since we don't
have a good way to separate them otherwise, for now.
Additionally, fix a few normalization issues.