clearing opfuncid vs. parallel query
readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid. This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change. We'd want to look it up again rather than using the
previous value.
As previously discussed, parallel query will use outfuncs/readfuncs to
copy the Plan to be executed by a parallel worker to that worker.
That plan may contain expressions, and the round-trip through
outfuncs/readfuncs will lose their opfuncids. In this case, that's
pretty annoying, if not outright wrong. It's annoying because it
means that, after the worker reads the plan tree, it's got to iterate
over the whole thing and repeat the lookups of all the opfuncid
fields. This turns out not to be straightforward to code, because we
don't have a generic plan tree walker, and even if we did, you still
need knowledge of which plan nodes have expressions inside which
fields, and we don't have a generic facility for that either: it's
there inside e.g. set_plan_refs, but not in a form other code can use.
Moreover, if we ever did have an ALTER OPERATOR command that could
change the pg_proc mapping, this would go from annoying to outright
broken, because it would be real bad if the worker and the leader came
to different conclusions about what opfuncid to use. Maybe we could
add heavyweight locking to prevent that, but that would be expensive
and we surely don't have it today.
So I think we should abandon the approach Amit took, namely trying to
reset all of the opfuncids. Instead, I think we should provide a
method of not throwing them out in the first place. The attached
patch does by adding an "int flags" field to the relevant read
routines. stringToNode() becomes a macro which passes
STRINGTONODE_INVALIDATE_OPFUNCID to stringToNodeExtended(), which is
one of the functions that now takes an additional "int flags"
argument. If a caller doesn't wish to ignore opfuncid, they can call
stringToNodeExtended directly. This way, existing stringToNode()
callers see no behavior change, but the parallel query code can easily
get the behavior that it wants.
Thoughts? Better ideas? Objections?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
stringtonode-extended-v1.patchapplication/x-patch; name=stringtonode-extended-v1.patchDownload+283-291
Robert Haas <robertmhaas@gmail.com> writes:
readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid. This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change. We'd want to look it up again rather than using the
previous value.
Right, but considering that nobody has even thought about implementing
such a command in the past twenty years, maybe we should just change
the behavior of those read routines? I've wondered for some time why
we don't just insist on the parser filling those nodes fully to begin
with, and get rid of the notion that assorted random places should
be expected to fix them up later. This is one of the behaviors that
would have to change to support such a simplification.
... The attached
patch does by adding an "int flags" field to the relevant read
routines.
Ick. TBH, this is just taking a bad design and putting another one
on top.
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 Wed, Sep 23, 2015 at 4:31 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
readfuncs.c deliberately ignores any opfuncid read in for an OpExpr,
DistinctExpr, NullIfExpr, or ScalarArrayOpExpr, instead resetting the
value in the newly-created node to InvalidOid. This is because it
assumes we're reading the node from a pg_node_tree column in some
system catalog, and if in the future we wanted to allow an ALTER
OPERATOR command to change the pg_proc mapping, then the opfuncid
could change. We'd want to look it up again rather than using the
previous value.Right, but considering that nobody has even thought about implementing
such a command in the past twenty years, maybe we should just change
the behavior of those read routines?
Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.
But if we're sure we don't want to support that, changing the behavior
of the read routines would be fine with me, too. It would even save a
few cycles. Would you also want to rip out the stuff that fixes up
opfuncid as dead code? I assume yes, but sometimes I assume things
that are false.
--
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:
But if we're sure we don't want to support that, changing the behavior
of the read routines would be fine with me, too. It would even save a
few cycles. Would you also want to rip out the stuff that fixes up
opfuncid as dead code? I assume yes, but sometimes I assume things
that are false.
Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later. I'm not sure right now that everyplace that initially
creates OpExpr etc. nodes is on board with having to supply opfuncid.
I do know that the main path through the parser provides 'em. (So another
reason I don't like the current approach is that I doubt all code that
should theoretically be doing set_opfuncid() is actually doing so; it
would be too easy to miss the need for it in simple testing.)
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 Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
But if we're sure we don't want to support that, changing the behavior
of the read routines would be fine with me, too. It would even save a
few cycles. Would you also want to rip out the stuff that fixes up
opfuncid as dead code? I assume yes, but sometimes I assume things
that are false.Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later.
So, you're thinking of something as simple as the attached?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
dont-clear-opfuncid.patchapplication/x-patch; name=dont-clear-opfuncid.patchDownload+0-44
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later.
So, you're thinking of something as simple as the attached?
Right. I'll make a mental to-do to see about getting rid of set_opfuncid
later.
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 Wed, Sep 23, 2015 at 7:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Sep 23, 2015 at 5:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah, though I think of that as a longer-term issue, ie we could clean it
up sometime later.So, you're thinking of something as simple as the attached?
Right. I'll make a mental to-do to see about getting rid of set_opfuncid
later.
Cool, thanks.
--
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
On 2015-09-23 17:29:50 -0400, Robert Haas wrote:
Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.
Wouldn't we use plan invalidation to deal with that anyway?
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2015-09-23 17:29:50 -0400, Robert Haas wrote:
Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.
Wouldn't we use plan invalidation to deal with that anyway?
Plan invalidation wouldn't help, because the obsolete data exists
on-disk in stored rules. You'd have to run through the pg_rewrite
entries and update them.
To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
is on par with our very limited ability to alter the contents of
an operator class. In principle it would be nice, but the practical
value is so small that it's not surprising it hasn't been done ---
and we shouldn't continue to hold the door open for a simple way of
implementing it when there are significant costs to doing so.
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 Thu, Sep 24, 2015 at 11:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
On 2015-09-23 17:29:50 -0400, Robert Haas wrote:
Well, I can't vouch for what any human being on earth has thought
about over a twenty-year period. It's not intrinsically unreasonable
in my mind to want to alter an operator to point at a different
procedure.Wouldn't we use plan invalidation to deal with that anyway?
Plan invalidation wouldn't help, because the obsolete data exists
on-disk in stored rules. You'd have to run through the pg_rewrite
entries and update them.To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
is on par with our very limited ability to alter the contents of
an operator class. In principle it would be nice, but the practical
value is so small that it's not surprising it hasn't been done ---
and we shouldn't continue to hold the door open for a simple way of
implementing it when there are significant costs to doing so.
Also, it's not like this change couldn't be UN-done at a future point.
I mean, Tom didn't like the flag I added aesthetically, but if we
needed it, we could have it. Or we could engineer something else.
--
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
Tom Lane wrote:
To my mind though, the lack of an ALTER OPERATOR SET FUNCTION command
is on par with our very limited ability to alter the contents of
an operator class. In principle it would be nice, but the practical
value is so small that it's not surprising it hasn't been done ---
and we shouldn't continue to hold the door open for a simple way of
implementing it when there are significant costs to doing so.
I think allowing an operator's implementation function to change would
be rather problematic, would it not? There's no way to know whether the
semantic changes to stored rules would make sense, not least because the
person running ALTER OPERATOR wouldn't know (== has no easy way to find
out) what is being changed in the first place.
To me, it looks like we should just not allow ALTER OPERATOR SET FUNCTION
to be implemented at all.
It's not like changing an operator's implementation is an oft-requested
feature anyway.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I think allowing an operator's implementation function to change would
be rather problematic, would it not? There's no way to know whether the
semantic changes to stored rules would make sense, not least because the
person running ALTER OPERATOR wouldn't know (== has no easy way to find
out) what is being changed in the first place.
To me, it looks like we should just not allow ALTER OPERATOR SET FUNCTION
to be implemented at all.
It's not like changing an operator's implementation is an oft-requested
feature anyway.
Well, the point is that usually anything you want in this line can be
accomplished by executing CREATE OR REPLACE FUNCTION on the operator's
function. It's up to you that that doesn't create any interesting
semantic incompatibilities. That would still be true for an ALTER
OPERATOR SET FUNCTION command: if you break it, you get to keep both
pieces. But the availability of that alternative really cuts down
on the plausible use-cases for ALTER OPERATOR SET FUNCTION.
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
Robert Haas <robertmhaas@gmail.com> writes:
Also, it's not like this change couldn't be UN-done at a future point.
I mean, Tom didn't like the flag I added aesthetically, but if we
needed it, we could have it. Or we could engineer something else.
For the record: that's true for the patch you just committed. But once
I remove the hopefully-now-dead planner support for recomputing opfuncid,
it would get a lot more painful to reverse the decision.
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 Thu, Sep 24, 2015 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
Also, it's not like this change couldn't be UN-done at a future point.
I mean, Tom didn't like the flag I added aesthetically, but if we
needed it, we could have it. Or we could engineer something else.For the record: that's true for the patch you just committed. But once
I remove the hopefully-now-dead planner support for recomputing opfuncid,
it would get a lot more painful to reverse the decision.
True. I think it's pretty wacky that we store the opfuncid in the
tree at all. If somebody were to propose adding a dependent value of
that sort to a node type that didn't already have it, I suspect either
you or I would do our best to shoot that down. The only possible
argument for having that in there at all is that the performance gains
from so doing are so large that we have no choice but to sacrifice a
principle to expediency.
--
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 Thu, Sep 24, 2015 at 12:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
For the record: that's true for the patch you just committed. But once
I remove the hopefully-now-dead planner support for recomputing opfuncid,
it would get a lot more painful to reverse the decision.
True. I think it's pretty wacky that we store the opfuncid in the
tree at all. If somebody were to propose adding a dependent value of
that sort to a node type that didn't already have it, I suspect either
you or I would do our best to shoot that down. The only possible
argument for having that in there at all is that the performance gains
from so doing are so large that we have no choice but to sacrifice a
principle to expediency.
Hm. It might be interesting to try removing those node fields altogether
and see what it costs us. Setting the field at parse time is zero-cost,
at least in the primary code path through make_op(), because we have our
hands on the pg_operator row at that time anyway. (I have a vague
recollection that that was once not true, but it certainly is true now
and has been for a long time.) Not having it would cost us one syscache
lookup per operator at execution start, and maybe more than that if there
are additional places that would need the function OID, which there
probably are --- volatility checks in the planner come to mind. But
I'm not sure how significant that would be. There are certainly plenty
of syscache lookups being done at plan startup anyway.
But this is mostly moot unless someone is actively planning to try to
implement ALTER OPERATOR SET FUNCTION.
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
Hello.
Currently using nodeToString and stringToNode you can not pass a full plan. In
this regard, what is the plan to fix it? Or in the under task parallel query
does not have such a problem?
This turns out not to be straightforward to code, because we
don't have a generic plan tree walker,
I have an inner development. I am using python analyzing header files and
generates a universal walker (parser, paths ,executer and etc trees), as well
as the serializer and deserializer to jsonb. Maybe I should publish this code?
Thanks.
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 22, 2015 at 07:15:35PM +0300, YUriy Zhuravlev wrote:
Hello.
Currently using nodeToString and stringToNode you can not pass a
full plan. In this regard, what is the plan to fix it? Or in the
under task parallel query does not have such a problem?This turns out not to be straightforward to code, because we don't
have a generic plan tree walker,I have an inner development. I am using python analyzing header
files and generates a universal walker (parser, paths ,executer and
etc trees), as well as the serializer and deserializer to jsonb.
Maybe I should publish this code?
Please do.
Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:
Hello.
Currently using nodeToString and stringToNode you can not pass a full plan. In
this regard, what is the plan to fix it? Or in the under task parallel query
does not have such a problem?
It's already fixed. See commits
a0d9f6e434bb56f7e5441b7988f3982feead33b3 and
9f1255ac859364a86264a67729dbd1a36dd63ff2.
--
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
On Thursday 22 October 2015 12:53:49 you wrote:
On Thu, Oct 22, 2015 at 12:15 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:
Hello.
Currently using nodeToString and stringToNode you can not pass a full
plan. In this regard, what is the plan to fix it? Or in the under task
parallel query does not have such a problem?It's already fixed. See commits
a0d9f6e434bb56f7e5441b7988f3982feead33b3 and
9f1255ac859364a86264a67729dbd1a36dd63ff2.
Ahh. Thanks.
And then another question:
What do you think if the generated equalfuncs.c, copyfuncs.c, readfuncs.c,
outfuncs.c from XML or JSON?
In order not to change the code in four places when changing nodes.
--
YUriy Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Oct 22, 2015 at 1:19 PM, YUriy Zhuravlev
<u.zhuravlev@postgrespro.ru> wrote:
And then another question:
What do you think if the generated equalfuncs.c, copyfuncs.c, readfuncs.c,
outfuncs.c from XML or JSON?
In order not to change the code in four places when changing nodes.
It would be more useful, if we're going to autogenerate code, to do it
from the actual struct definitions.
--
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
