Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix shmem allocation size.#42

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
keremet merged 1 commit intopostgrespro:masterfromrjuju:fix_maxbackends
Mar 31, 2022

Conversation

rjuju
Copy link
Contributor

MaxBackends is still 0 when _PG_init() is called, which means that we
don't request enough memory in RequestAddinShmemSpace(), while the rest of
the code sees (and allocate) a correct value.

It's technically usually not a problem as postgres adds an extra 100kB of
memory for small unaccounted memory usage, but it's better to avoid relying on
it too much.

Note that the value is still not guaranteed to be exact as other modules
_PG_init() could later change the underlying GUCs, but there is not available
API to handle that case accurately.

@maksm90
Copy link
Collaborator

@rjuju thanks for so swift fix of#41 . Review it shortly

@maksm90
Copy link
Collaborator

maksm90 commentedMar 25, 2022
edited
Loading

Some notes about the relevance of this kind of estimation of max processes inpg_wait_sampling module.

  • This value is used to allocate shared memory just for storing queryId (inproc_queryids array) as inpg_stat_kcache module. But instead ofpg_stat_kcache here we assign one slot for queryId to one slot of PGPROC therefore we have to use formula for the size ofProcGlobal->allProcs array. And the last sum itemmax_prepared_xacts might be removed as we access toproc_queryids array from regular backends and iterate over it up toProcGlobal->allProcCount index value that is without this item.
    Generally it's enough to allocateMaxBackends-sized array and access to the specific slot viabackendId-1 index value similar as it'sdone inpg_stat_kcache module.
  • Starting from PG14 we might usequeryId value fromPgBackendStatus entry of sharedBackendStatusArray and don't allocate own shared memory for this purpose.@rjuju what do you think what if we rewrote the storing and extracting ofqueryId in such way in future?

@rjuju
Copy link
ContributorAuthor

And the last sum item max_prepared_xacts might be removed as we access to proc_queryids array from regular backends and iterate over it up to ProcGlobal->allProcCount index value that is without this item.

Agreed, and the code should have a comment saying that the value has to be synced with ProcGlobal->allProcCount initialization in InitProcGlobal(), so that it's clear to anyone looking at the code why this value is used.

Starting from PG14 we might use queryId value from shared BackendStatusArray and don't allocate own shared memory for queryId values, in general.@rjuju what do you think what if we rewrote the storing and extracting of queryId in such way in future?

Unfortunately it's not possible :( The community insisted to report only the top-level queryid there, for consistency with e.g. pg_stat_activity.query, even if I initially suggested to report the current query. So we, extension owners, still have to allocate and handle our own queryid array. Unfortunately there isn't even a simple way to do it only once and not once per extension.

@maksm90
Copy link
Collaborator

the code should have a comment saying that the value has to be synced with ProcGlobal->allProcCount initialization in InitProcGlobal(), so that it's clear to anyone looking at the code why this value is used.

Yeah, it's a good idea to document the formula in that way.

Unfortunately it's not possible :( The community insisted to report only the top-level queryid there, for consistency with e.g. pg_stat_activity.query, even if I initially suggested to report the current query. So we, extension owners, still have to allocate and handle our own queryid array.

It's a pity.

Unfortunately there isn't even a simple way to do it only once and not once per extension.

Yes, in essence, multiple extensions have to maintain its own storage forqueryIds of currently executing queries for sharing these values between processes. And clearly, this is redundantly.

@rjuju
Copy link
ContributorAuthor

I just force-pushed the modifications we discussed. I tried to describe the problem with all consequences, so that if anyone tries to read or use the code for something else they will have all the information needed to know if that approach will work for them too.

MaxBackends is still 0 when _PG_init() is called, which means that wedon't request enough memory in RequestAddinShmemSpace(), while the rest ofthe code sees (and allocate) a correct value.It's technically usually not a problem as postgres adds an extra 100kB ofmemory for small unaccounted memory usage, but it's better to avoid relying onit too much.Note that the value is still not guaranteed to be exact as other modules_PG_init() could later change the underlying GUCs, but there is not availableAPI to handle that case accurately.
Copy link
Collaborator

@maksm90maksm90 left a comment

Choose a reason for hiding this comment

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

I just force-pushed the modifications we discussed. I tried to describe the problem with all consequences, so that if anyone tries to read or use the code for something else they will have all the information needed to know if that approach will work for them too.

Excellent! Thanks a lot. Approved.

@keremetkeremet merged commit680f8db intopostgrespro:masterMar 31, 2022
@rjuju
Copy link
ContributorAuthor

Thanks!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@maksm90maksm90maksm90 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@rjuju@maksm90@keremet

[8]ページ先頭

©2009-2025 Movatter.jp