Skip to content

Conversation

@dimitri
Copy link

@dimitri dimitri commented Jul 22, 2025

No description provided.

@dimitri dimitri requested review from MMeent and tristan957 July 22, 2025 10:55
Copy link
Contributor

@MMeent MMeent left a comment

Choose a reason for hiding this comment

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

Couldn't we use elog.h's emit_log_hook to collect error codes?

I'm fairly sure this isn't always called, and so this will skip e.g. XX000/ERRCODE_INTERNAL_ERROR.

While skipping those may be OK for the current use case, it's likely to cause issues down the line.

@tristan957 tristan957 changed the title Add sql error code hook (#32) Add sql error code hook Jul 24, 2025
@tristan957
Copy link
Member

Please remove the pull request number in the commit message.

@dimitri dimitri force-pushed the cherry-pick/16/37717cc9430 branch from ba0eb9b to 21cd295 Compare July 25, 2025 10:06
@dimitri
Copy link
Author

dimitri commented Jul 25, 2025

Ooops. On a series of rebasing branches for CI to run, I realized after the fact that this branch had received other commits. Now I can't fetch them from github anymore. We've lost some changes.

@HaoyuHuang
Copy link

Couldn't we use elog.h's emit_log_hook to collect error codes?

I'm fairly sure this isn't always called, and so this will skip e.g. XX000/ERRCODE_INTERNAL_ERROR.

While skipping those may be OK for the current use case, it's likely to cause issues down the line.

I checked and for this corruption errors, this errcode function is always called. Let's merge this? Suhas PR depends on this change. @thesuhas

@MMeent
Copy link
Contributor

MMeent commented Jul 25, 2025

I checked and for this corruption errors, this errcode function is always called. Let's merge this? Suhas PR depends on this change.

As shared in his PR and as I mentioned above, while that PR does depend on this PR (because it uses this new hook) it should instead use the existing emit_log_hook, which provides more log coverage than this hook and keeps our diff with upstream more manageable.

@HaoyuHuang
Copy link

I checked and for this corruption errors, this errcode function is always called. Let's merge this? Suhas PR depends on this change.

As shared in his PR and as I mentioned above, while that PR does depend on this PR (because it uses this new hook) it should instead use the existing emit_log_hook, which provides more log coverage than this hook and keeps our diff with upstream more manageable.

Yes. Let's merge this in and get rid of this hook once we get into one repo. WDYT?

@dimitri
Copy link
Author

dimitri commented Jul 28, 2025

From discussion, we can close this one and use emit_log_hook in the calling code.

@dimitri dimitri closed this Jul 28, 2025
github-merge-queue bot pushed a commit to neondatabase/neon that referenced this pull request Jul 29, 2025
## Problem

We don't have visibility into data/index corruption.

## Summary of changes
Add data/index corruptions metrics.

PG calls elog ERROR errcode to emit these corruption errors.

PG Changes: neondatabase/postgres#698
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.

5 participants