agora inbox for pgsql-hackers@postgresql.orghelp / color / mirror / Atom feed
[PATCH v6 2/2] Remove logic for unloading dynamically loaded files. 7+ messages / 2 participants [nested] [flat]
* [PATCH v7 2/2] Remove logic for unloading dynamically loaded files. @ 2022-05-06 20:53 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2022-05-06 20:53 UTC (permalink / raw) The code for unloading a library has been commented-out for over 12 years (602a9ef), and we're no closer to supporting it now than we were back then. --- contrib/auto_explain/auto_explain.c | 14 ---- contrib/passwordcheck/passwordcheck.c | 11 --- .../pg_stat_statements/pg_stat_statements.c | 19 ----- doc/src/sgml/xfunc.sgml | 18 +---- src/backend/postmaster/pgarch.c | 2 +- src/backend/utils/fmgr/dfmgr.c | 77 ++----------------- src/backend/utils/fmgr/fmgr.c | 14 ---- src/include/fmgr.h | 1 - src/pl/plpgsql/src/plpgsql.h | 2 - .../modules/delay_execution/delay_execution.c | 10 +-- .../ssl_passphrase_func.c | 7 -- .../modules/test_oat_hooks/test_oat_hooks.c | 22 +----- .../modules/test_rls_hooks/test_rls_hooks.c | 17 ---- 13 files changed, 13 insertions(+), 201 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index d3029f85ef..c9a0d947c8 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -77,7 +77,6 @@ static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; void _PG_init(void); -void _PG_fini(void); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, @@ -244,19 +243,6 @@ _PG_init(void) ExecutorEnd_hook = explain_ExecutorEnd; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; -} - /* * ExecutorStart hook: start up logging if needed */ diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c index 074836336d..9d8c58ded0 100644 --- a/contrib/passwordcheck/passwordcheck.c +++ b/contrib/passwordcheck/passwordcheck.c @@ -33,7 +33,6 @@ static check_password_hook_type prev_check_password_hook = NULL; #define MIN_PWD_LENGTH 8 extern void _PG_init(void); -extern void _PG_fini(void); /* * check_password @@ -149,13 +148,3 @@ _PG_init(void) prev_check_password_hook = check_password_hook; check_password_hook = check_password; } - -/* - * Module unload function - */ -void -_PG_fini(void) -{ - /* uninstall hook */ - check_password_hook = prev_check_password_hook; -} diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 87b75d779e..9ea887d887 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -306,7 +306,6 @@ static bool pgss_save; /* whether to save stats across shutdown */ /*---- Function declarations ----*/ void _PG_init(void); -void _PG_fini(void); PG_FUNCTION_INFO_V1(pg_stat_statements_reset); PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7); @@ -477,24 +476,6 @@ _PG_init(void) ProcessUtility_hook = pgss_ProcessUtility; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - shmem_request_hook = prev_shmem_request_hook; - shmem_startup_hook = prev_shmem_startup_hook; - post_parse_analyze_hook = prev_post_parse_analyze_hook; - planner_hook = prev_planner_hook; - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; - ProcessUtility_hook = prev_ProcessUtility; -} - /* * shmem_request hook: request additional shared resources. We'll allocate or * attach to the shared resources in pgss_shmem_startup(). diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 07c5fd198b..fbe718e3c2 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -1978,28 +1978,16 @@ PG_MODULE_MAGIC; <indexterm zone="xfunc-c-dynload"> <primary>_PG_init</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>_PG_fini</primary> - </indexterm> <indexterm zone="xfunc-c-dynload"> <primary>library initialization function</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>library finalization function</primary> - </indexterm> <para> - Optionally, a dynamically loaded file can contain initialization and - finalization functions. If the file includes a function named + Optionally, a dynamically loaded file can contain an initialization + function. If the file includes a function named <function>_PG_init</function>, that function will be called immediately after loading the file. The function receives no parameters and should - return void. If the file includes a function named - <function>_PG_fini</function>, that function will be called immediately before - unloading the file. Likewise, the function receives no parameters and - should return void. Note that <function>_PG_fini</function> will only be called - during an unload of the file, not during process termination. - (Presently, unloads are disabled and will never occur, but this may - change in the future.) + return void. There is presently no way to unload a dynamically loaded file. </para> </sect2> diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 0c8ca29f73..8beff4a53c 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -802,7 +802,7 @@ HandlePgArchInterrupts(void) * Ideally, we would simply unload the previous archive module and * load the new one, but there is presently no mechanism for * unloading a library (see the comment above - * internal_unload_library()). To deal with this, we simply restart + * internal_load_library()). To deal with this, we simply restart * the archiver. The new archive module will be loaded when the new * archiver process starts up. */ diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 3774f33e0e..7f9ea97280 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -37,9 +37,8 @@ #include "utils/hsearch.h" -/* signatures for PostgreSQL-specific library init/fini functions */ +/* signature for PostgreSQL-specific library init function */ typedef void (*PG_init_t) (void); -typedef void (*PG_fini_t) (void); /* hashtable entry for rendezvous variables */ typedef struct @@ -79,7 +78,6 @@ char *Dynamic_library_path; static void *internal_load_library(const char *libname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); -static void internal_unload_library(const char *libname); static bool file_exists(const char *name); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); @@ -154,9 +152,6 @@ load_file(const char *filename, bool restricted) /* Expand the possibly-abbreviated filename to an exact path name */ fullname = expand_dynamic_library_name(filename); - /* Unload the library if currently loaded */ - internal_unload_library(fullname); - /* Load the shared library */ (void) internal_load_library(fullname); @@ -179,6 +174,11 @@ lookup_external_function(void *filehandle, const char *funcname) * loaded. Return the pg_dl* handle for the file. * * Note: libname is expected to be an exact name for the library file. + * + * NB: There is presently no way to unload a dynamically loaded file. We might + * add one someday if we can convince ourselves we have safe protocols for un- + * hooking from hook function pointers, releasing custom GUC variables, and + * perhaps other things that are definitely unsafe currently. */ static void * internal_load_library(const char *libname) @@ -400,71 +400,6 @@ incompatible_module_error(const char *libname, errdetail_internal("%s", details.data))); } -/* - * Unload the specified dynamic-link library file, if it is loaded. - * - * Note: libname is expected to be an exact name for the library file. - * - * XXX for the moment, this is disabled, resulting in LOAD of an already-loaded - * library always being a no-op. We might re-enable it someday if we can - * convince ourselves we have safe protocols for un-hooking from hook function - * pointers, releasing custom GUC variables, and perhaps other things that - * are definitely unsafe currently. - */ -static void -internal_unload_library(const char *libname) -{ -#ifdef NOT_USED - DynamicFileList *file_scanner, - *prv, - *nxt; - struct stat stat_buf; - PG_fini_t PG_fini; - - /* - * We need to do stat() in order to determine whether this is the same - * file as a previously loaded file; it's also handy so as to give a good - * error message if bogus file name given. - */ - if (stat(libname, &stat_buf) == -1) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not access file \"%s\": %m", libname))); - - /* - * We have to zap all entries in the list that match on either filename or - * inode, else internal_load_library() will still think it's present. - */ - prv = NULL; - for (file_scanner = file_list; file_scanner != NULL; file_scanner = nxt) - { - nxt = file_scanner->next; - if (strcmp(libname, file_scanner->filename) == 0 || - SAME_INODE(stat_buf, *file_scanner)) - { - if (prv) - prv->next = nxt; - else - file_list = nxt; - - /* - * If the library has a _PG_fini() function, call it. - */ - PG_fini = (PG_fini_t) dlsym(file_scanner->handle, "_PG_fini"); - if (PG_fini) - (*PG_fini) (); - - clear_external_function_hash(file_scanner->handle); - dlclose(file_scanner->handle); - free((char *) file_scanner); - /* prv does not change */ - } - else - prv = file_scanner; - } -#endif /* NOT_USED */ -} - static bool file_exists(const char *name) { diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index b2e72b3243..a9dd068095 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -582,20 +582,6 @@ record_C_func(HeapTuple procedureTuple, entry->inforec = inforec; } -/* - * clear_external_function_hash: remove entries for a library being closed - * - * Presently we just zap the entire hash table, but later it might be worth - * the effort to remove only the entries associated with the given handle. - */ -void -clear_external_function_hash(void *filehandle) -{ - if (CFuncHash) - hash_destroy(CFuncHash); - CFuncHash = NULL; -} - /* * Copy an FmgrInfo struct diff --git a/src/include/fmgr.h b/src/include/fmgr.h index a1cf4bd646..d55abc5414 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -705,7 +705,6 @@ extern bytea *OidSendFunctionCall(Oid functionId, Datum val); * Routines in fmgr.c */ extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, const char *funcname); -extern void clear_external_function_hash(void *filehandle); extern Oid fmgr_internal_function(const char *proname); extern Oid get_fn_expr_rettype(FmgrInfo *flinfo); extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 6444347ce9..4e6ee1c619 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1100,8 +1100,6 @@ typedef struct PLpgSQL_execstate * variable "PLpgSQL_plugin" and set it to point to a PLpgSQL_plugin struct. * Typically the struct could just be static data in the plugin library. * We expect that a plugin would do this at library load time (_PG_init()). - * It must also be careful to set the rendezvous variable back to NULL - * if it is unloaded (_PG_fini()). * * This structure is basically a collection of function pointers --- at * various interesting points in pl_exec.c, we call these functions diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c index cf34e8c2d7..407ebc0eda 100644 --- a/src/test/modules/delay_execution/delay_execution.c +++ b/src/test/modules/delay_execution/delay_execution.c @@ -36,9 +36,8 @@ static int post_planning_lock_id = 0; /* Save previous planner hook user to be a good citizen */ static planner_hook_type prev_planner_hook = NULL; -/* Module load/unload functions */ +/* Module load function */ void _PG_init(void); -void _PG_fini(void); /* planner_hook function to provide the desired delay */ @@ -97,10 +96,3 @@ _PG_init(void) prev_planner_hook = planner_hook; planner_hook = delay_execution_planner; } - -/* Module unload function (pro forma, not used currently) */ -void -_PG_fini(void) -{ - planner_hook = prev_planner_hook; -} diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c index b5bb5580a0..e9f2329a5a 100644 --- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c +++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c @@ -21,7 +21,6 @@ PG_MODULE_MAGIC; void _PG_init(void); -void _PG_fini(void); static char *ssl_passphrase = NULL; @@ -55,12 +54,6 @@ _PG_init(void) openssl_tls_init_hook = set_rot13; } -void -_PG_fini(void) -{ - /* do nothing yet */ -} - static void set_rot13(SSL_CTX *context, bool isServerStart) { diff --git a/src/test/modules/test_oat_hooks/test_oat_hooks.c b/src/test/modules/test_oat_hooks/test_oat_hooks.c index 551da5d498..6f9838f93b 100644 --- a/src/test/modules/test_oat_hooks/test_oat_hooks.c +++ b/src/test/modules/test_oat_hooks/test_oat_hooks.c @@ -43,7 +43,7 @@ static bool REGRESS_userset_variable2 = false; static bool REGRESS_suset_variable1 = false; static bool REGRESS_suset_variable2 = false; -/* Saved hook values in case of unload */ +/* Saved hook values */ static object_access_hook_type next_object_access_hook = NULL; static object_access_hook_type_str next_object_access_hook_str = NULL; static ExecutorCheckPerms_hook_type next_exec_check_perms_hook = NULL; @@ -70,10 +70,9 @@ static char *accesstype_arg_to_string(ObjectAccessType access, void *arg); void _PG_init(void); -void _PG_fini(void); /* - * Module load/unload callback + * Module load callback */ void _PG_init(void) @@ -231,23 +230,6 @@ _PG_init(void) ProcessUtility_hook = REGRESS_utility_command; } -void -_PG_fini(void) -{ - /* Unload hooks */ - if (object_access_hook == REGRESS_object_access_hook) - object_access_hook = next_object_access_hook; - - if (object_access_hook_str == REGRESS_object_access_hook_str) - object_access_hook_str = next_object_access_hook_str; - - if (ExecutorCheckPerms_hook == REGRESS_exec_check_perms) - ExecutorCheckPerms_hook = next_exec_check_perms_hook; - - if (ProcessUtility_hook == REGRESS_utility_command) - ProcessUtility_hook = next_ProcessUtility_hook; -} - static void emit_audit_message(const char *type, const char *hook, char *action, char *objName) { diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c index 4f862d081b..b8e0aa2d0b 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -29,34 +29,17 @@ PG_MODULE_MAGIC; -/* Saved hook values in case of unload */ -static row_security_policy_hook_type prev_row_security_policy_hook_permissive = NULL; -static row_security_policy_hook_type prev_row_security_policy_hook_restrictive = NULL; - void _PG_init(void); -void _PG_fini(void); /* Install hooks */ void _PG_init(void) { - /* Save values for unload */ - prev_row_security_policy_hook_permissive = row_security_policy_hook_permissive; - prev_row_security_policy_hook_restrictive = row_security_policy_hook_restrictive; - /* Set our hooks */ row_security_policy_hook_permissive = test_rls_hooks_permissive; row_security_policy_hook_restrictive = test_rls_hooks_restrictive; } -/* Uninstall hooks */ -void -_PG_fini(void) -{ - row_security_policy_hook_permissive = prev_row_security_policy_hook_permissive; - row_security_policy_hook_restrictive = prev_row_security_policy_hook_restrictive; -} - /* * Return permissive policies to be added */ -- 2.25.1 --SUOF0GtieIMvvwua-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v6 2/2] Remove logic for unloading dynamically loaded files. @ 2022-05-06 20:53 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2022-05-06 20:53 UTC (permalink / raw) The code for unloading a library has been commented-out for over 12 years (602a9ef), and we're no closer to supporting it now than we were back then. --- contrib/auto_explain/auto_explain.c | 14 ---- contrib/passwordcheck/passwordcheck.c | 11 --- .../pg_stat_statements/pg_stat_statements.c | 19 ----- doc/src/sgml/xfunc.sgml | 12 +-- src/backend/postmaster/pgarch.c | 2 +- src/backend/utils/fmgr/dfmgr.c | 77 ++----------------- src/backend/utils/fmgr/fmgr.c | 14 ---- src/include/fmgr.h | 1 - src/pl/plpgsql/src/plpgsql.h | 2 - .../modules/delay_execution/delay_execution.c | 10 +-- .../ssl_passphrase_func.c | 7 -- .../modules/test_oat_hooks/test_oat_hooks.c | 22 +----- .../modules/test_rls_hooks/test_rls_hooks.c | 17 ---- 13 files changed, 13 insertions(+), 195 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index d3029f85ef..c9a0d947c8 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -77,7 +77,6 @@ static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; void _PG_init(void); -void _PG_fini(void); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, @@ -244,19 +243,6 @@ _PG_init(void) ExecutorEnd_hook = explain_ExecutorEnd; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; -} - /* * ExecutorStart hook: start up logging if needed */ diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c index 074836336d..9d8c58ded0 100644 --- a/contrib/passwordcheck/passwordcheck.c +++ b/contrib/passwordcheck/passwordcheck.c @@ -33,7 +33,6 @@ static check_password_hook_type prev_check_password_hook = NULL; #define MIN_PWD_LENGTH 8 extern void _PG_init(void); -extern void _PG_fini(void); /* * check_password @@ -149,13 +148,3 @@ _PG_init(void) prev_check_password_hook = check_password_hook; check_password_hook = check_password; } - -/* - * Module unload function - */ -void -_PG_fini(void) -{ - /* uninstall hook */ - check_password_hook = prev_check_password_hook; -} diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 87b75d779e..9ea887d887 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -306,7 +306,6 @@ static bool pgss_save; /* whether to save stats across shutdown */ /*---- Function declarations ----*/ void _PG_init(void); -void _PG_fini(void); PG_FUNCTION_INFO_V1(pg_stat_statements_reset); PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7); @@ -477,24 +476,6 @@ _PG_init(void) ProcessUtility_hook = pgss_ProcessUtility; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - shmem_request_hook = prev_shmem_request_hook; - shmem_startup_hook = prev_shmem_startup_hook; - post_parse_analyze_hook = prev_post_parse_analyze_hook; - planner_hook = prev_planner_hook; - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; - ProcessUtility_hook = prev_ProcessUtility; -} - /* * shmem_request hook: request additional shared resources. We'll allocate or * attach to the shared resources in pgss_shmem_startup(). diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 07c5fd198b..eebd5503e4 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -1989,17 +1989,11 @@ PG_MODULE_MAGIC; </indexterm> <para> - Optionally, a dynamically loaded file can contain initialization and - finalization functions. If the file includes a function named + Optionally, a dynamically loaded file can contain an initialization + function. If the file includes a function named <function>_PG_init</function>, that function will be called immediately after loading the file. The function receives no parameters and should - return void. If the file includes a function named - <function>_PG_fini</function>, that function will be called immediately before - unloading the file. Likewise, the function receives no parameters and - should return void. Note that <function>_PG_fini</function> will only be called - during an unload of the file, not during process termination. - (Presently, unloads are disabled and will never occur, but this may - change in the future.) + return void. There is presently no way to unload a dynamically loaded file. </para> </sect2> diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 0c8ca29f73..8beff4a53c 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -802,7 +802,7 @@ HandlePgArchInterrupts(void) * Ideally, we would simply unload the previous archive module and * load the new one, but there is presently no mechanism for * unloading a library (see the comment above - * internal_unload_library()). To deal with this, we simply restart + * internal_load_library()). To deal with this, we simply restart * the archiver. The new archive module will be loaded when the new * archiver process starts up. */ diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 3774f33e0e..7f9ea97280 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -37,9 +37,8 @@ #include "utils/hsearch.h" -/* signatures for PostgreSQL-specific library init/fini functions */ +/* signature for PostgreSQL-specific library init function */ typedef void (*PG_init_t) (void); -typedef void (*PG_fini_t) (void); /* hashtable entry for rendezvous variables */ typedef struct @@ -79,7 +78,6 @@ char *Dynamic_library_path; static void *internal_load_library(const char *libname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); -static void internal_unload_library(const char *libname); static bool file_exists(const char *name); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); @@ -154,9 +152,6 @@ load_file(const char *filename, bool restricted) /* Expand the possibly-abbreviated filename to an exact path name */ fullname = expand_dynamic_library_name(filename); - /* Unload the library if currently loaded */ - internal_unload_library(fullname); - /* Load the shared library */ (void) internal_load_library(fullname); @@ -179,6 +174,11 @@ lookup_external_function(void *filehandle, const char *funcname) * loaded. Return the pg_dl* handle for the file. * * Note: libname is expected to be an exact name for the library file. + * + * NB: There is presently no way to unload a dynamically loaded file. We might + * add one someday if we can convince ourselves we have safe protocols for un- + * hooking from hook function pointers, releasing custom GUC variables, and + * perhaps other things that are definitely unsafe currently. */ static void * internal_load_library(const char *libname) @@ -400,71 +400,6 @@ incompatible_module_error(const char *libname, errdetail_internal("%s", details.data))); } -/* - * Unload the specified dynamic-link library file, if it is loaded. - * - * Note: libname is expected to be an exact name for the library file. - * - * XXX for the moment, this is disabled, resulting in LOAD of an already-loaded - * library always being a no-op. We might re-enable it someday if we can - * convince ourselves we have safe protocols for un-hooking from hook function - * pointers, releasing custom GUC variables, and perhaps other things that - * are definitely unsafe currently. - */ -static void -internal_unload_library(const char *libname) -{ -#ifdef NOT_USED - DynamicFileList *file_scanner, - *prv, - *nxt; - struct stat stat_buf; - PG_fini_t PG_fini; - - /* - * We need to do stat() in order to determine whether this is the same - * file as a previously loaded file; it's also handy so as to give a good - * error message if bogus file name given. - */ - if (stat(libname, &stat_buf) == -1) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not access file \"%s\": %m", libname))); - - /* - * We have to zap all entries in the list that match on either filename or - * inode, else internal_load_library() will still think it's present. - */ - prv = NULL; - for (file_scanner = file_list; file_scanner != NULL; file_scanner = nxt) - { - nxt = file_scanner->next; - if (strcmp(libname, file_scanner->filename) == 0 || - SAME_INODE(stat_buf, *file_scanner)) - { - if (prv) - prv->next = nxt; - else - file_list = nxt; - - /* - * If the library has a _PG_fini() function, call it. - */ - PG_fini = (PG_fini_t) dlsym(file_scanner->handle, "_PG_fini"); - if (PG_fini) - (*PG_fini) (); - - clear_external_function_hash(file_scanner->handle); - dlclose(file_scanner->handle); - free((char *) file_scanner); - /* prv does not change */ - } - else - prv = file_scanner; - } -#endif /* NOT_USED */ -} - static bool file_exists(const char *name) { diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index b2e72b3243..a9dd068095 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -582,20 +582,6 @@ record_C_func(HeapTuple procedureTuple, entry->inforec = inforec; } -/* - * clear_external_function_hash: remove entries for a library being closed - * - * Presently we just zap the entire hash table, but later it might be worth - * the effort to remove only the entries associated with the given handle. - */ -void -clear_external_function_hash(void *filehandle) -{ - if (CFuncHash) - hash_destroy(CFuncHash); - CFuncHash = NULL; -} - /* * Copy an FmgrInfo struct diff --git a/src/include/fmgr.h b/src/include/fmgr.h index a1cf4bd646..d55abc5414 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -705,7 +705,6 @@ extern bytea *OidSendFunctionCall(Oid functionId, Datum val); * Routines in fmgr.c */ extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, const char *funcname); -extern void clear_external_function_hash(void *filehandle); extern Oid fmgr_internal_function(const char *proname); extern Oid get_fn_expr_rettype(FmgrInfo *flinfo); extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 6444347ce9..4e6ee1c619 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1100,8 +1100,6 @@ typedef struct PLpgSQL_execstate * variable "PLpgSQL_plugin" and set it to point to a PLpgSQL_plugin struct. * Typically the struct could just be static data in the plugin library. * We expect that a plugin would do this at library load time (_PG_init()). - * It must also be careful to set the rendezvous variable back to NULL - * if it is unloaded (_PG_fini()). * * This structure is basically a collection of function pointers --- at * various interesting points in pl_exec.c, we call these functions diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c index cf34e8c2d7..407ebc0eda 100644 --- a/src/test/modules/delay_execution/delay_execution.c +++ b/src/test/modules/delay_execution/delay_execution.c @@ -36,9 +36,8 @@ static int post_planning_lock_id = 0; /* Save previous planner hook user to be a good citizen */ static planner_hook_type prev_planner_hook = NULL; -/* Module load/unload functions */ +/* Module load function */ void _PG_init(void); -void _PG_fini(void); /* planner_hook function to provide the desired delay */ @@ -97,10 +96,3 @@ _PG_init(void) prev_planner_hook = planner_hook; planner_hook = delay_execution_planner; } - -/* Module unload function (pro forma, not used currently) */ -void -_PG_fini(void) -{ - planner_hook = prev_planner_hook; -} diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c index b5bb5580a0..e9f2329a5a 100644 --- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c +++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c @@ -21,7 +21,6 @@ PG_MODULE_MAGIC; void _PG_init(void); -void _PG_fini(void); static char *ssl_passphrase = NULL; @@ -55,12 +54,6 @@ _PG_init(void) openssl_tls_init_hook = set_rot13; } -void -_PG_fini(void) -{ - /* do nothing yet */ -} - static void set_rot13(SSL_CTX *context, bool isServerStart) { diff --git a/src/test/modules/test_oat_hooks/test_oat_hooks.c b/src/test/modules/test_oat_hooks/test_oat_hooks.c index 551da5d498..6f9838f93b 100644 --- a/src/test/modules/test_oat_hooks/test_oat_hooks.c +++ b/src/test/modules/test_oat_hooks/test_oat_hooks.c @@ -43,7 +43,7 @@ static bool REGRESS_userset_variable2 = false; static bool REGRESS_suset_variable1 = false; static bool REGRESS_suset_variable2 = false; -/* Saved hook values in case of unload */ +/* Saved hook values */ static object_access_hook_type next_object_access_hook = NULL; static object_access_hook_type_str next_object_access_hook_str = NULL; static ExecutorCheckPerms_hook_type next_exec_check_perms_hook = NULL; @@ -70,10 +70,9 @@ static char *accesstype_arg_to_string(ObjectAccessType access, void *arg); void _PG_init(void); -void _PG_fini(void); /* - * Module load/unload callback + * Module load callback */ void _PG_init(void) @@ -231,23 +230,6 @@ _PG_init(void) ProcessUtility_hook = REGRESS_utility_command; } -void -_PG_fini(void) -{ - /* Unload hooks */ - if (object_access_hook == REGRESS_object_access_hook) - object_access_hook = next_object_access_hook; - - if (object_access_hook_str == REGRESS_object_access_hook_str) - object_access_hook_str = next_object_access_hook_str; - - if (ExecutorCheckPerms_hook == REGRESS_exec_check_perms) - ExecutorCheckPerms_hook = next_exec_check_perms_hook; - - if (ProcessUtility_hook == REGRESS_utility_command) - ProcessUtility_hook = next_ProcessUtility_hook; -} - static void emit_audit_message(const char *type, const char *hook, char *action, char *objName) { diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c index 4f862d081b..b8e0aa2d0b 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -29,34 +29,17 @@ PG_MODULE_MAGIC; -/* Saved hook values in case of unload */ -static row_security_policy_hook_type prev_row_security_policy_hook_permissive = NULL; -static row_security_policy_hook_type prev_row_security_policy_hook_restrictive = NULL; - void _PG_init(void); -void _PG_fini(void); /* Install hooks */ void _PG_init(void) { - /* Save values for unload */ - prev_row_security_policy_hook_permissive = row_security_policy_hook_permissive; - prev_row_security_policy_hook_restrictive = row_security_policy_hook_restrictive; - /* Set our hooks */ row_security_policy_hook_permissive = test_rls_hooks_permissive; row_security_policy_hook_restrictive = test_rls_hooks_restrictive; } -/* Uninstall hooks */ -void -_PG_fini(void) -{ - row_security_policy_hook_permissive = prev_row_security_policy_hook_permissive; - row_security_policy_hook_restrictive = prev_row_security_policy_hook_restrictive; -} - /* * Return permissive policies to be added */ -- 2.25.1 --6TrnltStXW4iwmi0-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v7 2/2] Remove logic for unloading dynamically loaded files. @ 2022-05-06 20:53 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2022-05-06 20:53 UTC (permalink / raw) The code for unloading a library has been commented-out for over 12 years (602a9ef), and we're no closer to supporting it now than we were back then. --- contrib/auto_explain/auto_explain.c | 14 ---- contrib/passwordcheck/passwordcheck.c | 11 --- .../pg_stat_statements/pg_stat_statements.c | 19 ----- doc/src/sgml/xfunc.sgml | 18 +---- src/backend/postmaster/pgarch.c | 2 +- src/backend/utils/fmgr/dfmgr.c | 77 ++----------------- src/backend/utils/fmgr/fmgr.c | 14 ---- src/include/fmgr.h | 1 - src/pl/plpgsql/src/plpgsql.h | 2 - .../modules/delay_execution/delay_execution.c | 10 +-- .../ssl_passphrase_func.c | 7 -- .../modules/test_oat_hooks/test_oat_hooks.c | 22 +----- .../modules/test_rls_hooks/test_rls_hooks.c | 17 ---- 13 files changed, 13 insertions(+), 201 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index d3029f85ef..c9a0d947c8 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -77,7 +77,6 @@ static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; void _PG_init(void); -void _PG_fini(void); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, @@ -244,19 +243,6 @@ _PG_init(void) ExecutorEnd_hook = explain_ExecutorEnd; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; -} - /* * ExecutorStart hook: start up logging if needed */ diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c index 074836336d..9d8c58ded0 100644 --- a/contrib/passwordcheck/passwordcheck.c +++ b/contrib/passwordcheck/passwordcheck.c @@ -33,7 +33,6 @@ static check_password_hook_type prev_check_password_hook = NULL; #define MIN_PWD_LENGTH 8 extern void _PG_init(void); -extern void _PG_fini(void); /* * check_password @@ -149,13 +148,3 @@ _PG_init(void) prev_check_password_hook = check_password_hook; check_password_hook = check_password; } - -/* - * Module unload function - */ -void -_PG_fini(void) -{ - /* uninstall hook */ - check_password_hook = prev_check_password_hook; -} diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 87b75d779e..9ea887d887 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -306,7 +306,6 @@ static bool pgss_save; /* whether to save stats across shutdown */ /*---- Function declarations ----*/ void _PG_init(void); -void _PG_fini(void); PG_FUNCTION_INFO_V1(pg_stat_statements_reset); PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7); @@ -477,24 +476,6 @@ _PG_init(void) ProcessUtility_hook = pgss_ProcessUtility; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - shmem_request_hook = prev_shmem_request_hook; - shmem_startup_hook = prev_shmem_startup_hook; - post_parse_analyze_hook = prev_post_parse_analyze_hook; - planner_hook = prev_planner_hook; - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; - ProcessUtility_hook = prev_ProcessUtility; -} - /* * shmem_request hook: request additional shared resources. We'll allocate or * attach to the shared resources in pgss_shmem_startup(). diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 07c5fd198b..fbe718e3c2 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -1978,28 +1978,16 @@ PG_MODULE_MAGIC; <indexterm zone="xfunc-c-dynload"> <primary>_PG_init</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>_PG_fini</primary> - </indexterm> <indexterm zone="xfunc-c-dynload"> <primary>library initialization function</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>library finalization function</primary> - </indexterm> <para> - Optionally, a dynamically loaded file can contain initialization and - finalization functions. If the file includes a function named + Optionally, a dynamically loaded file can contain an initialization + function. If the file includes a function named <function>_PG_init</function>, that function will be called immediately after loading the file. The function receives no parameters and should - return void. If the file includes a function named - <function>_PG_fini</function>, that function will be called immediately before - unloading the file. Likewise, the function receives no parameters and - should return void. Note that <function>_PG_fini</function> will only be called - during an unload of the file, not during process termination. - (Presently, unloads are disabled and will never occur, but this may - change in the future.) + return void. There is presently no way to unload a dynamically loaded file. </para> </sect2> diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 0c8ca29f73..8beff4a53c 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -802,7 +802,7 @@ HandlePgArchInterrupts(void) * Ideally, we would simply unload the previous archive module and * load the new one, but there is presently no mechanism for * unloading a library (see the comment above - * internal_unload_library()). To deal with this, we simply restart + * internal_load_library()). To deal with this, we simply restart * the archiver. The new archive module will be loaded when the new * archiver process starts up. */ diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 3774f33e0e..7f9ea97280 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -37,9 +37,8 @@ #include "utils/hsearch.h" -/* signatures for PostgreSQL-specific library init/fini functions */ +/* signature for PostgreSQL-specific library init function */ typedef void (*PG_init_t) (void); -typedef void (*PG_fini_t) (void); /* hashtable entry for rendezvous variables */ typedef struct @@ -79,7 +78,6 @@ char *Dynamic_library_path; static void *internal_load_library(const char *libname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); -static void internal_unload_library(const char *libname); static bool file_exists(const char *name); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); @@ -154,9 +152,6 @@ load_file(const char *filename, bool restricted) /* Expand the possibly-abbreviated filename to an exact path name */ fullname = expand_dynamic_library_name(filename); - /* Unload the library if currently loaded */ - internal_unload_library(fullname); - /* Load the shared library */ (void) internal_load_library(fullname); @@ -179,6 +174,11 @@ lookup_external_function(void *filehandle, const char *funcname) * loaded. Return the pg_dl* handle for the file. * * Note: libname is expected to be an exact name for the library file. + * + * NB: There is presently no way to unload a dynamically loaded file. We might + * add one someday if we can convince ourselves we have safe protocols for un- + * hooking from hook function pointers, releasing custom GUC variables, and + * perhaps other things that are definitely unsafe currently. */ static void * internal_load_library(const char *libname) @@ -400,71 +400,6 @@ incompatible_module_error(const char *libname, errdetail_internal("%s", details.data))); } -/* - * Unload the specified dynamic-link library file, if it is loaded. - * - * Note: libname is expected to be an exact name for the library file. - * - * XXX for the moment, this is disabled, resulting in LOAD of an already-loaded - * library always being a no-op. We might re-enable it someday if we can - * convince ourselves we have safe protocols for un-hooking from hook function - * pointers, releasing custom GUC variables, and perhaps other things that - * are definitely unsafe currently. - */ -static void -internal_unload_library(const char *libname) -{ -#ifdef NOT_USED - DynamicFileList *file_scanner, - *prv, - *nxt; - struct stat stat_buf; - PG_fini_t PG_fini; - - /* - * We need to do stat() in order to determine whether this is the same - * file as a previously loaded file; it's also handy so as to give a good - * error message if bogus file name given. - */ - if (stat(libname, &stat_buf) == -1) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not access file \"%s\": %m", libname))); - - /* - * We have to zap all entries in the list that match on either filename or - * inode, else internal_load_library() will still think it's present. - */ - prv = NULL; - for (file_scanner = file_list; file_scanner != NULL; file_scanner = nxt) - { - nxt = file_scanner->next; - if (strcmp(libname, file_scanner->filename) == 0 || - SAME_INODE(stat_buf, *file_scanner)) - { - if (prv) - prv->next = nxt; - else - file_list = nxt; - - /* - * If the library has a _PG_fini() function, call it. - */ - PG_fini = (PG_fini_t) dlsym(file_scanner->handle, "_PG_fini"); - if (PG_fini) - (*PG_fini) (); - - clear_external_function_hash(file_scanner->handle); - dlclose(file_scanner->handle); - free((char *) file_scanner); - /* prv does not change */ - } - else - prv = file_scanner; - } -#endif /* NOT_USED */ -} - static bool file_exists(const char *name) { diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index b2e72b3243..a9dd068095 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -582,20 +582,6 @@ record_C_func(HeapTuple procedureTuple, entry->inforec = inforec; } -/* - * clear_external_function_hash: remove entries for a library being closed - * - * Presently we just zap the entire hash table, but later it might be worth - * the effort to remove only the entries associated with the given handle. - */ -void -clear_external_function_hash(void *filehandle) -{ - if (CFuncHash) - hash_destroy(CFuncHash); - CFuncHash = NULL; -} - /* * Copy an FmgrInfo struct diff --git a/src/include/fmgr.h b/src/include/fmgr.h index a1cf4bd646..d55abc5414 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -705,7 +705,6 @@ extern bytea *OidSendFunctionCall(Oid functionId, Datum val); * Routines in fmgr.c */ extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, const char *funcname); -extern void clear_external_function_hash(void *filehandle); extern Oid fmgr_internal_function(const char *proname); extern Oid get_fn_expr_rettype(FmgrInfo *flinfo); extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 6444347ce9..4e6ee1c619 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1100,8 +1100,6 @@ typedef struct PLpgSQL_execstate * variable "PLpgSQL_plugin" and set it to point to a PLpgSQL_plugin struct. * Typically the struct could just be static data in the plugin library. * We expect that a plugin would do this at library load time (_PG_init()). - * It must also be careful to set the rendezvous variable back to NULL - * if it is unloaded (_PG_fini()). * * This structure is basically a collection of function pointers --- at * various interesting points in pl_exec.c, we call these functions diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c index cf34e8c2d7..407ebc0eda 100644 --- a/src/test/modules/delay_execution/delay_execution.c +++ b/src/test/modules/delay_execution/delay_execution.c @@ -36,9 +36,8 @@ static int post_planning_lock_id = 0; /* Save previous planner hook user to be a good citizen */ static planner_hook_type prev_planner_hook = NULL; -/* Module load/unload functions */ +/* Module load function */ void _PG_init(void); -void _PG_fini(void); /* planner_hook function to provide the desired delay */ @@ -97,10 +96,3 @@ _PG_init(void) prev_planner_hook = planner_hook; planner_hook = delay_execution_planner; } - -/* Module unload function (pro forma, not used currently) */ -void -_PG_fini(void) -{ - planner_hook = prev_planner_hook; -} diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c index b5bb5580a0..e9f2329a5a 100644 --- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c +++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c @@ -21,7 +21,6 @@ PG_MODULE_MAGIC; void _PG_init(void); -void _PG_fini(void); static char *ssl_passphrase = NULL; @@ -55,12 +54,6 @@ _PG_init(void) openssl_tls_init_hook = set_rot13; } -void -_PG_fini(void) -{ - /* do nothing yet */ -} - static void set_rot13(SSL_CTX *context, bool isServerStart) { diff --git a/src/test/modules/test_oat_hooks/test_oat_hooks.c b/src/test/modules/test_oat_hooks/test_oat_hooks.c index 551da5d498..6f9838f93b 100644 --- a/src/test/modules/test_oat_hooks/test_oat_hooks.c +++ b/src/test/modules/test_oat_hooks/test_oat_hooks.c @@ -43,7 +43,7 @@ static bool REGRESS_userset_variable2 = false; static bool REGRESS_suset_variable1 = false; static bool REGRESS_suset_variable2 = false; -/* Saved hook values in case of unload */ +/* Saved hook values */ static object_access_hook_type next_object_access_hook = NULL; static object_access_hook_type_str next_object_access_hook_str = NULL; static ExecutorCheckPerms_hook_type next_exec_check_perms_hook = NULL; @@ -70,10 +70,9 @@ static char *accesstype_arg_to_string(ObjectAccessType access, void *arg); void _PG_init(void); -void _PG_fini(void); /* - * Module load/unload callback + * Module load callback */ void _PG_init(void) @@ -231,23 +230,6 @@ _PG_init(void) ProcessUtility_hook = REGRESS_utility_command; } -void -_PG_fini(void) -{ - /* Unload hooks */ - if (object_access_hook == REGRESS_object_access_hook) - object_access_hook = next_object_access_hook; - - if (object_access_hook_str == REGRESS_object_access_hook_str) - object_access_hook_str = next_object_access_hook_str; - - if (ExecutorCheckPerms_hook == REGRESS_exec_check_perms) - ExecutorCheckPerms_hook = next_exec_check_perms_hook; - - if (ProcessUtility_hook == REGRESS_utility_command) - ProcessUtility_hook = next_ProcessUtility_hook; -} - static void emit_audit_message(const char *type, const char *hook, char *action, char *objName) { diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c index 4f862d081b..b8e0aa2d0b 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -29,34 +29,17 @@ PG_MODULE_MAGIC; -/* Saved hook values in case of unload */ -static row_security_policy_hook_type prev_row_security_policy_hook_permissive = NULL; -static row_security_policy_hook_type prev_row_security_policy_hook_restrictive = NULL; - void _PG_init(void); -void _PG_fini(void); /* Install hooks */ void _PG_init(void) { - /* Save values for unload */ - prev_row_security_policy_hook_permissive = row_security_policy_hook_permissive; - prev_row_security_policy_hook_restrictive = row_security_policy_hook_restrictive; - /* Set our hooks */ row_security_policy_hook_permissive = test_rls_hooks_permissive; row_security_policy_hook_restrictive = test_rls_hooks_restrictive; } -/* Uninstall hooks */ -void -_PG_fini(void) -{ - row_security_policy_hook_permissive = prev_row_security_policy_hook_permissive; - row_security_policy_hook_restrictive = prev_row_security_policy_hook_restrictive; -} - /* * Return permissive policies to be added */ -- 2.25.1 --SUOF0GtieIMvvwua-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v8 1/2] Remove logic for unloading dynamically loaded files. @ 2022-05-06 20:53 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2022-05-06 20:53 UTC (permalink / raw) The code for unloading a library has been commented-out for over 12 years (602a9ef), and we're no closer to supporting it now than we were back then. --- contrib/auto_explain/auto_explain.c | 14 ---- contrib/passwordcheck/passwordcheck.c | 11 --- .../pg_stat_statements/pg_stat_statements.c | 18 ----- doc/src/sgml/xfunc.sgml | 18 +---- src/backend/postmaster/pgarch.c | 2 +- src/backend/utils/fmgr/dfmgr.c | 77 ++----------------- src/backend/utils/fmgr/fmgr.c | 14 ---- src/include/fmgr.h | 1 - src/pl/plpgsql/src/plpgsql.h | 2 - .../modules/delay_execution/delay_execution.c | 10 +-- .../ssl_passphrase_func.c | 7 -- .../modules/test_oat_hooks/test_oat_hooks.c | 22 +----- .../modules/test_rls_hooks/test_rls_hooks.c | 17 ---- 13 files changed, 13 insertions(+), 200 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index d3029f85ef..c9a0d947c8 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -77,7 +77,6 @@ static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; void _PG_init(void); -void _PG_fini(void); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, @@ -244,19 +243,6 @@ _PG_init(void) ExecutorEnd_hook = explain_ExecutorEnd; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; -} - /* * ExecutorStart hook: start up logging if needed */ diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c index 074836336d..9d8c58ded0 100644 --- a/contrib/passwordcheck/passwordcheck.c +++ b/contrib/passwordcheck/passwordcheck.c @@ -33,7 +33,6 @@ static check_password_hook_type prev_check_password_hook = NULL; #define MIN_PWD_LENGTH 8 extern void _PG_init(void); -extern void _PG_fini(void); /* * check_password @@ -149,13 +148,3 @@ _PG_init(void) prev_check_password_hook = check_password_hook; check_password_hook = check_password; } - -/* - * Module unload function - */ -void -_PG_fini(void) -{ - /* uninstall hook */ - check_password_hook = prev_check_password_hook; -} diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index df2ce63790..ceaad81a43 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -305,7 +305,6 @@ static bool pgss_save; /* whether to save stats across shutdown */ /*---- Function declarations ----*/ void _PG_init(void); -void _PG_fini(void); PG_FUNCTION_INFO_V1(pg_stat_statements_reset); PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7); @@ -481,23 +480,6 @@ _PG_init(void) ProcessUtility_hook = pgss_ProcessUtility; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - shmem_startup_hook = prev_shmem_startup_hook; - post_parse_analyze_hook = prev_post_parse_analyze_hook; - planner_hook = prev_planner_hook; - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; - ProcessUtility_hook = prev_ProcessUtility; -} - /* * shmem_startup hook: allocate or attach to shared memory, * then load any pre-existing statistics from file. diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 07c5fd198b..fbe718e3c2 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -1978,28 +1978,16 @@ PG_MODULE_MAGIC; <indexterm zone="xfunc-c-dynload"> <primary>_PG_init</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>_PG_fini</primary> - </indexterm> <indexterm zone="xfunc-c-dynload"> <primary>library initialization function</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>library finalization function</primary> - </indexterm> <para> - Optionally, a dynamically loaded file can contain initialization and - finalization functions. If the file includes a function named + Optionally, a dynamically loaded file can contain an initialization + function. If the file includes a function named <function>_PG_init</function>, that function will be called immediately after loading the file. The function receives no parameters and should - return void. If the file includes a function named - <function>_PG_fini</function>, that function will be called immediately before - unloading the file. Likewise, the function receives no parameters and - should return void. Note that <function>_PG_fini</function> will only be called - during an unload of the file, not during process termination. - (Presently, unloads are disabled and will never occur, but this may - change in the future.) + return void. There is presently no way to unload a dynamically loaded file. </para> </sect2> diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 0c8ca29f73..8beff4a53c 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -802,7 +802,7 @@ HandlePgArchInterrupts(void) * Ideally, we would simply unload the previous archive module and * load the new one, but there is presently no mechanism for * unloading a library (see the comment above - * internal_unload_library()). To deal with this, we simply restart + * internal_load_library()). To deal with this, we simply restart * the archiver. The new archive module will be loaded when the new * archiver process starts up. */ diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 3774f33e0e..7f9ea97280 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -37,9 +37,8 @@ #include "utils/hsearch.h" -/* signatures for PostgreSQL-specific library init/fini functions */ +/* signature for PostgreSQL-specific library init function */ typedef void (*PG_init_t) (void); -typedef void (*PG_fini_t) (void); /* hashtable entry for rendezvous variables */ typedef struct @@ -79,7 +78,6 @@ char *Dynamic_library_path; static void *internal_load_library(const char *libname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); -static void internal_unload_library(const char *libname); static bool file_exists(const char *name); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); @@ -154,9 +152,6 @@ load_file(const char *filename, bool restricted) /* Expand the possibly-abbreviated filename to an exact path name */ fullname = expand_dynamic_library_name(filename); - /* Unload the library if currently loaded */ - internal_unload_library(fullname); - /* Load the shared library */ (void) internal_load_library(fullname); @@ -179,6 +174,11 @@ lookup_external_function(void *filehandle, const char *funcname) * loaded. Return the pg_dl* handle for the file. * * Note: libname is expected to be an exact name for the library file. + * + * NB: There is presently no way to unload a dynamically loaded file. We might + * add one someday if we can convince ourselves we have safe protocols for un- + * hooking from hook function pointers, releasing custom GUC variables, and + * perhaps other things that are definitely unsafe currently. */ static void * internal_load_library(const char *libname) @@ -400,71 +400,6 @@ incompatible_module_error(const char *libname, errdetail_internal("%s", details.data))); } -/* - * Unload the specified dynamic-link library file, if it is loaded. - * - * Note: libname is expected to be an exact name for the library file. - * - * XXX for the moment, this is disabled, resulting in LOAD of an already-loaded - * library always being a no-op. We might re-enable it someday if we can - * convince ourselves we have safe protocols for un-hooking from hook function - * pointers, releasing custom GUC variables, and perhaps other things that - * are definitely unsafe currently. - */ -static void -internal_unload_library(const char *libname) -{ -#ifdef NOT_USED - DynamicFileList *file_scanner, - *prv, - *nxt; - struct stat stat_buf; - PG_fini_t PG_fini; - - /* - * We need to do stat() in order to determine whether this is the same - * file as a previously loaded file; it's also handy so as to give a good - * error message if bogus file name given. - */ - if (stat(libname, &stat_buf) == -1) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not access file \"%s\": %m", libname))); - - /* - * We have to zap all entries in the list that match on either filename or - * inode, else internal_load_library() will still think it's present. - */ - prv = NULL; - for (file_scanner = file_list; file_scanner != NULL; file_scanner = nxt) - { - nxt = file_scanner->next; - if (strcmp(libname, file_scanner->filename) == 0 || - SAME_INODE(stat_buf, *file_scanner)) - { - if (prv) - prv->next = nxt; - else - file_list = nxt; - - /* - * If the library has a _PG_fini() function, call it. - */ - PG_fini = (PG_fini_t) dlsym(file_scanner->handle, "_PG_fini"); - if (PG_fini) - (*PG_fini) (); - - clear_external_function_hash(file_scanner->handle); - dlclose(file_scanner->handle); - free((char *) file_scanner); - /* prv does not change */ - } - else - prv = file_scanner; - } -#endif /* NOT_USED */ -} - static bool file_exists(const char *name) { diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index b2e72b3243..a9dd068095 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -582,20 +582,6 @@ record_C_func(HeapTuple procedureTuple, entry->inforec = inforec; } -/* - * clear_external_function_hash: remove entries for a library being closed - * - * Presently we just zap the entire hash table, but later it might be worth - * the effort to remove only the entries associated with the given handle. - */ -void -clear_external_function_hash(void *filehandle) -{ - if (CFuncHash) - hash_destroy(CFuncHash); - CFuncHash = NULL; -} - /* * Copy an FmgrInfo struct diff --git a/src/include/fmgr.h b/src/include/fmgr.h index a1cf4bd646..d55abc5414 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -705,7 +705,6 @@ extern bytea *OidSendFunctionCall(Oid functionId, Datum val); * Routines in fmgr.c */ extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, const char *funcname); -extern void clear_external_function_hash(void *filehandle); extern Oid fmgr_internal_function(const char *proname); extern Oid get_fn_expr_rettype(FmgrInfo *flinfo); extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 6444347ce9..4e6ee1c619 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1100,8 +1100,6 @@ typedef struct PLpgSQL_execstate * variable "PLpgSQL_plugin" and set it to point to a PLpgSQL_plugin struct. * Typically the struct could just be static data in the plugin library. * We expect that a plugin would do this at library load time (_PG_init()). - * It must also be careful to set the rendezvous variable back to NULL - * if it is unloaded (_PG_fini()). * * This structure is basically a collection of function pointers --- at * various interesting points in pl_exec.c, we call these functions diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c index cf34e8c2d7..407ebc0eda 100644 --- a/src/test/modules/delay_execution/delay_execution.c +++ b/src/test/modules/delay_execution/delay_execution.c @@ -36,9 +36,8 @@ static int post_planning_lock_id = 0; /* Save previous planner hook user to be a good citizen */ static planner_hook_type prev_planner_hook = NULL; -/* Module load/unload functions */ +/* Module load function */ void _PG_init(void); -void _PG_fini(void); /* planner_hook function to provide the desired delay */ @@ -97,10 +96,3 @@ _PG_init(void) prev_planner_hook = planner_hook; planner_hook = delay_execution_planner; } - -/* Module unload function (pro forma, not used currently) */ -void -_PG_fini(void) -{ - planner_hook = prev_planner_hook; -} diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c index b5bb5580a0..e9f2329a5a 100644 --- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c +++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c @@ -21,7 +21,6 @@ PG_MODULE_MAGIC; void _PG_init(void); -void _PG_fini(void); static char *ssl_passphrase = NULL; @@ -55,12 +54,6 @@ _PG_init(void) openssl_tls_init_hook = set_rot13; } -void -_PG_fini(void) -{ - /* do nothing yet */ -} - static void set_rot13(SSL_CTX *context, bool isServerStart) { diff --git a/src/test/modules/test_oat_hooks/test_oat_hooks.c b/src/test/modules/test_oat_hooks/test_oat_hooks.c index 551da5d498..6f9838f93b 100644 --- a/src/test/modules/test_oat_hooks/test_oat_hooks.c +++ b/src/test/modules/test_oat_hooks/test_oat_hooks.c @@ -43,7 +43,7 @@ static bool REGRESS_userset_variable2 = false; static bool REGRESS_suset_variable1 = false; static bool REGRESS_suset_variable2 = false; -/* Saved hook values in case of unload */ +/* Saved hook values */ static object_access_hook_type next_object_access_hook = NULL; static object_access_hook_type_str next_object_access_hook_str = NULL; static ExecutorCheckPerms_hook_type next_exec_check_perms_hook = NULL; @@ -70,10 +70,9 @@ static char *accesstype_arg_to_string(ObjectAccessType access, void *arg); void _PG_init(void); -void _PG_fini(void); /* - * Module load/unload callback + * Module load callback */ void _PG_init(void) @@ -231,23 +230,6 @@ _PG_init(void) ProcessUtility_hook = REGRESS_utility_command; } -void -_PG_fini(void) -{ - /* Unload hooks */ - if (object_access_hook == REGRESS_object_access_hook) - object_access_hook = next_object_access_hook; - - if (object_access_hook_str == REGRESS_object_access_hook_str) - object_access_hook_str = next_object_access_hook_str; - - if (ExecutorCheckPerms_hook == REGRESS_exec_check_perms) - ExecutorCheckPerms_hook = next_exec_check_perms_hook; - - if (ProcessUtility_hook == REGRESS_utility_command) - ProcessUtility_hook = next_ProcessUtility_hook; -} - static void emit_audit_message(const char *type, const char *hook, char *action, char *objName) { diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c index 4f862d081b..b8e0aa2d0b 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -29,34 +29,17 @@ PG_MODULE_MAGIC; -/* Saved hook values in case of unload */ -static row_security_policy_hook_type prev_row_security_policy_hook_permissive = NULL; -static row_security_policy_hook_type prev_row_security_policy_hook_restrictive = NULL; - void _PG_init(void); -void _PG_fini(void); /* Install hooks */ void _PG_init(void) { - /* Save values for unload */ - prev_row_security_policy_hook_permissive = row_security_policy_hook_permissive; - prev_row_security_policy_hook_restrictive = row_security_policy_hook_restrictive; - /* Set our hooks */ row_security_policy_hook_permissive = test_rls_hooks_permissive; row_security_policy_hook_restrictive = test_rls_hooks_restrictive; } -/* Uninstall hooks */ -void -_PG_fini(void) -{ - row_security_policy_hook_permissive = prev_row_security_policy_hook_permissive; - row_security_policy_hook_restrictive = prev_row_security_policy_hook_restrictive; -} - /* * Return permissive policies to be added */ -- 2.25.1 --wac7ysb48OaltWcw Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0002-Add-a-new-shmem_request_hook-hook.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v8 1/2] Remove logic for unloading dynamically loaded files. @ 2022-05-06 20:53 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2022-05-06 20:53 UTC (permalink / raw) The code for unloading a library has been commented-out for over 12 years (602a9ef), and we're no closer to supporting it now than we were back then. --- contrib/auto_explain/auto_explain.c | 14 ---- contrib/passwordcheck/passwordcheck.c | 11 --- .../pg_stat_statements/pg_stat_statements.c | 18 ----- doc/src/sgml/xfunc.sgml | 18 +---- src/backend/postmaster/pgarch.c | 2 +- src/backend/utils/fmgr/dfmgr.c | 77 ++----------------- src/backend/utils/fmgr/fmgr.c | 14 ---- src/include/fmgr.h | 1 - src/pl/plpgsql/src/plpgsql.h | 2 - .../modules/delay_execution/delay_execution.c | 10 +-- .../ssl_passphrase_func.c | 7 -- .../modules/test_oat_hooks/test_oat_hooks.c | 22 +----- .../modules/test_rls_hooks/test_rls_hooks.c | 17 ---- 13 files changed, 13 insertions(+), 200 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index d3029f85ef..c9a0d947c8 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -77,7 +77,6 @@ static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; void _PG_init(void); -void _PG_fini(void); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, @@ -244,19 +243,6 @@ _PG_init(void) ExecutorEnd_hook = explain_ExecutorEnd; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; -} - /* * ExecutorStart hook: start up logging if needed */ diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c index 074836336d..9d8c58ded0 100644 --- a/contrib/passwordcheck/passwordcheck.c +++ b/contrib/passwordcheck/passwordcheck.c @@ -33,7 +33,6 @@ static check_password_hook_type prev_check_password_hook = NULL; #define MIN_PWD_LENGTH 8 extern void _PG_init(void); -extern void _PG_fini(void); /* * check_password @@ -149,13 +148,3 @@ _PG_init(void) prev_check_password_hook = check_password_hook; check_password_hook = check_password; } - -/* - * Module unload function - */ -void -_PG_fini(void) -{ - /* uninstall hook */ - check_password_hook = prev_check_password_hook; -} diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index df2ce63790..ceaad81a43 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -305,7 +305,6 @@ static bool pgss_save; /* whether to save stats across shutdown */ /*---- Function declarations ----*/ void _PG_init(void); -void _PG_fini(void); PG_FUNCTION_INFO_V1(pg_stat_statements_reset); PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7); @@ -481,23 +480,6 @@ _PG_init(void) ProcessUtility_hook = pgss_ProcessUtility; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - shmem_startup_hook = prev_shmem_startup_hook; - post_parse_analyze_hook = prev_post_parse_analyze_hook; - planner_hook = prev_planner_hook; - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; - ProcessUtility_hook = prev_ProcessUtility; -} - /* * shmem_startup hook: allocate or attach to shared memory, * then load any pre-existing statistics from file. diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 07c5fd198b..fbe718e3c2 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -1978,28 +1978,16 @@ PG_MODULE_MAGIC; <indexterm zone="xfunc-c-dynload"> <primary>_PG_init</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>_PG_fini</primary> - </indexterm> <indexterm zone="xfunc-c-dynload"> <primary>library initialization function</primary> </indexterm> - <indexterm zone="xfunc-c-dynload"> - <primary>library finalization function</primary> - </indexterm> <para> - Optionally, a dynamically loaded file can contain initialization and - finalization functions. If the file includes a function named + Optionally, a dynamically loaded file can contain an initialization + function. If the file includes a function named <function>_PG_init</function>, that function will be called immediately after loading the file. The function receives no parameters and should - return void. If the file includes a function named - <function>_PG_fini</function>, that function will be called immediately before - unloading the file. Likewise, the function receives no parameters and - should return void. Note that <function>_PG_fini</function> will only be called - during an unload of the file, not during process termination. - (Presently, unloads are disabled and will never occur, but this may - change in the future.) + return void. There is presently no way to unload a dynamically loaded file. </para> </sect2> diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 0c8ca29f73..8beff4a53c 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -802,7 +802,7 @@ HandlePgArchInterrupts(void) * Ideally, we would simply unload the previous archive module and * load the new one, but there is presently no mechanism for * unloading a library (see the comment above - * internal_unload_library()). To deal with this, we simply restart + * internal_load_library()). To deal with this, we simply restart * the archiver. The new archive module will be loaded when the new * archiver process starts up. */ diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 3774f33e0e..7f9ea97280 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -37,9 +37,8 @@ #include "utils/hsearch.h" -/* signatures for PostgreSQL-specific library init/fini functions */ +/* signature for PostgreSQL-specific library init function */ typedef void (*PG_init_t) (void); -typedef void (*PG_fini_t) (void); /* hashtable entry for rendezvous variables */ typedef struct @@ -79,7 +78,6 @@ char *Dynamic_library_path; static void *internal_load_library(const char *libname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); -static void internal_unload_library(const char *libname); static bool file_exists(const char *name); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); @@ -154,9 +152,6 @@ load_file(const char *filename, bool restricted) /* Expand the possibly-abbreviated filename to an exact path name */ fullname = expand_dynamic_library_name(filename); - /* Unload the library if currently loaded */ - internal_unload_library(fullname); - /* Load the shared library */ (void) internal_load_library(fullname); @@ -179,6 +174,11 @@ lookup_external_function(void *filehandle, const char *funcname) * loaded. Return the pg_dl* handle for the file. * * Note: libname is expected to be an exact name for the library file. + * + * NB: There is presently no way to unload a dynamically loaded file. We might + * add one someday if we can convince ourselves we have safe protocols for un- + * hooking from hook function pointers, releasing custom GUC variables, and + * perhaps other things that are definitely unsafe currently. */ static void * internal_load_library(const char *libname) @@ -400,71 +400,6 @@ incompatible_module_error(const char *libname, errdetail_internal("%s", details.data))); } -/* - * Unload the specified dynamic-link library file, if it is loaded. - * - * Note: libname is expected to be an exact name for the library file. - * - * XXX for the moment, this is disabled, resulting in LOAD of an already-loaded - * library always being a no-op. We might re-enable it someday if we can - * convince ourselves we have safe protocols for un-hooking from hook function - * pointers, releasing custom GUC variables, and perhaps other things that - * are definitely unsafe currently. - */ -static void -internal_unload_library(const char *libname) -{ -#ifdef NOT_USED - DynamicFileList *file_scanner, - *prv, - *nxt; - struct stat stat_buf; - PG_fini_t PG_fini; - - /* - * We need to do stat() in order to determine whether this is the same - * file as a previously loaded file; it's also handy so as to give a good - * error message if bogus file name given. - */ - if (stat(libname, &stat_buf) == -1) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not access file \"%s\": %m", libname))); - - /* - * We have to zap all entries in the list that match on either filename or - * inode, else internal_load_library() will still think it's present. - */ - prv = NULL; - for (file_scanner = file_list; file_scanner != NULL; file_scanner = nxt) - { - nxt = file_scanner->next; - if (strcmp(libname, file_scanner->filename) == 0 || - SAME_INODE(stat_buf, *file_scanner)) - { - if (prv) - prv->next = nxt; - else - file_list = nxt; - - /* - * If the library has a _PG_fini() function, call it. - */ - PG_fini = (PG_fini_t) dlsym(file_scanner->handle, "_PG_fini"); - if (PG_fini) - (*PG_fini) (); - - clear_external_function_hash(file_scanner->handle); - dlclose(file_scanner->handle); - free((char *) file_scanner); - /* prv does not change */ - } - else - prv = file_scanner; - } -#endif /* NOT_USED */ -} - static bool file_exists(const char *name) { diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index b2e72b3243..a9dd068095 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -582,20 +582,6 @@ record_C_func(HeapTuple procedureTuple, entry->inforec = inforec; } -/* - * clear_external_function_hash: remove entries for a library being closed - * - * Presently we just zap the entire hash table, but later it might be worth - * the effort to remove only the entries associated with the given handle. - */ -void -clear_external_function_hash(void *filehandle) -{ - if (CFuncHash) - hash_destroy(CFuncHash); - CFuncHash = NULL; -} - /* * Copy an FmgrInfo struct diff --git a/src/include/fmgr.h b/src/include/fmgr.h index a1cf4bd646..d55abc5414 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -705,7 +705,6 @@ extern bytea *OidSendFunctionCall(Oid functionId, Datum val); * Routines in fmgr.c */ extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, const char *funcname); -extern void clear_external_function_hash(void *filehandle); extern Oid fmgr_internal_function(const char *proname); extern Oid get_fn_expr_rettype(FmgrInfo *flinfo); extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 6444347ce9..4e6ee1c619 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1100,8 +1100,6 @@ typedef struct PLpgSQL_execstate * variable "PLpgSQL_plugin" and set it to point to a PLpgSQL_plugin struct. * Typically the struct could just be static data in the plugin library. * We expect that a plugin would do this at library load time (_PG_init()). - * It must also be careful to set the rendezvous variable back to NULL - * if it is unloaded (_PG_fini()). * * This structure is basically a collection of function pointers --- at * various interesting points in pl_exec.c, we call these functions diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c index cf34e8c2d7..407ebc0eda 100644 --- a/src/test/modules/delay_execution/delay_execution.c +++ b/src/test/modules/delay_execution/delay_execution.c @@ -36,9 +36,8 @@ static int post_planning_lock_id = 0; /* Save previous planner hook user to be a good citizen */ static planner_hook_type prev_planner_hook = NULL; -/* Module load/unload functions */ +/* Module load function */ void _PG_init(void); -void _PG_fini(void); /* planner_hook function to provide the desired delay */ @@ -97,10 +96,3 @@ _PG_init(void) prev_planner_hook = planner_hook; planner_hook = delay_execution_planner; } - -/* Module unload function (pro forma, not used currently) */ -void -_PG_fini(void) -{ - planner_hook = prev_planner_hook; -} diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c index b5bb5580a0..e9f2329a5a 100644 --- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c +++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c @@ -21,7 +21,6 @@ PG_MODULE_MAGIC; void _PG_init(void); -void _PG_fini(void); static char *ssl_passphrase = NULL; @@ -55,12 +54,6 @@ _PG_init(void) openssl_tls_init_hook = set_rot13; } -void -_PG_fini(void) -{ - /* do nothing yet */ -} - static void set_rot13(SSL_CTX *context, bool isServerStart) { diff --git a/src/test/modules/test_oat_hooks/test_oat_hooks.c b/src/test/modules/test_oat_hooks/test_oat_hooks.c index 551da5d498..6f9838f93b 100644 --- a/src/test/modules/test_oat_hooks/test_oat_hooks.c +++ b/src/test/modules/test_oat_hooks/test_oat_hooks.c @@ -43,7 +43,7 @@ static bool REGRESS_userset_variable2 = false; static bool REGRESS_suset_variable1 = false; static bool REGRESS_suset_variable2 = false; -/* Saved hook values in case of unload */ +/* Saved hook values */ static object_access_hook_type next_object_access_hook = NULL; static object_access_hook_type_str next_object_access_hook_str = NULL; static ExecutorCheckPerms_hook_type next_exec_check_perms_hook = NULL; @@ -70,10 +70,9 @@ static char *accesstype_arg_to_string(ObjectAccessType access, void *arg); void _PG_init(void); -void _PG_fini(void); /* - * Module load/unload callback + * Module load callback */ void _PG_init(void) @@ -231,23 +230,6 @@ _PG_init(void) ProcessUtility_hook = REGRESS_utility_command; } -void -_PG_fini(void) -{ - /* Unload hooks */ - if (object_access_hook == REGRESS_object_access_hook) - object_access_hook = next_object_access_hook; - - if (object_access_hook_str == REGRESS_object_access_hook_str) - object_access_hook_str = next_object_access_hook_str; - - if (ExecutorCheckPerms_hook == REGRESS_exec_check_perms) - ExecutorCheckPerms_hook = next_exec_check_perms_hook; - - if (ProcessUtility_hook == REGRESS_utility_command) - ProcessUtility_hook = next_ProcessUtility_hook; -} - static void emit_audit_message(const char *type, const char *hook, char *action, char *objName) { diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c index 4f862d081b..b8e0aa2d0b 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -29,34 +29,17 @@ PG_MODULE_MAGIC; -/* Saved hook values in case of unload */ -static row_security_policy_hook_type prev_row_security_policy_hook_permissive = NULL; -static row_security_policy_hook_type prev_row_security_policy_hook_restrictive = NULL; - void _PG_init(void); -void _PG_fini(void); /* Install hooks */ void _PG_init(void) { - /* Save values for unload */ - prev_row_security_policy_hook_permissive = row_security_policy_hook_permissive; - prev_row_security_policy_hook_restrictive = row_security_policy_hook_restrictive; - /* Set our hooks */ row_security_policy_hook_permissive = test_rls_hooks_permissive; row_security_policy_hook_restrictive = test_rls_hooks_restrictive; } -/* Uninstall hooks */ -void -_PG_fini(void) -{ - row_security_policy_hook_permissive = prev_row_security_policy_hook_permissive; - row_security_policy_hook_restrictive = prev_row_security_policy_hook_restrictive; -} - /* * Return permissive policies to be added */ -- 2.25.1 --wac7ysb48OaltWcw Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v8-0002-Add-a-new-shmem_request_hook-hook.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v6 2/2] Remove logic for unloading dynamically loaded files. @ 2022-05-06 20:53 Nathan Bossart <nathandbossart@gmail.com> 0 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2022-05-06 20:53 UTC (permalink / raw) The code for unloading a library has been commented-out for over 12 years (602a9ef), and we're no closer to supporting it now than we were back then. --- contrib/auto_explain/auto_explain.c | 14 ---- contrib/passwordcheck/passwordcheck.c | 11 --- .../pg_stat_statements/pg_stat_statements.c | 19 ----- doc/src/sgml/xfunc.sgml | 12 +-- src/backend/postmaster/pgarch.c | 2 +- src/backend/utils/fmgr/dfmgr.c | 77 ++----------------- src/backend/utils/fmgr/fmgr.c | 14 ---- src/include/fmgr.h | 1 - src/pl/plpgsql/src/plpgsql.h | 2 - .../modules/delay_execution/delay_execution.c | 10 +-- .../ssl_passphrase_func.c | 7 -- .../modules/test_oat_hooks/test_oat_hooks.c | 22 +----- .../modules/test_rls_hooks/test_rls_hooks.c | 17 ---- 13 files changed, 13 insertions(+), 195 deletions(-) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index d3029f85ef..c9a0d947c8 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -77,7 +77,6 @@ static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; void _PG_init(void); -void _PG_fini(void); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, @@ -244,19 +243,6 @@ _PG_init(void) ExecutorEnd_hook = explain_ExecutorEnd; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; -} - /* * ExecutorStart hook: start up logging if needed */ diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c index 074836336d..9d8c58ded0 100644 --- a/contrib/passwordcheck/passwordcheck.c +++ b/contrib/passwordcheck/passwordcheck.c @@ -33,7 +33,6 @@ static check_password_hook_type prev_check_password_hook = NULL; #define MIN_PWD_LENGTH 8 extern void _PG_init(void); -extern void _PG_fini(void); /* * check_password @@ -149,13 +148,3 @@ _PG_init(void) prev_check_password_hook = check_password_hook; check_password_hook = check_password; } - -/* - * Module unload function - */ -void -_PG_fini(void) -{ - /* uninstall hook */ - check_password_hook = prev_check_password_hook; -} diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 87b75d779e..9ea887d887 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -306,7 +306,6 @@ static bool pgss_save; /* whether to save stats across shutdown */ /*---- Function declarations ----*/ void _PG_init(void); -void _PG_fini(void); PG_FUNCTION_INFO_V1(pg_stat_statements_reset); PG_FUNCTION_INFO_V1(pg_stat_statements_reset_1_7); @@ -477,24 +476,6 @@ _PG_init(void) ProcessUtility_hook = pgss_ProcessUtility; } -/* - * Module unload callback - */ -void -_PG_fini(void) -{ - /* Uninstall hooks. */ - shmem_request_hook = prev_shmem_request_hook; - shmem_startup_hook = prev_shmem_startup_hook; - post_parse_analyze_hook = prev_post_parse_analyze_hook; - planner_hook = prev_planner_hook; - ExecutorStart_hook = prev_ExecutorStart; - ExecutorRun_hook = prev_ExecutorRun; - ExecutorFinish_hook = prev_ExecutorFinish; - ExecutorEnd_hook = prev_ExecutorEnd; - ProcessUtility_hook = prev_ProcessUtility; -} - /* * shmem_request hook: request additional shared resources. We'll allocate or * attach to the shared resources in pgss_shmem_startup(). diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 07c5fd198b..eebd5503e4 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -1989,17 +1989,11 @@ PG_MODULE_MAGIC; </indexterm> <para> - Optionally, a dynamically loaded file can contain initialization and - finalization functions. If the file includes a function named + Optionally, a dynamically loaded file can contain an initialization + function. If the file includes a function named <function>_PG_init</function>, that function will be called immediately after loading the file. The function receives no parameters and should - return void. If the file includes a function named - <function>_PG_fini</function>, that function will be called immediately before - unloading the file. Likewise, the function receives no parameters and - should return void. Note that <function>_PG_fini</function> will only be called - during an unload of the file, not during process termination. - (Presently, unloads are disabled and will never occur, but this may - change in the future.) + return void. There is presently no way to unload a dynamically loaded file. </para> </sect2> diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 0c8ca29f73..8beff4a53c 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -802,7 +802,7 @@ HandlePgArchInterrupts(void) * Ideally, we would simply unload the previous archive module and * load the new one, but there is presently no mechanism for * unloading a library (see the comment above - * internal_unload_library()). To deal with this, we simply restart + * internal_load_library()). To deal with this, we simply restart * the archiver. The new archive module will be loaded when the new * archiver process starts up. */ diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 3774f33e0e..7f9ea97280 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -37,9 +37,8 @@ #include "utils/hsearch.h" -/* signatures for PostgreSQL-specific library init/fini functions */ +/* signature for PostgreSQL-specific library init function */ typedef void (*PG_init_t) (void); -typedef void (*PG_fini_t) (void); /* hashtable entry for rendezvous variables */ typedef struct @@ -79,7 +78,6 @@ char *Dynamic_library_path; static void *internal_load_library(const char *libname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); -static void internal_unload_library(const char *libname); static bool file_exists(const char *name); static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); @@ -154,9 +152,6 @@ load_file(const char *filename, bool restricted) /* Expand the possibly-abbreviated filename to an exact path name */ fullname = expand_dynamic_library_name(filename); - /* Unload the library if currently loaded */ - internal_unload_library(fullname); - /* Load the shared library */ (void) internal_load_library(fullname); @@ -179,6 +174,11 @@ lookup_external_function(void *filehandle, const char *funcname) * loaded. Return the pg_dl* handle for the file. * * Note: libname is expected to be an exact name for the library file. + * + * NB: There is presently no way to unload a dynamically loaded file. We might + * add one someday if we can convince ourselves we have safe protocols for un- + * hooking from hook function pointers, releasing custom GUC variables, and + * perhaps other things that are definitely unsafe currently. */ static void * internal_load_library(const char *libname) @@ -400,71 +400,6 @@ incompatible_module_error(const char *libname, errdetail_internal("%s", details.data))); } -/* - * Unload the specified dynamic-link library file, if it is loaded. - * - * Note: libname is expected to be an exact name for the library file. - * - * XXX for the moment, this is disabled, resulting in LOAD of an already-loaded - * library always being a no-op. We might re-enable it someday if we can - * convince ourselves we have safe protocols for un-hooking from hook function - * pointers, releasing custom GUC variables, and perhaps other things that - * are definitely unsafe currently. - */ -static void -internal_unload_library(const char *libname) -{ -#ifdef NOT_USED - DynamicFileList *file_scanner, - *prv, - *nxt; - struct stat stat_buf; - PG_fini_t PG_fini; - - /* - * We need to do stat() in order to determine whether this is the same - * file as a previously loaded file; it's also handy so as to give a good - * error message if bogus file name given. - */ - if (stat(libname, &stat_buf) == -1) - ereport(ERROR, - (errcode_for_file_access(), - errmsg("could not access file \"%s\": %m", libname))); - - /* - * We have to zap all entries in the list that match on either filename or - * inode, else internal_load_library() will still think it's present. - */ - prv = NULL; - for (file_scanner = file_list; file_scanner != NULL; file_scanner = nxt) - { - nxt = file_scanner->next; - if (strcmp(libname, file_scanner->filename) == 0 || - SAME_INODE(stat_buf, *file_scanner)) - { - if (prv) - prv->next = nxt; - else - file_list = nxt; - - /* - * If the library has a _PG_fini() function, call it. - */ - PG_fini = (PG_fini_t) dlsym(file_scanner->handle, "_PG_fini"); - if (PG_fini) - (*PG_fini) (); - - clear_external_function_hash(file_scanner->handle); - dlclose(file_scanner->handle); - free((char *) file_scanner); - /* prv does not change */ - } - else - prv = file_scanner; - } -#endif /* NOT_USED */ -} - static bool file_exists(const char *name) { diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index b2e72b3243..a9dd068095 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -582,20 +582,6 @@ record_C_func(HeapTuple procedureTuple, entry->inforec = inforec; } -/* - * clear_external_function_hash: remove entries for a library being closed - * - * Presently we just zap the entire hash table, but later it might be worth - * the effort to remove only the entries associated with the given handle. - */ -void -clear_external_function_hash(void *filehandle) -{ - if (CFuncHash) - hash_destroy(CFuncHash); - CFuncHash = NULL; -} - /* * Copy an FmgrInfo struct diff --git a/src/include/fmgr.h b/src/include/fmgr.h index a1cf4bd646..d55abc5414 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -705,7 +705,6 @@ extern bytea *OidSendFunctionCall(Oid functionId, Datum val); * Routines in fmgr.c */ extern const Pg_finfo_record *fetch_finfo_record(void *filehandle, const char *funcname); -extern void clear_external_function_hash(void *filehandle); extern Oid fmgr_internal_function(const char *proname); extern Oid get_fn_expr_rettype(FmgrInfo *flinfo); extern Oid get_fn_expr_argtype(FmgrInfo *flinfo, int argnum); diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h index 6444347ce9..4e6ee1c619 100644 --- a/src/pl/plpgsql/src/plpgsql.h +++ b/src/pl/plpgsql/src/plpgsql.h @@ -1100,8 +1100,6 @@ typedef struct PLpgSQL_execstate * variable "PLpgSQL_plugin" and set it to point to a PLpgSQL_plugin struct. * Typically the struct could just be static data in the plugin library. * We expect that a plugin would do this at library load time (_PG_init()). - * It must also be careful to set the rendezvous variable back to NULL - * if it is unloaded (_PG_fini()). * * This structure is basically a collection of function pointers --- at * various interesting points in pl_exec.c, we call these functions diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c index cf34e8c2d7..407ebc0eda 100644 --- a/src/test/modules/delay_execution/delay_execution.c +++ b/src/test/modules/delay_execution/delay_execution.c @@ -36,9 +36,8 @@ static int post_planning_lock_id = 0; /* Save previous planner hook user to be a good citizen */ static planner_hook_type prev_planner_hook = NULL; -/* Module load/unload functions */ +/* Module load function */ void _PG_init(void); -void _PG_fini(void); /* planner_hook function to provide the desired delay */ @@ -97,10 +96,3 @@ _PG_init(void) prev_planner_hook = planner_hook; planner_hook = delay_execution_planner; } - -/* Module unload function (pro forma, not used currently) */ -void -_PG_fini(void) -{ - planner_hook = prev_planner_hook; -} diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c index b5bb5580a0..e9f2329a5a 100644 --- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c +++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c @@ -21,7 +21,6 @@ PG_MODULE_MAGIC; void _PG_init(void); -void _PG_fini(void); static char *ssl_passphrase = NULL; @@ -55,12 +54,6 @@ _PG_init(void) openssl_tls_init_hook = set_rot13; } -void -_PG_fini(void) -{ - /* do nothing yet */ -} - static void set_rot13(SSL_CTX *context, bool isServerStart) { diff --git a/src/test/modules/test_oat_hooks/test_oat_hooks.c b/src/test/modules/test_oat_hooks/test_oat_hooks.c index 551da5d498..6f9838f93b 100644 --- a/src/test/modules/test_oat_hooks/test_oat_hooks.c +++ b/src/test/modules/test_oat_hooks/test_oat_hooks.c @@ -43,7 +43,7 @@ static bool REGRESS_userset_variable2 = false; static bool REGRESS_suset_variable1 = false; static bool REGRESS_suset_variable2 = false; -/* Saved hook values in case of unload */ +/* Saved hook values */ static object_access_hook_type next_object_access_hook = NULL; static object_access_hook_type_str next_object_access_hook_str = NULL; static ExecutorCheckPerms_hook_type next_exec_check_perms_hook = NULL; @@ -70,10 +70,9 @@ static char *accesstype_arg_to_string(ObjectAccessType access, void *arg); void _PG_init(void); -void _PG_fini(void); /* - * Module load/unload callback + * Module load callback */ void _PG_init(void) @@ -231,23 +230,6 @@ _PG_init(void) ProcessUtility_hook = REGRESS_utility_command; } -void -_PG_fini(void) -{ - /* Unload hooks */ - if (object_access_hook == REGRESS_object_access_hook) - object_access_hook = next_object_access_hook; - - if (object_access_hook_str == REGRESS_object_access_hook_str) - object_access_hook_str = next_object_access_hook_str; - - if (ExecutorCheckPerms_hook == REGRESS_exec_check_perms) - ExecutorCheckPerms_hook = next_exec_check_perms_hook; - - if (ProcessUtility_hook == REGRESS_utility_command) - ProcessUtility_hook = next_ProcessUtility_hook; -} - static void emit_audit_message(const char *type, const char *hook, char *action, char *objName) { diff --git a/src/test/modules/test_rls_hooks/test_rls_hooks.c b/src/test/modules/test_rls_hooks/test_rls_hooks.c index 4f862d081b..b8e0aa2d0b 100644 --- a/src/test/modules/test_rls_hooks/test_rls_hooks.c +++ b/src/test/modules/test_rls_hooks/test_rls_hooks.c @@ -29,34 +29,17 @@ PG_MODULE_MAGIC; -/* Saved hook values in case of unload */ -static row_security_policy_hook_type prev_row_security_policy_hook_permissive = NULL; -static row_security_policy_hook_type prev_row_security_policy_hook_restrictive = NULL; - void _PG_init(void); -void _PG_fini(void); /* Install hooks */ void _PG_init(void) { - /* Save values for unload */ - prev_row_security_policy_hook_permissive = row_security_policy_hook_permissive; - prev_row_security_policy_hook_restrictive = row_security_policy_hook_restrictive; - /* Set our hooks */ row_security_policy_hook_permissive = test_rls_hooks_permissive; row_security_policy_hook_restrictive = test_rls_hooks_restrictive; } -/* Uninstall hooks */ -void -_PG_fini(void) -{ - row_security_policy_hook_permissive = prev_row_security_policy_hook_permissive; - row_security_policy_hook_restrictive = prev_row_security_policy_hook_restrictive; -} - /* * Return permissive policies to be added */ -- 2.25.1 --6TrnltStXW4iwmi0-- ^ permalink raw reply [nested|flat] 7+ messages in thread
* [PATCH v7 1/3] Add OR REPLACE option to CREATE MATERIALIZED VIEW @ 2024-05-21 16:35 Erik Wienhold <ewie@ewie.name> 0 siblings, 0 replies; 7+ messages in thread From: Erik Wienhold @ 2024-05-21 16:35 UTC (permalink / raw) --- .../sgml/ref/create_materialized_view.sgml | 15 +- src/backend/commands/createas.c | 219 ++++++++++++++---- src/backend/commands/tablecmds.c | 8 +- src/backend/commands/view.c | 106 ++++++--- src/backend/parser/gram.y | 15 ++ src/bin/psql/tab-complete.in.c | 26 ++- src/include/commands/view.h | 3 + src/include/nodes/parsenodes.h | 2 +- src/include/nodes/primnodes.h | 1 + src/test/regress/expected/matview.out | 205 ++++++++++++++++ src/test/regress/sql/matview.sql | 117 ++++++++++ 11 files changed, 624 insertions(+), 93 deletions(-) diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml index 62d897931c3..5e03320eb73 100644 --- a/doc/src/sgml/ref/create_materialized_view.sgml +++ b/doc/src/sgml/ref/create_materialized_view.sgml @@ -21,7 +21,7 @@ PostgreSQL documentation <refsynopsisdiv> <synopsis> -CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable> +CREATE [ OR REPLACE ] MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable> [ (<replaceable>column_name</replaceable> [, ...] ) ] [ USING <replaceable class="parameter">method</replaceable> ] [ WITH ( <replaceable class="parameter">storage_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] ) ] @@ -60,6 +60,17 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable> <title>Parameters</title> <variablelist> + <varlistentry> + <term><literal>OR REPLACE</literal></term> + <listitem> + <para> + Replaces a materialized view if it already exists. + Specifying <literal>OR REPLACE</literal> together with + <literal>IF NOT EXISTS</literal> is an error. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>IF NOT EXISTS</literal></term> <listitem> @@ -67,7 +78,7 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] <replaceable>table_name</replaceable> Do not throw an error if a materialized view with the same name already exists. A notice is issued in this case. Note that there is no guarantee that the existing materialized view is anything like the one that would - have been created. + have been created, unless you use <literal>OR REPLACE</literal> instead. </para> </listitem> </varlistentry> diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c index dfd2ab8e862..1620273f965 100644 --- a/src/backend/commands/createas.c +++ b/src/backend/commands/createas.c @@ -24,6 +24,7 @@ */ #include "postgres.h" +#include "miscadmin.h" #include "access/heapam.h" #include "access/reloptions.h" #include "access/tableam.h" @@ -34,6 +35,7 @@ #include "commands/matview.h" #include "commands/prepare.h" #include "commands/tablecmds.h" +#include "commands/tablespace.h" #include "commands/view.h" #include "executor/execdesc.h" #include "executor/executor.h" @@ -81,55 +83,161 @@ static void intorel_destroy(DestReceiver *self); static ObjectAddress create_ctas_internal(List *attrList, IntoClause *into) { - CreateStmt *create = makeNode(CreateStmt); - bool is_matview; + bool is_matview, + replace = false; char relkind; - Datum toast_options; - const char *const validnsps[] = HEAP_RELOPT_NAMESPACES; + Oid matviewOid = InvalidOid; ObjectAddress intoRelationAddr; /* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */ is_matview = (into->viewQuery != NULL); relkind = is_matview ? RELKIND_MATVIEW : RELKIND_RELATION; - /* - * Create the target relation by faking up a CREATE TABLE parsetree and - * passing it to DefineRelation. - */ - create->relation = into->rel; - create->tableElts = attrList; - create->inhRelations = NIL; - create->ofTypename = NULL; - create->constraints = NIL; - create->options = into->options; - create->oncommit = into->onCommit; - create->tablespacename = into->tableSpaceName; - create->if_not_exists = false; - create->accessMethod = into->accessMethod; + /* Check if an existing materialized view needs to be replaced. */ + if (is_matview) + { + LOCKMODE lockmode; - /* - * Create the relation. (This will error out if there's an existing view, - * so we don't need more code to complain if "replace" is false.) - */ - intoRelationAddr = DefineRelation(create, relkind, InvalidOid, NULL, NULL); + lockmode = into->replace ? AccessExclusiveLock : NoLock; + (void) RangeVarGetAndCheckCreationNamespace(into->rel, lockmode, + &matviewOid); + replace = OidIsValid(matviewOid) && into->replace; + } - /* - * If necessary, create a TOAST table for the target table. Note that - * NewRelationCreateToastTable ends with CommandCounterIncrement(), so - * that the TOAST table will be visible for insertion. - */ - CommandCounterIncrement(); + if (is_matview && replace) + { + Relation rel; + List *atcmds = NIL; + AlterTableCmd *atcmd; + TupleDesc descriptor; + + rel = relation_open(matviewOid, NoLock); + + if (rel->rd_rel->relkind != RELKIND_MATVIEW) + ereport(ERROR, + errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("\"%s\" is not a materialized view", + RelationGetRelationName(rel))); + + CheckTableNotInUse(rel, "CREATE OR REPLACE MATERIALIZED VIEW"); + + descriptor = BuildDescForRelation(attrList); + checkViewColumns(descriptor, rel->rd_att, true); + + /* add new attributes */ + if (list_length(attrList) > rel->rd_att->natts) + { + ListCell *c; + int skip = rel->rd_att->natts; + + foreach(c, attrList) + { + if (skip > 0) + { + skip--; + continue; + } + atcmd = makeNode(AlterTableCmd); + atcmd->subtype = AT_AddColumnToView; + atcmd->def = (Node *) lfirst(c); + atcmds = lappend(atcmds, atcmd); + } + } + + /* + * The following alters access method, tablespace, and storage options. + * When replacing an existing matview we need to alter the relation + * such that the defaults apply as if they have not been specified at + * all by the CREATE statement. + */ + + /* access method */ + atcmd = makeNode(AlterTableCmd); + atcmd->subtype = AT_SetAccessMethod; + atcmd->name = into->accessMethod ? into->accessMethod : default_table_access_method; + atcmds = lappend(atcmds, atcmd); + + /* tablespace */ + atcmd = makeNode(AlterTableCmd); + atcmd->subtype = AT_SetTableSpace; + if (into->tableSpaceName != NULL) + atcmd->name = into->tableSpaceName; + else + { + Oid spcid; + + /* + * Resolve the name of the default or database tablespace because + * we need to specify the tablespace by name. + * + * TODO: Move that to ATPrepSetTableSpace? Must allow AlterTableCmd.name to be NULL then. + */ + spcid = GetDefaultTablespace(RELPERSISTENCE_PERMANENT, false); + if (!OidIsValid(spcid)) + spcid = MyDatabaseTableSpace; + atcmd->name = get_tablespace_name(spcid); + } + atcmds = lappend(atcmds, atcmd); + + /* storage options */ + atcmd = makeNode(AlterTableCmd); + atcmd->subtype = AT_ReplaceRelOptions; + atcmd->def = (Node *) into->options; + atcmds = lappend(atcmds, atcmd); + + AlterTableInternal(matviewOid, atcmds, true); + CommandCounterIncrement(); + + relation_close(rel, NoLock); + ObjectAddressSet(intoRelationAddr, RelationRelationId, matviewOid); + } + else + { + CreateStmt *create = makeNode(CreateStmt); + Datum toast_options; + const static char *validnsps[] = HEAP_RELOPT_NAMESPACES; + + /* + * Create the target relation by faking up a CREATE TABLE parsetree + * and passing it to DefineRelation. + */ + create->relation = into->rel; + create->tableElts = attrList; + create->inhRelations = NIL; + create->ofTypename = NULL; + create->constraints = NIL; + create->options = into->options; + create->oncommit = into->onCommit; + create->tablespacename = into->tableSpaceName; + create->if_not_exists = false; + create->accessMethod = into->accessMethod; + + /* + * Create the relation. (This will error out if there's an existing + * view, so we don't need more code to complain if "replace" is + * false.) + */ + intoRelationAddr = DefineRelation(create, relkind, InvalidOid, NULL, + NULL); - /* parse and validate reloptions for the toast table */ - toast_options = transformRelOptions((Datum) 0, - create->options, - "toast", - validnsps, - true, false); + /* + * If necessary, create a TOAST table for the target table. Note that + * NewRelationCreateToastTable ends with CommandCounterIncrement(), so + * that the TOAST table will be visible for insertion. + */ + CommandCounterIncrement(); + + /* parse and validate reloptions for the toast table */ + toast_options = transformRelOptions((Datum) 0, + create->options, + "toast", + validnsps, + true, false); - (void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true); + (void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true); - NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options); + NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options); + } /* Create the "view" part of a materialized view. */ if (is_matview) @@ -137,7 +245,7 @@ create_ctas_internal(List *attrList, IntoClause *into) /* StoreViewQuery scribbles on tree, so make a copy */ Query *query = copyObject(into->viewQuery); - StoreViewQuery(intoRelationAddr.objectId, query, false); + StoreViewQuery(intoRelationAddr.objectId, query, replace); CommandCounterIncrement(); } @@ -234,7 +342,26 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt, /* Check if the relation exists or not */ if (CreateTableAsRelExists(stmt)) + { + /* An existing materialized view can be replaced. */ + if (is_matview && into->replace) + { + RefreshMatViewStmt *refresh; + + /* Change the relation to match the new query and other options. */ + (void) create_ctas_nodata(query->targetList, into); + + /* Refresh the materialized view with a fake statement. */ + refresh = makeNode(RefreshMatViewStmt); + refresh->relation = into->rel; + refresh->skipData = into->skipData; + refresh->concurrent = false; + + return ExecRefreshMatView(refresh, pstate->p_sourcetext, qc); + } + return InvalidObjectAddress; + } /* * Create the tuple receiver object and insert info it will need @@ -402,14 +529,15 @@ CreateTableAsRelExists(CreateTableAsStmt *ctas) oldrelid = get_relname_relid(into->rel->relname, nspid); if (OidIsValid(oldrelid)) { - if (!ctas->if_not_exists) + if (!ctas->if_not_exists && !into->replace) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_TABLE), errmsg("relation \"%s\" already exists", into->rel->relname))); /* - * The relation exists and IF NOT EXISTS has been specified. + * The relation exists and IF NOT EXISTS or OR REPLACE has been + * specified. * * If we are in an extension script, insist that the pre-existing * object be a member of the extension, to avoid security risks. @@ -417,11 +545,12 @@ CreateTableAsRelExists(CreateTableAsStmt *ctas) ObjectAddressSet(address, RelationRelationId, oldrelid); checkMembershipInCurrentExtension(&address); - /* OK to skip */ - ereport(NOTICE, - (errcode(ERRCODE_DUPLICATE_TABLE), - errmsg("relation \"%s\" already exists, skipping", - into->rel->relname))); + if (ctas->if_not_exists) + /* OK to skip */ + ereport(NOTICE, + (errcode(ERRCODE_DUPLICATE_TABLE), + errmsg("relation \"%s\" already exists, skipping", + into->rel->relname))); return true; } diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index cb811520c29..44dcd2c5b0d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -4645,7 +4645,7 @@ AlterTableGetLockLevel(List *cmds) * Subcommands that may be visible to concurrent SELECTs */ case AT_DropColumn: /* change visible to SELECT */ - case AT_AddColumnToView: /* CREATE VIEW */ + case AT_AddColumnToView: /* via CREATE OR REPLACE [MATERIALIZED] VIEW */ case AT_DropOids: /* used to equiv to DropColumn */ case AT_EnableAlwaysRule: /* may change SELECT rules */ case AT_EnableReplicaRule: /* may change SELECT rules */ @@ -4940,8 +4940,8 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, /* Recursion occurs during execution phase */ pass = AT_PASS_ADD_COL; break; - case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */ - ATSimplePermissions(cmd->subtype, rel, ATT_VIEW); + case AT_AddColumnToView: /* via CREATE OR REPLACE [MATERIALIZED] VIEW */ + ATSimplePermissions(cmd->subtype, rel, ATT_VIEW | ATT_MATVIEW); ATPrepAddColumn(wqueue, rel, recurse, recursing, true, cmd, lockmode, context); /* Recursion occurs during execution phase */ @@ -5372,7 +5372,7 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, switch (cmd->subtype) { case AT_AddColumn: /* ADD COLUMN */ - case AT_AddColumnToView: /* add column via CREATE OR REPLACE VIEW */ + case AT_AddColumnToView: /* via CREATE OR REPLACE [MATERIALIZED] VIEW */ address = ATExecAddColumn(wqueue, tab, rel, &cmd, cmd->recurse, false, lockmode, cur_pass, context); diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c index 6f0301555e0..53d4cacc4c1 100644 --- a/src/backend/commands/view.c +++ b/src/backend/commands/view.c @@ -30,8 +30,6 @@ #include "utils/lsyscache.h" #include "utils/rel.h" -static void checkViewColumns(TupleDesc newdesc, TupleDesc olddesc); - /*--------------------------------------------------------------------- * DefineVirtualRelation * @@ -129,7 +127,7 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace, * column list. */ descriptor = BuildDescForRelation(attrList); - checkViewColumns(descriptor, rel->rd_att); + checkViewColumns(descriptor, rel->rd_att, false); /* * If new attributes have been added, we must add pg_attribute entries @@ -263,15 +261,22 @@ DefineVirtualRelation(RangeVar *relation, List *tlist, bool replace, * added to generate specific complaints. Also, we allow the new view to have * more columns than the old. */ -static void -checkViewColumns(TupleDesc newdesc, TupleDesc olddesc) +void +checkViewColumns(TupleDesc newdesc, TupleDesc olddesc, bool is_matview) { int i; if (newdesc->natts < olddesc->natts) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot drop columns from view"))); + { + if (is_matview) + ereport(ERROR, + errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot drop columns from materialized view")); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot drop columns from view"))); + } for (i = 0; i < olddesc->natts; i++) { @@ -280,17 +285,34 @@ checkViewColumns(TupleDesc newdesc, TupleDesc olddesc) /* XXX msg not right, but we don't support DROP COL on view anyway */ if (newattr->attisdropped != oldattr->attisdropped) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot drop columns from view"))); + { + if (is_matview) + ereport(ERROR, + errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot drop columns from materialized view")); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot drop columns from view"))); + } if (strcmp(NameStr(newattr->attname), NameStr(oldattr->attname)) != 0) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot change name of view column \"%s\" to \"%s\"", - NameStr(oldattr->attname), - NameStr(newattr->attname)), - errhint("Use ALTER VIEW ... RENAME COLUMN ... to change name of view column instead."))); + { + if (is_matview) + ereport(ERROR, + errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot change name of materialized view column \"%s\" to \"%s\"", + NameStr(oldattr->attname), + NameStr(newattr->attname)), + errhint("Use ALTER MATERIALIZED VIEW ... RENAME COLUMN ... to change name of materialized view column instead.")); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot change name of view column \"%s\" to \"%s\"", + NameStr(oldattr->attname), + NameStr(newattr->attname)), + errhint("Use ALTER VIEW ... RENAME COLUMN ... to change name of view column instead."))); + } /* * We cannot allow type, typmod, or collation to change, since these @@ -299,26 +321,48 @@ checkViewColumns(TupleDesc newdesc, TupleDesc olddesc) */ if (newattr->atttypid != oldattr->atttypid || newattr->atttypmod != oldattr->atttypmod) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot change data type of view column \"%s\" from %s to %s", - NameStr(oldattr->attname), - format_type_with_typemod(oldattr->atttypid, - oldattr->atttypmod), - format_type_with_typemod(newattr->atttypid, - newattr->atttypmod)))); + { + if (is_matview) + ereport(ERROR, + errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot change data type of materialized view column \"%s\" from %s to %s", + NameStr(oldattr->attname), + format_type_with_typemod(oldattr->atttypid, + oldattr->atttypmod), + format_type_with_typemod(newattr->atttypid, + newattr->atttypmod))); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot change data type of view column \"%s\" from %s to %s", + NameStr(oldattr->attname), + format_type_with_typemod(oldattr->atttypid, + oldattr->atttypmod), + format_type_with_typemod(newattr->atttypid, + newattr->atttypmod)))); + } /* * At this point, attcollations should be both valid or both invalid, * so applying get_collation_name unconditionally should be fine. */ if (newattr->attcollation != oldattr->attcollation) - ereport(ERROR, - (errcode(ERRCODE_INVALID_TABLE_DEFINITION), - errmsg("cannot change collation of view column \"%s\" from \"%s\" to \"%s\"", - NameStr(oldattr->attname), - get_collation_name(oldattr->attcollation), - get_collation_name(newattr->attcollation)))); + { + if (is_matview) + ereport(ERROR, + errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot change collation of materialized view column \"%s\" from \"%s\" to \"%s\"", + NameStr(oldattr->attname), + get_collation_name(oldattr->attcollation), + get_collation_name(newattr->attcollation))); + else + ereport(ERROR, + (errcode(ERRCODE_INVALID_TABLE_DEFINITION), + errmsg("cannot change collation of view column \"%s\" from \"%s\" to \"%s\"", + NameStr(oldattr->attname), + get_collation_name(oldattr->attcollation), + get_collation_name(newattr->attcollation)))); + } } /* diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index db43034b9db..7aaf0e37ad8 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -4929,6 +4929,21 @@ CreateMatViewStmt: $8->skipData = !($11); $$ = (Node *) ctas; } + | CREATE OR REPLACE OptNoLog MATERIALIZED VIEW create_mv_target AS SelectStmt opt_with_data + { + CreateTableAsStmt *ctas = makeNode(CreateTableAsStmt); + + ctas->query = $9; + ctas->into = $7; + ctas->objtype = OBJECT_MATVIEW; + ctas->is_select_into = false; + ctas->if_not_exists = false; + /* cram additional flags into the IntoClause */ + $7->rel->relpersistence = $4; + $7->skipData = !($10); + $7->replace = true; + $$ = (Node *) ctas; + } ; create_mv_target: diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index 1f2ca946fc5..f49f0b60c95 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -2187,7 +2187,7 @@ match_previous_words(int pattern_id, /* complete with something you can create or replace */ else if (TailMatches("CREATE", "OR", "REPLACE")) COMPLETE_WITH("FUNCTION", "PROCEDURE", "LANGUAGE", "RULE", "VIEW", - "AGGREGATE", "TRANSFORM", "TRIGGER"); + "AGGREGATE", "TRANSFORM", "TRIGGER", "MATERIALIZED VIEW"); /* DROP, but not DROP embedded in other commands */ /* complete with something you can drop */ @@ -4072,28 +4072,34 @@ match_previous_words(int pattern_id, COMPLETE_WITH("SELECT"); /* CREATE MATERIALIZED VIEW */ - else if (Matches("CREATE", "MATERIALIZED")) + else if (Matches("CREATE", "MATERIALIZED") || + Matches("CREATE", "OR", "REPLACE", "MATERIALIZED")) COMPLETE_WITH("VIEW"); - /* Complete CREATE MATERIALIZED VIEW <name> with AS or USING */ - else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny)) + /* Complete CREATE [ OR REPLACE ] MATERIALIZED VIEW <name> with AS or USING */ + else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny) || + Matches("CREATE", "OR", "REPLACE", "MATERIALIZED", "VIEW", MatchAny)) COMPLETE_WITH("AS", "USING"); /* - * Complete CREATE MATERIALIZED VIEW <name> USING with list of access + * Complete CREATE [ OR REPLACE ] MATERIALIZED VIEW <name> USING with list of access * methods */ - else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "USING")) + else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "USING") || + Matches("CREATE", "OR", "REPLACE", "MATERIALIZED", "VIEW", MatchAny, "USING")) COMPLETE_WITH_QUERY(Query_for_list_of_table_access_methods); - /* Complete CREATE MATERIALIZED VIEW <name> USING <access method> with AS */ - else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "USING", MatchAny)) + /* Complete CREATE [ OR REPLACE ] MATERIALIZED VIEW <name> USING <access method> with AS */ + else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "USING", MatchAny) || + Matches("CREATE", "OR", "REPLACE", "MATERIALIZED", "VIEW", MatchAny, "USING", MatchAny)) COMPLETE_WITH("AS"); /* - * Complete CREATE MATERIALIZED VIEW <name> [USING <access method> ] AS + * Complete CREATE [ OR REPLACE ] MATERIALIZED VIEW <name> [USING <access method> ] AS * with "SELECT" */ else if (Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "AS") || - Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "USING", MatchAny, "AS")) + Matches("CREATE", "OR", "REPLACE", "MATERIALIZED", "VIEW", MatchAny, "AS") || + Matches("CREATE", "MATERIALIZED", "VIEW", MatchAny, "USING", MatchAny, "AS") || + Matches("CREATE", "OR", "REPLACE", "MATERIALIZED", "VIEW", MatchAny, "USING", MatchAny, "AS")) COMPLETE_WITH("SELECT"); /* CREATE EVENT TRIGGER */ diff --git a/src/include/commands/view.h b/src/include/commands/view.h index c41f51b161c..95290f0a1c4 100644 --- a/src/include/commands/view.h +++ b/src/include/commands/view.h @@ -22,4 +22,7 @@ extern ObjectAddress DefineView(ViewStmt *stmt, const char *queryString, extern void StoreViewQuery(Oid viewOid, Query *viewParse, bool replace); +extern void checkViewColumns(TupleDesc newdesc, TupleDesc olddesc, + bool is_matview); + #endif /* VIEW_H */ diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 86a236bd58b..74922f8ae89 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -2412,7 +2412,7 @@ typedef struct AlterTableStmt typedef enum AlterTableType { AT_AddColumn, /* add column */ - AT_AddColumnToView, /* implicitly via CREATE OR REPLACE VIEW */ + AT_AddColumnToView, /* implicitly via CREATE OR REPLACE [MATERIALIZED] VIEW */ AT_ColumnDefault, /* alter column default */ AT_CookedColumnDefault, /* add a pre-cooked column default */ AT_DropNotNull, /* alter column drop not null */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 6dfca3cb35b..3a0c3b06c81 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -169,6 +169,7 @@ typedef struct IntoClause /* materialized view's SELECT query */ struct Query *viewQuery pg_node_attr(query_jumble_ignore); bool skipData; /* true for WITH NO DATA */ + bool replace; /* replace existing matview? */ } IntoClause; diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out index c56c9fa3a25..56104b7ee8b 100644 --- a/src/test/regress/expected/matview.out +++ b/src/test/regress/expected/matview.out @@ -694,3 +694,208 @@ NOTICE: relation "matview_ine_tab" already exists, skipping (0 rows) DROP MATERIALIZED VIEW matview_ine_tab; +-- +-- test CREATE OR REPLACE MATERIALIZED VIEW +-- +-- matview does not already exist +DROP MATERIALIZED VIEW IF EXISTS mvtest_replace; +NOTICE: materialized view "mvtest_replace" does not exist, skipping +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 1 AS a; +SELECT * FROM mvtest_replace; + a +--- + 1 +(1 row) + +-- replace query with data +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 2 AS a; +SELECT * FROM mvtest_replace; + a +--- + 2 +(1 row) + +-- replace query without data +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 3 AS a + WITH NO DATA; +SELECT * FROM mvtest_replace; -- error: not populated +ERROR: materialized view "mvtest_replace" has not been populated +HINT: Use the REFRESH MATERIALIZED VIEW command. +REFRESH MATERIALIZED VIEW mvtest_replace; +SELECT * FROM mvtest_replace; + a +--- + 3 +(1 row) + +-- add column +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 4 AS a, 1 b; +SELECT * FROM mvtest_replace; + a | b +---+--- + 4 | 1 +(1 row) + +-- replace table options +SELECT m.*, c.relname, c.reloptions, s.spcname, a.amname + FROM mvtest_replace m + CROSS JOIN pg_class c + LEFT JOIN pg_tablespace s ON s.oid = c.reltablespace + LEFT JOIN pg_am a ON a.oid = c.relam + WHERE c.relname = 'mvtest_replace'; + a | b | relname | reloptions | spcname | amname +---+---+----------------+------------+---------+-------- + 4 | 1 | mvtest_replace | | | heap +(1 row) + +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace + USING heap2 + WITH (fillfactor = 50) + TABLESPACE regress_tblspace + AS SELECT 5 AS a, 1 AS b; +SELECT m.*, c.relname, c.reloptions, s.spcname, a.amname + FROM mvtest_replace m + CROSS JOIN pg_class c + LEFT JOIN pg_tablespace s ON s.oid = c.reltablespace + LEFT JOIN pg_am a ON a.oid = c.relam + WHERE c.relname = 'mvtest_replace'; + a | b | relname | reloptions | spcname | amname +---+---+----------------+-----------------+------------------+-------- + 5 | 1 | mvtest_replace | {fillfactor=50} | regress_tblspace | heap2 +(1 row) + +-- restore default options +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace + AS SELECT 5 AS a, 1 AS b; +SELECT m.*, c.relname, c.reloptions, s.spcname, a.amname + FROM mvtest_replace m + CROSS JOIN pg_class c + LEFT JOIN pg_tablespace s ON s.oid = c.reltablespace + LEFT JOIN pg_am a ON a.oid = c.relam + WHERE c.relname = 'mvtest_replace'; + a | b | relname | reloptions | spcname | amname +---+---+----------------+------------+---------+-------- + 5 | 1 | mvtest_replace | | | heap +(1 row) + +-- can replace matview that has a dependent view +CREATE VIEW mvtest_replace_v AS + SELECT * FROM mvtest_replace; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 6 AS a, 1 AS b; +SELECT * FROM mvtest_replace, mvtest_replace_v; + a | b | a | b +---+---+---+--- + 6 | 1 | 6 | 1 +(1 row) + +DROP VIEW mvtest_replace_v; +-- index gets rebuilt when replacing with data +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 7 AS a, 1 AS b; +CREATE UNIQUE INDEX ON mvtest_replace (b); +SELECT * FROM mvtest_replace; + a | b +---+--- + 7 | 1 +(1 row) + +SET enable_seqscan = off; -- force index scan +EXPLAIN (COSTS OFF) SELECT * FROM mvtest_replace WHERE b = 1; + QUERY PLAN +--------------------------------------------------------- + Index Scan using mvtest_replace_b_idx on mvtest_replace + Index Cond: (b = 1) +(2 rows) + +SELECT * FROM mvtest_replace WHERE b = 1; + a | b +---+--- + 7 | 1 +(1 row) + +RESET enable_seqscan; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 8 AS a, 1 AS b; +SET enable_seqscan = off; -- force index scan +EXPLAIN (COSTS OFF) SELECT * FROM mvtest_replace WHERE b = 1; + QUERY PLAN +--------------------------------------------------------- + Index Scan using mvtest_replace_b_idx on mvtest_replace + Index Cond: (b = 1) +(2 rows) + +SELECT * FROM mvtest_replace WHERE b = 1; + a | b +---+--- + 8 | 1 +(1 row) + +RESET enable_seqscan; +-- cannot change column data type +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 9 AS a, 'x' AS b; -- error +ERROR: cannot change data type of materialized view column "b" from integer to text +SELECT * FROM mvtest_replace; + a | b +---+--- + 8 | 1 +(1 row) + +-- cannot rename column +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 10 AS a, 1 AS b2; -- error +ERROR: cannot change name of materialized view column "b" to "b2" +HINT: Use ALTER MATERIALIZED VIEW ... RENAME COLUMN ... to change name of materialized view column instead. +SELECT * FROM mvtest_replace; + a | b +---+--- + 8 | 1 +(1 row) + +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 11 AS a, 1 AS b, 'y' COLLATE "C" AS c; +SELECT * FROM mvtest_replace; + a | b | c +----+---+--- + 11 | 1 | y +(1 row) + +-- cannot change column collation +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 12 AS a, 1 AS b, 'x' COLLATE "POSIX" AS c; -- error +ERROR: cannot change collation of materialized view column "c" from "C" to "POSIX" +SELECT * FROM mvtest_replace; + a | b | c +----+---+--- + 11 | 1 | y +(1 row) + +-- cannot drop column +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 13 AS a, 1 AS b; -- error +ERROR: cannot drop columns from materialized view +SELECT * FROM mvtest_replace; + a | b | c +----+---+--- + 11 | 1 | y +(1 row) + +-- must target a matview +CREATE VIEW mvtest_not_mv AS + SELECT 1 AS a; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_not_mv AS + SELECT 1 AS a; -- error +ERROR: "mvtest_not_mv" is not a materialized view +DROP VIEW mvtest_not_mv; +-- cannot use OR REPLACE with IF NOT EXISTS +CREATE OR REPLACE MATERIALIZED VIEW IF NOT EXISTS mvtest_replace AS + SELECT 1 AS a; +ERROR: syntax error at or near "NOT" +LINE 1: CREATE OR REPLACE MATERIALIZED VIEW IF NOT EXISTS mvtest_rep... + ^ +DROP MATERIALIZED VIEW mvtest_replace; diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql index 6704eeae2df..fe0cb4d25bf 100644 --- a/src/test/regress/sql/matview.sql +++ b/src/test/regress/sql/matview.sql @@ -314,3 +314,120 @@ EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF, BUFFERS OFF) CREATE MATERIALIZED VIEW IF NOT EXISTS matview_ine_tab AS SELECT 1 / 0 WITH NO DATA; -- ok DROP MATERIALIZED VIEW matview_ine_tab; + +-- +-- test CREATE OR REPLACE MATERIALIZED VIEW +-- + +-- matview does not already exist +DROP MATERIALIZED VIEW IF EXISTS mvtest_replace; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 1 AS a; +SELECT * FROM mvtest_replace; + +-- replace query with data +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 2 AS a; +SELECT * FROM mvtest_replace; + +-- replace query without data +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 3 AS a + WITH NO DATA; +SELECT * FROM mvtest_replace; -- error: not populated +REFRESH MATERIALIZED VIEW mvtest_replace; +SELECT * FROM mvtest_replace; + +-- add column +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 4 AS a, 1 b; +SELECT * FROM mvtest_replace; + +-- replace table options +SELECT m.*, c.relname, c.reloptions, s.spcname, a.amname + FROM mvtest_replace m + CROSS JOIN pg_class c + LEFT JOIN pg_tablespace s ON s.oid = c.reltablespace + LEFT JOIN pg_am a ON a.oid = c.relam + WHERE c.relname = 'mvtest_replace'; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace + USING heap2 + WITH (fillfactor = 50) + TABLESPACE regress_tblspace + AS SELECT 5 AS a, 1 AS b; +SELECT m.*, c.relname, c.reloptions, s.spcname, a.amname + FROM mvtest_replace m + CROSS JOIN pg_class c + LEFT JOIN pg_tablespace s ON s.oid = c.reltablespace + LEFT JOIN pg_am a ON a.oid = c.relam + WHERE c.relname = 'mvtest_replace'; +-- restore default options +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace + AS SELECT 5 AS a, 1 AS b; +SELECT m.*, c.relname, c.reloptions, s.spcname, a.amname + FROM mvtest_replace m + CROSS JOIN pg_class c + LEFT JOIN pg_tablespace s ON s.oid = c.reltablespace + LEFT JOIN pg_am a ON a.oid = c.relam + WHERE c.relname = 'mvtest_replace'; + +-- can replace matview that has a dependent view +CREATE VIEW mvtest_replace_v AS + SELECT * FROM mvtest_replace; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 6 AS a, 1 AS b; +SELECT * FROM mvtest_replace, mvtest_replace_v; +DROP VIEW mvtest_replace_v; + +-- index gets rebuilt when replacing with data +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 7 AS a, 1 AS b; +CREATE UNIQUE INDEX ON mvtest_replace (b); +SELECT * FROM mvtest_replace; +SET enable_seqscan = off; -- force index scan +EXPLAIN (COSTS OFF) SELECT * FROM mvtest_replace WHERE b = 1; +SELECT * FROM mvtest_replace WHERE b = 1; +RESET enable_seqscan; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 8 AS a, 1 AS b; +SET enable_seqscan = off; -- force index scan +EXPLAIN (COSTS OFF) SELECT * FROM mvtest_replace WHERE b = 1; +SELECT * FROM mvtest_replace WHERE b = 1; +RESET enable_seqscan; + +-- cannot change column data type +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 9 AS a, 'x' AS b; -- error +SELECT * FROM mvtest_replace; + +-- cannot rename column +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 10 AS a, 1 AS b2; -- error +SELECT * FROM mvtest_replace; + +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 11 AS a, 1 AS b, 'y' COLLATE "C" AS c; +SELECT * FROM mvtest_replace; + +-- cannot change column collation +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 12 AS a, 1 AS b, 'x' COLLATE "POSIX" AS c; -- error +SELECT * FROM mvtest_replace; + +-- cannot drop column +CREATE OR REPLACE MATERIALIZED VIEW mvtest_replace AS + SELECT 13 AS a, 1 AS b; -- error +SELECT * FROM mvtest_replace; + +-- must target a matview +CREATE VIEW mvtest_not_mv AS + SELECT 1 AS a; +CREATE OR REPLACE MATERIALIZED VIEW mvtest_not_mv AS + SELECT 1 AS a; -- error +DROP VIEW mvtest_not_mv; + +-- cannot use OR REPLACE with IF NOT EXISTS +CREATE OR REPLACE MATERIALIZED VIEW IF NOT EXISTS mvtest_replace AS + SELECT 1 AS a; + +DROP MATERIALIZED VIEW mvtest_replace; -- 2.50.1 --r33ycpluvyfavkwy Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="v7-0002-Handle-default-tablespace-in-AlterTableInternal.patch" ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-05-21 16:35 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-05-06 20:53 [PATCH v7 2/2] Remove logic for unloading dynamically loaded files. Nathan Bossart <nathandbossart@gmail.com> 2022-05-06 20:53 [PATCH v6 2/2] Remove logic for unloading dynamically loaded files. Nathan Bossart <nathandbossart@gmail.com> 2022-05-06 20:53 [PATCH v7 2/2] Remove logic for unloading dynamically loaded files. Nathan Bossart <nathandbossart@gmail.com> 2022-05-06 20:53 [PATCH v8 1/2] Remove logic for unloading dynamically loaded files. Nathan Bossart <nathandbossart@gmail.com> 2022-05-06 20:53 [PATCH v8 1/2] Remove logic for unloading dynamically loaded files. Nathan Bossart <nathandbossart@gmail.com> 2022-05-06 20:53 [PATCH v6 2/2] Remove logic for unloading dynamically loaded files. Nathan Bossart <nathandbossart@gmail.com> 2024-05-21 16:35 [PATCH v7 1/3] Add OR REPLACE option to CREATE MATERIALIZED VIEW Erik Wienhold <ewie@ewie.name>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox