-
Notifications
You must be signed in to change notification settings - Fork 201
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
Upgrade embedded Postgres and fix compatibility issues. #1199
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
0.21 | ||
0.22 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -264,30 +264,25 @@ public void updateProcMemoryUsage(FrameInterface f, long rss, long maxRss, | |
"SELECT pk_frame FROM proc WHERE pk_frame=? FOR UPDATE", | ||
String.class, f.getFrameId()).equals(f.getFrameId())) { | ||
|
||
getJdbcTemplate().update(UPDATE_PROC_MEMORY_USAGE, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure this was a merge mistake. This block and the one below it seem to duplicate each other, and this block is missing one of the placeholder values. |
||
rss, maxRss, vss, maxVss, | ||
usedGpuMemory, maxUsedGpuMemory, f.getFrameId()); | ||
} | ||
Comment on lines
-267
to
-270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, this is was a weird merge issue. Not sure where it came from. Good catch. |
||
getJdbcTemplate().update(new PreparedStatementCreator() { | ||
@Override | ||
public PreparedStatement createPreparedStatement(Connection conn) | ||
throws SQLException { | ||
PreparedStatement updateProc = conn.prepareStatement( | ||
UPDATE_PROC_MEMORY_USAGE); | ||
updateProc.setLong(1, rss); | ||
updateProc.setLong(2, maxRss); | ||
updateProc.setLong(3, vss); | ||
updateProc.setLong(4, maxVss); | ||
updateProc.setLong(5, usedGpuMemory); | ||
updateProc.setLong(6, maxUsedGpuMemory); | ||
updateProc.setBytes(7, children); | ||
updateProc.setString(8, f.getFrameId()); | ||
return updateProc; | ||
} | ||
} | ||
); | ||
@Override | ||
public PreparedStatement createPreparedStatement(Connection conn) | ||
throws SQLException { | ||
PreparedStatement updateProc = conn.prepareStatement( | ||
UPDATE_PROC_MEMORY_USAGE); | ||
updateProc.setLong(1, rss); | ||
updateProc.setLong(2, maxRss); | ||
updateProc.setLong(3, vss); | ||
updateProc.setLong(4, maxVss); | ||
updateProc.setLong(5, usedGpuMemory); | ||
updateProc.setLong(6, maxUsedGpuMemory); | ||
updateProc.setBytes(7, children); | ||
updateProc.setString(8, f.getFrameId()); | ||
return updateProc; | ||
} | ||
}); | ||
} | ||
catch (DataAccessException dae) { | ||
} catch (DataAccessException dae) { | ||
logger.info("The proc for frame " + f + | ||
" could not be updated with new memory stats: " + dae); | ||
} | ||
|
@@ -608,7 +603,7 @@ public boolean increaseReservedMemory(ProcInterface p, long value) { | |
"AND " + | ||
"proc.int_mem_reserved != 0 " + | ||
"ORDER BY " + | ||
"proc.int_virt_used / proc.int_mem_pre_reserved DESC " + | ||
"CAST(proc.int_virt_used AS numeric) / proc.int_mem_pre_reserved DESC " + | ||
") AS t1 LIMIT 1"; | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading postgres docs raised some concerns regarding the performance of this conversion:
https://www.postgresql.org/docs/current/datatype-numeric.html :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to dig a bit deeper to understand where this is coming from. Was the issue impacting every query or a specific subset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed to impact every query that used a
SUM
on abigint
field. I didn't test all of these queries but once I saw it a few times I just went through and changed all of them, it seemed consistent.According to postgres docs
SUM(bigint)
always returns anumeric
, so I'm assuming something is going wrong in that conversion process somewhere.It could be a problem specific to the embedded postgres, I don't think I was able to reproduce it anywhere else. And I can't get an older version of the embedded postgres on ARM so I wasn't able to try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DiegoTavares Any update here? Possible to do a test on your side to see if it produces any performance issues of note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've submitted a ticket to EDB asking about the error.
It looks there's a change this update will actually have performance implications. We can try a different version of postgres and see if this is a bug specific for this version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcipriano I have few questions for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcipriano Can you confirm the source of postgres download? can you try https://www.enterprisedb.com/downloads/postgres-postgresql-downloads for download postgres ? Let me know how it goes with EDB 14.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to mention -- this only seems to happen in Docker. When I run tests directly on my ARM Macbook, they pass fine, in fact this PR isn't needed at all. It's only when I try to build the Cuebot Docker image (which does the Gradle build/test as part of the build process) that this problem appears.
Postgres is installed via Gradle/Maven. You can see the versions defined in
build.gradle
: https://github.com/AcademySoftwareFoundation/OpenCue/pull/1199/files#diff-7e57ab670111c27c23069909d0cc32c88d266c9a1f3e6ca38052c973418146feI don't know if it's possible use Postgres binaries from some other source with this embedded Postgres thing we're using.
Same issue on 11.13.0 and 12.12.0. But only on Docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a bit more research into this today:
almalinux
instead ofgradle
). Same issue.postgres
image on my mac. Was not able to reproduce the issue.Still kind of stumped. I filed zonkyio/embedded-postgres#99 to see if those folks have any idea.