Fix a comment in basic_archive about NO_INSTALLCHECK

Started by Bharath Rupireddyalmost 3 years ago5 messages

bharath.rupireddyforpostgres@gmail.com

almost 3 years ago

1 attachment(s)

Hi,

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.

Thoughts?

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

Attachments:

v1-0001-Fix-a-comment-in-basic_archive-about-NO_INSTALLCH.patchapplication/x-patch; name=v1-0001-Fix-a-comment-in-basic_archive-about-NO_INSTALLCH.patchDownload

From a3d9c1e6e1f080403b48232c07f136c173865106 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 1 Apr 2023 08:31:52 +0000
Subject: [PATCH v1] Fix a comment in basic_archive about NO_INSTALLCHECK

---
 contrib/basic_archive/Makefile    | 5 +++--
 contrib/basic_archive/meson.build | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/contrib/basic_archive/Makefile b/contrib/basic_archive/Makefile
index 55d299d650..9216a3060b 100644
--- a/contrib/basic_archive/Makefile
+++ b/contrib/basic_archive/Makefile
@@ -5,8 +5,9 @@ PGFILEDESC = "basic_archive - basic archive module"
 
 REGRESS = basic_archive
 REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/basic_archive/basic_archive.conf
-# Disabled because these tests require "shared_preload_libraries=basic_archive",
-# which typical installcheck users do not have (e.g. buildfarm clients).
+# Disabled because these tests require wal_level = replica and archive related
+# parameters to be enabled, which typical installcheck users do not have
+# (e.g. buildfarm clients).
 NO_INSTALLCHECK = 1
 
 ifdef USE_PGXS
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..0753b07bf5 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -27,8 +27,9 @@ tests += {
     'regress_args': [
       '--temp-config', files('basic_archive.conf'),
     ],
-    # Disabled because these tests require "shared_preload_libraries=basic_archive",
-    # which typical runningcheck users do not have (e.g. buildfarm clients).
+    # Disabled because these tests require wal_level = replica and archive related
+    # parameters to be enabled, which typical installcheck users do not have
+    # (e.g. buildfarm clients).
     'runningcheck': false,
   },
 }
-- 
2.34.1

Michael Paquier

michael@paquier.xyz

almost 3 years ago

In reply to: Bharath Rupireddy (#1)

Re: 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

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

over 2 years ago

In reply to: Michael Paquier (#2)

Re: Fix a comment in basic_archive about NO_INSTALLCHECK

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

Bharath Rupireddy

bharath.rupireddyforpostgres@gmail.com

about 2 years ago

In reply to: Michael Paquier (#2)

1 attachment(s)

Re: Fix a comment in basic_archive about NO_INSTALLCHECK

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

Attachments:

v2-0001-Fix-a-comment-in-basic_archive-about-NO_INSTALLCH.patchapplication/octet-stream; name=v2-0001-Fix-a-comment-in-basic_archive-about-NO_INSTALLCH.patchDownload

From 1ab19cdc053424ba512412894e0b96bd8d2689f6 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Mon, 18 Dec 2023 05:55:56 +0000
Subject: [PATCH v2] Fix a comment in basic_archive about NO_INSTALLCHECK

---
 contrib/basic_archive/Makefile    | 6 ++++--
 contrib/basic_archive/meson.build | 5 +++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/contrib/basic_archive/Makefile b/contrib/basic_archive/Makefile
index 55d299d650..450144de2a 100644
--- a/contrib/basic_archive/Makefile
+++ b/contrib/basic_archive/Makefile
@@ -5,8 +5,10 @@ PGFILEDESC = "basic_archive - basic archive module"
 
 REGRESS = basic_archive
 REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/basic_archive/basic_archive.conf
-# Disabled because these tests require "shared_preload_libraries=basic_archive",
-# which typical installcheck users do not have (e.g. buildfarm clients).
+
+# Disabled because these tests require special configuration e.g. archive
+# related parameters to be enabled, which typical installcheck users do not
+# have # (e.g. buildfarm clients).
 NO_INSTALLCHECK = 1
 
 ifdef USE_PGXS
diff --git a/contrib/basic_archive/meson.build b/contrib/basic_archive/meson.build
index bc1380e6f6..012f44e10e 100644
--- a/contrib/basic_archive/meson.build
+++ b/contrib/basic_archive/meson.build
@@ -27,8 +27,9 @@ tests += {
     'regress_args': [
       '--temp-config', files('basic_archive.conf'),
     ],
-    # Disabled because these tests require "shared_preload_libraries=basic_archive",
-    # which typical runningcheck users do not have (e.g. buildfarm clients).
+    # Disabled because these tests require special configuration e.g. archive
+    # related parameters to be enabled, which typical runningcheck users do not
+    # have # (e.g. buildfarm clients).
     'runningcheck': false,
   },
 }
-- 
2.34.1

Michael Paquier

michael@paquier.xyz

about 2 years ago

In reply to: Bharath Rupireddy (#4)

Re: Fix a comment in basic_archive about NO_INSTALLCHECK

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