Thread: Custom PGC_POSTMASTER GUC variables ... feasible?

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

I wrote:
> ... This doesn't actually work in the current system.

I have a solution for this, I think.  I propose that guc.c only allow
custom PGC_POSTMASTER variables to be created during
process_shared_preload_libraries().  (The implementation for this
would involve exporting a bool flag that process_shared_preload_libraries
sets while it's running.)  That would come down to two cases:

1. A loadable library is being preloaded into the postmaster.  This
happens only at postmaster startup, so it's clearly okay to create
a PGC_POSTMASTER variable then.

2. A loadable library is being loaded during startup of a child process
in the EXEC_BACKEND case.  Since the shared_preload_libraries list is
itself a PGC_POSTMASTER variable, it can't have been changed since
postmaster start.  Therefore, the library we are loading is also present
in the postmaster and the no-change-after-start rule has been enforced
on the variable all along.  We're just instantiating it in the current
process, and we can trust that the value we inherited matches other
processes.

You could break this if you tried hard enough, like replace a library
with a new version underneath a running EXEC_BACKEND system, where
the new copy creates a PGC_POSTMASTER variable that the original
didn't.  But that requires superuser privileges so it's not a security
hazard, just a don't-do-that issue.  (There are plenty of other ways
such a replacement could break things, anyhow.)

What this would mean is that a library that needs to define a
PGC_POSTMASTER variable would need to fail if loaded with an ordinary
LOAD command.  The most graceful way to do that is for it to examine the
process_shared_preload_libraries_in_progress flag for itself in its
_PG_init hook, and if not set report a warning and exit without doing
anything.  If it fails to do so, and control actually gets to the point
of guc.c having to reject the PGC_POSTMASTER variable creation, I think
we'd better make that be a FATAL error.  The reason is that the
noncooperative library may be partially hooked into the backend already
and so its behavior is likely to be messed up.
        regards, tom lane

On Sat, Jan 3, 2009 at 09:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> ... This doesn't actually work in the current system.
>
> I have a solution for this, I think.  I propose that guc.c only allow
> custom PGC_POSTMASTER variables to be created during
> process_shared_preload_libraries().

Sounds simple enough.

> You could break this if you tried hard enough, like replace a library
> with a new version underneath a running EXEC_BACKEND system, where
> the new copy creates a PGC_POSTMASTER variable that the original
> didn't.  But that requires superuser privileges so it's not a security
> hazard, just a don't-do-that issue.  (There are plenty of other ways
> such a replacement could break things, anyhow.)

Right I agree this is a non-issue.  For that matter if I really wanted
to muck with it I could just set
process_shared_preload_libraries_in_progress = true in my _PG_init
function.  And I guess if anyone thinks thats a problem we can mark
the flag as static and only export a function for reading the value.

> What this would mean is that a library that needs to define a
> PGC_POSTMASTER variable would need to fail if loaded with an ordinary
> LOAD command.  The most graceful way to do that is for it to examine the
> process_shared_preload_libraries_in_progress flag for itself in its
> _PG_init hook, and if not set report a warning and exit without doing
> anything.  If it fails to do so, and control actually gets to the point
> of guc.c having to reject the PGC_POSTMASTER variable creation, I think
> we'd better make that be a FATAL error.  The reason is that the
> noncooperative library may be partially hooked into the backend already
> and so its behavior is likely to be messed up.

Agreed.

"Alex Hunsaker" <badalex@gmail.com> writes:
> Right I agree this is a non-issue.  For that matter if I really wanted
> to muck with it I could just set
> process_shared_preload_libraries_in_progress = true in my _PG_init
> function.  And I guess if anyone thinks thats a problem we can mark
> the flag as static and only export a function for reading the value.

Yeah, I thought about that and decided to leave it as a variable ---
if anyone actually has a good reason to do it, they have an (ugly)
workaround available this way.  We're only trying to catch errors
of omission, not prevent C-level code from subverting the system
if it wants to.
        regards, tom lane

In PG10.3, guc.c's init_custom_variables issues a FATAL error if an attempt
is made to define a custom PGC_POSTMASTER variable after startup (see
below), and that ERROR wouldn't be safe, in general.

But this means that if a user does:

    CREATE EXTENSION anyExtensionThatDefinesA_PGC_POSTMASTER_Variable;

but forgets to add the extension to shared_preload_libraries first, then
said user will crash the server, rather than just getting an error message.
This is an easy mistake for a user to make, and a severe consequence.  It
may even be considered a security hole if a malicious user can crash the
server so easily.

What were the possible failure scenarios that throwing a fatal error was
intended to avoid, i.e. what sort of "hooking into" is the comment below
referring to that was regarded as a fate worse than death?

If throwing just an ERROR level elog is truly a fate worse than FATAL, how
should the extension writer protect their users from crashing the server
when they make this mistake?


   │8012            /*
   │8013             * Only allow custom PGC_POSTMASTER variables to be
created during shared
   │8014             * library preload; any later than that, we can't ensure
that the value
   │8015             * doesn't change after startup.  This is a fatal elog
if it happens; just
   │8016             * erroring out isn't safe because we don't know what
the calling loadable
   │8017             * module might already have hooked into.
   │8018             */
B+>│8019            if (context == PGC_POSTMASTER &&
   │8020                    !process_shared_preload_libraries_in_progress)
   │8021                    elog(FATAL, "cannot create PGC_POSTMASTER
variables after startup");





--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Hello
See for example contrib pg_stat_statements extension. Setting pg_stat_statements.max defined as PGC_POSTMASTER

regards, Sergei

ok - FATAL just causes the current session to abort, as opposed to PANIC.



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Jim Finnerty <jfinnert@amazon.com> writes:
> What were the possible failure scenarios that throwing a fatal error was
> intended to avoid, i.e. what sort of "hooking into" is the comment below
> referring to that was regarded as a fate worse than death?

The point is that if the extension is marking the variable as
PGC_POSTMASTER, it's presumably relying on that variable having the same
value in every process.  It might be using it as the size of an array in
shared memory, say.  If some processes have a different value, that could
end in a memory stomp, or some other crash that's substantially less clean
than a FATAL exit.

            regards, tom lane