Thread: [HACKERS] domain type smashing is expensive
On short-running queries that return a lot of columns,
SendRowDescriptionMessage's calls to getBaseTypeAndTypmod() are a
noticeable expense. The following change improves performance on a
query returning 100 columns by about 6% when prepared queries are in
use (Mithun Cy and I both tested and got similar results; his testing
was more rigorous than mine).
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -2295,6 +2295,9 @@ getBaseTypeAndTypmod(Oid typid, int32 *typmod) HeapTuple tup;
Form_pg_typetypTup;
+ if (typid < FirstBootstrapObjectId)
+ break;
+ tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(typid)); if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for type %u", typid);
I cannot claim to love that hack, but the performance improvement is
nice. There's no real problem with the hack -- it just embeds an
assumption that pg_type.h will never define a domain type, which
doesn't seem like a terribly problematic assumption, and we could
always add a comment someplace to validate it, or even teach the
script that processes the catalog scripts to enforce it. But it's
not, like, the most elegant thing anybody's ever done.
I see two other options:
1. Revisit the decision to smash domain types to base types here.
That change was made by Tom Lane back in 2003
(d9b679c13a820eb7b464a1eeb1f177c3fea13ece) but the commit message only
says *that* we decided to do it, not *why* we decided to do it, and
the one-line comment added by that commit doesn't do any better.
2. Precompute the list of types to be sent to the client during
planning instead of during execution. The point of prepared
statements is supposed to be to do as much of the work as possible at
prepare time so that bind/execute is as fast as possible, but we're
not really adhering to that design philosophy here. However, I don't
have a clear idea of exactly how to do that.
Thoughts?
--
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 short-running queries that return a lot of columns,
> SendRowDescriptionMessage's calls to getBaseTypeAndTypmod() are a
> noticeable expense.
Yeah, I was never very happy with the way that the original domain
patch dealt with that. I think you're not even focusing on the
worst part, which is all the getBaseType calls in the parser.
I do not have a good idea about how to get rid of them though.
> + if (typid < FirstBootstrapObjectId)
> + break;
I'm really unwilling to buy into an assumption that we'll never
have any built-in domains just to support such a crock as this.
> 1. Revisit the decision to smash domain types to base types here.
> That change was made by Tom Lane back in 2003
> (d9b679c13a820eb7b464a1eeb1f177c3fea13ece) but the commit message only
> says *that* we decided to do it, not *why* we decided to do it, and
> the one-line comment added by that commit doesn't do any better.
You'd need to dig around in the archives from around that time. But
my hazy recollection is that the argument was that clients would be
much more likely to know what to do with a built-in type than with
some domain over it. psql, for example, knows about right-justifying
the built-in numeric types, but it'd fail to do so for domains.
> 2. Precompute the list of types to be sent to the client during
> planning instead of during execution. The point of prepared
> statements is supposed to be to do as much of the work as possible at
> prepare time so that bind/execute is as fast as possible, but we're
> not really adhering to that design philosophy here. However, I don't
> have a clear idea of exactly how to do that.
That'd help for prepared statements, but not for simple query execution.
The trick here is that I don't think we want to change the returned column
types for queries that are not being sent to a client. The parser and
planner aren't really aware of that context ATM. Maybe we could make them
so? But it still seems like a kluge that is only addressing a small part
of the domain-smashing issue.
I wonder if it'd help to put some kind of bespoke cache into getBaseType.
We've done that elsewhere, eg operator lookup.
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, Sep 12, 2017 at 1:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On short-running queries that return a lot of columns, >> SendRowDescriptionMessage's calls to getBaseTypeAndTypmod() are a >> noticeable expense. > > Yeah, I was never very happy with the way that the original domain > patch dealt with that. I think you're not even focusing on the > worst part, which is all the getBaseType calls in the parser. > I do not have a good idea about how to get rid of them though. Well, I'm focusing on the part that shows up in the profile. Prepared queries don't get re-parsed repeatedly, so the calls in the parser don't matter in that context. I'm not saying it wouldn't be nice to get rid of them, but it only helps people who aren't preparing their queries. >> + if (typid < FirstBootstrapObjectId) >> + break; > > I'm really unwilling to buy into an assumption that we'll never > have any built-in domains just to support such a crock as this. I more or less expected that reaction, but I think it's a bit short-sighted. If somebody wanted to define a domain type in pg_type.h, they'd have to write any domain constraint out in pg_constraint.h in nodeToString() form, and it seems to me that the chances that we'd accept a patch are pretty much nil, because it would be a maintenance nuisance. Now, maybe you could argue that somebody might want to define a constraint-less domain in pg_type.h, but I can't recall any proposal to do such a thing and don't see why anybody'd want to do it. > You'd need to dig around in the archives from around that time. But > my hazy recollection is that the argument was that clients would be > much more likely to know what to do with a built-in type than with > some domain over it. psql, for example, knows about right-justifying > the built-in numeric types, but it'd fail to do so for domains. Mmm, that's a good point. >> 2. Precompute the list of types to be sent to the client during >> planning instead of during execution. The point of prepared >> statements is supposed to be to do as much of the work as possible at >> prepare time so that bind/execute is as fast as possible, but we're >> not really adhering to that design philosophy here. However, I don't >> have a clear idea of exactly how to do that. > > That'd help for prepared statements, but not for simple query execution. Sure, but that's kinda my point. We've got to send a RowDescription message for every query, and if that requires smashing domain types to base types, we have to do it. What we don't have to do is repeat that work for every execution of a prepared query. > The trick here is that I don't think we want to change the returned column > types for queries that are not being sent to a client. The parser and > planner aren't really aware of that context ATM. Maybe we could make them > so? I guess it depends on whether that context is mutable. Can I Parse a query to create a prepared statement, then use that from a stored procedure? If so, then it's not firmly known at plan time what the execution context will be. > But it still seems like a kluge that is only addressing a small part > of the domain-smashing issue. > > I wonder if it'd help to put some kind of bespoke cache into getBaseType. > We've done that elsewhere, eg operator lookup. That might be a possibility, although I feel like it's likely to be substantially less effective than the quick hack, and it's not really attacking the problem at the root anyway. -- 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, Sep 12, 2017 at 1:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The trick here is that I don't think we want to change the returned column
>> types for queries that are not being sent to a client. The parser and
>> planner aren't really aware of that context ATM. Maybe we could make them
>> so?
> I guess it depends on whether that context is mutable. Can I Parse a
> query to create a prepared statement, then use that from a stored
> procedure? If so, then it's not firmly known at plan time what the
> execution context will be.
Um, good point; I'm pretty sure that we don't distinguish. This may
well be the reason it's done like this right now.
>> I wonder if it'd help to put some kind of bespoke cache into getBaseType.
>> We've done that elsewhere, eg operator lookup.
> That might be a possibility, although I feel like it's likely to be
> substantially less effective than the quick hack, and it's not really
> attacking the problem at the root anyway.
I'd say that what you're proposing is the exact opposite of attacking
the problem at the root.
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, Sep 12, 2017 at 3:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'd say that what you're proposing is the exact opposite of attacking > the problem at the root. I agree. But if we're going to install a cache here, on a cycle-for-cycle basis, it's going to be hard to beat "caching" the knowledge that OIDs under 10000 are not domains. I don't find that to be an optimal solution, but I don't find dumping a bunch of caching code in there that involves writing more code to buy less performance to be superior, either. If we're going to install a point fix, I think there's much to be said for installing one that works well. If we want to revisit this more strategically, I think we should throw the whole idea of having the executor compute slot descriptors from the tlist out the window. Every executor node is walking over a linked list (uggh) of nodes and running not one but two fairly complex functions (exprType, exprTypmod) on each one. Then, each type OID has to be looked up by TupleDescInitEntry to get attlen/byval/align/storage/collation. Now, suppose we instead had an array of structures associated with each plan node, with each element containing <type OID, typmod, typlen, typbyval, typalign, typstorage, typcollation, smashed-to-base-type OID>. Then we wouldn't need syscache lookups to initialize the individual executor nodes or to build the RowDescription message, because we'd already have all the relevant bits in hand. Plus iterating over an array would probably be faster than iterating over a list. The downside is that we'd still need the tlists for other reasons, so plans would get bigger. But I don't think that's a huge problem if it makes them run faster. -- 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
Hi, On 2017-09-12 14:28:51 -0400, Robert Haas wrote: > On Tue, Sep 12, 2017 at 1:37 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> On short-running queries that return a lot of columns, > >> SendRowDescriptionMessage's calls to getBaseTypeAndTypmod() are a > >> noticeable expense. > > > > Yeah, I was never very happy with the way that the original domain > > patch dealt with that. I think you're not even focusing on the > > worst part, which is all the getBaseType calls in the parser. > > I do not have a good idea about how to get rid of them though. > > Well, I'm focusing on the part that shows up in the profile. Prepared > queries don't get re-parsed repeatedly, so the calls in the parser > don't matter in that context. I'm not saying it wouldn't be nice to > get rid of them, but it only helps people who aren't preparing their > queries. In my experience syscache lookups aren't particularly prominent in profiles of non-prepared workloads. That's commonly memory allocator (due to all the lists), raw parser, and then parse-analysis (with a small proportion spend in the syscaches). Leaving executor side aside. > >> + if (typid < FirstBootstrapObjectId) > >> + break; > > > > I'm really unwilling to buy into an assumption that we'll never > > have any built-in domains just to support such a crock as this. I'm not super happy about that solution either, but it has the big advantage of being simple and consisting of very little code. Adding a couple comments here and a type_sanity.sql check seems to buy a good chunk of performance for little effort. Adding additional hashtable searches is far from free. > I more or less expected that reaction, but I think it's a bit > short-sighted. If somebody wanted to define a domain type in > pg_type.h, they'd have to write any domain constraint out in > pg_constraint.h in nodeToString() form, and it seems to me that the > chances that we'd accept a patch are pretty much nil, because it would > be a maintenance nuisance. Now, maybe you could argue that somebody > might want to define a constraint-less domain in pg_type.h, but I > can't recall any proposal to do such a thing and don't see why > anybody'd want to do it. Due to that reason we'd probably create such domain types outside of bootstrap, and therefore in a separate oid range... > > You'd need to dig around in the archives from around that time. But > > my hazy recollection is that the argument was that clients would be > > much more likely to know what to do with a built-in type than with > > some domain over it. psql, for example, knows about right-justifying > > the built-in numeric types, but it'd fail to do so for domains. > > Mmm, that's a good point. Yea, I don't think we want to revise that just because of this performance issue - it'd likely cause some subtle breakage. I'm far from convinced that this "downcasting" is a good idea on a semantical basis, but that seems like a separate discussion and I can't recall complaints. > >> 2. Precompute the list of types to be sent to the client during > >> planning instead of during execution. The point of prepared > >> statements is supposed to be to do as much of the work as possible at > >> prepare time so that bind/execute is as fast as possible, but we're > >> not really adhering to that design philosophy here. However, I don't > >> have a clear idea of exactly how to do that. > > > > That'd help for prepared statements, but not for simple query execution. > Sure, but that's kinda my point. We've got to send a RowDescription > message for every query, and if that requires smashing domain types to > base types, we have to do it. What we don't have to do is repeat that > work for every execution of a prepared query. We also have done a bunch of those lookups in the planner already, so if we'd move it there it might still be be an advantage performancewise even for the single execution case. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers