Thread: Fix a comment in basic_archive about NO_INSTALLCHECK

On Mon, Apr 03, 2023 at 08:56:10AM +0530, Bharath Rupireddy wrote:
> It looks like comments in make file and meson file about not running
> basic_archive tests in NO_INSTALLCHECK mode are wrong. The comments say the
> module needs to be loaded via shared_preload_libraries=basic_archive, but
> it actually doesn't. The custom file needs archive related parameters and
> wal_level=replica. Here's a patch correcting that comment.

Wouldn't it be better to also set shared_preload_libraries in
basic_archive.conf?  It is true that the test works fine if setting
only archive_library, which would cause the library with its
_PG_init() to be loaded in the archiver process.  However the GUC
basic_archive.archive_directory is missing from the backends.

Saying that, updating the comments about the dependency with
archive_library and the module's GUC is right.
--
Michael

On Thu, Apr 6, 2023 at 9:26 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Apr 03, 2023 at 08:56:10AM +0530, Bharath Rupireddy wrote:
> > It looks like comments in make file and meson file about not running
> > basic_archive tests in NO_INSTALLCHECK mode are wrong. The comments say the
> > module needs to be loaded via shared_preload_libraries=basic_archive, but
> > it actually doesn't. The custom file needs archive related parameters and
> > wal_level=replica. Here's a patch correcting that comment.
>
> Wouldn't it be better to also set shared_preload_libraries in
> basic_archive.conf?  It is true that the test works fine if setting
> only archive_library, which would cause the library with its
> _PG_init() to be loaded in the archiver process.  However the GUC
> basic_archive.archive_directory is missing from the backends.

Hm, I think the other backends will still see the value of the GUC
without shared_preload_libraries=basic_archive. You can verify it with
adding SHOW basic_archive.archive_directory; to basic_archive.sql. The
basic_archive library gets loaded by archiver via _PG_init. It's the
archiver defining a custom GUC variable which will propagate to all
the postgres processes via set_config_option_ext. Therefore, we don't
need shared_preload_libraries=basic_archive.

#3  0x00007f75306406b6 in _PG_init () at basic_archive.c:86
#4  0x0000562652d0c87c in internal_load_library (
    libname=0x5626549102d8
"/home/ubuntu/postgres/tmp_install/home/ubuntu/postgres/inst/lib/basic_archive.so")
at dfmgr.c:289
#5  0x0000562652d0c1e7 in load_external_function
(filename=0x562654930698 "basic_archive",
    funcname=0x562652eca81b "_PG_archive_module_init",
signalNotFound=false, filehandle=0x0) at dfmgr.c:116
#6  0x0000562652a3a400 in LoadArchiveLibrary () at pgarch.c:841
#7  0x0000562652a39489 in PgArchiverMain () at pgarch.c:256
#8  0x0000562652a353de in AuxiliaryProcessMain
(auxtype=ArchiverProcess) at auxprocess.c:145
#9  0x0000562652a40b8e in StartChildProcess (type=ArchiverProcess) at
postmaster.c:5341
#10 0x0000562652a3e529 in process_pm_child_exit () at postmaster.c:3072
#11 0x0000562652a3c329 in ServerLoop () at postmaster.c:1767
#12 0x0000562652a3bc52 in PostmasterMain (argc=8, argv=0x56265490e1e0)
at postmaster.c:1462
#13 0x00005626528efbbf in main (argc=8, argv=0x56265490e1e0) at main.c:198

> Saying that, updating the comments about the dependency with
> archive_library and the module's GUC is right.

Thanks. Any thoughts on the v1 patch attached upthread?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Thu, Apr 6, 2023 at 9:26 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Apr 03, 2023 at 08:56:10AM +0530, Bharath Rupireddy wrote:
> > It looks like comments in make file and meson file about not running
> > basic_archive tests in NO_INSTALLCHECK mode are wrong. The comments say the
> > module needs to be loaded via shared_preload_libraries=basic_archive, but
> > it actually doesn't. The custom file needs archive related parameters and
> > wal_level=replica. Here's a patch correcting that comment.
>
> Saying that, updating the comments about the dependency with
> archive_library and the module's GUC is right.

Reworded the comment a bit and attached the v2 patch.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Dec 18, 2023 at 11:28:46AM +0530, Bharath Rupireddy wrote:
> Reworded the comment a bit and attached the v2 patch.

Forgot about this one, thanks!  I've simplified it a bit and applied
it.
--
Michael