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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers