Feature patch 1 for plperl - open issues
От | Tim Bunce |
---|---|
Тема | Feature patch 1 for plperl - open issues |
Дата | |
Msg-id | 20100111203857.GH59689@timac.local обсуждение исходный текст |
Список | pgsql-hackers |
Thanks for all the feedback! I'm going to try to summarize and address the issues raised. (Meanwhile I've started spliting the patch into at least three parts, per Andrew's suggestion: utility functions, the GUCs, the rest.) * Long GUC values I think the underlying assumption that they'll be long values isn't valid. Any non-trivial use should be implemented by loading a private module, like Andrew's example: 'use lib "/my/app"; use MyApp::Pg;' The lack of multi-line GUCs will naturally encourage that and I'll ensure the docs do as well. * Running arbitrary user-defined code in the postmaster I think there's agreement that the ability to load code in the postmaster is very useful (http://markmail.org/message/wl7dmcbcrkwv2jz2) The concern is only over the safety of combining on_perl_init with shared_preload_libraries. Others have pointed out that the very purpose of shared_preload_libraries is to load and run (init) user-defined code in the postmaster. Albeit in the form of C code. I thought I'd sufficiently addressed the concerns that were raised previously (http://markmail.org/message/u7zhkjkwmztk5hkw). Specifically, on_perl_init is PGC_SUSET (I'll change this to PGC_SIGHUP per Tom's recent email) and SPI functions aren't available for on_*_init code. So the on_*_init code has no natural way to interact with the server. If the code fails (dies/throws an exception) an error is raised. * Running arbitrary user-defined code at backend shutdown In the same way that on_*_init code has no natural way to interact with the server I'm keen for the same to apply to END block code run at backend shutdown. I'd assumed that the SPI functions would fail anyway during shutdown but I hadn't explicitly tested for it (viewing it as a foot shooting gun). I'll explicitly arrange for SPI functions to fail during server shutdown and test for that in the next revision of the patch. > What happens when the supplied code has errors takes an unreasonable amount > of time to run, does something unsafe, depends on the backend not being in an > error state already, etc etc? Since there's no interaction with the server I think the only one of those that needs addressing is "takes an unreasonable amount of time to run" and (from a different reply) "delaying or even preventing shutdown". I'm puzzled by that one as I don't see how it's different to the user doingDO $$ sleep 99999 $$ language plperl; # or anyother PL I'd be grateful if someone could explain the difference. * Visibility of shared library loading event > loading of the plperl shared > library should not be a semantically significant event from the user's > viewpoint. If these GUCs were PGC_POSTMASTER it wouldn't be so bad, but > Tim still seems to want them to be settable inside an individual session > before the library is loaded, which makes the loading extremely visible. The perl interpreter has an initial state. The only effect of on_*_init should be to allow the user to choose a different initial state (by running arbitrary code to move from the default state to another). I presume some might say that the first use of a temporary table should not be a semantically significant event from the user's viewpoint. Yet the temp_buffers GUC says: The setting can be changed within individual sessions, but only up until the first use of temporary tables within a session A practical use-case for this plperl.on_*trusted_init behaviour isplperl.on_untrusted_init='use Devel::NYTProf::PgPLPerl' (Or plperl.on_trusted_init if the DBA has pre-loaded it to make it available.) NYTProf needs to be loaded before the code it's going to profile in order to provide full functionality. > As an example, if people were using such functionality then the DBA > couldn't start preloading plperl for performance without breaking > behavior that some of his users might be depending on. This isn't the case. Users can only set the on_trusted_init and on_untrusted_init GUCs. The code in those is only executed when the interpreter is transitioned from its initial HELD state by the user's first use. So the timing of plperl.on_*trusted_init execution isn't altered by shared_preload_libraries. (The on_perl_init GUC, under DBA control, is the only one executed at the time plperl is loaded.) I'd be happy to document plperl.on_trusted_init and on_untrusted_init in the Developer Options section of the docs if that would help set user-expectations. On Sun, Jan 10, 2010 at 07:26:01PM -0500, Tom Lane wrote: > > Just looking over this patch, I don't think it's nearly robust enough > against initialization failures. The original code wasn't very good > about that either, but that was (more or less) okay because it was > executing predetermined, pretested code that we really don't expect to > fail. I think the standard has to be a *lot* higher if we are going to > execute user-supplied perl code there. You need to make sure that > things are left in a reasonably consistent, safe state if an error > occurs. An exception from plperl.on_perl_init causes elog(ERROR, ...); Currently an exception from plperl.on_trusted_init and on_untrusted_init cause a WARNING. I'll change those to ERROR. If there are other issues in this regard I'd appreciate specific details. > Along the same line, there needs to be more effort put into the errors > that can be thrown when one of these failures happen. The current > messages don't follow our style guidelines very well, and aren't exposed > for translation. Per http://developer.postgresql.org/pgdocs/postgres/error-message-reporting.html and http://developer.postgresql.org/pgdocs/postgres/error-style-guide.html Okay, I'll fix those. Tim.
В списке pgsql-hackers по дате отправления: