Re: Access to dynamic SQL in PL/pgSQL

It's not currently possible to access the SQL used in a dynamic PL/pgSQL
statement using a PL/pgSQL plugin.

In src/pl/plpgsql/src/pl_exec.c's exec_stmt() we call each dynamic
statement type, evaluate the SQL and free memory again before the plugin
gains control again.

It seems simple to attach querystr to PLpgSQL_execstate and free it
after the plugin has seen it. The difference in lifetime of the memory
allocation is very small.

Would a patch to make this simple change be acceptable? It would need to
be backpatched to 8.1 also?

--
Simon Riggs www.2ndQuadrant.com

2010/1/22 Simon Riggs <simon(at)2ndquadrant(dot)com>:
>
> It's not currently possible to access the SQL used in a dynamic PL/pgSQL
> statement using a PL/pgSQL plugin.
>
> In src/pl/plpgsql/src/pl_exec.c's exec_stmt() we call each dynamic
> statement type, evaluate the SQL and free memory again before the plugin
> gains control again.

can you show some example, please?

Pavel

>
> It seems simple to attach querystr to PLpgSQL_execstate and free it
> after the plugin has seen it. The difference in lifetime of the memory
> allocation is very small.
>
> Would a patch to make this simple change be acceptable? It would need to
> be backpatched to 8.1 also?
>
> --
>  Simon Riggs           www.2ndQuadrant.com
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Fri, 2010-01-22 at 11:41 +0100, Pavel Stehule wrote:
> 2010/1/22 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> >
> > It's not currently possible to access the SQL used in a dynamic PL/pgSQL
> > statement using a PL/pgSQL plugin.
> >
> > In src/pl/plpgsql/src/pl_exec.c's exec_stmt() we call each dynamic
> > statement type, evaluate the SQL and free memory again before the plugin
> > gains control again.
>
> can you show some example, please?

exec_stmt_dynexecute() evaluates querystr at start and pfrees it before
end of exec_stmt_dynexecute(). So plugin never gets to see the SQL
executed.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:
> It's not currently possible to access the SQL used in a dynamic PL/pgSQL
> statement using a PL/pgSQL plugin.
>
> In src/pl/plpgsql/src/pl_exec.c's exec_stmt() we call each dynamic
> statement type, evaluate the SQL and free memory again before the plugin
> gains control again.
>
> It seems simple to attach querystr to PLpgSQL_execstate and free it
> after the plugin has seen it. The difference in lifetime of the memory
> allocation is very small.

Is this for pl/debugger? How would you use the query string there?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, 2010-01-22 at 13:39 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > It's not currently possible to access the SQL used in a dynamic PL/pgSQL
> > statement using a PL/pgSQL plugin.
> >
> > In src/pl/plpgsql/src/pl_exec.c's exec_stmt() we call each dynamic
> > statement type, evaluate the SQL and free memory again before the plugin
> > gains control again.
> >
> > It seems simple to attach querystr to PLpgSQL_execstate and free it
> > after the plugin has seen it. The difference in lifetime of the memory
> > allocation is very small.
>
> Is this for pl/debugger? How would you use the query string there?

Yes, PL/debugger would have access to the text of the dynamic SQL
string.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> It's not currently possible to access the SQL used in a dynamic PL/pgSQL
> statement using a PL/pgSQL plugin.

> In src/pl/plpgsql/src/pl_exec.c's exec_stmt() we call each dynamic
> statement type, evaluate the SQL and free memory again before the plugin
> gains control again.

> It seems simple to attach querystr to PLpgSQL_execstate and free it
> after the plugin has seen it. The difference in lifetime of the memory
> allocation is very small.

That seems like a complete crock --- you're talking about leaving a
dangling pointer to transient data in a permanent data structure.
In most contexts it would be difficult to be sure if the pointer was
valid or not.

If we need this it would be better to provide another plugin hook call
during the execution of a statement that uses a dynamic query.

> Would a patch to make this simple change be acceptable? It would need to
> be backpatched to 8.1 also?

As for the first, I vote "not like that", and as for backpatching,
you're out of your mind. This would be an incompatible ABI change,
and we don't lightly make those in stable branches. Even if we were
willing to take the risk, how would a plugin know if it were dealing
with a version of plpgsql that had this field?

regards, tom lane

On Fri, 2010-01-22 at 10:56 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > It's not currently possible to access the SQL used in a dynamic PL/pgSQL
> > statement using a PL/pgSQL plugin.
>
> > In src/pl/plpgsql/src/pl_exec.c's exec_stmt() we call each dynamic
> > statement type, evaluate the SQL and free memory again before the plugin
> > gains control again.
>
> > It seems simple to attach querystr to PLpgSQL_execstate and free it
> > after the plugin has seen it. The difference in lifetime of the memory
> > allocation is very small.
>
> That seems like a complete crock --- you're talking about leaving a
> dangling pointer to transient data in a permanent data structure.

No, I wouldn't call it that. You read the bit where I said "free"?

> In most contexts it would be difficult to be sure if the pointer was
> valid or not.
>
> If we need this it would be better to provide another plugin hook call
> during the execution of a statement that uses a dynamic query.
>
> > Would a patch to make this simple change be acceptable? It would need to
> > be backpatched to 8.1 also?
>
> As for the first, I vote "not like that", and as for backpatching,
> you're out of your mind.

I think by most people's standards asking for acceptance here really is
insane, I agree.

> This would be an incompatible ABI change,
> and we don't lightly make those in stable branches. Even if we were
> willing to take the risk, how would a plugin know if it were dealing
> with a version of plpgsql that had this field?

ISTM there are ways though I think what you mean is that they would be
unpalatable.

Anyway, there already was another way of doing this, so it shall be done
that way instead. The beauty of a pluggable architecture is that one
does not need approval to implement customer solutions.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> Anyway, there already was another way of doing this, so it shall be done
> that way instead. The beauty of a pluggable architecture is that one
> does not need approval to implement customer solutions.

If you're talking about a bundle that you're shipping to customers,
of course you can do whatever you want, since you can ensure that
your plpgsql.so and your plugin are compatible. Stuff we release
into the wide world doesn't have that luxury. We can't assume much
more than what PG_MODULE_MAGIC will enforce for us, and that means
ABI breaks within a major release series are dangerous.

But to get back to the point, what have you got against adding
another plugin call within the statements that build dynamic
queries? It seems like a much cleaner solution to me, and not
any different from the standpoint of back-portability.

regards, tom lane

On Fri, 2010-01-22 at 11:33 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > Anyway, there already was another way of doing this, so it shall be done
> > that way instead. The beauty of a pluggable architecture is that one
> > does not need approval to implement customer solutions.
>
> If you're talking about a bundle that you're shipping to customers,
> of course you can do whatever you want, since you can ensure that
> your plpgsql.so and your plugin are compatible.

> Stuff we release
> into the wide world doesn't have that luxury. We can't assume much
> more than what PG_MODULE_MAGIC will enforce for us, and that means
> ABI breaks within a major release series are dangerous.

Understood

> But to get back to the point, what have you got against adding
> another plugin call within the statements that build dynamic
> queries? It seems like a much cleaner solution to me, and not
> any different from the standpoint of back-portability.

So a "dynamic query hook". When called, the hook would give access to
the query text actually executed. Existing plugins wouldn't know about
the hook, so would never call it, so the language run time would bypass
that hook altogether. Newer versions of plugins would be able to decide
whether to add-in code for the new hook based upon the version number.
Neat solution, nothing against it at all.

Would such a solution be backpatchable? I wasn't sure what your last
sentence meant.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> So a "dynamic query hook". When called, the hook would give access to
> the query text actually executed. Existing plugins wouldn't know about
> the hook, so would never call it, so the language run time would bypass
> that hook altogether. Newer versions of plugins would be able to decide
> whether to add-in code for the new hook based upon the version number.
> Neat solution, nothing against it at all.

> Would such a solution be backpatchable? I wasn't sure what your last
> sentence meant.

It doesn't seem backpatchable to me, because there is no mechanism
beyond PG_MODULE_MAGIC that would ensure that plpgsql.so and the plugin
have the same idea about the contents of struct PLpgSQL_plugin. In
hindsight it might've been a good idea if that struct contained a
version number, so if we're gonna change it I'd vote for adding one.

What my previous comment was meant to say was that this approach doesn't
seem practically different from the other as far as backwards
compatibility goes. To wit: I don't think we can back-patch it in the
community sources, but there is nothing stopping you from back-patching
in a custom release where you have some confidence that compatible
versions of plpgsql.so and plugin will be used together.

regards, tom lane