Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Thu, Jan 23, 2014 at 1:57 PM, Florian Pflug f...@phlo.org wrote: On Jan23, 2014, at 01:17 , David Rowley dgrowle...@gmail.com wrote: On Wed, Jan 22, 2014 at 12:46 AM, Florian Pflug f...@phlo.org wrote: If you want to play with this, I think the first step has to be to find a set of guarantees that SUM(float) is supposed to meet. Currently, SUM(float) guarantees that if the summands all have the same sign, the error is bound by C * N, where C is some (machine-specific?) constant (about 1e-15 or so), and N is the number of input rows. Or at least so I think from looking at SUMs over floats in descending order, which I guess is the worst case. You could, for example, try to see if you can find a invertibility conditions which guarantees the same, but allows C to be larger. That would put a bound on the number of digits lost by the new SUM(float) compared to the old one. I guess if the case is that normal (non-window) sum(float) results are undefined unless you add an order by to the aggregate then I guess there is no possible logic to put in for inverse transitions that will make them behave the same as an undefined behaviour. Actually, if sum(float) was really undefined, it'd be *very* easy to provide an inverse transition function - just make it a no-op. Heck, you could then even make the forward transition function a no-op, since the very definition of undefined behaviour is result can be anything, including setting your house on fire. The point is, it's *not* undefined - it's just imprecise. And the imprecision can even be quantified, it just depends on the number of input rows (the equal-sign requirement is mostly there to make imprecision equivalent to relative error). My apologies, I meant to use the term nondeterministic rather than undefined. There's really not any need that I can see to turn things silly here. My point was more that since sum(float) can give different results if it used an index scan rather than a seq scan, trying to get the inverse transition to match something that gives varying results sounds like a tricky task to take on. If it seems sound enough, then I may implement it in C to see how much overhead it adds to forward aggregation for floating point types, but even if it did add too much overhead to forward aggregation it might be worth allowing aggregates to have 2 forward transition functions and if the 2nd one exists then it could be used in windowing functions where the frame does not have unbounded following. I don't think adding yet another type of aggregation function is the solution here. Why not? I thought, if in the cases where we need to burden the forward transition functions with extra code to make inverse transitions possible, then why not have an extra transition function so that it does not slow down normal aggregation? My testing of sum(numeric) last night does not show any slow down by that extra dscale tracking code that was added, but if it did then I'd be thinking seriously about 2 forward transition functions to get around the problem. I don't understand what would be wrong with that. The core code could just make use of the 2nd function if it existed and inverse transitions were thought to be required. i.e in a window context and does not have UNBOUNDED PRECEDING. First, every additional function increases the maintenance burden, and at some point the expected benefit simply isn't worth it. IMHO at least. There's little point in arguing for this as we've managed to narrow any forward transition over head to background noise so far, my point more was if there was no other way to maintain performance and have an inverse transition function that this may be a solution to that problem. Secondly, you'd really need a second aggregate definition - usually, the non-invertible aggregate will get away with a much smaller state representation. Aggregates with state type internal could hack their way around that by simply not using the additional parts of their state structure, but e.g. plpgsql aggregates would still incur overhead due to repeated copying of a unnecessary large state value. If it's possible to represent the forward-only but not the invertible state state with a copy-by-value type, that overhead could easily be a large part of the total overhead you paid for having an inverse transition function. I had imagined they keep the same state type and don't use any extra variables that are defined for the forward transition function that is invertible. And lastly, we could achieve much of the benefit of a second transition function by simply telling the forward transition function whether to expect inverse transition function calls. We already expose things like the aggregation memory context to C-language transition functions, so the basic mechanism is already there. In fact, I pondered whether to do this -
Re: [HACKERS] extension_control_path
I haven't actually looked at the patch itself, but I noted this from the other review: On Fri, Jan 24, 2014 at 6:43 PM, Sergey Muraviov sergey.k.murav...@gmail.com wrote: = postgresql.conf: extension_control_path = '/extensions/postgis-2.0.4:/extensions/postgis-2.1.1' Using colon as the path separator is going to break on windows. The patch notices this and uses semicolon on Windows instead. Do we really want to go down that path - that means that everybody who writes any sorts of installation instructions including this will have to make them separate for different platforms. Shouldn't we just use semicolon on all platforms, for consistency? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] proposal: hide application_name from other users
On Fri, Jan 24, 2014 at 6:46 AM, Magnus Hagander mag...@hagander.net wrote: What actually happens if you set the application_name in the connection string in that environment? Does it override it to it's own default? If so, the developers there clearly need to be taught about fallback_application_name. And what happens if you set it in PGAPPNAME? My point wasn't that an application couldn't control this. The point is that this isn't so easy to manage and users might not realize there's anything to do. And it's not necessarily the case that the library could warn users. No one of the parts of the code here has the whole picture. In this case one part of the code is stuffing the information in $0 and another part is defaulting application_name to $0. Long term I agree we should really have some way of controlling these permissions more fine grained, but I just blanket hiding application name for non-superusers seems like a bad solution that still only fixes a small part of the problem. It makes a lot of sense to me to treat it the same way as sql_query. It's pretty similar (especially in the above given that we put the sql query in $0 after all) -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: hide application_name from other users
On Sat, Jan 25, 2014 at 10:42 AM, Greg Stark st...@mit.edu wrote: On Fri, Jan 24, 2014 at 6:46 AM, Magnus Hagander mag...@hagander.net wrote: What actually happens if you set the application_name in the connection string in that environment? Does it override it to it's own default? If so, the developers there clearly need to be taught about fallback_application_name. And what happens if you set it in PGAPPNAME? My point wasn't that an application couldn't control this. The point is that this isn't so easy to manage and users might not realize there's anything to do. And it's not necessarily the case that the library could warn users. No one of the parts of the code here has the whole picture. In this case one part of the code is stuffing the information in $0 and another part is defaulting application_name to $0. We still show the ip address. And the client port number. and the username. And the database. These may also give away information. No, not as much as if you stick a password into application_name for example, but they still give out information. Perhaps what you really would need is for pg_stat_activity to be *completely* superuser only? Because it *does* tell you about what other users are doing. Now, actually having the ability to do that would be a good thing, because there are certainly environments where it might make sense. But that's back to the long term solution of actually making it configurable. Not cherry-picking which features should break for some users and not others. Long term I agree we should really have some way of controlling these permissions more fine grained, but I just blanket hiding application name for non-superusers seems like a bad solution that still only fixes a small part of the problem. It makes a lot of sense to me to treat it the same way as sql_query. It's pretty similar (especially in the above given that we put the sql query in $0 after all) Except we *don't* put the SQL query in $0. We only put SELECT (or other commandtags), not the actual contents of the query. So *we* make sure we don't put the sensitive information there, since the wrong people may see it in argv, because that's our responsibility. Just like it's the responsibility of the client to make sure they don't put security sensitive information in application_name. If we restrict application_name to superusers only this way, we punish those who *don't* do the wrong thing by requiring their monitoring to now use superuser, in favor of those who *do* the wrong thing, which is put security sensitive information in application_name. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] proposal: hide application_name from other users
On Fri, Jan 24, 2014 at 5:21 PM, Harold Giménez har...@heroku.com wrote: On Fri, Jan 24, 2014 at 6:46 AM, Magnus Hagander mag...@hagander.net wrote: On Thu, Jan 23, 2014 at 2:01 AM, Greg Stark st...@mit.edu wrote: On Wed, Jan 22, 2014 at 1:03 PM, Josh Berkus j...@agliodbs.com wrote: Probably Heroku has some more specific exploit case to be concerned about here; if so, might I suggest taking it up with the -security list? I don't think there's a specific vulnerability that needs to be kept secret here. Here's an example. I just created a new hobby database which is on a multi-tenant cluster and ran select * from pg_stat_activity. Here are two of the more interesting examples: 463752 | de5nmf0gbii3u5 | 32250 | 463751 | qspfkgrwgqtbcu | unicorn worker[1] -p 30390 -c ./config/unicorn.rb || | | | | | | || insufficient privilege 463752 | de5nmf0gbii3u5 | 32244 | 463751 | qspfkgrwgqtbcu | unicorn worker[0] -p 30390 -c ./config/unicorn.rb || | | | | | | || insufficient privilege Note that the contents of the ARGV array are being set by the unicorn task queuing library. It knows it's making this information visible to other users with shell access on this machine. But the decision to stuff the ARGV information into the application_name is being made by the Pg driver. Neither is under the control of the application author who may not even be aware this is happening. Neither component has the complete information to make a competent decision about whether this information is safe to be in application_name or not. Note that the query is showing as insufficient privilege even though it is listed in the ps output -- the same ps output that is listing the unicorn ARGV that is being shown in the application_name You might say that the Pg gem is at fault for making a blanket policy decision for applications that the ARGV is safe to show to other database users but realistically it's so useful to see this information for your own connections that it's probably the right decision. Without it it's awfully hard to tell which worker is on which connection. It would just be nice to be able to treat application_name the same as query. I would say that yes, this is clearly broken in the Pg gem. I can see it having such a default, but not allowing an override... Uhm, it does allow an override as I said before. Oops, sorry, I missed that when reading back in the thread. The application can of course issue a SET application_name, assuming there is a hook somewhere in the system that will run after the connection has been established. I've had customers use that many times in java based systems for example, but I don't know enough about the pg gem, or unicorn, to have a clue if anything like it exists there. This is also a good way to track how connections are used throughout a pooled system where the same connection might be used for different things at different times. What actually happens if you set the application_name in the connection string in that environment? Does it override it to it's own default? If so, the developers there clearly need to be taught about fallback_application_name. It can be overridden using any of these methods. It does in fact use fallback_application_name when it defaults to $0, see https://bitbucket.org/ged/ruby-pg/src/6c2444dc63e17eb695363993e8887cc5d67750bc/lib/pg/connection.rb?at=default#cl-46 And what happens if you set it in PGAPPNAME? It works fine: ... With that many options of hiding it, I would still argue for just picking one of those. For example, of Heroku wants to protect their customers against the behaviour of the pg gem, you can for example set PGAPPNAME in the environment. That will override what the gem sets in fallback_application_name, but those users that actually use it and specify it in their connection string, will override that default. And all of that without removing a valuable debugging/tracing tool from other users. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [bug fix] postgres.exe fails to start on Windows Server 2012 due to ASLR
On 01/24/2014 06:42 PM, MauMau wrote: Hello, My customer reported the following problem on Windows. I'm afraid this is a serious problem, and I think we should provide a fix in the next minor release. I've attached a fix, and I would wish it to be back-ported. [Problem] The customer is using 64-bit PostgreSQL 9.1.x Which x? Does this issue also occur on 9.3.2, or in 9.4 HEAD, when tested on Win2k12? -- Craig Ringer 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
Re: [HACKERS] pg_get_viewdefs() indentation considered harmful
Robert Haas robertmh...@gmail.com writes: On Fri, Jan 24, 2014 at 8:53 PM, Greg Stark st...@mit.edu wrote: Indeed even aside from the performance questions, once you're indented 5-10 times the indention stops being useful at all. The query would probably be even more readable if we just made indentation modulo 40 so once you get too far indented it wraps around which is not unlike how humans actually indent things in this case. Ha! That seems a little crazy, but *capping* the indentation at some reasonable value might not be dumb. I could go for either of those approaches, if applied uniformly, and actually Greg's suggestion sounds a bit better: it seems more likely to preserve some readability in deeply nested constructs. With either approach you need to ask where the limit value is going to come from. Is it OK to just hard-wire a magic number, or do we need to expose a knob somewhere? 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
Re: [HACKERS] [COMMITTERS] pgsql: libpq: Support TLS versions beyond TLSv1.
Noah Misch n...@leadboat.com writes: libpq: Support TLS versions beyond TLSv1. Per report from Jeffrey Walton, libpq has been accepting only TLSv1 exactly. Along the lines of the backend code, libpq will now support new versions as OpenSSL adds them. This patch seems fishy. The commit comment claims that it makes libpq consistent with the backend, but it doesn't: in the backend, we use SSLv23_method() but then set only the option SSL_OP_NO_SSLv2. With the patch, libpq now also sets the option SSL_OP_NO_SSLv3, which I assume means that we just disabled SSL v3 protocol. Did we actually want to do that? If so, why wasn't this patch advertised as doing that, and why wasn't the backend also made to reject SSL v3? 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
Re: [HACKERS] extension_control_path
Magnus Hagander mag...@hagander.net writes: Using colon as the path separator is going to break on windows. The patch notices this and uses semicolon on Windows instead. Do we really want to go down that path - that means that everybody who writes any sorts of installation instructions including this will have to make them separate for different platforms. Shouldn't we just use semicolon on all platforms, for consistency? Semicolon, being a valid filename character on most platforms (dunno about Windows), isn't a terribly good choice either. Since I disagree with the goal of this patch in the first place, I'm disinclined to spend brain cells on inventing a more robust format for a list of path names. I'm sure there is one though, if you're giving up on being consistent with traditional PATH format. 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
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
David Rowley dgrowle...@gmail.com writes: My point was more that since sum(float) can give different results if it used an index scan rather than a seq scan, trying to get the inverse transition to match something that gives varying results sounds like a tricky task to take on. This is just a variant of the same excuse we heard before. The question is not whether sum(float8) can give bad results; the question is whether we are going to break applications that are using it carefully (ie with an appropriate ORDER BY) and expecting to get good results. Secondly, you'd really need a second aggregate definition - usually, the non-invertible aggregate will get away with a much smaller state representation. Yeah. I think the idea of multiple transition functions in a single aggregate definition is pretty broken to start with, but the likelihood that they couldn't share aggregate state types puts it completely beyond sanity. We're not going to start inventing stype2 etc. 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
Re: [HACKERS] [COMMITTERS] pgsql: libpq: Support TLS versions beyond TLSv1.
On Sat, Jan 25, 2014 at 11:24:19AM -0500, Tom Lane wrote: Noah Misch n...@leadboat.com writes: libpq: Support TLS versions beyond TLSv1. Per report from Jeffrey Walton, libpq has been accepting only TLSv1 exactly. Along the lines of the backend code, libpq will now support new versions as OpenSSL adds them. This patch seems fishy. The commit comment claims that it makes libpq consistent with the backend, but it doesn't: in the backend, we use SSLv23_method() but then set only the option SSL_OP_NO_SSLv2. With the patch, libpq now also sets the option SSL_OP_NO_SSLv3, which I assume means that we just disabled SSL v3 protocol. Did we actually want to do that? If so, why wasn't this patch advertised as doing that, and why wasn't the backend also made to reject SSL v3? The backend allows SSLv3, TLSv1, TLSv1.1 and TLSv1.2. Before the patch, libpq allowed TLSv1 only. Since the patch, libpq allows TLSv1, TLSv1.1 and TLSv1.2. I did twitch a bit over leaving them non-identical. However, disabling SSLv3 in the backend would be a separate discussion due to the compatibility break. I also didn't see the point of initiating SSLv3 support in libpq when it has been disabled so long without complaint. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] extension_control_path
Magnus Hagander mag...@hagander.net writes: Using colon as the path separator is going to break on windows. The patch notices this and uses semicolon on Windows instead. Do we really want to go down that path - that means that everybody who writes any sorts of installation instructions including this will have to make them separate for different platforms. Shouldn't we just use semicolon on all platforms, for consistency? Well, I've been considering that what I found already in the backend to solve the same problem was a valid model to build against. Pick any reasonnable choice you want to, fix dynamic_library_path along the new lines or maybe ask me to, and then let's apply the same design to the new GUC doing about exactly the same thing? Tom Lane t...@sss.pgh.pa.us writes: Since I disagree with the goal of this patch in the first place, I'm Should we remove dynamic_library_path? If not, why do we keep it? Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: libpq: Support TLS versions beyond TLSv1.
Noah Misch n...@leadboat.com writes: On Sat, Jan 25, 2014 at 11:24:19AM -0500, Tom Lane wrote: why wasn't the backend also made to reject SSL v3? The backend allows SSLv3, TLSv1, TLSv1.1 and TLSv1.2. Before the patch, libpq allowed TLSv1 only. Since the patch, libpq allows TLSv1, TLSv1.1 and TLSv1.2. I did twitch a bit over leaving them non-identical. However, disabling SSLv3 in the backend would be a separate discussion due to the compatibility break. I also didn't see the point of initiating SSLv3 support in libpq when it has been disabled so long without complaint. I looked into the git history to see how it got like this, because it surely wasn't inconsistent to start with. Commit 19570420f5318343ed7a263cc6046fd5605b22cc of 2002-06-14 switched both backend and libpq from using SSLv23_method() to using TLSv1_method() (along with a lot of other changes). [released in 7.3.0] Commit 750a0e676e1f8f71bf1c6aba05d3264a7c57218b of 2002-12-18 changed both backend and libpq back to using SSLv23_method(). [released in 7.3.1] Commit 6ccb5aebaddd9e7aefaa7d1e7baa3264148be3c5 of 2003-01-08 installed the SSL_OP_NO_SSLv2 switch on the backend side and switched libpq back to using TLSv1_method(). [released in 7.3.2] AFAICT it's been stable since 7.3.2. I would offer, however, that probably *none* of those three patches got reviewed with any care. SSL wasn't a particularly mainstream concern back then, and cross-openssl-library-version compatibility issues even less so. I would argue that we ought to not reject SSLv3 in libpq if we're not doing so in the backend. It's certainly moot from a functional standpoint, since every post-7.3 libpq version has only been able to talk to servers that had TLS-capable libraries, so it's impossible to imagine a case where they wouldn't end up negotiating TLS-something. My beef is that leaving it as it is will confuse everybody who looks at this code in the future. Alternatively, given that TLS has been around for a dozen years and openssl versions that old have not gotten security updates for a long time, why don't we just reject SSLv3 on the backend side too? I guess it's barely possible that somebody out there is using a non-libpq-based client that uses a non-TLS-capable SSL library, but surely anybody like that is overdue to move into the 21st century. An SSL library that old is probably riddled with security issues. 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
Re: [HACKERS] extension_control_path
On Sat, Jan 25, 2014 at 6:07 PM, Dimitri Fontaine dimi...@2ndquadrant.frwrote: Magnus Hagander mag...@hagander.net writes: Using colon as the path separator is going to break on windows. The patch notices this and uses semicolon on Windows instead. Do we really want to go down that path - that means that everybody who writes any sorts of installation instructions including this will have to make them separate for different platforms. Shouldn't we just use semicolon on all platforms, for consistency? Well, I've been considering that what I found already in the backend to solve the same problem was a valid model to build against. Pick any reasonnable choice you want to, fix dynamic_library_path along the new lines or maybe ask me to, and then let's apply the same design to the new GUC doing about exactly the same thing? Ha, I didn't realize dynamic_library_paty had the same problem. In fact, I have to admit I didn't realize I could put more than one path in there - I don't think I've ever used that :D So based on the previous behaviour there, I withdraw my comment - being consistent with the existing behaviour of that parameter makes perfect sense. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] [COMMITTERS] pgsql: libpq: Support TLS versions beyond TLSv1.
On Sat, Jan 25, 2014 at 12:25:30PM -0500, Tom Lane wrote: Noah Misch n...@leadboat.com writes: On Sat, Jan 25, 2014 at 11:24:19AM -0500, Tom Lane wrote: why wasn't the backend also made to reject SSL v3? The backend allows SSLv3, TLSv1, TLSv1.1 and TLSv1.2. Before the patch, libpq allowed TLSv1 only. Since the patch, libpq allows TLSv1, TLSv1.1 and TLSv1.2. I did twitch a bit over leaving them non-identical. However, disabling SSLv3 in the backend would be a separate discussion due to the compatibility break. I also didn't see the point of initiating SSLv3 support in libpq when it has been disabled so long without complaint. I looked into the git history to see how it got like this, because it surely wasn't inconsistent to start with. [...] Interesting. I would argue that we ought to not reject SSLv3 in libpq if we're not doing so in the backend. It's certainly moot from a functional standpoint, since every post-7.3 libpq version has only been able to talk to servers that had TLS-capable libraries, so it's impossible to imagine a case where they wouldn't end up negotiating TLS-something. My beef is that leaving it as it is will confuse everybody who looks at this code in the future. Quaintness aside, I can't envision a user benefit of a fall 2014 introduction of SSLv3 support to libpq. Alternatively, given that TLS has been around for a dozen years and openssl versions that old have not gotten security updates for a long time, why don't we just reject SSLv3 on the backend side too? I guess it's barely possible that somebody out there is using a non-libpq-based client that uses a non-TLS-capable SSL library, but surely anybody like that is overdue to move into the 21st century. An SSL library that old is probably riddled with security issues. +1. If you can upgrade to 9.4, you can also bring your TLS protocol out of the iron age. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_get_viewdefs() indentation considered harmful
On 01/25/2014 11:06 AM, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Fri, Jan 24, 2014 at 8:53 PM, Greg Stark st...@mit.edu wrote: Indeed even aside from the performance questions, once you're indented 5-10 times the indention stops being useful at all. The query would probably be even more readable if we just made indentation modulo 40 so once you get too far indented it wraps around which is not unlike how humans actually indent things in this case. Ha! That seems a little crazy, but *capping* the indentation at some reasonable value might not be dumb. I could go for either of those approaches, if applied uniformly, and actually Greg's suggestion sounds a bit better: it seems more likely to preserve some readability in deeply nested constructs. With either approach you need to ask where the limit value is going to come from. Is it OK to just hard-wire a magic number, or do we need to expose a knob somewhere? Simply capping it is probably the best bang for the buck. I suspect most people would prefer to have q1 union q2 union q3 union q4 with the subqueries all indented to the same level. But I understand the difficulties in doing so. A knob seems like overkill. I'd just hardwire some number, say three or four levels of indentation. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Fwd: [HACKERS] patch: make_timestamp function
Looks good to me. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Postgresql for cygwin - 3rd
On 01/24/2014 07:50 AM, Marco Atzeri wrote: Those two issues need to be fixed. And yes, they are regressions from my Cygwin 1.7.7 setup where they pass consistently, just about every day. See http://www.pgbuildfarm.org/cgi-bin/show_history.pl?nm=brolgabr=HEAD 1.7.7 is 3.5 years hold. In the mean time we added 64 bit and moved to gcc-4.8.x No doubt, but that doesn't mean that extra things failing is OK. You don't get to choose which regression tests you're going to pass and which you're not. Disabling the tests or providing nonsensical results files are unacceptable. This is a Cygwin behavior issue and needs to be fixed. Distributing broken binary that crashes after standard rebase, it is also not acceptable and it is also worst. Your farm is not testing this case, I presume. Quite so. But this is not a case of either/or. I have now tested the central part of the proposed changes on both old and new Cygwin installations, and they appear to work. I'm going to commit them and backpatch back to 9.0, which is where we currently have buildfarm coverage (and 8.4 will be at EOL in a few months anyway). That will take care of your rebase issue. That leaves several issues to be handled: * LDAP libraries - the way you have proposed surely isn't right. What we want is something more like this in the Makefile.global.in: ifeq ($(PORTNAME), cygwin) libpq_pgport += $(LDAP_LIBS_FE) endif * isolation tests fail with an indefinite hang on newer Cygwin * prepared_xacts test fails with an indefinite hang on newer Cygwin if run in parallel with other tests * tsearch tests fail on non-C locale (or at least on en_US.utf8). It turns out this is actually an old bug, and can be reproduced on my old Cygwin instance. I wonder if it's caused by faulty locale files? Really, for a properly working port all these need to be fixed. I am available to work on tests regression, but I am not a postgresql expert so do not expect all the job from my side. We can help you some, but very few people in the community run Cygwin, and my time to spend on it is fairly limited. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Questionable coding in orderedsetaggs.c
In ordered_set_startup() sorts are initialised in non-randomAccess mode (tuplesort_begin_heap() and ~datum(), last argument). The use of tuplesort_skip_tuples() feels very like a random access to me. I think it doesn't fail because the only use (and implementation) is to skip forwards; if backwards were tried (as the interface permits) external sorts would fail because multiple tapes are present for FINALMERGE. -- Cheers, Jeremy -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
Peter Geoghegan p...@heroku.com writes: On Wed, Jan 22, 2014 at 1:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: BTW, have you got any sort of test scenario you could share for this purpose? I'm sure I could build something, but if you've already done it ... I simply ran the standard regression tests, and then straced a backend as it executed the pgss-calling query. I'm not sure that I've understood your question. As previously noted, my approach to testing this patch involved variations of: running make installcheck-parallel Do the regression tests fail for you when doing this? What I see is a duplicate occurrence of an escape_string_warning bleat: *** /home/postgres/pgsql/src/test/regress/expected/plpgsql.out Fri Jan 3 17:07 :46 2014 --- /home/postgres/pgsql/src/test/regress/results/plpgsql.out Sat Jan 25 13:37 :20 2014 *** *** 4568,4573 --- 4568,4579 HINT: Use the escape string syntax for backslashes, e.g., E'\\'. QUERY: SELECT 'foo\\bar\041baz' CONTEXT: PL/pgSQL function strtest() line 4 at RETURN + WARNING: nonstandard use of \\ in a string literal + LINE 1: SELECT 'foo\\bar\041baz' +^ + HINT: Use the escape string syntax for backslashes, e.g., E'\\'. + QUERY: SELECT 'foo\\bar\041baz' + CONTEXT: PL/pgSQL function strtest() line 4 at RETURN strtest - foo\bar!baz == which seems to happen because generate_normalized_query() reruns the core lexer on the given statement, and if you got a warning the first time, you'll get another one. It seems this only happens with rather small values of pg_stat_statements.max, else there's already a RETURN text-constant query in the hashtable so we don't need to do reparsing right here. (I'm testing with max = 100 to exercise the garbage collection code.) I'm not sure this is a big deal for real-world use, because surely by now everyone has either fixed their escape-string issues or disabled the warning. But it's annoying for testing. The big picture of course is that having this warning issued this way is broken anyhow, and has been since day one, so it's not entirely pg_stat_statements' fault. I'm just wondering if it's worth taking the trouble to turn off the warning when reparsing. 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
Re: [HACKERS] GIN improvements part2: fast scan
On 23.1.2014 17:22, Heikki Linnakangas wrote:
I measured the time that query takes, and the number of pages hit, using
explain (analyze, buffers true)
patchestime (ms)buffers
---
unpatched6501316
patch 10.521316
patches 1+20.501316
patches 1+2+30.1315
So, the second patch isn't doing much in this particular case. But it's
trivial, and I think it will make a difference in other queries where
you have the opportunity skip, but return a lot of tuples overall.
In summary, these are fairly small patches, and useful on their, so I
think these should be committed now. But please take a look and see if
the logic in scanGetItem/keyGetItem looks correct to you. After this, I
think the main fast scan logic will go into keyGetItem.
Hi,
I've done some testing of the three patches today, and I've ran into an
infinite loop caused by the third patch. I don't know why exactly it
gets stuck, but with patches #1+#2 it works fine, and after applying #3
it runs infinitely.
I can't point to a particular line / condition causing this, but this is
wthat I see in 'perf top'
54.16% postgres [.] gingetbitmap
32.38% postgres [.] ginPostingListDecodeAllSegments
3.03% libc-2.17.so [.] 0x0007fb88
I've tracked it down to this loop in ginget.c:840 (I've added the
logging for debugging / demonstration purposes):
=
elog(WARNING, scanning entries);
elog(WARNING, advacepast=(%u,%d),
BlockIdGetBlockNumber(advancePast.ip_blkid),
advancePast.ip_posid);
while (entry-isFinished == FALSE
ginCompareItemPointers(entry-curItem, advancePast) = 0)
{
elog(WARNING, current=(%u,%d),
BlockIdGetBlockNumber(entry-curItem.ip_blkid),
entry-curItem.ip_posid);
entryGetItem(ginstate, entry, advancePast);
}
elog(WARNING, entries scanned);
=
which is executed repeatedly, but the last invocation gets stuck and
produces this output:
WARNING: scanning entries
WARNING: advacepast=(172058,0)
LOG: entryLoadMoreItems, 172058/0, skip: 1
WARNING: getting item current=(171493,7)
WARNING: getting item current=(116833,2)
WARNING: getting item current=(116833,3)
WARNING: getting item current=(116833,4)
WARNING: getting item current=(116833,5)
WARNING: getting item current=(116838,1)
WARNING: getting item current=(116838,2)
... increasing sequence of block IDs ...
WARNING: getting item current=(170743,5)
WARNING: getting item current=(170746,4)
WARNING: getting item current=(171493,7)
LOG: entryLoadMoreItems, 172058/0, skip: 1
WARNING: getting item current=(116833,2)
WARNING: getting item current=(116833,3)
WARNING: getting item current=(116833,4)
WARNING: getting item current=(116833,5)
... and repeat
=
Not sure what went wrong, though - I suspect it does not set the
isFinished flag or something like that, but I don't know where/when
should that happen.
This is rather easy to reproduce - download the dump I already provided
two weeks ago [http://www.fuzzy.cz/tmp/message-b.data.gz] and load it
into a simple table:
CREATE TABLE msgs (body tsvector);
COPY msgs FROM '/tmp/message-b.data';
CREATE INDEX msgidx ON msgs USING gin(body);
ANALYZE msgs;
And then run this query:
SELECT body FROM msgs
WHERE body @@ plainto_tsquery('english','string | x')
AND body @@ plainto_tsquery('english','versions | equivalent')
AND body @@ plainto_tsquery('english','usually | contain');
It should run infinitely. I suspect it's not perfectly stable, i.e, the
this query may work fine / another one will block. In that case try to
run this [http://www.fuzzy.cz/tmp/random-queries.sql] - it's a file with
1000 generated queries, at least one of them should block (that's how I
discovered the issue).
regards
Tomas
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Questionable coding in orderedsetaggs.c
Jeremy Harris j...@wizmail.org writes: In ordered_set_startup() sorts are initialised in non-randomAccess mode (tuplesort_begin_heap() and ~datum(), last argument). The use of tuplesort_skip_tuples() feels very like a random access to me. I think it doesn't fail because the only use (and implementation) is to skip forwards; if backwards were tried (as the interface permits) external sorts would fail because multiple tapes are present for FINALMERGE. Well, we certainly don't want to incur the overhead of randomAccess mode when we're not actually going to use it, so I'd resist changing the code in ordered_set_startup(). It's true that if tuplesort_skip_tuples() supported backwards skip, it would need to insist that randomAccess mode be enabled *when a backwards skip is used*. But such a feature is purely hypothetical ATM. 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
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
On Sat, Jan 25, 2014 at 11:04 AM, Tom Lane t...@sss.pgh.pa.us wrote: Do the regression tests fail for you when doing this? What I see is a duplicate occurrence of an escape_string_warning bleat: *** /home/postgres/pgsql/src/test/regress/expected/plpgsql.out Fri Jan 3 17:07 :46 2014 --- /home/postgres/pgsql/src/test/regress/results/plpgsql.out Sat Jan 25 13:37 :20 2014 *** *** 4568,4573 --- 4568,4579 HINT: Use the escape string syntax for backslashes, e.g., E'\\'. QUERY: SELECT 'foo\\bar\041baz' CONTEXT: PL/pgSQL function strtest() line 4 at RETURN + WARNING: nonstandard use of \\ in a string literal + LINE 1: SELECT 'foo\\bar\041baz' +^ + HINT: Use the escape string syntax for backslashes, e.g., E'\\'. + QUERY: SELECT 'foo\\bar\041baz' + CONTEXT: PL/pgSQL function strtest() line 4 at RETURN strtest - foo\bar!baz == which seems to happen because generate_normalized_query() reruns the core lexer on the given statement, and if you got a warning the first time, you'll get another one. Oh, yes, I noticed that and reached the same conclusion. Sorry, I probably should have mentioned this pro-actively. -- 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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote:
D'Arcy J.M. Cain da...@druid.net
Although, the more I think about it, the more I think that the comment
is both confusing and superfluous. The code itself is much clearer.
Seriously, if there is any comment there at all, it should be a
succinct explanation for why we didn't do this (which passes `make
check-world`):
Is everyone OK with me applying this patch from Kevin, attached?
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
new file mode 100644
index aea9d40..7616cfc
*** a/src/backend/access/common/heaptuple.c
--- b/src/backend/access/common/heaptuple.c
*** bool
*** 1321,1327
slot_attisnull(TupleTableSlot *slot, int attnum)
{
HeapTuple tuple = slot-tts_tuple;
! TupleDesc tupleDesc = slot-tts_tupleDescriptor;
/*
* system attributes are handled by heap_attisnull
--- 1321,1328
slot_attisnull(TupleTableSlot *slot, int attnum)
{
HeapTuple tuple = slot-tts_tuple;
!
! Assert(attnum = slot-tts_tupleDescriptor-natts);
/*
* system attributes are handled by heap_attisnull
*** slot_attisnull(TupleTableSlot *slot, int
*** 1342,1353
return slot-tts_isnull[attnum - 1];
/*
- * return NULL if attnum is out of range according to the tupdesc
- */
- if (attnum tupleDesc-natts)
- return true;
-
- /*
* otherwise we had better have a physical tuple (tts_nvalid should equal
* natts in all virtual-tuple cases)
*/
--- 1343,1348
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote: On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote: D'Arcy J.M. Cain da...@druid.net Although, the more I think about it, the more I think that the comment is both confusing and superfluous. The code itself is much clearer. Seriously, if there is any comment there at all, it should be a succinct explanation for why we didn't do this (which passes `make check-world`): Is everyone OK with me applying this patch from Kevin, attached? No. I still think this is stupid. Not at all clearer and possibly breaks stuff. 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
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Sat, Jan 25, 2014 at 10:29:36PM +0100, Andres Freund wrote: On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote: On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote: D'Arcy J.M. Cain da...@druid.net Although, the more I think about it, the more I think that the comment is both confusing and superfluous. The code itself is much clearer. Seriously, if there is any comment there at all, it should be a succinct explanation for why we didn't do this (which passes `make check-world`): Is everyone OK with me applying this patch from Kevin, attached? No. I still think this is stupid. Not at all clearer and possibly breaks stuff. OK, how about if we change the comment to this: /* -- * assume NULL if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in PL/pgSQL function which CREATE/SELECT/DROP a temporary table
On Tue, Jun 18, 2013 at 09:07:59PM +0300, Heikki Linnakangas wrote: Hmm. I could repeat this, and it seems that the catcache for pg_statistic accumulates negative cache entries. Those slowly take up the memory. Digging a bit deeper, this is a rather common problem with negative catcache entries. In general, nothing stops you from polluting the cache with as many negative cache entries as you like. Just do select * from table_that_doesnt_exist for as many non-existent table names as you want, for example. Those entries are useful at least in theory; they speed up throwing the error the next time you try to query the same non-existent table. But there is a crucial difference in this case; the system created a negative cache entry for the pg_statistic row of the table, but once the relation is dropped, the cache entry keyed on the relation's OID, is totally useless. It should be removed. We have this problem with a few other catcaches too, which have what is effectively a foreign key relationship with another catalog. For example, the RELNAMENSP catcache is keyed on pg_class.relname, pg_class.relnamespace, yet any negative entries are not cleaned up when the schema is dropped. If you execute this repeatedly in a session: CREATE SCHEMA foo; SELECT * from foo.invalid; -- throws an error DROP SCHEMA foo; it will leak similarly to the original test case, but this time the leak is into the RELNAMENSP catcache. To fix that, I think we'll need to teach the catalog cache about the relationships between the caches. Is this a TODO? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
On 2014-01-25 16:33:16 -0500, Bruce Momjian wrote: On Sat, Jan 25, 2014 at 10:29:36PM +0100, Andres Freund wrote: On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote: On Tue, Jun 18, 2013 at 11:04:25AM -0700, Kevin Grittner wrote: D'Arcy J.M. Cain da...@druid.net Although, the more I think about it, the more I think that the comment is both confusing and superfluous. The code itself is much clearer. Seriously, if there is any comment there at all, it should be a succinct explanation for why we didn't do this (which passes `make check-world`): Is everyone OK with me applying this patch from Kevin, attached? No. I still think this is stupid. Not at all clearer and possibly breaks stuff. OK, how about if we change the comment to this: /* -- * assume NULL if attnum is out of range according to the tupdesc */ if (attnum tupleDesc-natts) return true; I don't think it improves things relevantly, but it doesn't make anything worse either. So if that makes anybody happy... I think this style of pinhole copy editing is pretty pointless. There's dozen checks just like this around. If somebody wants to change the rules or improve comment it takes more than picking a random one. 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
Re: [HACKERS] Postgresql for cygwin - 3rd
On 25/01/2014 19:23, Andrew Dunstan wrote: On 01/24/2014 07:50 AM, Marco Atzeri wrote: Those two issues need to be fixed. And yes, they are regressions from my Cygwin 1.7.7 setup where they pass consistently, just about every day. See http://www.pgbuildfarm.org/cgi-bin/show_history.pl?nm=brolgabr=HEAD 1.7.7 is 3.5 years hold. In the mean time we added 64 bit and moved to gcc-4.8.x No doubt, but that doesn't mean that extra things failing is OK. Andrew, I never wrote that. You don't get to choose which regression tests you're going to pass and which you're not. Disabling the tests or providing nonsensical results files are unacceptable. This is a Cygwin behavior issue and needs to be fixed. some indication where to look for in the code will help. Distributing broken binary that crashes after standard rebase, it is also not acceptable and it is also worst. Your farm is not testing this case, I presume. Quite so. But this is not a case of either/or. No, but I spent a lot of time on understanding that DLLTOLL/DLLWRAP produce buggy binaries, and that was a CRITICAL failure. I have now tested the central part of the proposed changes on both old and new Cygwin installations, and they appear to work. I'm going to commit them and backpatch back to 9.0, which is where we currently have buildfarm coverage (and 8.4 will be at EOL in a few months anyway). That will take care of your rebase issue. That leaves several issues to be handled: * LDAP libraries - the way you have proposed surely isn't right. What we want is something more like this in the Makefile.global.in: ifeq ($(PORTNAME), cygwin) libpq_pgport += $(LDAP_LIBS_FE) endif I will test in this way. I have no preferance on the implemented solution. * isolation tests fail with an indefinite hang on newer Cygwin * prepared_xacts test fails with an indefinite hang on newer Cygwin if run in parallel with other tests It hangs also stand alone. I guess some race or deadlock issue. * tsearch tests fail on non-C locale (or at least on en_US.utf8). It turns out this is actually an old bug, and can be reproduced on my old Cygwin instance. I wonder if it's caused by faulty locale files? Tested tsearch with export LANG=C works export LANG=C.utf8 fails export LANG=it_IT works export LANG=it_IT.utf8 fails faulty locale or wrong assumption on utf8 implementation ? Really, for a properly working port all these need to be fixed. No objection. Step by step I am available to work on tests regression, but I am not a postgresql expert so do not expect all the job from my side. We can help you some, but very few people in the community run Cygwin, and my time to spend on it is fairly limited. For the time being, I can run tests and builds. cheers andrew cheers Marco -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote: I don't think it improves things relevantly, but it doesn't make anything worse either. So if that makes anybody happy... I think this style of pinhole copy editing is pretty pointless. There's dozen checks just like this around. If somebody wants to change the rules or improve comment it takes more than picking a random one. OK, change made. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What is happening on buildfarm member crake?
On 01/19/2014 08:22 PM, Robert Haas wrote: Hmm, that looks an awful lot like the SIGUSR1 signal handler is getting called after we've already completed shmem_exit. And indeed that seems like the sort of thing that would result in dying horribly in just this way. The obvious fix seems to be to check proc_exit_inprogress before doing anything that might touch shared memory, but there are a lot of other SIGUSR1 handlers that don't do that either. However, in those cases, the likely cause of a SIGUSR1 would be a sinval catchup interrupt or a recovery conflict, which aren't likely to be so far delayed that they arrive after we've already disconnected from shared memory. But the dynamic background workers stuff adds a new possible cause of SIGUSR1: the postmaster letting us know that a child has started or died. And that could happen even after we've detached shared memory. Is anything happening about this? We're still getting quite a few of these: http://www.pgbuildfarm.org/cgi-bin/show_failures.pl?max_days=3member=crake cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
Andres Freund and...@2ndquadrant.com writes: On 2014-01-25 16:28:09 -0500, Bruce Momjian wrote: Is everyone OK with me applying this patch from Kevin, attached? No. I still think this is stupid. Not at all clearer and possibly breaks stuff. I agree; this patch is flat out wrong. It converts a valid situation which is correctly handled into an Assert trap, or probably a core dump in a non-assert build. 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
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
Uh, were are we on this? Is it a TODO? --- On Wed, Jul 3, 2013 at 01:39:28PM +0530, Atri Sharma wrote: Hi all, I have been working on a patch for the above discussed functionalities. I made an array of int32s, one for each bucket in a hash table(the array is per hash table). I am using the hash value directly to set the corresponding bit in the bit field.Specifically: bitfield_orvalue = 1hashvalue; hashtable-bitFields[bucketNumber] = (hashtable-bitFields[bucketNumber]) |bitfield_orvalue; But,the hash values are way beyond this, and I admit that my choice of int32 as bitfield isn't correct here. The hash values are like: 1359910425 1359910425 -1662820951 -1662820951 What should I be using for the bit map? Regards, Atri -- Regards, Atri l'apprenant -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A minor correction in comment in heaptuple.c
Bruce Momjian br...@momjian.us writes: On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote: I think this style of pinhole copy editing is pretty pointless. There's dozen checks just like this around. If somebody wants to change the rules or improve comment it takes more than picking a random one. OK, change made. FWIW, I don't find that an improvement either. As Andres says, this is just applying the same rule that's used in many other places, ie return null if the requested attnum is off the end of the tuple. 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
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
* Bruce Momjian (br...@momjian.us) wrote: Uh, were are we on this? Is it a TODO? I've been strongly considering my previous patch which tweaked NTUP_PER_BUCKET to '1' (instead of the default '10') when there's sufficient work_mem for it. There was recently another complaint on IRC about our tendency to hash the larger partition rather than the smaller one which I believe would be resolved by doing so. The main thing holding me back has been concern that there may be cases which perform worse with the change, either because hashing the larger partition actually ended up being faster or due to the increase in memory usage. In the end, I believe we absolutely should do something about this. Hashing a 64M-row table (requiring upwards of 8G) instead of hashing a 2M-row table is really bad of us. Thoughts? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Jan25, 2014, at 09:50 , David Rowley dgrowle...@gmail.com wrote: On Thu, Jan 23, 2014 at 1:57 PM, Florian Pflug f...@phlo.org wrote: On Jan23, 2014, at 01:17 , David Rowley dgrowle...@gmail.com wrote: On Wed, Jan 22, 2014 at 12:46 AM, Florian Pflug f...@phlo.org wrote: If you want to play with this, I think the first step has to be to find a set of guarantees that SUM(float) is supposed to meet. Currently, SUM(float) guarantees that if the summands all have the same sign, the error is bound by C * N, where C is some (machine-specific?) constant (about 1e-15 or so), and N is the number of input rows. Or at least so I think from looking at SUMs over floats in descending order, which I guess is the worst case. You could, for example, try to see if you can find a invertibility conditions which guarantees the same, but allows C to be larger. That would put a bound on the number of digits lost by the new SUM(float) compared to the old one. I guess if the case is that normal (non-window) sum(float) results are undefined unless you add an order by to the aggregate then I guess there is no possible logic to put in for inverse transitions that will make them behave the same as an undefined behaviour. Actually, if sum(float) was really undefined, it'd be *very* easy to provide an inverse transition function - just make it a no-op. Heck, you could then even make the forward transition function a no-op, since the very definition of undefined behaviour is result can be anything, including setting your house on fire. The point is, it's *not* undefined - it's just imprecise. And the imprecision can even be quantified, it just depends on the number of input rows (the equal-sign requirement is mostly there to make imprecision equivalent to relative error). My apologies, I meant to use the term nondeterministic rather than undefined. There's really not any need that I can see to turn things silly here. I wasn't trying to be absurd, I was trying to get a point across. My point was more that since sum(float) can give different results if it used an index scan rather than a seq scan, trying to get the inverse transition to match something that gives varying results sounds like a tricky task to take on. You don't have to match it digit-by-digit! In that I fully agree with Kevin - floats are *always* an approximation, and so is thus SUM(float). Summarization order is BTW not the only source of non-determinism for SUM(float) - the exact result can very between architectures, and for some architectures between compilers. (Intel is one of these, due to their 80-bit extended precision format that gets truncated to 64-bit when stored to main memory). But in a large number of cases, they won't vary by *much*, which is *the* reason why SUM(float) is *not* totally useless. And reasoning about SUM(float) which ignores that by simply calling it non-deterministic, undefined or whatever, without any quantification of the possible error, has thus zero chance of leading to interesting conclusions. Secondly, you'd really need a second aggregate definition - usually, the non-invertible aggregate will get away with a much smaller state representation. Aggregates with state type internal could hack their way around that by simply not using the additional parts of their state structure, but e.g. plpgsql aggregates would still incur overhead due to repeated copying of a unnecessary large state value. If it's possible to represent the forward-only but not the invertible state state with a copy-by-value type, that overhead could easily be a large part of the total overhead you paid for having an inverse transition function. I had imagined they keep the same state type and don't use any extra variables that are defined for the forward transition function that is invertible. Yeah, and the above explains that at least for non-C-language aggregates, passing around that unnecessarily large state may very well prove to be the source of a large part, if not almost all, of the overhead. So having just a separate forward transition function will buy you almost nothing or some cases. I just tried this. I defined two aggregates mymax(int4) and myfastmax(int4), both with just a forward transition function, both SQL-language functions. mymax uses a composite type for the state containing an int4 field holding the current maximum, and a dummy int8 field. myfastmax uses a plain int4 for the state, holding the current maximum. Both forward transition functions essentially do case when current_max v then current_max else v end On my laptop, computing the maximum of 1e6 rows takes about 4.5 seconds with myfastmax and 7.8 seconds with mymax. If make mymax's transition function increment the dummy field on every transition, the time increases from 7.8 to 8.2 seconds. So here, using a composite type for the state accounts for about 3.3 seconds, or 40%,
Re: [HACKERS] What is happening on buildfarm member crake?
Andrew Dunstan and...@dunslane.net writes: On 01/19/2014 08:22 PM, Robert Haas wrote: Hmm, that looks an awful lot like the SIGUSR1 signal handler is getting called after we've already completed shmem_exit. And indeed that seems like the sort of thing that would result in dying horribly in just this way. The obvious fix seems to be to check proc_exit_inprogress before doing anything that might touch shared memory, but there are a lot of other SIGUSR1 handlers that don't do that either. However, in those cases, the likely cause of a SIGUSR1 would be a sinval catchup interrupt or a recovery conflict, which aren't likely to be so far delayed that they arrive after we've already disconnected from shared memory. But the dynamic background workers stuff adds a new possible cause of SIGUSR1: the postmaster letting us know that a child has started or died. And that could happen even after we've detached shared memory. Is anything happening about this? We're still getting quite a few of these: http://www.pgbuildfarm.org/cgi-bin/show_failures.pl?max_days=3member=crake Yeah. If Robert's diagnosis is correct, and it sounds pretty plausible, then this is really just one instance of a bug that's probably pretty widespread in our signal handlers. Somebody needs to go through 'em all and look for touches of shared memory. I'm not sure if we can just disable signal response the moment the proc_exit_inprogress flag goes up, though. In some cases such as lock handling, it's likely that we need that functionality to keep working for some part of the shutdown process. We might end up having to disable individual signal handlers at appropriate places. Ick. 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
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
On Sat, Jan 25, 2014 at 1:11 PM, Tom Lane t...@sss.pgh.pa.us wrote: I've chosen to handle failures to load query text data by just returning NULL for that query text, which seems reasonable given the new mindset that the query text is auxiliary data less important than the actual counts. I guess that's okay. I've not done much more than smoke-testing on this; I'm thinking about hot-wiring the code to sometimes force it through the error paths, just to make sure they act as expected. I've also not done any real performance testing. I was never too worried about the impact on performance, because any price that must be paid is paid only when new entries are created, which is typically a very rare event that was already expensive due to requiring an exclusive lock. This patch will make it much rarer in practice by increasing the pg_stat_statements.max default, and thus reducing the impact on such exclusive locks on *all* sessions, not just those executing less popular queries. I don't know about you, but the reason that I didn't performance test this is that it's really hard to think of a representative scenario where it could possibly matter, even though it seems like there might be one. -- 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
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
Why do you think it's better to release the shared lock while generating a normalized query text, only to acquire it once more? I'm not suggesting that it's the wrong thing to do. I'm curious about the reasoning around assessing the costs. -- 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
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
Stephen Frost sfr...@snowman.net writes: * Bruce Momjian (br...@momjian.us) wrote: Uh, were are we on this? Is it a TODO? I've been strongly considering my previous patch which tweaked NTUP_PER_BUCKET to '1' (instead of the default '10') when there's sufficient work_mem for it. There was recently another complaint on IRC about our tendency to hash the larger partition rather than the smaller one which I believe would be resolved by doing so. The main thing holding me back has been concern that there may be cases which perform worse with the change, either because hashing the larger partition actually ended up being faster or due to the increase in memory usage. In the end, I believe we absolutely should do something about this. Hashing a 64M-row table (requiring upwards of 8G) instead of hashing a 2M-row table is really bad of us. Huh? I don't see anything in the thread suggesting that we're doing that, or that changing NTUP_PER_BUCKET would fix it if we do. Are you thinking of some other discussion? AFAICT, there was no consensus in this thread on what to do, which probably has something to do with the lack of concrete performance tests presented to back up any particular proposal. 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
Re: [HACKERS] [COMMITTERS] pgsql: libpq: Support TLS versions beyond TLSv1.
* Noah Misch (n...@leadboat.com) wrote: +1. If you can upgrade to 9.4, you can also bring your TLS protocol out of the iron age. Agreed- this was going to be my 2c. Anyone w/ an SSL library that old isn't likely to be upgrading to 9.4 of libpq or PG. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] A minor correction in comment in heaptuple.c
On Sat, Jan 25, 2014 at 04:56:37PM -0500, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote: I think this style of pinhole copy editing is pretty pointless. There's dozen checks just like this around. If somebody wants to change the rules or improve comment it takes more than picking a random one. OK, change made. FWIW, I don't find that an improvement either. As Andres says, this is just applying the same rule that's used in many other places, ie return null if the requested attnum is off the end of the tuple. OK, I can revert it, but I don't see any other cases of the string 'return NULL if' in the executor code. What the code really is doing is Assume NULL so return true if. The code was never returning NULL, it was assuming the attribute was NULL and returning true. Am I missing something? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
Peter Geoghegan p...@heroku.com writes: Why do you think it's better to release the shared lock while generating a normalized query text, only to acquire it once more? I'm not suggesting that it's the wrong thing to do. I'm curious about the reasoning around assessing the costs. Well, it's fairly expensive to generate that text, in the case of a large/complex statement. It's possible that continuing to hold the lock is nonetheless the right thing to do because release+reacquire is also expensive; but there is no proof of that AFAIK, and I believe that release+reacquire is not likely to be expensive unless the lock is heavily contended, which would be exactly the conditions under which keeping it wouldn't be such a good idea anyway. So I'd prefer to leave it doing what it did before, until there's some concrete evidence that keeping the lock is a better idea. 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
Re: [HACKERS] Changeset Extraction v7.1
Hi Robert, all, On 2014-01-24 20:38:11 -0500, Robert Haas wrote: On Fri, Jan 24, 2014 at 12:49 PM, Andres Freund and...@2ndquadrant.com wrote: But this code is riddled with places where you track a catalog xmin and a data xmin separately. The only point of doing it that way is to support a division that hasn't been made yet. If you think it will make stuff more manageable I can try separating all lines dealing with catalog_xmin into another patch as long as data_xmin doesn't have to be renamed. That said, I don't really think it's a big problem that the division hasn't been made, essentially the meaning is different, even if we don't take advantage of it yet. data_xmin is there for streaming replication where we need to prevent all removal, catalog_xmin is there for changeset extraction. I spent some more time studying the 0001 and 0002 patches this afternoon, with a side dish of 0004. I'm leaning toward thinking we should go ahead and make that division. Ok. I'm also wondering about whether we've got the right naming here. AFAICT, it's not the case that we're going to use the catalog xmin for catalogs and the data xmin for non-catalogs. Rather, the catalog xmin is going to always be included in globalxmin calculations, so IOW it should just be called xmin. Well, not really. That's true for GetSnapshotData(), but not for GetOldestXmin() where we calculate an xmin *not* including the catalog xmin. And the data_xmin is always used, regardless of catalog/non_catalog, we peg the xmin further for catalog tables, based on that value. The reason for doing things this way is that it makes all current usages of RecentGlobalXmin safe, since that is the more conservative value. Only in inspected location we can use RecentGlobalDataXmin which *does* include data_xmin but *not* catalog_xmin. It's interesting (though not immediately relevant) to speculate about how we might extend this to fine-grained xmin tracking more generally. [musings for another time] Yea, I have wondered about that as well. I think the easiest thing would be to to compute RecentGlobalDataXmin in a database specific manner since by definition it will *not* include shared tables. We do that already for GetOldestXmin() but that's not used for heap pruning. I'd quickly tested that some months back and it gave significant speedups for two pgbenches in two databases. I have zero confidence that it's OK to treat fsync() as an operation that won't fail. Linux documents EIO as a plausible error return, for example. (And really, how could it not?) But quite fundamentally having a the most basic persistency building block fail is something you can't really handle safely. Note that issue_xlog_fsync() has always (and I wager, will always) treated that as a PANIC. I don't recall many complaints about that for WAL. All of the ones I found in a quick search were like oh, the fs invalidated my fd because of corruption. And few. Well, you have a point. And certainly this version looks much better than the previous version in terms of the likelihood of PANIC occurring in practice. But I wonder if we couldn't cut it down even further without too much effort. Suppose we define a slot to exist if, and only if, the state file exists. A directory without a state file is subject to arbitrary removal. Then we can proceed as follows: - mkdir() the directory. - open() state.tmp - write() state.tmp - close() state.tmp - fsync() parent directory, directory and state.tmp - rename() state.tmp to state - fsync() state If any step except the last one fails, no problem. The next startup can nuke the leftovers; any future attempt to create a slot with the name can ignore an EEXIST failure from mkdir() and procedure just as above. Only a failure of the very last fsync is a PANIC. In some ways I think this'd be simpler than what you've got now, because we'd eliminate the dance with renaming the directory as well as the state file; only the state file matters. Hm. I think this is pretty exactly what happens in the current patch, right? There's an additional fsync() of the parent directory at the end, but that's it. To drop a slot, just unlink the state file and fsync() the directory. If the unlink fails, it's just an error. If the fsync() fails, it's a PANIC. Once the state file is gone, removing everything else is only an ERROR, and you don't even need to fsync() it again. Well, the patch as is renames the directory first and fsyncs that. Only a failure in fsyncing is punishable by PANIC, if rmtree() on the temp directory file fails it generates WARNINGs, that's it. To update a slot, open, write, close, and fsync state.tmp, then rename it to state and fsync() again. None of these steps need PANIC; hold off on updating the values in memory until they're all done. If any step fails, the attempt to update the slot fails, but either memory and disk are still
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: In the end, I believe we absolutely should do something about this. Hashing a 64M-row table (requiring upwards of 8G) instead of hashing a 2M-row table is really bad of us. Huh? I don't see anything in the thread suggesting that we're doing that, or that changing NTUP_PER_BUCKET would fix it if we do. Are you thinking of some other discussion? This thread sprung from or was at least related to, as I recall, my issues with NTUP_PER_BUCKET over the summer. Perhaps I'm wrong, but here's the thread I was referring to: http://www.postgresql.org/message-id/20130404201612.gm4...@tamriel.snowman.net Where I demonstrated that we decide to hash a much larger table, rather than the smaller one, based on the estimated depth of the buckets and including the costing from that, which is driven based on how big we decide to make the hash table where we use NTUP_PER_BUCKET to pick a table size much smaller than we really should be. AFAICT, there was no consensus in this thread on what to do, which probably has something to do with the lack of concrete performance tests presented to back up any particular proposal. This I entirely agree with- more testing and more information on how such a change impacts other workloads would be great. Unfortunately, while I've provided a couple of test cases and seen similar situations on IRC, this is very data-dependent which makes it difficult to have concrete answers for every workload. Still, I'll try and spend some time w/ pg_bench's schema definition and writing up some larger queries to run through it (aiui, the default set of queries won't typically result in a hashjoin) and see what happens there. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Postgresql for cygwin - 3rd
On 25/01/2014 22:42, Marco Atzeri wrote: On 25/01/2014 19:23, Andrew Dunstan wrote: On 01/24/2014 07:50 AM, Marco Atzeri wrote: * LDAP libraries - the way you have proposed surely isn't right. What we want is something more like this in the Makefile.global.in: ifeq ($(PORTNAME), cygwin) libpq_pgport += $(LDAP_LIBS_FE) endif I will test in this way. I have no preferance on the implemented solution. your proposal builds fine. --- origsrc/postgresql-9.3.2/src/Makefile.global.in 2013-12-02 21:57:48.0 +0100 +++ src/postgresql-9.3.2/src/Makefile.global.in 2014-01-25 22:46:36.484816700 +0100 @@ -508,6 +508,11 @@ ifeq ($(PORTNAME),win32) LIBS += -lws2_32 -lshfolder endif +# missing for link on cygwin ? +ifeq ($(PORTNAME),cygwin) +libpq_pgport += $(LDAP_LIBS_FE) +endif + # Not really standard libc functions, used by the backend. Of course no difference on test results, as expected Attached full patch as I am currently testing on 9.3.2 Regards Marco --- origsrc/postgresql-9.3.2/src/Makefile.global.in 2013-12-02 21:57:48.0 +0100 +++ src/postgresql-9.3.2/src/Makefile.global.in 2014-01-25 22:46:36.484816700 +0100 @@ -508,6 +508,11 @@ ifeq ($(PORTNAME),win32) LIBS += -lws2_32 -lshfolder endif +# missing for link on cygwin ? +ifeq ($(PORTNAME),cygwin) +libpq_pgport += $(LDAP_LIBS_FE) +endif + # Not really standard libc functions, used by the backend. TAS = @TAS@ --- origsrc/postgresql-9.3.2/src/Makefile.shlib 2013-12-02 21:57:48.0 +0100 +++ src/postgresql-9.3.2/src/Makefile.shlib 2014-01-24 23:05:08.601675200 +0100 @@ -281,8 +281,9 @@ ifeq ($(PORTNAME), unixware) endif ifeq ($(PORTNAME), cygwin) + LINK.shared = $(CC) -shared ifdef SO_MAJOR_VERSION -shlib = cyg$(NAME)$(DLSUFFIX) +shlib = cyg$(NAME)-$(SO_MAJOR_VERSION)$(DLSUFFIX) endif haslibarule = yes endif @@ -371,6 +372,12 @@ else # PORTNAME == cygwin || PORTNAME == # If SHLIB_EXPORTS is set, the rules below will build a .def file from # that. Else we build a temporary one here. +ifeq ($(PORTNAME), cygwin) +$(shlib) $(stlib): $(OBJS) | $(SHLIB_PREREQS) + $(CC) $(CFLAGS) -shared -o $(shlib) -Wl,--out-implib=$(stlib) $(OBJS) $(LDFLAGS) $(LDFLAGS_SL) $(SHLIB_LINK) $(LIBS) $(LDAP_LIBS_BE) + + +else ifeq (,$(SHLIB_EXPORTS)) DLL_DEFFILE = lib$(NAME)dll.def exports_file = $(DLL_DEFFILE) @@ -387,6 +394,7 @@ $(shlib): $(OBJS) $(DLL_DEFFILE) | $(SHL $(stlib): $(shlib) $(DLL_DEFFILE) | $(SHLIB_PREREQS) $(DLLTOOL) --dllname $(shlib) $(DLLTOOL_LIBFLAGS) --def $(DLL_DEFFILE) --output-lib $@ +endif # PORTNAME == cygwin endif # PORTNAME == cygwin || PORTNAME == win32 --- origsrc/postgresql-9.3.2/src/backend/Makefile 2013-12-02 21:57:48.0 +0100 +++ src/postgresql-9.3.2/src/backend/Makefile 2014-01-24 23:05:08.621675200 +0100 @@ -62,18 +62,8 @@ endif ifeq ($(PORTNAME), cygwin) -postgres: $(OBJS) postgres.def libpostgres.a - $(DLLTOOL) --dllname $@$(X) --output-exp $@.exp --def postgres.def - $(CC) $(CFLAGS) $(LDFLAGS) $(LDFLAGS_EX) -o $@$(X) -Wl,--base-file,$@.base $@.exp $(call expand_subsys,$(OBJS)) $(LIBS) - $(DLLTOOL) --dllname $@$(X) --base-file $@.base --output-exp $@.exp --def postgres.def - $(CC) $(CFLAGS) $(LDFLAGS) $(LDFLAGS_EX) -Wl,--stack,$(WIN32_STACK_RLIMIT) -o $@$(X) $@.exp $(call expand_subsys,$(OBJS)) $(LIBS) - rm -f $@.exp $@.base - -postgres.def: $(OBJS) - $(DLLTOOL) --export-all --output-def $@ $(call expand_subsys,$^) - -libpostgres.a: postgres.def - $(DLLTOOL) --dllname postgres.exe --def postgres.def --output-lib $@ +postgres libpostgres.a: $(OBJS) + $(CC) $(CFLAGS) $(LDFLAGS) $(LDFLAGS_EX) $(export_dynamic) $(call expand_subsys,$^) $(LIBS) -o $@ -Wl,--stack,$(WIN32_STACK_RLIMIT) -Wl,--export-all-symbols -Wl,--out-implib=libpostgres.a endif # cygwin --- origsrc/postgresql-9.3.2/src/interfaces/libpq/Makefile 2013-12-02 21:57:48.0 +0100 +++ src/postgresql-9.3.2/src/interfaces/libpq/Makefile 2014-01-24 23:05:08.621675200 +0100 @@ -45,7 +45,7 @@ OBJS += ip.o md5.o OBJS += encnames.o wchar.o ifeq ($(PORTNAME), cygwin) -override shlib = cyg$(NAME)$(DLSUFFIX) +override shlib = cyg$(NAME)-$(SO_MAJOR_VERSION)$(DLSUFFIX) endif ifeq ($(PORTNAME), win32) --- origsrc/postgresql-9.3.2/src/makefiles/Makefile.cygwin 2013-12-02 21:57:48.0 +0100 +++ src/postgresql-9.3.2/src/makefiles/Makefile.cygwin 2014-01-24 23:05:08.641675200 +0100 @@ -1,6 +1,4 @@ # src/makefiles/Makefile.cygwin -DLLTOOL= dlltool -DLLWRAP= dllwrap ifdef PGXS BE_DLLLIBS= -L$(libdir) -lpostgres else @@ -44,6 +42,4 @@ endif # Rule for building a shared library from a single .o file %.dll: %.o - $(DLLTOOL) --export-all --output-def $*.def $ -
Re: [HACKERS] A minor correction in comment in heaptuple.c
On 2014-01-25 17:15:01 -0500, Bruce Momjian wrote: On Sat, Jan 25, 2014 at 04:56:37PM -0500, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Sat, Jan 25, 2014 at 10:40:28PM +0100, Andres Freund wrote: I think this style of pinhole copy editing is pretty pointless. There's dozen checks just like this around. If somebody wants to change the rules or improve comment it takes more than picking a random one. OK, change made. FWIW, I don't find that an improvement either. As Andres says, this is just applying the same rule that's used in many other places, ie return null if the requested attnum is off the end of the tuple. OK, I can revert it, but I don't see any other cases of the string 'return NULL if' in the executor code. What the code really is doing is Assume NULL so return true if. The code was never returning NULL, it was assuming the attribute was NULL and returning true. Am I missing something? The friggin function in which you whacked around the comment is called slot_attisnull(). Referring to the functions meaning in a comment above an early return isn't a novel thing. Just search for attnum tupleDesc-natts to find lots of similar chunks of code, several of them even in the same file. 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
Re: [HACKERS] What is happening on buildfarm member crake?
On 01/25/2014 05:04 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 01/19/2014 08:22 PM, Robert Haas wrote: Hmm, that looks an awful lot like the SIGUSR1 signal handler is getting called after we've already completed shmem_exit. And indeed that seems like the sort of thing that would result in dying horribly in just this way. The obvious fix seems to be to check proc_exit_inprogress before doing anything that might touch shared memory, but there are a lot of other SIGUSR1 handlers that don't do that either. However, in those cases, the likely cause of a SIGUSR1 would be a sinval catchup interrupt or a recovery conflict, which aren't likely to be so far delayed that they arrive after we've already disconnected from shared memory. But the dynamic background workers stuff adds a new possible cause of SIGUSR1: the postmaster letting us know that a child has started or died. And that could happen even after we've detached shared memory. Is anything happening about this? We're still getting quite a few of these: http://www.pgbuildfarm.org/cgi-bin/show_failures.pl?max_days=3member=crake Yeah. If Robert's diagnosis is correct, and it sounds pretty plausible, then this is really just one instance of a bug that's probably pretty widespread in our signal handlers. Somebody needs to go through 'em all and look for touches of shared memory. I'm not sure if we can just disable signal response the moment the proc_exit_inprogress flag goes up, though. In some cases such as lock handling, it's likely that we need that functionality to keep working for some part of the shutdown process. We might end up having to disable individual signal handlers at appropriate places. Ick. Yeah. Since we're now providing for user-defined backends, maybe we need some mechanism for white-listing handlers that can run in whole or part during shutdown. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On 25 January 2014 22:33, Stephen Frost sfr...@snowman.net wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: AFAICT, there was no consensus in this thread on what to do, which probably has something to do with the lack of concrete performance tests presented to back up any particular proposal. This I entirely agree with- more testing and more information on how such a change impacts other workloads would be great. Unfortunately, while I've provided a couple of test cases and seen similar situations on IRC, this is very data-dependent which makes it difficult to have concrete answers for every workload. Still, I'll try and spend some time w/ pg_bench's schema definition and writing up some larger queries to run through it (aiui, the default set of queries won't typically result in a hashjoin) and see what happens there. The case that action of some kind was needed was clear, for me. Hopefully some small improvement can be found from that investigation, even if the greatest gain is in some way under dispute. -- Simon Riggs 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
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Sat, Jan 25, 2014 at 3:21 PM, Florian Pflug f...@phlo.org wrote: On Jan24, 2014, at 08:47 , Dean Rasheed dean.a.rash...@gmail.com wrote: The invtrans_minmax patch doesn't contain any patches yet - David, could you provide some for these functions, and also for bool_and and bool_or? We don't need to test every datatype, but a few would be nice. I've added a few regression tests for min, min and bool_or and bool_and. I've pushed these up to here: https://github.com/david-rowley/postgres/commits/invtrans_minmax I did wonder if you'd want to see uses of FILTER in there as I'm thinking that should really be covered by the core patch and the tests here really should just be checking the inverse transition functions for min and max. As for the data types tested, I ended just adding tests for int and text for min and max. Let me know if you think that more should be tested. As for bool_or and bool_and. I didn't think there was much extra that would need tested after I added 1 test. It's pretty simple code and adding anything extra seems like it would be testing something else. Regards David Rowley
Re: [HACKERS] Storing pg_stat_statements query texts externally, pg_stat_statements in core
On Sat, Jan 25, 2014 at 2:20 PM, Tom Lane t...@sss.pgh.pa.us wrote: Well, it's fairly expensive to generate that text, in the case of a large/complex statement. It's possible that continuing to hold the lock is nonetheless the right thing to do because release+reacquire is also expensive; but there is no proof of that AFAIK, and I believe that release+reacquire is not likely to be expensive unless the lock is heavily contended, which would be exactly the conditions under which keeping it wouldn't be such a good idea anyway. My reason for only acquiring the shared lock once, and performing text normalization with it held was that in practice most query texts are not all that expensive to lex/normalize, and the cost of holding the lock for the vast majority of sessions (that won't experience contention) is nil. Acquiring the shared lock twice for something like that struck me as unjustified. I though it was closer to the original coding to lex with the lock, because of course the original coding will only ever acquire the shared lock once. The original lex-with-no-lock coding is only justified by well, might as well. -- 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
Re: [HACKERS] Visual Studio 2013 build
On 12/02/2013 05:12 PM, Brar Piening wrote: Hackers, the attached patch enables Microsoft Visual Studio 2013 as additional build environment. After some tweaking (VS now has got its own rint and a few macro definitions that were previously missing) the build runs without errors or warnings and the product passes the regression tests. I didn't test any special configurations though. I'm using full Visual Studio 2013 actually so I can't conform that everything still works with Visual Studio Express 2013 for Windows Desktop, but there are no documented limitations that make any problems foreseeable. I will add it to the CommitFest 2014-01 so that there is time for testing and tweaking. OK, I have tested this out with the development branch and Visual Studio Express 2013 for Windows Desktop, on Windows Server 2008 R2-SP1 64 bit. With a slight adjustment to make the patch apply it works fine. How far back should we go? About a year ago when we did this we applied it for 9.2 (then the latest stable release) and 9.3dev. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: libpq: Support TLS versions beyond TLSv1.
Stephen Frost escribió: * Noah Misch (n...@leadboat.com) wrote: +1. If you can upgrade to 9.4, you can also bring your TLS protocol out of the iron age. Agreed- this was going to be my 2c. Anyone w/ an SSL library that old isn't likely to be upgrading to 9.4 of libpq or PG. What about people doing SSL connections through JDBC? As far as I understand, these don't use openssl. -- Álvaro Herrerahttp://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
Re: [HACKERS] INTERVAL overflow detection is terribly broken
On Sun, Jun 23, 2013 at 03:00:59PM +0200, Rok Kralj wrote: Hi, after studying ITERVAL and having a long chat with RhoidumToad and StuckMojo on #postgresql, I am presenting you 3 bugs regarding INTERVAL. OK, I am going to merge this with the previous report/patch which fixes: SELECT INTERVAL '20 years'; ERROR: interval out of range LINE 1: SELECT INTERVAL '20 years'; and SELECT INTERVAL '20-3 years'; ERROR: interval field value out of range: 20-3 years LINE 1: SELECT INTERVAL '20-3 years'; As far as I understand, the Interval struct (binary internal representation) consists of: int32 months int32 days int64 microseconds 1. OUTPUT ERRORS: Since 2^63 microseconds equals 2,562,047,788 2^31 hours, the overflow in pg_tm when displaying the value causes overflow. The value of Interval struct is actually correct, error happens only on displaying it. SELECT INTERVAL '2147483647 hours' + INTERVAL '5 hours' -2147483644:00:00 Fixed: SELECT INTERVAL '2147483647 hours' + INTERVAL '5 hours'; ERROR: interval out of range Even wireder: SELECT INTERVAL '2147483647 hours' + '1 hour' --2147483648:00:00 Fixed: SELECT INTERVAL '2147483647 hours' + '1 hour'; ERROR: interval out of range notice the double minus? Don't ask how I came across this two bugs. 2. OPERATION ERRORS: When summing two intervals, the user is not notified when overflow occurs: SELECT INT '2147483647' + INT '1' ERROR: integer out of range SELECT INTERVAL '2147483647 days' + INTERVAL '1 day' -2147483648 days This should be analogous. Fixed: SELECT INTERVAL '2147483647 days' + INTERVAL '1 day'; ERROR: interval out of range 3. PARSER / INPUT ERRORS: This is perhaps the hardest one to explain, since this is an architectural flaw. You are checking the overflows when parsing string - pg_tm struct. However, at this point, the parser doesn't know, that weeks and days are going to get combined, or years are going to get converted to months, for example. Unawarness of underlying Interval struct causes two types of suberrors: a) False positive SELECT INTERVAL '2147483648 microseconds' ERROR: interval field value out of range: 2147483648 microseconds This is not right. Microseconds are internally stored as 64 bit signed integer. The catch is: this amount of microseconds is representable in Interval data structure, this shouldn't be an error. I don't see a way to fix this as we are testing too early to know what type of value it is, as you stated. b) False negative SELECT INTERVAL '10 years' -73741824 years We don't catch errors like this, because parser only checks for overflow in pg_tm. If overflow laters happens in Interval, we don't seem to care. Fixed: SELECT INTERVAL '10 years'; ERROR: interval out of range LINE 1: SELECT INTERVAL '10 years'; 4. POSSIBLE SOLUTIONS: a) Move the overflow checking just after the conversion of pg_tm - Interval is made. This way, you can accurately predict if the result is really not store-able. b) Because of 1), you have to declare tm_hour as int64, if you want to use that for the output. But, why not use Interval struct for printing directly, without intermediate pg_tm? 5. Offtopic: Correct the documentation, INTERVAL data size is 16 bytes, not 12. Fixed. Patch attached. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/doc/src/sgml/datatype.sgml b/doc/src/sgml/datatype.sgml new file mode 100644 index 6bf4cf6..b7d7d80 *** a/doc/src/sgml/datatype.sgml --- b/doc/src/sgml/datatype.sgml *** SELECT E'\\xDEADBEEF'; *** 1587,1593 /row row entrytypeinterval [ replaceablefields/replaceable ] [ (replaceablep/replaceable) ]/type/entry ! entry12 bytes/entry entrytime interval/entry entry-17800 years/entry entry17800 years/entry --- 1587,1593 /row row entrytypeinterval [ replaceablefields/replaceable ] [ (replaceablep/replaceable) ]/type/entry ! entry16 bytes/entry entrytime interval/entry entry-17800 years/entry entry17800 years/entry diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c new file mode 100644 index a61b40e..ae4e9e7 *** a/src/backend/utils/adt/datetime.c --- b/src/backend/utils/adt/datetime.c *** DecodeInterval(char **field, int *ftype, *** 2976,2981 --- 2976,2983 type = DTK_MONTH; if (*field[i] == '-') val2 = -val2; + if (((float)val * MONTHS_PER_YEAR + val2) INT_MAX) + return DTERR_FIELD_OVERFLOW; val = val * MONTHS_PER_YEAR + val2; fval =
Re: [HACKERS] Questionable coding in orderedsetaggs.c
On Sunday, January 26, 2014, Tom Lane t...@sss.pgh.pa.us wrote: Jeremy Harris j...@wizmail.org javascript:; writes: In ordered_set_startup() sorts are initialised in non-randomAccess mode (tuplesort_begin_heap() and ~datum(), last argument). The use of tuplesort_skip_tuples() feels very like a random access to me. I think it doesn't fail because the only use (and implementation) is to skip forwards; if backwards were tried (as the interface permits) external sorts would fail because multiple tapes are present for FINALMERGE. Well, we certainly don't want to incur the overhead of randomAccess mode when we're not actually going to use it, so I'd resist changing the code in ordered_set_startup(). It's true that if tuplesort_skip_tuples() supported backwards skip, it would need to insist that randomAccess mode be enabled *when a backwards skip is used*. But such a feature is purely hypothetical ATM. +1 In ordered set functions, we normally don't skip backwards and skip tuples while sorting in,for e.g. Hypothetical set functions in only a forward manner. -- Regards, Atri *l'apprenant*
Re: [HACKERS] Freezing without write I/O
Shouldn't this patch be in the January commitfest? -- 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
Re: [HACKERS] GIN improvements part2: fast scan
Hi!
On 25.1.2014 22:21, Heikki Linnakangas wrote:
Attached is a new version of the patch set, with those bugs fixed.
I've done a bunch of tests with all the 4 patches applied, and it seems
to work now. I've done tests with various conditions (AND/OR, number of
words, number of conditions) and I so far I did not get any crashes,
infinite loops or anything like that.
I've also compared the results to 9.3 - by dumping the database and
running the same set of queries on both machines, and indeed I got 100%
match.
I also did some performance tests, and that's when I started to worry.
For example, I generated and ran 1000 queries that look like this:
SELECT id FROM messages
WHERE body_tsvector @@ to_tsquery('english','(header 53 32
useful dropped)')
ORDER BY ts_rank(body_tsvector, to_tsquery('english','(header 53
32 useful dropped)')) DESC;
i.e. in this case the query always was 5 words connected by AND. This
query is a pretty common pattern for fulltext search - sort by a list of
words and give me the best ranked results.
On 9.3, the script was running for ~23 seconds, on patched HEAD it was
~40. It's perfectly reproducible, I've repeated the test several times
with exactly the same results. The test is CPU bound, there's no I/O
activity at all. I got the same results with more queries (~100k).
Attached is a simple chart with x-axis used for durations measured on
9.3.2, y-axis used for durations measured on patched HEAD. It's obvious
a vast majority of queries is up to 2x slower - that's pretty obvious
from the chart.
Only about 50 queries are faster on HEAD, and 700 queries are more than
50% slower on HEAD (i.e. if the query took 100ms on 9.3, it takes 150ms
on HEAD).
Typically, the EXPLAIN ANALYZE looks something like this (on 9.3):
http://explain.depesz.com/s/5tv
and on HEAD (same query):
http://explain.depesz.com/s/1lI
Clearly the main difference is in the Bitmap Index Scan which takes
60ms on 9.3 and 120ms on HEAD.
On 9.3 the perf top looks like this:
34.79% postgres [.] gingetbitmap
28.96% postgres [.] ginCompareItemPointers
9.36% postgres [.] TS_execute
5.36% postgres [.] check_stack_depth
3.57% postgres [.] FunctionCall8Coll
while on 9.4 it looks like this:
28.20% postgres [.] gingetbitmap
21.17% postgres [.] TS_execute
8.08% postgres [.] check_stack_depth
7.11% postgres [.] FunctionCall8Coll
4.34% postgres [.] shimTriConsistentFn
Not sure how to interpret that, though. For example where did the
ginCompareItemPointers go? I suspect it's thanks to inlining, and that
it might be related to the performance decrease. Or maybe not.
I've repeated the test several times, checked all I could think of, but
I've found nothing so far. The flags were exactly the same in both cases
(just --enable-debug and nothing else).
regards
Tomas
attachment: gin_query_durations.png
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
