At Fri, 22 Jul 2022 10:00:34 +0900, Michael Paquier <mich...@paquier.xyz> wrote in > On Thu, Jul 21, 2022 at 05:39:35PM -0700, Gurjeet Singh wrote: > > One notable side effect of this change is that > > process_session_preload_libraries() is now called _before_ we > > SetProcessingMode(NormalProcessing). Which means any database access > > performed by _PG_init() of an extension will be doing it in > > InitProcessing mode. I'm not sure if that's problematic. > > I cannot see a reason why on top of my mind. The restrictions of > InitProcessing apply to two code paths of bgworkers connecting to a > database, and normal processing is used as a barrier to prevent the > creation of some objects. > > > The patch also adds an assertion in pg_parameter_aclcheck() to ensure > > that there's a transaction is in progress before it's called. > > + /* It's pointless to call this function, unless we're in a > transaction. */ > + Assert(IsTransactionState()); > > This can involve extension code, I think that this should be at least > an elog(ERROR) so as we have higher chances of knowing if something > still goes wrong in the wild.
pg_parameter_aclmask involves the same assertion, so the same backtrace can be obtained without it. I think it is no bad of users so I'm not sure ERROR is appropriate even if we were to add something there. > > The patch now lets the user connect, throws a warning, and does not crash. > > > > $ PGOPTIONS="-c plperl.on_plperl_init=" psql -U test > > WARNING: permission denied to set parameter "plperl.on_plperl_init" > > Expanded display is used automatically. > > psql (15beta1) > > Type "help" for help. > > I am wondering whether we'd better have a test on this one with a > non-superuser. Except for a few tests in the unsafe section, > session_preload_libraries has a limited amount of coverage. +1 > + /* > + * process any libraries that should be preloaded at backend start > (this > + * can't be done until GUC settings are complete). Note that these > libraries > + * can declare new GUC variables. > + */ > + process_session_preload_libraries(); > There is no point in doing that during bootstrap anyway, no? This patch makes process_session_preload_libraries called in autovacuum worker/launcher and background worker in addition to client backends. It seems to me we also need to prevent that. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
