Rethinking the parameter access hooks for plpgsql's benefit
We already knew that plpgsql's setup_param_list() is a bit of a
performance hog. Trying to improve that was the primary motivation
behind commit f4e031c662a6b600b786c4849968a099c58fcce7 (the
bms_next_member business), though that was just micro-optimization
rather than any fundamental rethink of how things are done.
Some benchmarking at Salesforce has shown that there's a really
significant penalty in plpgsql functions that have a lot of local
variables, because large "ndatums" increases the size of the
ParamListInfo array that has to be allocated (and zeroed!) for
each expression invocation.
Thinking about this, it struck me that the real problem is that
the parameter list APIs are still too stuck in the last century,
in that they're optimized for static lists of parameters which
is not what plpgsql needs at all. In fact, if we redefined the
usage of ParamFetchHook so that it would be called on every parameter
fetch, plpgsql could be perfectly happy with just *one* ParamListInfo
slot that it would re-fill each time. This would entirely eliminate
the need for any per-expression-execution ParamListInfo allocation,
because a function could just use one single-entry ParamListInfo array
for all purposes.
The attached proposed patch represents an exploration of this idea.
I've also attached a SQL script with some simple functions for testing
its performance. The first one is for exploring what happens with more
or fewer local variables. I compared current HEAD against the patch
with asserts off, for a call like "SELECT timingcheck(10000000);".
It looks like this (times in ms):
HEAD patch
no extra variables 8695 8390
10 extra variables 9218 8419
30 extra variables 9424 8255
100 extra variables 9433 8202
These times are only repeatable to within a few percent, but they do show
that there is a gain rather than loss of performance even in the base
case, and that the patched code's performance is pretty much independent
of the number of local variables whereas HEAD isn't.
The case where the patch potentially loses is where a single expression
contains multiple references to the same plpgsql variable, since with
HEAD, additional references to a variable don't incur any extra trips
through the ParamFetchHook, whereas with the patch they do. I set up
the reusecheck10() and reusecheck5() functions to see how bad that is.
HEAD patch
5 uses of same variable 4105 3962
10 uses of same variable 5738 6076
So somewhere between 5 and 10 uses of the same variable in a single
expression, you start to lose.
I claim that that's a very unlikely scenario and so this patch should
be an unconditional win for just about everybody.
The patch does create an API break for any code that is using
ParamFetchHooks, but it's an easy fix (just return the address of
the array element you stored data into), and any decent C compiler
will complain about the function type mismatch so the API break
should not go unnoticed.
Objections? Even better ideas?
regards, tom lane
On Sun, Mar 8, 2015 at 11:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Objections? Even better ideas?
I object on the grounds that we're three weeks past the deadline for
the last CommitFest, and that we should be trying to get committed
those patches that were submitted on time, not writing new ones that
will further increase the amount of reviewing that needs to be done
before we can get to beta, and perhaps the bug count of that
eventually when it arrives. In particular, I think that the fact that
you haven't made more of an effort to give the GROUPING SETS patch a
more detailed review is quite unfair to Andrew Gierth. But regardless
of that, this is untimely and should be pushed to 9.6.
On the technical side, I do agree that the requirement to allocate and
zero an array for every new simple expression is unfortunate, but I'm
not convinced that repeatedly invoking the hook-function is a good way
to solve that problem. Calling the hook-function figures to be
significantly more expensive than an array-fetch, so you can almost
think of the ParamListInfo entries themselves as a cache of data
retrieved from the hook function. In that view of the world, the
problem here is that initializing the cache is too expensive. Your
solution to that is to rip the cache out completely, but what would be
better is keep the cache and make initializing it cheaper - e.g. by
sharing one ParamListInfo across all expressions in the same scope; or
by not zeroing the memory and instead tracking the the first N slots
are the only ones we've initialized; or $your_idea_here.
--
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 03/09/2015 09:11 AM, Robert Haas wrote:
On Sun, Mar 8, 2015 at 11:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Objections? Even better ideas?
I object on the grounds that we're three weeks past the deadline for
the last CommitFest, and that we should be trying to get committed
those patches that were submitted on time, not writing new ones that
will further increase the amount of reviewing that needs to be done
before we can get to beta, and perhaps the bug count of that
eventually when it arrives. In particular, I think that the fact that
you haven't made more of an effort to give the GROUPING SETS patch a
more detailed review is quite unfair to Andrew Gierth. But regardless
of that, this is untimely and should be pushed to 9.6.
From the reading the original post it seems like the patch was
developed on Sales Force's time, not TGLs. I do not think we get to have
an opinion on that.
Thank you Tom for the efforts you made here, making the largest used PL
better, faster, stronger is exactly the type of micro-changes we need to
be looking at for further polish within the database.
JD
--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
Now I get it: your service is designed for a customer
base that grew up with Facebook, watches Japanese seizure
robot anime, and has the attention span of a gnat.
I'm not that user., "Tyler Riddle"
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 9, 2015 at 12:26 PM, Joshua D. Drake <jd@commandprompt.com> wrote:
From the reading the original post it seems like the patch was developed on
Sales Force's time, not TGLs. I do not think we get to have an opinion on
that.
Salesforce gets to develop their patches whenever they want, but they
don't get to control the community process for getting those patches
committed to PostgreSQL.
Thank you Tom for the efforts you made here, making the largest used PL
better, faster, stronger is exactly the type of micro-changes we need to be
looking at for further polish within the database.
So, just to be clear, are you proposing that everybody is exempted
from the CommitFest deadlines, or just Tom?
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
"Joshua D. Drake" <jd@commandprompt.com> writes:
On 03/09/2015 09:11 AM, Robert Haas wrote:
I object on the grounds that we're three weeks past the deadline for
the last CommitFest, and that we should be trying to get committed
those patches that were submitted on time, not writing new ones that
will further increase the amount of reviewing that needs to be done
before we can get to beta, and perhaps the bug count of that
eventually when it arrives. In particular, I think that the fact that
you haven't made more of an effort to give the GROUPING SETS patch a
more detailed review is quite unfair to Andrew Gierth. But regardless
of that, this is untimely and should be pushed to 9.6.
From the reading the original post it seems like the patch was
developed on Sales Force's time, not TGLs. I do not think we get to have
an opinion on that.
JD sees the situation correctly: this is $dayjob work, and it's going
to get done now not in four months because I have a deadline to meet.
I would like to push it into the community sources to reduce divergence
between our copy and Salesforce's, but if I'm told it has to wait till
9.6, I may or may not remember to try to do something then.
I will admit that I'm been slacking on commitfest work. This is not
unrelated to the fact that we've been in commitfest mode continuously
since last August. I'm afraid whatever enthusiasm I had for reviewing
other peoples' patches burned out some time ago.
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
JD sees the situation correctly: this is $dayjob work, and it's going
to get done now not in four months because I have a deadline to meet.
I would like to push it into the community sources to reduce divergence
between our copy and Salesforce's, but if I'm told it has to wait till
9.6, I may or may not remember to try to do something then.
I think most of the committers are pretty much in that situation?
I will admit that I'm been slacking on commitfest work. This is not
unrelated to the fact that we've been in commitfest mode continuously
since last August. I'm afraid whatever enthusiasm I had for reviewing
other peoples' patches burned out some time ago.
I can certainly relate to that :(.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On 03/09/2015 09:29 AM, Robert Haas wrote:
On Mon, Mar 9, 2015 at 12:26 PM, Joshua D. Drake <jd@commandprompt.com> wrote:
From the reading the original post it seems like the patch was developed on
Sales Force's time, not TGLs. I do not think we get to have an opinion on
that.Salesforce gets to develop their patches whenever they want, but they
don't get to control the community process for getting those patches
committed to PostgreSQL.
Did I miss the part of the post that said, (speaking as Tom): FYI, I am
ignoring the commit fest deadline and committing this?
Are we supposed to hold patches when we are post-deadline and not seek
feedback? That seems rather hostile.
Thank you Tom for the efforts you made here, making the largest used PL
better, faster, stronger is exactly the type of micro-changes we need to be
looking at for further polish within the database.So, just to be clear, are you proposing that everybody is exempted
from the CommitFest deadlines, or just Tom?
I am not suggesting that anyone is exempted from a CommitFest deadline.
I am actually rather hard line that we should be keeping them.
I think it is ridiculous to post on the bad/good/timing of a patch
submission unless there is a case being made that the process isn't
actually being followed. I don't see that here.
In short, soap boxing does nobody any good.
JD
--
Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, @cmdpromptinc
Now I get it: your service is designed for a customer
base that grew up with Facebook, watches Japanese seizure
robot anime, and has the attention span of a gnat.
I'm not that user., "Tyler Riddle"
--
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@2ndquadrant.com) wrote:
JD sees the situation correctly: this is $dayjob work, and it's going
to get done now not in four months because I have a deadline to meet.
I would like to push it into the community sources to reduce divergence
between our copy and Salesforce's, but if I'm told it has to wait till
9.6, I may or may not remember to try to do something then.I think most of the committers are pretty much in that situation?
Indeed.. And, further, the commitfest wasn't intended to prevent
committers from working on things, at least as I understood it. Core
sets a feature freeze date which is quite a different thing.
Considering feature freeze is generally after the last commitfest it
also seems counter-intuitive that committers are expected to have
finished all of their work for the next release prior to the start of
the last commitfest.
I will admit that I'm been slacking on commitfest work. This is not
unrelated to the fact that we've been in commitfest mode continuously
since last August. I'm afraid whatever enthusiasm I had for reviewing
other peoples' patches burned out some time ago.I can certainly relate to that :(.
+1.
Thanks,
Stephen
On Mon, Mar 9, 2015 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Joshua D. Drake" <jd@commandprompt.com> writes:
On 03/09/2015 09:11 AM, Robert Haas wrote:
I object on the grounds that we're three weeks past the deadline for
the last CommitFest, and that we should be trying to get committed
those patches that were submitted on time, not writing new ones that
will further increase the amount of reviewing that needs to be done
before we can get to beta, and perhaps the bug count of that
eventually when it arrives. In particular, I think that the fact that
you haven't made more of an effort to give the GROUPING SETS patch a
more detailed review is quite unfair to Andrew Gierth. But regardless
of that, this is untimely and should be pushed to 9.6.From the reading the original post it seems like the patch was
developed on Sales Force's time, not TGLs. I do not think we get to have
an opinion on that.JD sees the situation correctly: this is $dayjob work, and it's going
to get done now not in four months because I have a deadline to meet.
I would like to push it into the community sources to reduce divergence
between our copy and Salesforce's, but if I'm told it has to wait till
9.6, I may or may not remember to try to do something then.
Sure, I have things that I've wanted to push into the community
sources for the benefit of EnterpriseDB, too. Nobody's offered to let
me ignore the community process when it happens to be good for
EnterpriseDB. If we're changing that policy for patches submitted by
Salesforce employes, I'm afraid I must object unless EnterpriseDB
employees will get the same privilege. And I think 2ndQuadrant will
want in on it, too.
I will admit that I'm been slacking on commitfest work. This is not
unrelated to the fact that we've been in commitfest mode continuously
since last August. I'm afraid whatever enthusiasm I had for reviewing
other peoples' patches burned out some time ago.
Yeah, it would be so much easier to get the release out the door if
people didn't keep submitting patches to make the product better. We
should really try to discourage that.
--
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 Mon, Mar 9, 2015 at 12:53 PM, Joshua D. Drake <jd@commandprompt.com> wrote:
I think it is ridiculous to post on the bad/good/timing of a patch
submission unless there is a case being made that the process isn't actually
being followed. I don't see that here.
The CommitFest deadline was three weeks ago and I think it's
abundantly clear that Tom is not submitting this for the June
CommitFest. If that wasn't clear from his initial post, his follow-up
points have removed any doubt on that point.
--
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-03-09 12:54:44 -0400, Robert Haas wrote:
If we're changing that policy for patches submitted by Salesforce
employes, I'm afraid I must object unless EnterpriseDB employees will
get the same privilege. And I think 2ndQuadrant will want in on it,
too.
Right. I'm not really sure how much actual policy there is around this,
and how much (differently) perceived policy it is.
I will admit that I'm been slacking on commitfest work. This is not
unrelated to the fact that we've been in commitfest mode continuously
since last August. I'm afraid whatever enthusiasm I had for reviewing
other peoples' patches burned out some time ago.Yeah, it would be so much easier to get the release out the door if
people didn't keep submitting patches to make the product better. We
should really try to discourage that.
I don't think that's really the point. There's been awfully little
progress on the committing side, and what did happen has been born by
few shoulders. Which leaves little time that's officially dedicated to
the fun parts.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Mon, Mar 9, 2015 at 1:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2015-03-09 12:54:44 -0400, Robert Haas wrote:
If we're changing that policy for patches submitted by Salesforce
employes, I'm afraid I must object unless EnterpriseDB employees will
get the same privilege. And I think 2ndQuadrant will want in on it,
too.Right. I'm not really sure how much actual policy there is around this,
and how much (differently) perceived policy it is.
Yes, that's fair. I do realize that long-time, highly-trusted
committers, especially Tom, get more slack than newer committers or,
say, first-time patch submitters, and that is fair too. At the same
time, at some point we have to cut off 9.5 development.
I will admit that I'm been slacking on commitfest work. This is not
unrelated to the fact that we've been in commitfest mode continuously
since last August. I'm afraid whatever enthusiasm I had for reviewing
other peoples' patches burned out some time ago.Yeah, it would be so much easier to get the release out the door if
people didn't keep submitting patches to make the product better. We
should really try to discourage that.I don't think that's really the point. There's been awfully little
progress on the committing side, and what did happen has been born by
few shoulders. Which leaves little time that's officially dedicated to
the fun parts.
Yeah, sorry, I lost my temper there a bit. Sorry, Tom.
--
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
I don't think Tom, or that matter anyone needs to forgo working on changes
at any time. The only effect missing a commitfest deadline means is that
other reviewers don't offer any promises to give any feedback on it before
this round of the commitfest is done.
We don't have a policy of requiring code reviews before changes are
committed -- it's up to the commuters judgement whether the patch is type
for committing.
There has been the suggestion that commiters should concentrate on reviews
rather than fresh development during commitfests but nobody is going to do
100% reviewing work for the whole time and nobody could seriously suggest
Tom hasn't pulled his weight reviewing patches.
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Mar 9, 2015 at 12:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
JD sees the situation correctly: this is $dayjob work, and it's going
to get done now not in four months because I have a deadline to meet.
I would like to push it into the community sources to reduce divergence
between our copy and Salesforce's, but if I'm told it has to wait till
9.6, I may or may not remember to try to do something then.
Sure, I have things that I've wanted to push into the community
sources for the benefit of EnterpriseDB, too. Nobody's offered to let
me ignore the community process when it happens to be good for
EnterpriseDB. If we're changing that policy for patches submitted by
Salesforce employes, I'm afraid I must object unless EnterpriseDB
employees will get the same privilege. And I think 2ndQuadrant will
want in on it, too.
As far as that goes, it has never been the case that we expected every
patch to go through the commitfest review process. (If we did, our
response time to bugs would be probably a couple orders of magnitude
longer than it is.) The way I see the policy is that committers have
authority to commit things that they feel are ready and that haven't
been objected to by other developers. Depending on the particular
committer and the particular patch, feeling that a patch is "ready"
might or might not require that it's been through a commitfest review.
There is no process-based restriction on committing "ready" patches
except when we are in alpha/beta/RC states, which is surely months
away for 9.5.
If I were looking for a formal review on this one, I would certainly
not expect that it would get one during the current CF. I wasn't
though.
For context, I have currently got half a dozen irons in the fire:
* "expanded objects" (array performance fixes): needs review, was
submitted timely to current CF
* operator precedence changes: committable IMO, but it's not entirely
clear if we have consensus
* adjust loop count estimates for semijoins: committable IMO, waiting
for objections
* find stats on-the-fly for children of UNION ALL appendrels: WIP, far
from done though
* flatten empty sub-SELECTs and VALUES where feasible: committable IMO,
waiting for objections
* parameter fetch hook rethink: WIP, not as close to done
as I thought last night
I wasn't really planning to use the CF process for any but the first
of these. If we start insisting that committers go through CF for
even rather small patches like these other things, it's going to
destroy our productivity completely.
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 Mon, Mar 9, 2015 at 2:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
As far as that goes, it has never been the case that we expected every
patch to go through the commitfest review process. (If we did, our
response time to bugs would be probably a couple orders of magnitude
longer than it is.) The way I see the policy is that committers have
authority to commit things that they feel are ready and that haven't
been objected to by other developers. Depending on the particular
committer and the particular patch, feeling that a patch is "ready"
might or might not require that it's been through a commitfest review.
There is no process-based restriction on committing "ready" patches
except when we are in alpha/beta/RC states, which is surely months
away for 9.5.If I were looking for a formal review on this one, I would certainly
not expect that it would get one during the current CF. I wasn't
though.
Yes, I understand. I don't really have anything more to say about
this. Nothing here changes my basic feeling that we need to stop
putting new irons in the fire and start working hard on taking irons
out of the fire; and I think most if not all other committers are
already doing that. Even to the extent that they are focusing on
their own patches rather than other people's patches, I think it is
mostly patches that they wrote some time ago, not new things that they
have only just written.
I also will say, in fairness, that I do not really care about this
particular patch all that much one way or the other. I am less
convinced than you that this will always work out to a win, but when
it comes right down to it, you're a pretty smart guy, and if this gets
complaints about some scenario you've overlooked, I have confidence
you'll get around to repairing it. So whatever. What I do care about
is that we as a project have to at some point be willing to begin
closing the spigot on new patches and focusing on polishing and
shipping the patches we've already got. I think it's perfectly
reasonable to worry about where we are on that continuum when it's
already several weeks after the start of the last CommitFest, but if
I'm in the minority on that, so be it. But it's got to be done at
some point, or we will not be able to ship a beta, or a release, on
the anticipated timetable.
--
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 Mon, Mar 9, 2015 at 12:06 PM, Robert Haas <robertmhaas@gmail.com> wrote:
Yes, I understand. I don't really have anything more to say about
this. Nothing here changes my basic feeling that we need to stop
putting new irons in the fire and start working hard on taking irons
out of the fire; and I think most if not all other committers are
already doing that. Even to the extent that they are focusing on
their own patches rather than other people's patches, I think it is
mostly patches that they wrote some time ago, not new things that they
have only just written.
I must say that I share your concern here. I have no idea what's going
to happen with my ON CONFLICT patch, 9.5-wise. I hope that at least
the IGNORE variant makes it into 9.5, but I'm not sure that it will.
The ON CONFLICT IGNORE/UPDATE patch has existed in essentially the
same form since August, although research level work went into that
project as long ago as 2012. I'm not blaming anyone, but it's quite
discouraging that it has taken so much to get it to where it is now,
even though it's something that there is a huge, tangible demand for
(few other things are in the same category, and few of those have
actually been implemented). An enormous amount of effort went into
internals documentation (the Wiki pages), and into stress-testing
(Jeff Janes' "double entry bookkeeping" stress test, among others). I
hate to say it, but at some point I'll need to cut my loses.
To any interested contributor: If there is something that I can do to
help to advance the state of the ON CONFLICT UPDATE patch, let me
know. Perhaps there can be some useful reciprocal arrangement with
review time. That's not ideal, but I see no alternative.
--
Peter Geoghegan
--
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-03-09 13:15:59 -0700, Peter Geoghegan wrote:
I must say that I share your concern here. I have no idea what's going
to happen with my ON CONFLICT patch, 9.5-wise. I hope that at least
the IGNORE variant makes it into 9.5, but I'm not sure that it will.The ON CONFLICT IGNORE/UPDATE patch has existed in essentially the
same form since August, although research level work went into that
project as long ago as 2012. I'm not blaming anyone, but it's quite
discouraging that it has taken so much to get it to where it is now,
even though it's something that there is a huge, tangible demand for
(few other things are in the same category, and few of those have
actually been implemented). An enormous amount of effort went into
internals documentation (the Wiki pages), and into stress-testing
(Jeff Janes' "double entry bookkeeping" stress test, among others). I
hate to say it, but at some point I'll need to cut my loses.To any interested contributor: If there is something that I can do to
help to advance the state of the ON CONFLICT UPDATE patch, let me
know. Perhaps there can be some useful reciprocal arrangement with
review time. That's not ideal, but I see no alternative.
FWIW, I think you actually don't have much reason to complain. This work
has probably gotten more attention in total than any other recent
patch. Certainly, by far, more than any other in the 9.5 cycle.
So far I've not seen a single version that could be considered 'ready
for committer'. Even if there's points to be resolved you can make one
(presumably yours) solution ready.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Mon, Mar 9, 2015 at 4:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
FWIW, I think you actually don't have much reason to complain. This work
has probably gotten more attention in total than any other recent
patch. Certainly, by far, more than any other in the 9.5 cycle.
That has to be true, because the patch has been around in various
forms for so long.
So far I've not seen a single version that could be considered 'ready
for committer'. Even if there's points to be resolved you can make one
(presumably yours) solution ready.
Of course there isn't, since (for example, hint hint) the logical
decoding stuff isn't fully resolved (there is really only one other
issue with unique index inference). Those issues are the only two real
impediments to at least committing ON CONFLICT IGNORE that I'm aware
of, and the former is by far the biggest.
My frustration is down to not having anything I can do without
feedback on these issues...I've run out of things to do without
feedback entirely. It's not for me to decide whether or not I'm
justified in complaining about that. I don't think it matters much
either way, but as a matter of fact I am feeling very concerned about
it. It would be very unfortunate if the UPSERT patch missed the 9.5
release, and for reasons that don't have much to do with me in
particular.
--
Peter Geoghegan
--
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 the technical side, I do agree that the requirement to allocate and
zero an array for every new simple expression is unfortunate, but I'm
not convinced that repeatedly invoking the hook-function is a good way
to solve that problem. Calling the hook-function figures to be
significantly more expensive than an array-fetch, so you can almost
think of the ParamListInfo entries themselves as a cache of data
retrieved from the hook function. In that view of the world, the
problem here is that initializing the cache is too expensive. Your
solution to that is to rip the cache out completely, but what would be
better is keep the cache and make initializing it cheaper - e.g. by
sharing one ParamListInfo across all expressions in the same scope; or
by not zeroing the memory and instead tracking the the first N slots
are the only ones we've initialized; or $your_idea_here.
Getting back to the actual merits of this patch --- you're right, it
was not a good idea as proposed. I'd been thinking about the scalar
expression case only, and forgot that the same code is used to pass
parameters to queries, which might evaluate those parameters quite
a large number of times. So any savings from reducing the setup time
is sure to be swamped if the hook-function gets called repeatedly.
Another problem is that for ROW and REC datums, exec_eval_datum()
pallocs memory that won't get freed till the end of the statement,
so repeat evaluation would cause memory leaks. So we really need
evaluate-at-most-once semantics in there.
However, this attempt did confirm that we don't need to create a separate
ParamListInfo struct for each evaluation attempt. So the attached,
greatly stripped-down patch just allocates a ParamListInfo once at
function startup, and then zeroes it each time. This does nothing for
the memset overhead but it does get rid of the palloc traffic, which
seems to be good for a few percent on simple-assignment-type statements.
AFAICS this is an unconditional win compared to HEAD. What's more, it
provides at least a basis for doing better later: if we could think of
a reasonably cheap way of tracking which datums get invalidated by an
assignment, we might be able to reset only selected entries in the array
rather than blindly blowing away all of them.
I propose to apply this and leave it at that for now.
regards, tom lane
Attachments:
dont-realloc-ParamListInfo.patchtext/x-diff; charset=us-ascii; name=dont-realloc-ParamListInfo.patchDownload+76-73
On Mon, Mar 09, 2015 at 03:06:24PM -0400, Robert Haas wrote:
What I do care about
is that we as a project have to at some point be willing to begin
closing the spigot on new patches and focusing on polishing and
shipping the patches we've already got. I think it's perfectly
reasonable to worry about where we are on that continuum when it's
already several weeks after the start of the last CommitFest, but if
I'm in the minority on that, so be it.
I am in your minority on that point.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
