Обсуждение: MV patch broke users of ExplainOneQuery_hook
The materialized views patch adjusted ExplainOneQuery to take an additional DestReceiver argument, but failed to add a matching argument to the definition of ExplainOneQuery_hook. This makes the hook unusable. The idea of this hook is that your hook function will do something before and/or after calling pg_plan_query and ExplainOnePlan. But this no longer works, because ExplainOnePlan needs the DestReceiver, which hasn't been exposed to the hook. :-( -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote: > The materialized views patch adjusted ExplainOneQuery to take an > additional DestReceiver argument, but failed to add a matching > argument to the definition of ExplainOneQuery_hook. This makes the > hook unusable. The idea of this hook is that your hook function will > do something before and/or after calling pg_plan_query and > ExplainOnePlan. But this no longer works, because ExplainOnePlan > needs the DestReceiver, which hasn't been exposed to the hook. :-( Feel free to go ahead and fix this if you like; otherwise I'll look at it later today. -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> The materialized views patch adjusted ExplainOneQuery to take an
> additional DestReceiver argument, but failed to add a matching
> argument to the definition of ExplainOneQuery_hook. This makes the
> hook unusable. The idea of this hook is that your hook function will
> do something before and/or after calling pg_plan_query and
> ExplainOnePlan. But this no longer works, because ExplainOnePlan
> needs the DestReceiver, which hasn't been exposed to the hook. :-(
TBH I'd like to revert all of that anyway; it seemed to me to be
basically gratuitous breakage of an API used by plugins. I've not
had time to look at whether there was an actual reason for it and
if so how we might solve that differently.
regards, tom lane
On Tue, Apr 9, 2013 at 10:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> The materialized views patch adjusted ExplainOneQuery to take an >> additional DestReceiver argument, but failed to add a matching >> argument to the definition of ExplainOneQuery_hook. This makes the >> hook unusable. The idea of this hook is that your hook function will >> do something before and/or after calling pg_plan_query and >> ExplainOnePlan. But this no longer works, because ExplainOnePlan >> needs the DestReceiver, which hasn't been exposed to the hook. :-( > > TBH I'd like to revert all of that anyway; it seemed to me to be > basically gratuitous breakage of an API used by plugins. I've not > had time to look at whether there was an actual reason for it and > if so how we might solve that differently. Oops. I just pushed a fix for the situation as it stands, but it's only 2 lines, so it shouldn't be too much trouble to revert it if we decide on a different approach. I do agree it would be nice not to need to change the API there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 9, 2013 at 10:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> TBH I'd like to revert all of that anyway; it seemed to me to be
>> basically gratuitous breakage of an API used by plugins. I've not
>> had time to look at whether there was an actual reason for it and
>> if so how we might solve that differently.
> Oops. I just pushed a fix for the situation as it stands, but it's
> only 2 lines, so it shouldn't be too much trouble to revert it if we
> decide on a different approach. I do agree it would be nice not to
> need to change the API there.
If we don't revert then what you pushed is clearly necessary, so
no objection to having done that. I'll look at the larger situation
as soon as I get a chance.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > If we don't revert then what you pushed is clearly necessary, so > no objection to having done that. I'll look at the larger > situation as soon as I get a chance. Any objections to my pushing the patch I posted Friday to draw a distinction between populated and scannable, which also attempted to address a couple points raised by you, or would you rather the code didn't change at the moment? -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Kevin Grittner <kgrittn@ymail.com> writes:
> Any objections to my pushing the patch I posted Friday to draw a
> distinction between populated and scannable, which also attempted
> to address a couple points raised by you, or would you rather the
> code didn't change at the moment?
I didn't look at it yet --- had my head in trigram regex stuff all
weekend. Gimme a couple hours ...
regards, tom lane
I wrote:
> Kevin Grittner <kgrittn@ymail.com> writes:
>> Any objections to my pushing the patch I posted Friday to draw a
>> distinction between populated and scannable, which also attempted
>> to address a couple points raised by you, or would you rather the
>> code didn't change at the moment?
> I didn't look at it yet --- had my head in trigram regex stuff all
> weekend. Gimme a couple hours ...
[ looks... ] TBH, most of that code is code that I'd like to see
removed, not just renamed; I do not think its problems are primarily
cosmetic. However I have no objection to pushing what you have for
the moment. Perhaps clarifying the intent will help us think about
where to go from here.
One point that does come to mind, though, as long as you're trying
to draw a distinction between "populated" and "scannable", is why
pg_dump should be paying attention to one or the other; or indeed
why it should care about the matviews' current contents at all.
It seems to me that it would be more useful to not pay any attention
to that, but instead have a command-line switch to control whether
the dump includes commands to repopulate matviews or not. Driving
this off the current state seems rather akin to expecting pg_dump
to reproduce the contents of indexes exactly, rather than build
them from scratch.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > I wrote: >> Kevin Grittner <kgrittn@ymail.com> writes: >>> Any objections to my pushing the patch I posted Friday to draw a >>> distinction between populated and scannable, which also attempted >>> to address a couple points raised by you, or would you rather the >>> code didn't change at the moment? > >> I didn't look at it yet > [ looks... ] TBH, most of that code is code that I'd like to see > removed, not just renamed; I do not think its problems are primarily > cosmetic. However I have no objection to pushing what you have for > the moment. Perhaps clarifying the intent will help us think about > where to go from here. My hope is that it will do that at a minimum. Thanks for looking at this, BTW. > One point that does come to mind, though, as long as you're trying > to draw a distinction between "populated" and "scannable", is why > pg_dump should be paying attention to one or the other; or indeed > why it should care about the matviews' current contents at all. > It seems to me that it would be more useful to not pay any attention > to that, but instead have a command-line switch to control whether > the dump includes commands to repopulate matviews or not. Driving > this off the current state seems rather akin to expecting pg_dump > to reproduce the contents of indexes exactly, rather than build > them from scratch. I guess my thinking was that without that you might dump and restore and have a database which is not usable without further work (if the matview data is needed for correct operation) or you might populate a matview which was not yet intended to *be* populated. I'm not tied to that, but it seemed reasonable and likely to be useful. It's going to be hard to sell me that by default a materialized view which has not been populated should quietly behaves as though empty or should execute the underlying query as though it were a "normal" view -- those seem like correctness issues to me; but behavior for pg_dump seems less definite. -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company