Re: archive modules loose ends

Hi,

On 2023-11-13 16:42:31 -0600, Nathan Bossart wrote:
> 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.

I think it might just have arrived too shortly before the feature freeze to be
worth looking at at the time, and then it didn't really re-raise attention
until now.  I'm so far behind on keeping up with the list that I rarely end up
looking far back for things I'd like to have answered... Sorry.

I think it's somewhat important to fix this - having a dedicated "recover from
error" implementation in a bunch of extension modules seems quite likely to
cause problems down the line, when another type of resource needs to be dealt
with after errors.  I think many non-toy implementations would e.g. need to
release lwlocks in case of errors (e.g. because they use a shared hashtable to
queue jobs for workers or such).


> 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.

I think passing in a short-lived memory context is a lot nicer to deal with.


> 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.)

Alternatively we could provide a longer-lived memory context in
ArchiveModuleState, set up by the genric infrastructure. That context would
obviously still need to be explicitly utilized by a module, but no duplicated
setup code would be required.




>  /*
>   * check_archive_directory
>   *
> @@ -172,67 +147,19 @@ basic_archive_configured(ArchiveModuleState *state)
>  static bool
>  basic_archive_file(ArchiveModuleState *state, const char *file, const char *path)
>  {
> ...
> +    PG_TRY();
> +    {
> +        /* Archive the file! */
> +        basic_archive_file_internal(file, path);
> +    }
> +    PG_CATCH();
>      {
> -        /* Since not using PG_TRY, must reset error stack by hand */
> -        error_context_stack = NULL;
> -
> -        /* Prevent interrupts while cleaning up */
> -        HOLD_INTERRUPTS();
> -
> -        /* Report the error and clear ErrorContext for next time */
> -        EmitErrorReport();
> -        FlushErrorState();
> -
>          /* Close any files left open by copy_file() or compare_files() */
> -        AtEOSubXact_Files(false, InvalidSubTransactionId, InvalidSubTransactionId);
> -
> -        /* Reset our memory context and switch back to the original one */
> -        MemoryContextSwitchTo(oldcontext);
> -        MemoryContextReset(basic_archive_context);
> -
> -        /* Remove our exception handler */
> -        PG_exception_stack = NULL;
> +        AtEOXact_Files(false);
>
> -        /* Now we can allow interrupts again */
> -        RESUME_INTERRUPTS();
> -
> -        /* Report failure so that the archiver retries this file */
> -        return false;
> +        PG_RE_THROW();
>      }

I think we should just have the AtEOXact_Files() in pgarch.c, then no
PG_TRY/CATCH is needed here. At the moment I think just about every possible
use of an archive modules would require using files, so there doesn't seem
much of a reason to not handle it in pgarch.c.

I'd probably reset a few other subsystems at the same time (there's probably
more):
- disable_all_timeouts()
- LWLockReleaseAll()
- ConditionVariableCancelSleep()
- pgstat_report_wait_end()
- ReleaseAuxProcessResources()


> @@ -511,7 +519,58 @@ pgarch_archiveXlog(char *xlog)
>      snprintf(activitymsg, sizeof(activitymsg), "archiving %s", xlog);
>      set_ps_display(activitymsg);
>
> -    ret = ArchiveCallbacks->archive_file_cb(archive_module_state, xlog, pathname);
> +    oldcontext = MemoryContextSwitchTo(archive_context);
> +
> +    /*
> +     * Since the archiver operates at the bottom of the exception stack,
> +     * ERRORs turn into FATALs and cause the archiver process to restart.
> +     * However, using ereport(ERROR, ...) when there are problems is easy to
> +     * code and maintain.  Therefore, we create our own exception handler to
> +     * catch ERRORs and return false instead of restarting the archiver
> +     * whenever there is a failure.
> +     */
> +    if (sigsetjmp(local_sigjmp_buf, 1) != 0)
> +    {
> +        /* Since not using PG_TRY, must reset error stack by hand */
> +        error_context_stack = NULL;
> +
> +        /* Prevent interrupts while cleaning up */
> +        HOLD_INTERRUPTS();
> +
> +        /* Report the error and clear ErrorContext for next time */
> +        EmitErrorReport();
> +        MemoryContextSwitchTo(oldcontext);
> +        FlushErrorState();
> +
> +        /* Flush any leaked data */
> +        MemoryContextReset(archive_context);
> +
> +        /* Remove our exception handler */
> +        PG_exception_stack = NULL;
> +
> +        /* Now we can allow interrupts again */
> +        RESUME_INTERRUPTS();
> +
> +        /* Report failure so that the archiver retries this file */
> +        ret = false;
> +    }
> +    else
> +    {
> +        /* Enable our exception handler */
> +        PG_exception_stack = &local_sigjmp_buf;
> +
> +        /* Archive the file! */
> +        ret = ArchiveCallbacks->archive_file_cb(archive_module_state,
> +                                                xlog, pathname);
> +
> +        /* Remove our exception handler */
> +        PG_exception_stack = NULL;
> +
> +        /* Reset our memory context and switch back to the original one */
> +        MemoryContextSwitchTo(oldcontext);
> +        MemoryContextReset(archive_context);
> +    }

It could be worth setting up an errcontext providing the module and file
that's being processed. I personally find that at least as important as
setting up a ps string detailing the log file... But I guess that could be a
separate patch.


It'd be nice to add a comment explaining why pgarch_archiveXlog() is the right
place to handle errors.

Greetings,

Andres Freund