The pg_stat_statements patch tries to introduce a custom GUC variable
that's marked with context PGC_POSTMASTER, betokening the fact that
it's setting the allocated size of a portion of shared memory and so
changing it after startup is pointless/impossible.
This doesn't actually work in the current system. The patch adds
this diff hunk in the vain hope of trying to make it work:
diff -cprN HEAD/src/backend/utils/misc/guc.c
pg_stat_statements/src/backend/utils/misc/guc.c
*** HEAD/src/backend/utils/misc/guc.c 2008-12-05 01:03:08.315984000 +0900
--- pg_stat_statements/src/backend/utils/misc/guc.c 2008-12-26
14:51:58.078125000 +0900
*************** define_custom_variable(struct config_gen
*** 5707,5713 ****
case PGC_S_ENV_VAR:
case PGC_S_FILE:
case PGC_S_ARGV:
! phcontext = PGC_SIGHUP;
break;
case PGC_S_DATABASE:
case PGC_S_USER:
--- 5717,5726 ----
case PGC_S_ENV_VAR:
case PGC_S_FILE:
case PGC_S_ARGV:
! if (variable->context == PGC_POSTMASTER)
! phcontext = PGC_POSTMASTER;
! else
! phcontext = PGC_SIGHUP;
break;
case PGC_S_DATABASE:
case PGC_S_USER:
but all that does is prevent DefineCustomIntVariable() from failing
outright. That's not nearly good enough IMHO. The problem with it
is that the you-can't-change-it rule will only be enforced after the
placeholder variable has been replaced. So for example, if I have
custom_variable_classes = 'foo' and module foo expects to define
a PGC_POSTMASTER variable foo.bar, then I can do this:
postgres=# set foo.bar = 'this';
postgres=# load 'foo';
and in another session I can do this:
postgres=# set foo.bar = 'that';
postgres=# load 'foo';
and now I have two sessions running concurrently with different settings
of a "postmaster" variable.
Now, to the extent that the variable is actually only *used* to
determine the size of a shared-memory request, this isn't really a
problem because the relevant action is taken only once. The danger that
I'm seeing is that the programmer might assume that the value is the
same across all sessions --- a trap Itagaki-san actually did fall into
in pg_stat_statements, so this isn't academic. Safe coding would
require that whichever backend initializes the shmem structure copy the
size value it used into shmem, and subsequently make backends look at that
copy instead of whatever their local GUC variable happens to contain.
I'm thinking we should not apply the above diff, which would have the
effect of (continuing to) prevent custom GUCs with ratings higher than
PGC_SIGHUP, which might help discourage extension programmers from
imagining that the variable's value is guaranteed stable. This still
seems pretty wide open for coding errors though. It would be better
if we could somehow make PGC_POSTMASTER work as intended, but I'm not
seeing how.
The case that actually works safely, which is the intended use-case for
pg_stat_statements, is that the module is preloaded into the postmaster
using shared_preload_libraries. If we could require PGC_POSTMASTER
variables to be created only then, it would work alright, but we haven't
enough context to make this work in the EXEC_BACKEND case. (When
DefineCustomFooVariable is executed in a child process, it doesn't know
what happened in the postmaster; and even if it did, throwing an error
would be unhelpful. The module is already loaded and probably partially
hooked into the system...)
So it looks pretty much like a mess. Ideas?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
