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
