Re: archive modules loose ends
От | Nathan Bossart |
---|---|
Тема | Re: archive modules loose ends |
Дата | |
Msg-id | 20231113224231.GA1759974@nathanxps13 обсуждение исходный текст |
Ответ на | archive modules loose ends (Nathan Bossart <nathandbossart@gmail.com>) |
Ответы |
Re: archive modules loose ends
|
Список | pgsql-hackers |
There seems to be no interest in this patch, so I plan to withdraw it from the commitfest system by the end of the month unless such interest materializes. On Fri, Feb 17, 2023 at 01:56:24PM -0800, Nathan Bossart wrote: > The first one is the requirement that archive module authors create their > own exception handlers if they want to make use of ERROR. Ideally, there > would be a handler in pgarch.c so that authors wouldn't need to deal with > this. I do see some previous dicussion about this [1] in which I expressed > concerns about memory management. Looking at this again, I may have been > overthinking it. IIRC I was thinking about creating a memory context that > would be switched into for only the archiving callback (and reset > afterwards), but that might not be necessary. Instead, we could rely on > module authors to handle this. One example is basic_archive, which > maintains its own memory context. Alternatively, authors could simply > pfree() anything that was allocated. > > Furthermore, by moving the exception handling to pgarch.c, module authors > can begin using PG_TRY, etc. in their archiving callbacks, which simplifies > things a bit. I've attached a work-in-progress patch for this change. I took another look at this, and I think І remembered what I was worried about with memory management. One example is the built-in shell archiving. Presently, whenever there is an ERROR during archiving via shell, it gets bumped up to FATAL because the archiver operates at the bottom of the exception stack. Consequently, there's no need to worry about managing memory contexts to ensure that palloc'd memory is cleared up after an error. With the attached patch, we no longer call the archiving callback while we're at the bottom of the exception stack, so ERRORs no longer get bumped up to FATALs, and any palloc'd memory won't be freed. I see two main options for dealing with this. One option is to simply have shell_archive (and any other archive modules out there) maintain its own memory context like basic_archive does. This ends up requiring a whole lot of duplicate code between the two built-in modules, though. Another option is to have the archiver manage a memory context that it resets after every invocation of the archiving callback, ERROR or not. This has the advantage of avoiding code duplication and simplifying things for the built-in modules, but any external modules that rely on palloc'd state being long-lived would need to be adjusted to manage their own long-lived context. (This would need to be appropriately documented.) However, I'm not aware of any archive modules that would be impacted by this. The attached patch is an attempt at the latter option. As I noted above, this probably deserves some discussion in the archive modules documentation, but I don't intend to spend too much more time on this patch right now given it is likely going to be withdrawn. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Вложения
В списке pgsql-hackers по дате отправления: