Add error context during loading of libraries

From: Andres Freund <andres(at)anarazel(dot)de>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: eng(dot)khaledghazy(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Add error context during loading of libraries
Date: 2023-06-28 00:03:07
Message-ID: 20230628000307.rf6jnv4v4zyf6dqg@awork3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

This discussion started at https://postgr.es/m/20230627212423.p56zxuqeh5kyatqg@awork3.anarazel.de
but isn't really related to the bug.

On 2023-06-27 17:44:57 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > That's not going to help you / the reporter, but to make issues like this
> > easier to debug in the future, I think we should
> > a) install an error context in load_libraries() printing the GUC name and
> > b) install an error context in internal_load_library() printing the name of
> > the shared library name
>
> +1. I'm not sure this specific issue justifies it, but it seems like that
> might make it easier to diagnose other shared-library-load-time issues
> as well.

Yea, this bug alone wouldn't have made me suggest it, but I've looked at
enough issues where I regretted not knowing what library caused and error
and/or what caused a library to be loaded that I think it's worth it.

I first added an error context to both the places mentioned above, but that
leads to annoyingly redundant messages. And I realized that it'd also be
useful to print a message when loading a library due to
load_external_function() - I've definitely wondered about errors triggered
below that in the past.

I ended up adding a reason enum and a detail const char* to
internal_load_library(). I don't like that approach a whole lot, but couldn't
really come up with something better.

Example errors that now have a context:

Error in _PG_init() of library called via shared_preload_libraries (error added by me):

FATAL: not today
CONTEXT: in "_PG_init()" callback of library "/tmp/meson-install/lib/x86_64-linux-gnu/postgresql/auto_explain.so"
library load for "shared_preload_libraries" parameter

or

FATAL: could not access file "dont_exist": No such file or directory
CONTEXT: library load for "shared_preload_libraries" parameter

Creating a C function referencing a library that needs to be loaded with
shared_preload_libraries:

=# CREATE FUNCTION frak()
RETURNS text IMMUTABLE STRICT
AS '/srv/dev/build/m/src/test/modules/test_slru/test_slru.so' LANGUAGE C;
ERROR: XX000: cannot load "test_slru" after startup
DETAIL: "test_slru" must be loaded with shared_preload_libraries.
CONTEXT: in "_PG_init()" callback of library "/srv/dev/build/m/src/test/modules/test_slru/test_slru.so"
library load for C function "frak"

LOAD of a non-postgres library:

=# LOAD '/usr/lib/libarmadillo.so.11';
ERROR: XX000: incompatible library "/usr/lib/libarmadillo.so.11": missing magic block
HINT: Extension libraries are required to use the PG_MODULE_MAGIC macro.
CONTEXT: library load for LOAD statement

Note that here the errcontext callback prints the reason for the library being
loaded, but not the library name. I made it so that the library name is only
printed during _PG_init(), otherwise it's always duplicating the primary error
message. Which looks messy - but perhaps it's more important to be
"predictable"?

I don't love "library load for ..." and played around with a few other
variants, but I didn't come up with anything particularly satisfying.

I was tempted to invent a separate "library load reason" for
fmgr_info_C_lang() and other uses of load_external_function(), but concluded
that that reaches diminishing-returns territory.

Is it worth adding tests for:
1) an error during shared_preload_libraries, local_preload_libraries, session_preload_libraries
2) loading a non-postgres library and hitting "missing magic block"
3) local_preload_libraries not being allowed to load libraries outside of plugins/?
4) session_preload_libraries being allowed to load libraries outside of plugins/?
?

Greetings,

Andres Freund

Attachment Content-Type Size
v1-0001-Add-error-context-callback-to-internal_load_libra.patch text/x-diff 10.1 KB

Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro Horiguchi 2023-06-28 00:20:27 Re: Add TLI number to name of files generated by pg_waldump --save-fullpage
Previous Message David Christensen 2023-06-27 23:58:39 Re: Add TLI number to name of files generated by pg_waldump --save-fullpage