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

conn#readReadyForQuery(): on error (E) message throw that specific error #1136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Al2Klimov
Copy link

not a generic one, so that the application can decide how to handle it. Sure it is an "unexpected message", but not a protocol error:

A frontend must be prepared to accept ErrorResponse and NoticeResponse
messages whenever it is expecting any other type of message.

-- https://www.postgresql.org/docs/current/protocol-flow.html

fixes Icinga/icingadb#478

not a generic one, so that the application can decide how to handle it.
Sure it is an "unexpected message", but not a protocol error:

> A frontend must be prepared to accept ErrorResponse and NoticeResponse
> messages whenever it is expecting any other type of message.

-- https://www.postgresql.org/docs/current/protocol-flow.html
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.

thanks for your contribution! is there a way you can test this change?

@Al2Klimov
Copy link
Author

@log1-c You're affected by Icinga/icingadb#620. Please could you give this one a try? Don't hesitate to ask me if anything.

@log1-c
Copy link

log1-c commented Sep 29, 2023

I'd like to, but not sure how exactly.

I clonedthe icingadb repo and edited all occurences of github.com/lib/pq to your fork and commit

for i in $(grep -ri lib/pq pkg/ | cut -d ":" -f1 |sort -n | uniq);do sed -i 's/lib\/pg/Al2Klimov\/pq/g' $i;done

Then I tried running go build, which tells me

# go build
go: updates to go.mod needed; to update it:
        go mod tidy

And go mod tidy still finds some lib/pq references, but I can't find them

# go mod tidy
go: downloading github.com/Al2Klimov/pq v0.0.0-20230810163137-b7fc3ebbfd5f
go: github.com/Al2Klimov/pq@v0.0.0-20230810163137-b7fc3ebbfd5f: parsing go.mod:
        module declares its path as: github.com/lib/pq
                but was required as: github.com/Al2Klimov/pq
# grep -ri lib/pq *

Plan was to create a new icingadb binary that I can replace the v1.1.1 version with.

Can you point me in the right direction?

@Al2Klimov
Copy link
Author

At best:

  • Clone my fork and checkout the branch of this PR
  • Checkout the version of Icinga DB you're using in prod, as-is
  • In its go.mod add something like replace github.com/lib/pq => /path/to/local/github.com/Al2Klimov/pq as in https://go.dev/ref/mod#go-mod-file-replace

@log1-c
Copy link

log1-c commented Sep 29, 2023

Ok, now I get the following when trying to run go build

# go build
no Go files in /home/username/icingadb

Let me say that I have no experience with Go ;)

Simplest thing (for me) would be if you can provide me the ready-to-use icingadb binary/executable which I can swap in for the existing v1.1.1.

@Al2Klimov
Copy link
Author

Sorry! My fault. You need go build ./cmd/icingadb

@log1-c
Copy link

log1-c commented Oct 30, 2023

# md5sum /sbin/icingadb
4050712828dfbdc961adbae4a143bd27  /sbin/icingadb

# icingadb --version
Icinga DB version: v1.1.1

Build information:
  Go version: go1.20.7 (linux, amd64)
  Git commit: 6c8b52f2033cd94466863c92d3df632e3c87743c

System information:
  Platform: Red Hat Enterprise Linux
  Platform version: 8.8 (Ootpa)
-----------------------------------------------------------------

# md5sum icingadb
7865b358d23970d51312a051613279ce  icingadb

# ./icingadb --version
Icinga DB version: 1.1.1-g6c8b52f-dirty

Build information:
  Go version: go1.19.13 (linux, amd64)
  Git commit: 6c8b52f2033cd94466863c92d3df632e3c87743c (modified)

System information:
  Platform: Red Hat Enterprise Linux
  Platform version: 8.8 (Ootpa)

Had some time to test today @Al2Klimov
Changed the binary on both master nodes and watched the service status while executing a cluster node switch.
Sadly one one of the masters the service isn't running currently (see Icinga/icinga2#9942), so I'm not sure how valuable the information, that the service didn't crash during the switch on the second master, is at the moment.

As this is our DEV env we will simply let the modified icingadb binary active. In case when see any behavioral changes, all update the issue.

@Al2Klimov
Copy link
Author

Thank you for testing. But 6c8b52f2033cd94466863c92d3df632e3c87743c is plain v1.1.1, so I think you should modify it on both nodes. Also, regarding crashes, this PR is only the first step. Its goal is to make the error thrown in Icinga/icingadb#620 more specific, so we know what to handle.

@log1-c
Copy link

log1-c commented Nov 7, 2023

This binary is modified on both nodes.

sbin]# ls -la icingadb*
-rwxr-xr-x. 1 root root 11761960 Oct 30 14:51 icingadb
-rwxr-xr-x. 1 root root  8685056 Aug  8 17:05 icingadb.backup
-rwxr-xr-x. 1 root root 10927808 Aug  8 17:05 icingadb-migrate
[root@msd-ic-ma02 sbin]# md5sum icingadb.backup
4050712828dfbdc961adbae4a143bd27  icingadb.backup
[root@msd-ic-ma02 sbin]# md5sum icingadb
7865b358d23970d51312a051613279ce  icingadb

But the testing is not really conclusive as our config master currently fails with the error message mentioned in Icinga/icinga2#9942
Could you have a look at that and give some feedback how to solve it to go further with the testing of the pgsql issue?

@Al2Klimov
Copy link
Author

You need either Icinga 2.14.1 (TBD):

or this workaround: Icinga/icinga2#9942

Or, if the above workaround is too much manual work, you could also FLUSHDB, or better: DEL icinga:history:stream:downtime, the Redis in question. After all, IIRC, both Icinga DBs write the history.

@Al2Klimov
Copy link
Author

thanks for your contribution! is there a way you can test this change?

Look, @rafiss, @log1-c has successfully tested this PR: Icinga/icingadb#620 (comment) 🎉

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.

None yet

3 participants