Re: 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.com
PostgreSQL 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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)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.com
PostgreSQL Training, Services and Support

Dimitri Fontaine <dfontaine(at)hi-media(dot)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

Dimitri Fontaine <dfontaine(at)hi-media(dot)com> writes:
> Dimitri Fontaine <dfontaine(at)hi-media(dot)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.com
PostgreSQL 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

Dimitri Fontaine <dfontaine(at)hi-media(dot)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(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Dimitri Fontaine <dfontaine(at)hi-media(dot)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