Emit error if pg_ivm is not in session/shared_preload_libraries

[adamguo0]

re #119

Since pg_ivm registers hooks and transaction callbacks, it needs to be loaded in session_preload_libraries or shared_preload_libraries. However, even if pg_ivm is not preloaded, a user can still use the extension like normal. The hooks will just be silently ignored, which can cause the metadata table to go out of sync.

This PR throws an error if the user tries to CREATE EXTENSION pg_ivm but pg_ivm is not in the preload libraries. It also emits warnings when the pg_ivm SQL functions or triggers are used but pg_ivm is not in preload libraries.

A new SQL test file is added for this functionality.

[yugo-n]

Thank you for your proposal.
However, I wonder we don't need tests for this warnings.
Probably, it is enough to add a simple check in PG_init like the following, for example.
https://github.com/pgaudit/pgaudit/blob/main/pgaudit.c#L2111

[adamguo0]

Thanks for taking a look! I looked and could not find an equivalent to process_shared_preload_libraries_in_progress for session_preload_libraries, not sure if I missed something.

If I understand correctly, _PG_init is executed once per session backend, whereas I think we should emit the warning every time the user calls a pg_ivm function or executes a pg_ivm trigger.

[yugo-n]

If we have to allow user to load the module by session_preload_libraries, that check may be useful. However, I don't feel the necessity and requiring to use shared_preload_libraries seems simple solution with lesser code-maintenance cost.

[adamguo0]

Hmm I think allowing users to load pg_ivm in session_preload_libraries can be quite helpful since it does not require them to restart Postgres in order to use pg_ivm. I will look for a cleaner solution here

[adamguo0]

Hi @yugo-n, I agree that the WARNINGs are somewhat overkill and required a lot of extra C code to implement. I decided to remove the WARNINGs and just keep the ERRORs in the SQL installation scripts. This significantly reduces the size of this change. Hope you can take another look, thanks!

[yugo-n]

I'm sorry for late reply.
After a bit thoughts, I still think raising the error message during installation is also overkill.
This force users to configure postgresql.conf before executing CREATE EXTENSION.
I wonder other extensions requiring session/shared_preload_libraries do not check so strictly.

[adamguo0]

This force users to configure postgresql.conf before executing CREATE EXTENSION.

IMO this is a good thing, since after all pg_ivm requires users to set session_preload_libraries or shared_preload_libraries in order to use the extension properly. Otherwise, I can see users missing that warning in the documentation and making a mistake in their deployment, which may result in an inconsistent pg_ivm_immv metadata table.

I think it's a fairly common requirement, e.g. pgaudit. pg_stat_statements uses a slightly different approach where _PG_init doesn't check for shared_preload_libraries but each function checks separately.

[yugo-n]

IMO this is a good thing, since after all pg_ivm requires users to set session_preload_libraries or shared_preload_libraries in order to use the extension properly.

However, even if we check it at installation, the configuration can be changed afterward and we don't prevent it completely.

I think it's a fairly common requirement, e.g. pgaudit. pg_stat_statements uses a slightly different approach where _PG_init doesn't check for shared_preload_libraries but each function checks separately.

I prefer this approach to checking in a installation/update script if we would save careless users.
Although session_preload_librariess could not be checked in this way, I think we can make setting shared_preload_librariess mandatory.

[adamguo0]

I think making shared_preload_libraries mandatory is a little overkill for this purpose -- it doesn't seem worth requiring users to reboot Postgres if they need to enable/disable the extension. I'll think about this some more, thanks for looking!

[adamguo0]

Hi @yugo-n, what if we provide an explicit pgivm.drop_immv function, and use a DDL trigger to prevent users from calling DROP TABLE immv? I believe that way we can avoid the requirement for shared/session_preload_libraries.

[yugo-n]

What about other callback functions registered by RegisterXactCallback etc. in Pg_init ?

[adamguo0]

The PG_init function is called once Postgres calls the IMMV trigger functions. I did a quick test where pg_ivm is not preloaded, and the xact/subxact callbacks are still triggered. So currently I think only the object access hook requires SPL (though there may be further requirements in the future).

[michaelpq]

I have been looking at the patch series posted here, at the request of Adam, and I have to say that the "warnings" branch posted is an incremental mess, TBH. The patch is not organized, and the tests are IMO bloated.

And nothing proposed in the patch set is actually doing something about the fact that the restriction about session_preload_libraries and shared_preload_libraries should be enforced when a backend starts. For example, taking a diff with a rebased "warnings" branch and branch "main" at 96fdf6f:

$ git diff main --stat
 .gitignore                             | 28 +++++++++++++++++++++++++---
 Makefile                               |  2 +-
 expected/pg_ivm.out                    |  8 ++++++++
 expected/preload_libraries_warning.out | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 pg_ivm--1.10.sql                       | 21 +++++++++++++++++++++
 pg_ivm--1.9--1.10.sql                  | 21 +++++++++++++++++++++
 sql/pg_ivm.sql                         |  4 ++++
 sql/preload_libraries_warning.sql      | 28 ++++++++++++++++++++++++++++
 8 files changed, 159 insertions(+), 4 deletions(-)

The only restriction you have put in place results in a weird state, where one would only detect the problem when running in CREATE EXTENSION. This does not take care of the problem where the extension is created, and the server is restarted. The moral of the story is that you have to include a restriction of some kind in _PG_init(), which is the path that backends take, or you will never be able to make the drops safe as the library needs to be loaded.

Side note 1: there is no need for pg_ivm--1.10.sql, only the upgrade scripts are OK so that's one duplication less.
Side note 2: I'd suggest to just force a rule based on shared_preload_libraries, that's going to keep the code much simpler. This would still break your dependencies, but that removes from the equation the fact that session_preload_libraries can be manipulated by superusers as it is a SUSET parameter.

So, Nagata-san, I'd suggest to make things depend on shared_preload_libraries with a simple check in _PG_init(), like pgaudit, and call it a day. I don't see an express need for tests.