Thread: Feedback on writing extensible modules
Some feedback 1. Want some very clear and supported way to know whether Postgres is fully up. Currently, if you write _PG_init you sometimes need to know if it is being executed by LOAD or as a reload. So actual initialisation sometimes needs to happen outside of _PG_init. 2. shmem_startup_hook doesn't allow multiple modules to create shmem. All callers of the hook think they are the only caller, causing chaos if multiple people need this. Currently, whoever sets up the hook gets to create shmem. (There's no docs for this yet). Would prefer something like RequestAddinShmemSpace() which can be called by multiple callers. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Hi, Le 20 mai 09 à 20:51, Simon Riggs a écrit : > 1. Want some very clear and supported way to know whether Postgres is > fully up. Currently, if you write _PG_init you sometimes need to > know if > it is being executed by LOAD or as a reload. So actual initialisation > sometimes needs to happen outside of _PG_init. And currently calling SPI_connect() from _PG_init will crash the backend. I'll try to obtain a gdb backtrace, I've just been told about pre_auth_delay and post_auth_delay parameters. Regards, -- dim
Simon Riggs <simon@2ndQuadrant.com> writes:
> 2. shmem_startup_hook doesn't allow multiple modules to create shmem.
> All callers of the hook think they are the only caller, causing chaos if
> multiple people need this.
The only known caller, contrib/pg_stat_statements/, does not think that.
Do what it does.
I agree it's undocumented, but so are the other hooks ...
regards, tom lane
On Wed, 2009-05-20 at 16:03 -0400, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > > 2. shmem_startup_hook doesn't allow multiple modules to create shmem. > > All callers of the hook think they are the only caller, causing chaos if > > multiple people need this. > > The only known caller, contrib/pg_stat_statements/, does not think that. > Do what it does. Hmmm, it works, I guess. As long as everyone does that. Thanks for the workaround. > I agree it's undocumented, but so are the other hooks ... Some are well documented in the code, not all though. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Dimitri Fontaine <dfontaine@hi-media.com> writes:
> And currently calling SPI_connect() from _PG_init will crash the
> backend. I'll try to obtain a gdb backtrace, I've just been told about
> pre_auth_delay and post_auth_delay parameters.
Here we go:
(gdb) handle SIGABRT nopass
Signal Stop Print Pass to program Description
SIGABRT Yes Yes No Aborted
(gdb) continue
Program received signal SIGABRT, Aborted.
0xb802d424 in __kernel_vsyscall ()
(gdb) bt
#0 0xb802d424 in __kernel_vsyscall ()
#1 0xb7e7c640 in raise () from /lib/i686/cmov/libc.so.6
#2 0xb7e7dfa1 in abort () from /lib/i686/cmov/libc.so.6
#3 0x082dadde in ExceptionalCondition (conditionName=0x83cbfe0 "!(((context) != ((void *)0) &&
(((((Node*)((context)))->type)== T_AllocSetContext))))",
errorType=0x830bc09 "BadArgument", fileName=0x83be166 "mcxt.c", lineNumber=507) at assert.c:57
#4 0x082f8abb in MemoryContextAlloc (context=0x0, size=448) at mcxt.c:507
#5 0x081a93a3 in SPI_connect () at spi.c:81
#6 0xb582cf15 in _PG_init () at pre_prepare.c:150
#7 0x082df913 in internal_load_library (libname=0x9808da4 "/home/dim/pgsql/8.3/lib/plugins/pre_prepare.so") at
dfmgr.c:296
#8 0x082dfc38 in load_file (filename=0x9809d00 "$libdir/plugins/pre_prepare", restricted=1 '\001') at dfmgr.c:153
#9 0x082e7554 in load_libraries (libraries=<value optimized out>, gucname=0x9809d00 "$libdir/plugins/pre_prepare",
restricted=1'\001') at miscinit.c:1185
#10 0x08233ce2 in PostgresMain (argc=4, argv=0x9807fb8, username=0x9807f90 "dim") at postgres.c:3314
#11 0x0820054c in ServerLoop () at postmaster.c:3207
#12 0x0820124b in PostmasterMain (argc=3, argv=0x97f1bd8) at postmaster.c:1029
#13 0x081b2b39 in main (argc=3, argv=0x97f1bd8) at main.c:188
And I'm runnin a CVS version of 8.3 I'm not sure is the last update in
the branch, so here's what I have at mcxt.c:507
504 void *
505 MemoryContextAlloc(MemoryContext context, Size size)
506 {
507 AssertArg(MemoryContextIsValid(context));
508
509 if (!AllocSizeIsValid(size))
510 elog(ERROR, "invalid memory alloc request size %lu",
511 (unsigned long) size);
That's with attached patch to pre_prepare.c from pgfoundry:
http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/preprepare/preprepare/
If you need any more information from me, or for me to rerun with
another server version, please ask. I'm very interrested in being able
to prepare a query at local_preload_libraries time, if possible in 8.3
and following releases.
Regards,
--
dim
Index: pre_prepare.c
===================================================================
RCS file: /cvsroot/preprepare/preprepare/pre_prepare.c,v
retrieving revision 1.1
diff -p -u -r1.1 pre_prepare.c
--- pre_prepare.c 13 May 2009 20:54:04 -0000 1.1
+++ pre_prepare.c 25 May 2009 13:37:52 -0000
@@ -35,6 +35,7 @@
PG_MODULE_MAGIC;
+static bool pre_prepare_at_init = NULL;
static char *pre_prepare_relation = NULL;
void _PG_init(void);
@@ -125,6 +126,15 @@ int pre_prepare_all() {
*/
void
_PG_init(void) {
+ DefineCustomBoolVariable("preprepare.at_init",
+ "Do we prepare the statements at backend init start",
+ "You have to setup local_preload_libraries too",
+ &pre_prepare_at_init,
+ PGC_USERSET,
+ NULL,
+ NULL);
+ EmitWarningsOnPlaceholders("prepare.at_init");
+
DefineCustomStringVariable("preprepare.relation",
"Table name where to find statements to prepare",
"Can be schema qualified, must have columns \"name\" and \"statement\"",
@@ -132,8 +142,21 @@ _PG_init(void) {
PGC_USERSET,
NULL,
NULL);
-
EmitWarningsOnPlaceholders("prepare.relation");
+
+ if( pre_prepare_at_init ) {
+ int err;
+
+ err = SPI_connect();
+ if (err != SPI_OK_CONNECT)
+ elog(ERROR, "SPI_connect: %s", SPI_result_code_string(err));
+
+ pre_prepare_all();
+
+ err = SPI_finish();
+ if (err != SPI_OK_FINISH)
+ elog(ERROR, "SPI_finish: %s", SPI_result_code_string(err));
+ }
}
/*
Dimitri Fontaine <dfontaine@hi-media.com> writes:
> Dimitri Fontaine <dfontaine@hi-media.com> writes:
>> And currently calling SPI_connect() from _PG_init will crash the
>> backend. I'll try to obtain a gdb backtrace, I've just been told about
>> pre_auth_delay and post_auth_delay parameters.
> Here we go:
The reason this doesn't work is that SPI can only be invoked inside a
transaction, and you're not inside one when a library is being
preloaded.
> I'm very interrested in being able
> to prepare a query at local_preload_libraries time, if possible in 8.3
> and following releases.
You could maybe make this work by executing your own transaction
to do it, but I really have to wonder if it's a good idea. One
point to think about is that elog(ERROR) still means elog(FATAL)
at this point, so any brokenness in the queries you're trying to
prepare will result in locking all users out of the database.
regards, tom lane
Hi, First, thank you to have taken the time to see about the case. Le 31 mai 09 à 18:21, Tom Lane a écrit : > The reason this doesn't work is that SPI can only be invoked inside a > transaction, and you're not inside one when a library is being > preloaded. Makes sense. Still crashing with basic naive testing, will report back when I have more time to work on it. > You could maybe make this work by executing your own transaction > to do it, but I really have to wonder if it's a good idea. One > point to think about is that elog(ERROR) still means elog(FATAL) > at this point, so any brokenness in the queries you're trying to > prepare will result in locking all users out of the database. Yeah that's a pretty good foot gun, yet another one. But preprepare.at_init is optional and defaults to off. If you broke it all, you can turn it off again and reload. As for the FATAL, I guess that having preprepare crashing backend creations rather than having your EXECUTE fail and ROLLBACK your transactions is not so much a difference when you need preprepare in the first place... I'll add a note in the documentation to always manually call SELECT prepare_all() at each prepare statements list modification before to turn at_init on, as soon as at_init is possible. Regards, -- dim
Dimitri Fontaine wrote: > Le 31 mai 09 à 18:21, Tom Lane a écrit : >> You could maybe make this work by executing your own transaction >> to do it, but I really have to wonder if it's a good idea. One >> point to think about is that elog(ERROR) still means elog(FATAL) >> at this point, so any brokenness in the queries you're trying to >> prepare will result in locking all users out of the database. > > Yeah that's a pretty good foot gun, yet another one. But > preprepare.at_init is optional and defaults to off. If you broke it all, > you can turn it off again and reload. Maybe you could set a callback to be called during the first transaction in InitPostgres ... in full knowledge that if it fails, people will be locked out of the database. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Mon, 2009-06-01 at 12:23 -0400, Alvaro Herrera wrote: > Dimitri Fontaine wrote: > > > Le 31 mai 09 à 18:21, Tom Lane a écrit : > > >> You could maybe make this work by executing your own transaction > >> to do it, but I really have to wonder if it's a good idea. One > >> point to think about is that elog(ERROR) still means elog(FATAL) > >> at this point, so any brokenness in the queries you're trying to > >> prepare will result in locking all users out of the database. > > > > Yeah that's a pretty good foot gun, yet another one. But > > preprepare.at_init is optional and defaults to off. If you broke it all, > > you can turn it off again and reload. > > Maybe you could set a callback to be called during the first transaction > in InitPostgres ... in full knowledge that if it fails, people will be > locked out of the database. Should be possible to define a custom variable that has an assign hook that does nothing unless called with PGC_S_DATABASE or PGC_S_USER. That should guarantee the code only runs after connection, rather than at server startup. Not tried it yet. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Hi, Le 31 mai 09 à 18:21, Tom Lane a écrit : > The reason this doesn't work is that SPI can only be invoked inside a > transaction, and you're not inside one when a library is being > preloaded. Please find attached a little little patch which run process_local_preload_libraries from within a transaction. The following patch to preprepare makes it working fine when the GUC preprepare.at_init is on and the module is being loaded from local_preload_librairies: http://cvs.pgfoundry.org/cgi-bin/cvsweb.cgi/preprepare/preprepare/pre_prepare.c.diff?r1=1.1&r2=1.2 > You could maybe make this work by executing your own transaction > to do it I took the option to have PostgreSQL provide the same context in preloading that when loading later, in that in both case _PG_init() runs inside an already existing transaction. I don't see a current way for _PG_init() to distinguish between being called at backend fork() time or from a user transaction, so figured it was better this way. > but I really have to wonder if it's a good idea. One > point to think about is that elog(ERROR) still means elog(FATAL) > at this point, so any brokenness in the queries you're trying to > prepare will result in locking all users out of the database. Well loading custom code at init time is a foot-gun reserved to superusers with access to the local file system (where to put the custom .so) and allowed to signal postmaster. You're right that a failure in the module init routine will prevent anyone from connecting to the server, but the cure is to clean local_preload_librairies then restart. Or with the preprepare example, to set preprepare.at_init to off then reload. Regards, -- dim PS: sorry I don't have the toolsuite to provide a context diff tonight, but given the size of the patch I figured I'd send it anyway. Will cook a context diff tomorrow if needed, it's running late here. Oh, we're already tomorrow, even.
Attachment
Dimitri Fontaine <dfontaine@hi-media.com> writes:
> Please find attached a little little patch which run
> process_local_preload_libraries from within a transaction.
This is inevitably going to break other people's code. Put the
transaction wrapper in your own stuff if you have to have it.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Dimitri Fontaine <dfontaine@hi-media.com> writes: >> Please find attached a little little patch which run >> process_local_preload_libraries from within a transaction. > > This is inevitably going to break other people's code. Put the > transaction wrapper in your own stuff if you have to have it. The module is working fine on HEAD without any patch if it cares about starting a transaction itself into _PG_init(), even when _PG_init() is called at function call time rather than at local_preload_libraries time. My reserve was that I thought the transaction arround _PG_init() was existing in a 'normal' call, so the explicit creation of it in the module would fail: StartTransactionCommand(); ... CommitTransactionCommand(); Now my only problem is related to making the module 8.3 compliant: pre_prepare.c:19:27: error: utils/snapmgr.h: No such file or directory pre_prepare.c: In function ‘_PG_init’: pre_prepare.c:188: warning: implicit declaration of function ‘PushActiveSnapshot’ pre_prepare.c:207: warning: implicit declaration of function ‘PopActiveSnapshot’ I guess I can document that having pre_prepare in local_preload_libraries with preprepare.at_init = on is only support from 8.4 onward... Regards, -- dim