proposal, patch: allow multiple plpgsql plugins

Hello

We talked about enhancing a plpgsql plugin API to support more active
plugins.

I wrote a prototype based on function plpgsql_register_plugin instead
rendezvous variable.

There are two basic questions:

a) will we support rendezvous variable still?

b) will we support same API still - a reference on plugin_info in exec
state is a issue - described in patch.

without a) a d b) we will break a current plugins little bit more than is
usual - not terrible hard to fix it. But without a) and b) a
implementation can be significantly cleaner.

Comments, notes?

Regards

Pavel

On 1/9/14, 11:41 PM, Pavel Stehule wrote:
> There are two basic questions:
>
> b) will we support same API still - a reference on plugin_info in exec
> state is a issue - described in patch.

Pardon my ignorance, but why does the plugin_info have to be in the
executor state? If we're going to change the API, can't we pass it
directly to the callback function? Am I missing something completely
obvious? :-)

Regards,
Marko Tiikkaja

On 1/12/14, 5:33 PM, I wrote:
> On 1/9/14, 11:41 PM, Pavel Stehule wrote:
>> There are two basic questions:
>>
>> b) will we support same API still - a reference on plugin_info in exec
>> state is a issue - described in patch.
>
> Pardon my ignorance, but why does the plugin_info have to be in the
> executor state? If we're going to change the API, can't we pass it
> directly to the callback function?

Oh, I think I'm being stupid -- we'd only have to do what *if* we don't
want to change the API? Then my vote is for breaking the API.

Regards,
Marko Tiikkaja

2014/1/12 Marko Tiikkaja <marko(at)joh(dot)to>

> On 1/12/14, 5:33 PM, I wrote:
>
>> On 1/9/14, 11:41 PM, Pavel Stehule wrote:
>>
>>> There are two basic questions:
>>>
>>> b) will we support same API still - a reference on plugin_info in exec
>>> state is a issue - described in patch.
>>>
>>
>> Pardon my ignorance, but why does the plugin_info have to be in the
>> executor state? If we're going to change the API, can't we pass it
>> directly to the callback function?
>>
>
> Oh, I think I'm being stupid -- we'd only have to do what *if* we don't
> want to change the API? Then my vote is for breaking the API.
>

yes. It is my vote too.

It is trouble - but support same API is really ugly - on second hand -
there are only few plpgsql plugins - and every plugin needs recompilation
for new mayor version and fixing will be easy.

Regards

Pavel Stehule

>
>
> Regards,
> Marko Tiikkaja
>

Hello

Updated version

I still not happy with plugin_info - it is only per plugin now and should
be per plugin and per function.

Regards

Pavel

2014/1/12 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>

>
>
>
> 2014/1/12 Marko Tiikkaja <marko(at)joh(dot)to>
>
>> On 1/12/14, 5:33 PM, I wrote:
>>
>>> On 1/9/14, 11:41 PM, Pavel Stehule wrote:
>>>
>>>> There are two basic questions:
>>>>
>>>> b) will we support same API still - a reference on plugin_info in exec
>>>> state is a issue - described in patch.
>>>>
>>>
>>> Pardon my ignorance, but why does the plugin_info have to be in the
>>> executor state? If we're going to change the API, can't we pass it
>>> directly to the callback function?
>>>
>>
>> Oh, I think I'm being stupid -- we'd only have to do what *if* we don't
>> want to change the API? Then my vote is for breaking the API.
>>
>
> yes. It is my vote too.
>
> It is trouble - but support same API is really ugly - on second hand -
> there are only few plpgsql plugins - and every plugin needs recompilation
> for new mayor version and fixing will be easy.
>
> Regards
>
> Pavel Stehule
>
>
>>
>>
>> Regards,
>> Marko Tiikkaja
>>
>
>

Hi Pavel,

First of all, thanks for working on this!

On 1/12/14, 8:58 PM, Pavel Stehule wrote:
> I still not happy with plugin_info - it is only per plugin now and should
> be per plugin and per function.

I'm not sure I understand the point of plugin_info in the first place,
but what would having a separate info per (plugin, function) achieve
that can't be done otherwise?

As for the current patch, I'd like to see improvements on a few things:

1) It doesn't currently compile because of extra semicolons in the
PLpgSQL_plugin struct.

2) The previous comment above the same struct still talk about the
rendezvous variable which is now gone. The comment should be
updated to reflect the new API.

3) The same comment talks about how important it is to unregister a
plugin if its _PG_fini() is ever called, but the current API does
not support unregistering. That should probably be added? I'm not
sure when _PG_fini() would be called.

4) The comment /* reserved for use by optional plugin */ seems a bit
weird in its new context.

Regards,
Marko Tiikkaja

2014/1/16 Marko Tiikkaja <marko(at)joh(dot)to>

> Hi Pavel,
>
> First of all, thanks for working on this!
>
>
> On 1/12/14, 8:58 PM, Pavel Stehule wrote:
>
>> I still not happy with plugin_info - it is only per plugin now and should
>> be per plugin and per function.
>>
>
> I'm not sure I understand the point of plugin_info in the first place, but
> what would having a separate info per (plugin, function) achieve that can't
> be done otherwise?
>

First use case - I would to protect repeated call of plpgsql_check_function
in passive mode. Where I have to store information about successful first
start? It is related to the function instance, so function oid can be
ambiguous (for function with polymorphic parameters). When function
instance is destroyed, then this information should be destroyed. It is
impossible do this check from plugin. Second use case - attach session life
cycle plugin data with some function - for example for coverage
calculation. Inside plugin without function specific data you have to hold
a hash of all used function, and you have to search again and again. When
plpgsql hold this info in internal plpgsql function structures, then you
don't need search anything.

Regards

Pavel

>
>
> As for the current patch, I'd like to see improvements on a few things:
>
> 1) It doesn't currently compile because of extra semicolons in the
> PLpgSQL_plugin struct.
>
> 2) The previous comment above the same struct still talk about the
> rendezvous variable which is now gone. The comment should be
> updated to reflect the new API.
>
> 3) The same comment talks about how important it is to unregister a
> plugin if its _PG_fini() is ever called, but the current API does
> not support unregistering. That should probably be added? I'm not
> sure when _PG_fini() would be called.
>
> 4) The comment /* reserved for use by optional plugin */ seems a bit
> weird in its new context.
>
>
> Regards,
> Marko Tiikkaja
>

Hello

updated patch - now plugin_info is per plpgsq_estate/plugin again.

Regards

Pavel

2014-01-17 20:26 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
>
> 2014/1/16 Marko Tiikkaja <marko(at)joh(dot)to>
>
>> Hi Pavel,
>>
>> First of all, thanks for working on this!
>>
>>
>> On 1/12/14, 8:58 PM, Pavel Stehule wrote:
>>
>>> I still not happy with plugin_info - it is only per plugin now and should
>>> be per plugin and per function.
>>>
>>
>> I'm not sure I understand the point of plugin_info in the first place,
>> but what would having a separate info per (plugin, function) achieve that
>> can't be done otherwise?
>>
>
> First use case - I would to protect repeated call of
> plpgsql_check_function in passive mode. Where I have to store information
> about successful first start? It is related to the function instance, so
> function oid can be ambiguous (for function with polymorphic parameters).
> When function instance is destroyed, then this information should be
> destroyed. It is impossible do this check from plugin. Second use case -
> attach session life cycle plugin data with some function - for example for
> coverage calculation. Inside plugin without function specific data you have
> to hold a hash of all used function, and you have to search again and
> again. When plpgsql hold this info in internal plpgsql function structures,
> then you don't need search anything.
>
>
>
>
> Regards
>
> Pavel
>
>
>
>>
>>
>> As for the current patch, I'd like to see improvements on a few things:
>>
>> 1) It doesn't currently compile because of extra semicolons in the
>> PLpgSQL_plugin struct.
>>
>> 2) The previous comment above the same struct still talk about the
>> rendezvous variable which is now gone. The comment should be
>> updated to reflect the new API.
>>
>> 3) The same comment talks about how important it is to unregister a
>> plugin if its _PG_fini() is ever called, but the current API does
>> not support unregistering. That should probably be added? I'm not
>> sure when _PG_fini() would be called.
>>
>> 4) The comment /* reserved for use by optional plugin */ seems a bit
>> weird in its new context.
>>
>>
>> Regards,
>> Marko Tiikkaja
>>
>
>

Hello Marko

2014-01-16 23:54 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> Hi Pavel,
>
> First of all, thanks for working on this!
>
>
> On 1/12/14, 8:58 PM, Pavel Stehule wrote:
>
>> I still not happy with plugin_info - it is only per plugin now and should
>> be per plugin and per function.
>>
>
> I'm not sure I understand the point of plugin_info in the first place, but
> what would having a separate info per (plugin, function) achieve that can't
> be done otherwise?
>
>
> As for the current patch, I'd like to see improvements on a few things:
>
> 1) It doesn't currently compile because of extra semicolons in the
> PLpgSQL_plugin struct.
>

fixed

>
> 2) The previous comment above the same struct still talk about the
> rendezvous variable which is now gone. The comment should be
> updated to reflect the new API.
>

removed

>
> 3) The same comment talks about how important it is to unregister a
> plugin if its _PG_fini() is ever called, but the current API does
> not support unregistering. That should probably be added? I'm not
> sure when _PG_fini() would be called.
>

removed

These plugins should not be removed - there is no any mechanism how to
remove active plugin without close session

Regards

Pavel

>
> 4) The comment /* reserved for use by optional plugin */ seems a bit
> weird in its new context.
>
>
> Regards,
> Marko Tiikkaja
>

Hi Pavel,

The extra semicolons are still in there; around line 525 in this patch.
However, I removed them to compile the patch, but I can't compile my
plugin on OS X. The plugin is simple, it just does:

void
_PG_init(void)
{
DirectFunctionCall1(plpgsql_register_plugin,
&pgt_plpgsql_plugin_struct);
}

I get:

Undefined symbols for architecture x86_64:
"_plpgsql_register_plugin", referenced from:
__PG_init in plpgtest.o

I'm guessing this is because PL/PgSQL is a shared library and not in
core? Is there a way around this?

Regards,
Marko Tiikkaja

Hi

2014-03-02 19:59 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> Hi Pavel,
>
> The extra semicolons are still in there; around line 525 in this patch.
> However, I removed them to compile the patch, but I can't compile my
> plugin on OS X. The plugin is simple, it just does:
>
> void
> _PG_init(void)
> {
> DirectFunctionCall1(plpgsql_register_plugin,
> &pgt_plpgsql_plugin_struct);
> }
>
> I get:
>
> Undefined symbols for architecture x86_64:
> "_plpgsql_register_plugin", referenced from:
> __PG_init in plpgtest.o
>
> I'm guessing this is because PL/PgSQL is a shared library and not in core?
> Is there a way around this?
>

yes, PLpgSQL is not referenced and, if I remember well, clang is too
restrictive.

probably
http://stackoverflow.com/questions/17281901/ignoring-an-undefined-symbol-in-a-dynamic-library-from-xcode

or you can add a reference on plpgsql to your Makefile

Regards

Pavel

>
>
> Regards,
> Marko Tiikkaja
>

On 3/2/14, 8:47 PM, Pavel Stehule wrote:
> 2014-03-02 19:59 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:
>> Undefined symbols for architecture x86_64:
>> "_plpgsql_register_plugin", referenced from:
>> __PG_init in plpgtest.o
>>
>> I'm guessing this is because PL/PgSQL is a shared library and not in core?
>> Is there a way around this?
>>
>
> yes, PLpgSQL is not referenced and, if I remember well, clang is too
> restrictive.
>
> probably
> http://stackoverflow.com/questions/17281901/ignoring-an-undefined-symbol-in-a-dynamic-library-from-xcode
>
> or you can add a reference on plpgsql to your Makefile

That seems unbelievably ugly, but worse, loading the library in
shared_preload_libraries doesn't work:

14782 FATAL: could not load library
"/usr/local/pgsql/lib/plpgtest.so":
dlopen(/usr/local/pgsql/lib/plpgtest.so, 10): Symbol not found:
_plpgsql_register_plugin
Referenced from: /usr/local/pgsql/lib/plpgtest.so
Expected in: flat namespace
in /usr/local/pgsql/lib/plpgtest.so

I even tried putting plpgsql.so before it in the list, but no go.

Regards,
Marko Tiikkaja

Dne 2. 3. 2014 21:55 "Marko Tiikkaja" <marko(at)joh(dot)to> napsal(a):
>
> On 3/2/14, 8:47 PM, Pavel Stehule wrote:
>>
>> 2014-03-02 19:59 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:
>>>
>>> Undefined symbols for architecture x86_64:
>>>
>>> "_plpgsql_register_plugin", referenced from:
>>> __PG_init in plpgtest.o
>>>
>>> I'm guessing this is because PL/PgSQL is a shared library and not in
core?
>>> Is there a way around this?
>>>
>>
>> yes, PLpgSQL is not referenced and, if I remember well, clang is too
>> restrictive.
>>
>> probably
>>
http://stackoverflow.com/questions/17281901/ignoring-an-undefined-symbol-in-a-dynamic-library-from-xcode
>>
>> or you can add a reference on plpgsql to your Makefile
>
>
> That seems unbelievably ugly, but worse, loading the library in
shared_preload_libraries doesn't work:
>
> 14782 FATAL: could not load library "/usr/local/pgsql/lib/plpgtest.so":
dlopen(/usr/local/pgsql/lib/plpgtest.so, 10): Symbol not found:
_plpgsql_register_plugin
> Referenced from: /usr/local/pgsql/lib/plpgtest.so
> Expected in: flat namespace
> in /usr/local/pgsql/lib/plpgtest.so
>
> I even tried putting plpgsql.so before it in the list, but no go.

>
>
> Regards,
> Marko Tiikkaja
In this moment, pls, try to use Load plpgsql

Regards

pavel

2014-03-03 6:09 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
> Dne 2. 3. 2014 21:55 "Marko Tiikkaja" <marko(at)joh(dot)to> napsal(a):
>
> >
> > On 3/2/14, 8:47 PM, Pavel Stehule wrote:
> >>
> >> 2014-03-02 19:59 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:
> >>>
> >>> Undefined symbols for architecture x86_64:
> >>>
> >>> "_plpgsql_register_plugin", referenced from:
> >>> __PG_init in plpgtest.o
> >>>
> >>> I'm guessing this is because PL/PgSQL is a shared library and not in
> core?
> >>> Is there a way around this?
> >>>
> >>
> >> yes, PLpgSQL is not referenced and, if I remember well, clang is too
> >> restrictive.
> >>
> >> probably
> >>
> http://stackoverflow.com/questions/17281901/ignoring-an-undefined-symbol-in-a-dynamic-library-from-xcode
> >>
> >> or you can add a reference on plpgsql to your Makefile
> >
> >
> > That seems unbelievably ugly, but worse, loading the library in
> shared_preload_libraries doesn't work:
> >
> > 14782 FATAL: could not load library
> "/usr/local/pgsql/lib/plpgtest.so":
> dlopen(/usr/local/pgsql/lib/plpgtest.so, 10): Symbol not found:
> _plpgsql_register_plugin
> > Referenced from: /usr/local/pgsql/lib/plpgtest.so
> > Expected in: flat namespace
> > in /usr/local/pgsql/lib/plpgtest.so
> >
> > I even tried putting plpgsql.so before it in the list, but no go.
>
> >
> >
> > Regards,
> > Marko Tiikkaja
> In this moment, pls, try to use Load plpgsql
>
I though about it this morning - we should to move plugin registration to
core - it should to work like ddl loader

a) it can solve problems with loading
b) it can be usable for all PL environment.

Pavel

> Regards
>
> pavel
>