-
Notifications
You must be signed in to change notification settings - Fork 15
Add DSA #286
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
base: OPENGPDB_STABLE
Are you sure you want to change the base?
Add DSA #286
Conversation
x4m
left a comment
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.
This PR mostly brings Postgres code, so, probably this might be OK, but don't see much integration into GP stuff. Are tests actually triggered?
| @@ -0,0 +1,33 @@ | |||
| # Copyright (c) 2022-2023, PostgreSQL Global Development Group | |||
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.
Meson?
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.
Removed
| SUBDIRS += connection | ||
| SUBDIRS += test_extensions | ||
| SUBDIRS += test_bloomfilter | ||
| SUBDIRS += test_dsa |
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.
Is this test included into any of our schedules?
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.
The ic-modules job is added
src/backend/storage/ipc/dsm.c
Outdated
| LWLockAcquire(DynamicSharedMemoryControlLock, LW_EXCLUSIVE); | ||
| for (i = 0; i < dsm_control->nitems; ++i) | ||
| { | ||
| /* Skip unused slots. */ |
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.
IDK, maybe backport these?
postgres/postgres@0b55aaa
postgres/postgres@6c0fb94
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.
The postgres/postgres@6c0fb94 commit has already been backported in OPENGPDB_STABLE
gpdb/src/backend/storage/ipc/dsm.c
Lines 574 to 594 in 94f8b86
| for (i = 0; i < nitems; ++i) | |
| { | |
| /* | |
| * If the reference count is 0, the slot is actually unused. If the | |
| * reference count is 1, the slot is still in use, but the segment is | |
| * in the process of going away; even if the handle matches, another | |
| * slot may already have started using the same handle value by | |
| * coincidence so we have to keep searching. | |
| */ | |
| if (dsm_control->item[i].refcnt <= 1) | |
| continue; | |
| /* If the handle doesn't match, it's not the slot we want. */ | |
| if (dsm_control->item[i].handle != seg->handle) | |
| continue; | |
| /* Otherwise we've found a match. */ | |
| dsm_control->item[i].refcnt++; | |
| seg->control_slot = i; | |
| break; | |
| } |
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 added 0b55aaa to the PR, because this commit fixes the same problem as postgres/postgres@6c0fb94
b6db26b to
46af0ab
Compare
C doesn't have any sort of built-in understanding of a pointer relative to some arbitrary base address, but dynamic shared memory segments can be mapped at different addresses in different processes, so any sort of shared data structure stored within a dynamic shared memory segment can't use absolute pointers. We could use something like Size to represent a relative pointer, but then the compiler provides no type-checking. Use stupid macro tricks to get some type-checking. Patch originally by me. Concept suggested by Andres Freund. Recently resubmitted as part of Thomas Munro's work on dynamic shared memory allocation. Discussion: [email protected] Discussion: CAEepm=1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA@mail.gmail.com (cherry picked from commit fbc1c12)
This is intended as infrastructure for a full-fledged allocator for dynamic shared memory. The interface looks a bit like a real allocator, but only supports allocating and freeing memory in multiples of the 4kB page size. Further, to free memory, you must know the size of the span you wish to free, in pages. While these are make it unsuitable as an allocator in and of itself, it still serves as very useful scaffolding for a full-fledged allocator. Robert Haas and Thomas Munro. This code is mostly the same as my 2014 submission, but Thomas fixed quite a few bugs and made some changes to the interface. Discussion: CA+TgmobkeWptGwiNa+SGFWsTLzTzD-CeLz0KcE-y6LFgoUus4A@mail.gmail.com Discussion: CAEepm=1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA@mail.gmail.com (cherry picked from commit 13e14a7)
If you have previously pinned a segment and decide that you don't actually want to keep it around until shutdown, this new API lets you remove the pin. This is pretty trivial except on Windows, where it requires closing the duplicate handle that was used to implement the pin. Thomas Munro and Amit Kapila, reviewed by Amit Kapila and by me. (cherry picked from commit 0fda682) Changes from original commit: remove API changes and windows compatibility code to keep binary compatibility with 6.x
Programmers discovered decades ago that it was useful to have a simple interface for allocating and freeing memory, which is why malloc() and free() were invented. Unfortunately, those handy tools don't work with dynamic shared memory segments because those are specific to PostgreSQL and are not necessarily mapped at the same address in every cooperating process. So invent our own allocator instead. This makes it possible for processes cooperating as part of parallel query execution to allocate and free chunks of memory without having to reserve them prior to the start of execution. It could also be used for longer lived objects; for example, we could consider storing data for pg_stat_statements or the stats collector in shared memory using these interfaces, rather than writing them to files. Basically, anything that needs shared memory but can't predict in advance how much it's going to need might find this useful. Thomas Munro and Robert Haas. The original code (of mine) on which Thomas based his work was actually designed to be a new backend-local memory allocator for PostgreSQL, but that hasn't gone anywhere - or not yet, anyway. Thomas took that work and performed major refactoring and extensive modifications to make it work with dynamic shared memory, including the addition of appropriate locking. Discussion: CA+TgmobkeWptGwiNa+SGFWsTLzTzD-CeLz0KcE-y6LFgoUus4A@mail.gmail.com Discussion: CAEepm=1z5WLuNoJ80PaCvz6EtG9dN0j-KuHcHtU6QEfcPP5-qA@mail.gmail.com (cherry picked from commit 13df76a) Changes from original commit: removed extra argument from dsm_create() call
The comments in dsa.c suggested that areas were owned by resource owners, but it was not in fact tracked explicitly. The DSM attachments held by the dsa were owned by resource owners, but not the area itself. That led to confusion if you used one resource owner to attach or create the area, but then switched to a different resource owner before allocating or even just accessing the allocations in the area with dsa_get_address(). The additional DSM segments associated with the area would get owned by a different resource owner than the initial segment. To fix, add an explicit 'resowner' field to dsa_area. It replaces the 'mapping_pinned' flag; resowner == NULL now indicates that the mapping is pinned. This is arguably a bug fix, but I'm not backpatching because it doesn't seem to be a live bug in the back branches. In 'master', it is a bug because commit b8bff07daa made ResourceOwners more strict so that you are no longer allowed to remember new resources in a ResourceOwner after you have started to release it. Merely accessing a dsa pointer might need to attach a new DSM segment, and before this commit it was temporarily remembered in the current owner for a very brief period even if the DSA was pinned. And that could happen in AtEOXact_PgStat(), which is called after the owner is already released. Reported-by: Alexander Lakhin Reviewed-by: Alexander Lakhin, Thomas Munro, Andres Freund Discussion: https://www.postgresql.org/message-id/11b70743-c5f3-3910-8e5b-dd6c115ff829%40gmail.com (cherry picked from commit postgres/postgres@a8b330f)
This covers basic calls within a single backend process, and also calling dsa_allocate() or dsa_get_address() while in a different resource owners. The latter case was fixed by the previous commit. Discussion: https://www.postgresql.org/message-id/11b70743-c5f3-3910-8e5b-dd6c115ff829%40gmail.com (cherry picked from commit postgres/postgres@325f540) Changes from original commit: test was adapted to 6.x remove meson.build
dsm_create and dsm_attach assumed that a current resource owner was always in place. Exploration with the API show that this is inconvenient: sometimes one must create a dummy resowner, create/attach the DSM, only to pin the mapping later, which is wasteful. Change create/attach so that if there is no current resowner, the dsm is effectively pinned right from the start. Discussion: https://postgr.es/m/[email protected] Reviewed by Thomas Munro. This is backport of commit 767bc02 Co-authored-by: Alvaro Herrera <[email protected]>
Rewrite the test_bloomfilter function call to make the test_bloomfilter test passed
46af0ab to
18e5420
Compare
Teach dsm_unpin_segment() to skip segments that are in the process of being destroyed by another backend, when searching for a handle. Such a segment cannot possibly be the one we are looking for, even if its handle matches. Another slot might hold a recently created segment that has the same handle value by coincidence, and we need to keep searching for that one. The bug caused rare "cannot unpin a segment that is not pinned" errors on 10 and 11. Similar to commit 6c0fb94 for dsm_attach(). Back-patch to 10, where dsm_unpin_segment() landed. Author: Thomas Munro Reported-by: Justin Pryzby Tested-by: Justin Pryzby (along with other recent DSA/DSM fixes) Discussion: https://postgr.es/m/[email protected] cherry-picked from 0b55aaa
DSA will be useful to dynamically allocate shared memory when the required amount is not known in advance.
DSA will allow to get rid of GUCs in our extensions, which set the maximum shared memory amount for the extension. The extension will be easier to use, because users won't have to remember about the GUCs and set them. We will not need to handle situations where the preallocated shared memory limit is not enough.