Enable logging collector

[wildtayne]

This PR fixes #202 by enabling logging_collector in enableConnectionLoggingReco.psql. I am creating this PR so others can test the code if they wish. I believe approval should be withheld for now, because this change will require some significant documentation changes:

Unfortunately, logging_collector is fixed at runtime, so now users will have to restart their Postgres instance in order to enable connection logging. My concern is that restarting the instance introduces a significant roadblock to anyone installing ClassDB to an in-production system. I am debating whether the documentation should unconditionally state that users need to restart that their Postgres instance, or if the script should check if logging_collector is already enabled and notify the user that they do not need to restart their instance. Any input is welcome.

[afig]

I agree that this change will involve updating documentation. However, requiring a restart of the Postgres instance in not a huge deal, since it doesn't have to be done immediately. Most DBMS servers have some sort of maintenance window, and one this option is set, nothing special has to be done at restart time. More so, ClassDB can still be used even without the restart, though logs will not be stored.

If it does not take too much effort to implement, I think checking the state of logging_collector beforehand to see if a restart is needed is the best course of action. This avoid having to unnecessarily having someone restart a server (and giving a specific message with specific consequences will increase the chances that someone does restart the server when they need to).

[wildtayne]

I also think that adding the check is the best course of action. We also need to highlight that ClassDB.importConnectionLog() won't work until after the restart. I think this will have to work with the solution to #200, since that will handle missing log files more gracefully. Perhaps ClassDB.importConnectionLog() can even check logging_collector when it starts and provide a helpful error message.

[smurthys]

Thanks for identifying the potential issue and making the change @srrollo.

• I wonder if the order of turning on log_connections and logging_collector matter. It probably does not
• I recommend conditionally raising a warning that a restart is needed if we just turned on logging_collector. I recommend against raising notice because users habitually disable/ignore them

[wildtayne]

I pushed another commit that raises a warning if logging_collector is changed from off to on. I used an anon code block for this, since there is no guarantee any ClassDB functions will exist when the server scripts are run. I also reordered the ALTER SYSTEM statements to more logically show the changes made to the Postgres config.

[smurthys]

I have two sets of recommendations.

At start of script:

• Assert current user is superuser: just copy code from addHelpersCore.sql
• Use SELECT ... INTO to store the current value of logging_connector in a variable.
• Use COALESCE or other means to store FALSE in the variable to handle the WHERE predicate failing: I don't know what Postgres does, but DBMSs tend to not store a configuration row if a value has not been explicitly set. (An alternative is to use IF NOT FOUND immediately after SELECT.)
• Note: NOT(SELECT x ...) is "unknown" is SELECT returns no rows. As written, this situation results in no warning to be raised

EDIT: struck the first point in the recommendations above after comment by @afig below.

Structure the code as follows:

• Store the current value of logging_connector in a variable
• Execute ALTER SYSTEM as is presently done
• Reload configuration if logging_connector was already on; else raise a warning about restarting the server

[afig]

About @smurthys's comment:

• Interesting point about the WHERE predicate failing. The behavior noted about not having rows for unset parameters doesn't seem to apply to pg_settings (possibly because it is a view and already performs an internal COALESCE), however, it is certainly good practice to perform the above behavior and worth keeping in mind for other scenarios.

• Unfortunately, we cannot make a full assertion since as mentioned in L23-L25, these ALTER SYSTEM commands cannot be run in a transaction block. We can raise a message, but the remaining commands will still attempt to execute (and will all raise their own message about not being a superuser). However, it may still be worthwhile to raise our own message.

---

Apart from the changes already suggested, this looks good. I tested this on an instance that had the logging_collector turned off and the script successfully turned it on while issuing a WARNING. It also did not raise a message if the logging_collector was already turned on. This will need to be reevaluated after any changes, however.

I think having ClassDB.importConnectionLog() check for the state of the logging_collector is somewhat unnecessary. Rather, this may be better addressed by expanding the Troubleshooting page on the ClassDB docs to include issues that may come up when importing logs.

We should also update the Setup page to discuss the potential need for a restart.

[smurthys]

Thanks @afig for the reminder about the per-line execution nature of this script. I totally forgot that aspect.

BTW, I have some observations on logging related checks in ClassDB.importConnectionLog(), but have not shared them here because that code is (presently) not part of this PR.

[wildtayne]

Thanks for the feedback. I suspect that using SELECT INTO will not work in this situation either, because it is a PL/pgSQL statement. Since all BEGIN END blocks are atomic, I suspect we will be unable to use ALTER SYSTEM within them. I will verify this behavior later.

If this proves to be the case, perhaps we can place the IF statement after the ALTER SYSTEM statements that either executes pg_reload_conf() or notifies the user they need to restart.

[wildtayne]

Also, I plan on introducing other logging changes in a separate PR. I think they still need some work, but I can open that PR so we can begin a discussion.

[wildtayne]

After testing, I can confirm that ALTER SYSTEM cannot be used inside a DO block.

I just pushed a commit reorganizing the code based on feedback from @smurthys and @afig, with some modifications to accommodate these limitations. Now, the first three ALTER SYSTEM statements are performed, and then the IF statement is used. If logging_collector is off, the warning is raised, and if it is on, pg_reload_conf() is called. Then logging_collector is set to on.

[afig]

The new behavior looks good and works as expected. Since pg_relod_conf() is now executed inside of a block, it no longer shows output (the t value seen in earlier versions of the script) when running the script in psql, which is a beneficial side effect.

I like the idea of using of a HINT , however, I believe it may not be appropriate for this situation, since it contains important information. Postgres has an error message style guide used for development of Postgres. In it, they state:

The primary message should be short, factual, and avoid reference to implementation details such as specific function names. "Short" means "should fit on one line under normal conditions". Use a detail message if needed to keep the primary message short, or if you feel a need to mention implementation details such as the particular system call that failed. Both primary and detail messages should be factual. Use a hint message for suggestions about what to do to fix the problem, especially if the suggestion might not always be applicable.

Currently, the primary message does not say what is wrong, and the information in the HINT is factual. Using a DETAIL message and stating that a restart is required in the primary message may be more appropriate. I also slightly tweaked the message shown. I suggest:

WARNING:  Enabling 'logging_collector': restart required
DETAIL:  You must restart this PostgreSQL instance for ClassDB connection logging to function.

[smurthys]

I am curious about the choice to turn on logging_collector at end. Shouldn't that be done before reloading the changed config?

[wildtayne]

Technically, pg_reload_conf() does not need to be called when logging_collector is set, because the server must be restarted for the setting to take effect. Once the server is restarted, all new settings value are used, even if pg_reload_conf() was never called. That being said, I had arranged the ALTER SYSTEM statements in this way to get around the perceived limitation that we would need to store the value of logging_collector.

After thinking about this a bit, I realized that logging_collector always reflects the current effective value, so setting logging_collector before the IF statement does not matter. I initially rewrote this to store logging_collector in a temporary table, which works but is not really necessary. If we feel that storing the original value is safer, I can re-add that code.

@afig Thanks for linking the exception style guide. It's pretty helpful that they actually documented this. I've updated the error message based on your suggestion.

For reference, here is the temp table code:

CREATE TEMPORARY TABLE LoggingCollectorValue AS
SELECT COALESCE(setting::BOOLEAN, FALSE) AS value
FROM pg_settings
WHERE name = 'logging_collector';

[smurthys]

Actually, I had written the wrong setting name. I meant to say log_connections, which indeed must be set before reloading config. However, I see the newest commit does this correctly.

IMO, logging_collector should be conditionally turned ON just before raising warning.

The only reason to turn on logging_collector early is if config requires it in that order.

[wildtayne]

The order does not matter. I have placed logging_collector at the end in the latest commit.

Edit: I have also made updated to relevant documentation: Setup and Managing Log Files

[smurthys]

I agree logging_collector can be forcibly turned on before checking if it is on, but that code does not flow well and is not as maintainable as turning that parameter on conditionally:

The following organization is more natural:

set parameters other than logging_collector
if logging_collector is off
   set logging_collector on
   raise warning
else
  reload configuration

[afig]

Thanks for taking the time to update the docs @srrollo. I agree that the organization pointed out by @smurthys in the previous comment is preferred, however, it cannot be easily implemented because setting logging_collector to on uses the ALTER SYSTEM command, which cannot be performed inside of a transaction block.

Apart from this remaining discussion, I think this looks good to merge.

[smurthys]

Don't mind this absent-minded professor. At least in this thread. I'll strive to be more conscious.