Re: archive modules - Mailing list pgsql-hackers

On Fri, Oct 14, 2022 at 6:00 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Oct 13, 2022 at 02:53:38PM -0400, Tom Lane wrote:
> > Yeah, it really does not work to use GUC hooks to enforce multi-variable
> > constraints.  We've learned that the hard way (more than once, if memory
> > serves).
>
> 414c2fd is one of the most recent ones.  Its thread is about the same
> thing.

Got it. Thanks. Just thinking if we must move below comment somewhere
to guc related files?

 * XXX this code is broken by design.  Throwing an error from a GUC assign
 * hook breaks fundamental assumptions of guc.c.  So long as all the variables
 * for which this can happen are PGC_POSTMASTER, the consequences are limited,
 * since we'd just abort postmaster startup anyway.  Nonetheless it's likely
 * that we have odd behaviors such as unexpected GUC ordering dependencies.
 */

FWIW, I see check_stage_log_stats() and check_log_stats() that set
errdetail and return false causing the similar error:

postgres=# alter system set log_statement_stats = true;
postgres=# select pg_reload_conf();
postgres=# alter system set log_statement_stats = false;
postgres=# alter system set log_parser_stats = true;
ERROR:  invalid value for parameter "log_parser_stats": 1
DETAIL:  Cannot enable parameter when "log_statement_stats" is true.

On Thu, Oct 13, 2022 at 11:54 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
>
> On Thu, Oct 13, 2022 at 03:25:27PM +0530, Bharath Rupireddy wrote:
> > The intent here looks reasonable to me. However, why should the user
> > be able to set both archive_command and archive_library in the first
> > place only to later fail in LoadArchiveLibrary() per the patch? IMO,
> > the check_hook() is the right way to disallow any sorts of GUC
> > misconfigurations, no?
>
> There was some discussion upthread about using the GUC hooks to enforce
> this [0].  In general, it doesn't seem to be a recommended practice.  One
> basic example of the problems with this approach is the following:
>
>   1. Set archive_command and leave archive_library unset and restart
>      the server.
>   2. Unset archive_command and set archive_library and call 'pg_ctl
>      reload'.

Thanks. And yes, if GUC 'foo' is reset but not reloaded and the
check_hook() in the GUC 'bar' while setting it uses the old value of
'foo' and fails.

I'm re-attaching Nathan's patch as-is from [1] here again, just to
make CF bot test the correct patch. Few comments on that patch:

1)
+    if (XLogArchiveLibrary[0] != '\0' && XLogArchiveCommand[0] != '\0')
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                 errmsg("both archive_command and archive_library specified"),
+                 errdetail("Only one of archive_command,
archive_library may be set.")));

The above errmsg looks informational. Can we just say something like
below?  It doesn't require errdetail as the errmsg says it all. See
the other instances elsewhere [2].

        ereport(ERROR,
                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                 errmsg("cannot specify both \"archive_command\" and
\"archive_library\"")));

2) I think we have a problem - set archive_mode and archive_library
and start the server, then set archive_command, reload the conf, see
[3] - the archiver needs to error out right? The archiver gets
restarted whenever archive_library changes but not when
archive_command changes. I think the right place for the error is
after or at the end of HandlePgArchInterrupts().

[1] https://www.postgresql.org/message-id/20221005185716.GB201192%40nathanxps13
[2] errmsg("cannot specify both PARSER and COPY options")));
errmsg("cannot specify both %s and %s",
errmsg("cannot specify both %s and %s",
[3]
./psql -c "alter system set archive_mode='on'" postgres
./psql -c "alter system set
archive_library='/home/ubuntu/postgres/contrib/basic_archive/basic_archive.so'"
postgres
./pg_ctl -D data -l logfile restart
./psql -c "alter system set archive_command='cp %p
/home/ubuntu/archived_wal/%f'" postgres
./psql -c "select pg_reload_conf();" postgres
postgres=# show archive_mode;
 archive_mode
--------------
 on
(1 row)
postgres=# show archive_command ;
          archive_command
------------------------------------
 cp %p /home/ubuntu/archived_wal/%f
(1 row)
postgres=# show archive_library ;
                       archive_library
--------------------------------------------------------------
 /home/ubuntu/postgres/contrib/basic_archive/basic_archive.so
(1 row)
postgres=# select pid, wait_event_type, backend_type from
pg_stat_activity where backend_type = 'archiver';
   pid   | wait_event_type | backend_type
---------+-----------------+--------------
 2116760 | Activity        | archiver
(1 row)

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