Re: archive modules

On Fri, Oct 14, 2022 at 6:00 AM Michael Paquier <michael(at)paquier(dot)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(at)gmail(dot)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