On Wed, Apr 11, 2018 at 2:34 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > That test is, therefore, wrong. Otherwise, no non-builtin function > could ever be marked parallel safe, for fear that the shlib it lives > in might try to set up a custom variable at load time.
I don't follow that logic. If the check is more stringent than it needs to be for safety, then we can relax it. If the check is less stringent than it needs to be for safety, we need to tighten it. If it's exactly right, then it has to stay the way it is, even if that prohibits some things we wish we could allow. > I'm of the opinion that having such a test here at all is crummy design. > It implies that set_config_option is in charge of knowing about every > one of its callers and passing judgment on whether they represent > parallel-safe usages, which is the exact opposite of modularity, > even if set_config_option had enough information to make that judgment > which it does not. The point of SerializeGUCState and RestoreGUCState is to enforce an invariant that all cooperating processes have the same idea about the values of every GUC. If you let the GUCs be changed, then isn't that invariant is violated regardless of why you let it happen? For example: rhaas=# set pg_trgm.similarity_threshold = '002'; SET rhaas=# select current_setting('pg_trgm.similarity_threshold'); current_setting ----------------- 002 (1 row) rhaas=# load 'pg_trgm'; WARNING: 2 is outside the valid range for parameter "pg_trgm.similarity_threshold" (0 .. 1) LOAD rhaas=# select current_setting('pg_trgm.similarity_threshold'); current_setting ----------------- 0.3 (1 row) This shows that if you load a library that defines a custom variable in process A but not process B, it's trivial to construct a query that no longer returns the same answer in both backends, which isn't too good, because the point of parallel query is to deliver the same answer with parallel query that you would have gotten without it, or at the very least, to make it look like the answer was generated by a single process with a unified view of the world rather than multiple uncoordinated processes. I tried the following test with Jeff's example function and with plperl.on_init='' and it does not produce a warning: rhaas=# set force_parallel_mode = on; SET rhaas=# select foobar('dsgsdg'); foobar -------- gdsgsd (1 row) I haven't tracked it down, but I think what must be going on here is that, when the function is called via a query, something triggers the shared library to get loaded before we enter parallel mode. Then things work OK, because the worker will load the shared library before restoring the leader's GUC state, and so the state is actually guaranteed to match. Here it will *probably* end up matching because most likely every process that loads the module will interpret the existing textual setting of the newly-defined GUC in the same way, but it's a bit less ironclad, and if they decide to emit warnings (as in my first example, above) then you'll get multiple copies of them. So I don't think just drilling a hole through the prohibition in set_config_option() is a particularly appealing solution. It would amount to suppressing a warning that is basically legitimate. I wonder if there's an easy way to force the libraries that we're going to need to be loaded before we reach CreateParallelContext(). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company