Обсуждение: add queryEnv to ExplainOneQuery_hook

Hi,

I found a variable (queryEnv) which should be added in
ExplainOneQuery_hook because if it is missing, hook function
can't call ExplainOnePlan.
Sorry if this wasn't correct.

Please find attached a patch.

Tatsuro Yamada
NTT Open Source Software Center

On Fri, Jan 12, 2018 at 12:16 AM, Tatsuro Yamada
<yamada.tatsuro@lab.ntt.co.jp> wrote:
> I found a variable (queryEnv) which should be added in
> ExplainOneQuery_hook because if it is missing, hook function
> can't call ExplainOnePlan.
> Sorry if this wasn't correct.
>
> Please find attached a patch.

Yeah, I think you're right.  That's an oversight in 18ce3a4a.  I'm
surprised we haven't heard any complaints sooner if there are advisors
using that hook[1] and expecting to be able to forward to
ExplainOnePlan(), but I suppose it would nearly always works to call
ExplainOnePlan() with NULL as queryEnv.  It'd currently only be
non-NULL for trigger functions running SQL to access transition
tables, which is a bit obscure: you'd need to run EXPLAIN inside a
suitable trigger function (though in future there might be more ways
to create named tuplestore relations).

[1] https://www.postgresql.org/message-id/20161113222804.o2uw7c2fish67iej%40alap3.anarazel.de

-- 
Thomas Munro
http://www.enterprisedb.com

On Fri, Jan 12, 2018 at 12:43:22AM +1300, Thomas Munro wrote:
> Yeah, I think you're right.  That's an oversight in 18ce3a4a.

+1.

> I'm surprised we haven't heard any complaints sooner if there are
> advisors using that hook[1] and expecting to be able to forward to
> ExplainOnePlan(), but I suppose it would nearly always works to call
> ExplainOnePlan() with NULL as queryEnv.  It'd currently only be
> non-NULL for trigger functions running SQL to access transition
> tables, which is a bit obscure: you'd need to run EXPLAIN inside a
> suitable trigger function (though in future there might be more ways
> to create named tuplestore relations).

It seems to me that QueryEnv should be pushed to the hook, but only on
HEAD. You surely don't want to break people's extensions after a minor
upgrade.
--
Michael

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Fri, Jan 12, 2018 at 12:16 AM, Tatsuro Yamada
> <yamada.tatsuro@lab.ntt.co.jp> wrote:
>> I found a variable (queryEnv) which should be added in
>> ExplainOneQuery_hook because if it is missing, hook function
>> can't call ExplainOnePlan.

> Yeah, I think you're right.  That's an oversight in 18ce3a4a.

Clearly :-(.  Too bad we didn't find this sooner --- I suppose changing it
in v10 now is a nonstarter.  Will push to HEAD though.

            regards, tom lane

On 2018/01/11 21:46, Michael Paquier wrote:
>> I'm surprised we haven't heard any complaints sooner if there are
>> advisors using that hook[1] and expecting to be able to forward to
>> ExplainOnePlan(), but I suppose it would nearly always works to call
>> ExplainOnePlan() with NULL as queryEnv.  It'd currently only be
>> non-NULL for trigger functions running SQL to access transition
>> tables, which is a bit obscure: you'd need to run EXPLAIN inside a
>> suitable trigger function (though in future there might be more ways
>> to create named tuplestore relations).
> 
> It seems to me that QueryEnv should be pushed to the hook, but only on
> HEAD. You surely don't want to break people's extensions after a minor
> upgrade.

Thanks guys! :)

I also surprised that there is no complaint from extension creators.
I suppose that if possible, it would be better to create a unit test
for the hook function to avoid the same bug because there is no contrib
module using ExplainOneQuery_hook in contrib directory.
(It might unnecessary thing, maybe.)

Regards,
Tatsuro Yamada

On 2018/01/12 2:02, Tom Lane wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> On Fri, Jan 12, 2018 at 12:16 AM, Tatsuro Yamada
>> <yamada.tatsuro@lab.ntt.co.jp> wrote:
>>> I found a variable (queryEnv) which should be added in
>>> ExplainOneQuery_hook because if it is missing, hook function
>>> can't call ExplainOnePlan.
> 
>> Yeah, I think you're right.  That's an oversight in 18ce3a4a.
> 
> Clearly :-(.  Too bad we didn't find this sooner --- I suppose changing it
> in v10 now is a nonstarter.  Will push to HEAD though.
> 
>             regards, tom lane

Thanks Tom!
I read that commit log on HEAD.

Regards,
Tatsuro Yamada

On Fri, Jan 12, 2018 at 10:43:40AM +0900, Tatsuro Yamada wrote:
> Thanks guys! :)
>
> I also surprised that there is no complaint from extension creators.
> I suppose that if possible, it would be better to create a unit test
> for the hook function to avoid the same bug because there is no contrib
> module using ExplainOneQuery_hook in contrib directory.
> (It might unnecessary thing, maybe.)

Patches welcome if you can come up with a simple concept that proves to
be useful as a template example and has some useful application cases so
as this stuff is stressed a bit by regression tests.
--
Michael

Passing NULL in place of queryEnv in PG10 causes a failure in installcheck
tests portals and plpgsql.

For PG10, if you want a both an ExplainOneQuery hook and clean run of
installcheck, is there a better workaround than to (a) pass NULL in place of
queryEnv, and (b) to comment out the portals and plpgsql tests?




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

On Wed, Mar 14, 2018 at 2:57 PM, Jim Finnerty <jfinnert@amazon.com> wrote:
> Passing NULL in place of queryEnv in PG10 causes a failure in installcheck
> tests portals and plpgsql.
>
> For PG10, if you want a both an ExplainOneQuery hook and clean run of
> installcheck, is there a better workaround than to (a) pass NULL in place of
> queryEnv, and (b) to comment out the portals and plpgsql tests?

Hi Jim,

I can't think of a good way right now.  It's unfortunate that we
couldn't back-patch 4d41b2e0 because 10 was out the door; but perhaps
you can?

Hmm.  I suppose we could have invented a new extended hook with a
different name and back-patched it so that PG10 would support both.
Then binary compatibility with existing compiled extensions wouldn't
be affected AFAICS, but you could use the new extended hook in (say)
10.4 or higher.  Then for PG11 (or later) we could remove the old hook
and just keep the new one.  I suppose that option is still technically
open to us, though I'm not sure of the committers' appetite for messing
with back branches like that.

-- 
Thomas Munro
http://www.enterprisedb.com

On Wed, Mar 14, 2018 at 05:36:26PM +1300, Thomas Munro wrote:
> Hmm.  I suppose we could have invented a new extended hook with a
> different name and back-patched it so that PG10 would support both.
> Then binary compatibility with existing compiled extensions wouldn't
> be affected AFAICS, but you could use the new extended hook in (say)
> 10.4 or higher.  Then for PG11 (or later) we could remove the old hook
> and just keep the new one.  I suppose that option is still technically
> open to us, though I'm not sure of the committers' appetite for messing
> with back branches like that.

The interactions between both hooks would not be difficult to define: if
the original hook is not defined, just do not trigger the second.  Still
that's too late for v10, so I would rather let it go.  New features are
not backpatched.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Mar 14, 2018 at 05:36:26PM +1300, Thomas Munro wrote:
>> Hmm.  I suppose we could have invented a new extended hook with a
>> different name and back-patched it so that PG10 would support both.
>> Then binary compatibility with existing compiled extensions wouldn't
>> be affected AFAICS, but you could use the new extended hook in (say)
>> 10.4 or higher.  Then for PG11 (or later) we could remove the old hook
>> and just keep the new one.  I suppose that option is still technically
>> open to us, though I'm not sure of the committers' appetite for messing
>> with back branches like that.

> The interactions between both hooks would not be difficult to define: if
> the original hook is not defined, just do not trigger the second.  Still
> that's too late for v10, so I would rather let it go.  New features are
> not backpatched.

Yeah.  There would be no good way for a v10 extension to know whether the
additional hook is available --- it would have to know that at compile
time, and it can't --- so I don't see that this is practical.

Ideally we'd have noticed the problem before v10 got out ... so my own
takeaway here is that this is a reminder to extension authors that they
ought to test their stuff during beta period.

            regards, tom lane