Re: [HACKERS] Statistics and selectivity estimation for ranges
On Tue, Aug 14, 2012 at 7:46 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: On 14.08.2012 09:45, Alexander Korotkov wrote: After fixing few more bugs, I've a version with much more reasonable accuracy. Great! One little thing just occurred to me: You're relying on the regular scalar selectivity estimators for the , , and operators. That seems bogus, in particular for and , because ineq_histogram_selectivity then performs a binary search of the histogram using those operators. and compare the *upper* bound of the value in table against the lower bound of constant, but the histogram is constructed using regular operator, which sorts the entries by lower bound. I think the estimates you now get for those operators are quite bogus if there is a non-trivial amount of overlap between ranges. For example: postgres=# create table range_test as select int4range(-a, a) as r from generate_series(1,100) a; analyze range_test; SELECT 100 ANALYZE postgres=# EXPLAIN ANALYZE SELECT * FROM range_test WHERE r int4range(20, 21); QUERY PLAN --**--** --**- Seq Scan on range_test (cost=0.00..17906.00 rows=100 width=14) (actual time=0. 060..1340.147 rows=20 loops=1) Filter: (r '[20,21)'::int4range) Rows Removed by Filter: 80 Total runtime: 1371.865 ms (4 rows) It would be quite easy to provide reasonable estimates for those operators, if we had a separate histogram of upper bounds. I also note that the estimation of overlap selectivity could be implemented using separate histograms of lower bounds and upper bounds, without requiring a histogram of range lengths, because a b == NOT (a b OR a b). I'm not sure if the estimates we'd get that way would be better or worse than your current method, but I think it would be easier to understand. I don't think the and operators could be implemented in terms of a lower and upper bound histogram, though, so you'd still need the current length histogram method for that. Oh, actually I didn't touch those operators. Selectivity estimation functions for them were already in the catalog, they didn't work previously just because no statistics. Histogram of upper bounds would be both more accurate and natural for some operators. However, it requires collecting additional statistics while AFAICS it doesn't liberate us from having histogram of range lengths. The code in that traverses the lower bound and length histograms in lockstep looks quite scary. Any ideas on how to simplify that? My first thought is that there should be helper function that gets a range length as argument, and returns the fraction of tuples with length = argument. It would do the lookup in the length histogram to find the right histogram bin, and do the linear interpolation within the bin. You're assuming that length is independent of lower/upper bound, so you shouldn't need any other parameters than range length for that estimation. You could then loop through only the lower bounds, and call the helper function for each bin to get the fraction of ranges long enough in that bin, instead dealing with both histograms in the same loop. I think a helper function like that might simplify those scary loops significantly, but I wasn't sure if there's some more intelligence in the way you combine values from the length and lower bound histograms that you couldn't do with such a helper function. Yes, I also thought about something like this. But, in order to save current estimate accuracy, it should be more complicated in following reasons: 1) In last version, I don't estimate just fraction of tuples with length = argument, but area under length histogram between two length bounds (length_hist_summ). 2) In histogram ends up before reaching given length bound we also need to return place where it happened. Now it is performed by hist_frac *= (length - prev_dist) / (dist - prev_dist). I'm going to try some simplification with taking care about both mentioned aspects. -- With best regards, Alexander Korotkov.
Re: [HACKERS] Statistics and selectivity estimation for ranges
On 15.08.2012 10:38, Alexander Korotkov wrote: On Tue, Aug 14, 2012 at 7:46 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: It would be quite easy to provide reasonable estimates for those operators, if we had a separate histogram of upper bounds. I also note that the estimation of overlap selectivity could be implemented using separate histograms of lower bounds and upper bounds, without requiring a histogram of range lengths, because a b == NOT (a b OR a b). I'm not sure if the estimates we'd get that way would be better or worse than your current method, but I think it would be easier to understand. I don't think the and operators could be implemented in terms of a lower and upper bound histogram, though, so you'd still need the current length histogram method for that. Oh, actually I didn't touch those operators. Selectivity estimation functions for them were already in the catalog, they didn't work previously just because no statistics. Yeah, without the histogram, the scalar selectivity estimator sort-of works, in that it returns the estimate just based on the most common values and a constant. Histogram of upper bounds would be both more accurate and natural for some operators. However, it requires collecting additional statistics while AFAICS it doesn't liberate us from having histogram of range lengths. Hmm, if we collected a histogram of lower bounds and a histogram of upper bounds, that would be roughly the same amount of data as for the standard histogram with both bounds in the same histogram. The code in that traverses the lower bound and length histograms in lockstep looks quite scary. Any ideas on how to simplify that? My first thought is that there should be helper function that gets a range length as argument, and returns the fraction of tuples with length= argument. It would do the lookup in the length histogram to find the right histogram bin, and do the linear interpolation within the bin. You're assuming that length is independent of lower/upper bound, so you shouldn't need any other parameters than range length for that estimation. You could then loop through only the lower bounds, and call the helper function for each bin to get the fraction of ranges long enough in that bin, instead dealing with both histograms in the same loop. I think a helper function like that might simplify those scary loops significantly, but I wasn't sure if there's some more intelligence in the way you combine values from the length and lower bound histograms that you couldn't do with such a helper function. Yes, I also thought about something like this. But, in order to save current estimate accuracy, it should be more complicated in following reasons: 1) In last version, I don't estimate just fraction of tuples with length= argument, but area under length histogram between two length bounds (length_hist_summ). 2) In histogram ends up before reaching given length bound we also need to return place where it happened. Now it is performed by hist_frac *= (length - prev_dist) / (dist - prev_dist). I'm going to try some simplification with taking care about both mentioned aspects. Thanks. -- Heikki Linnakangas 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] Statistics and selectivity estimation for ranges
On Wed, Aug 15, 2012 at 12:14 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Histogram of upper bounds would be both more accurate and natural for some operators. However, it requires collecting additional statistics while AFAICS it doesn't liberate us from having histogram of range lengths. Hmm, if we collected a histogram of lower bounds and a histogram of upper bounds, that would be roughly the same amount of data as for the standard histogram with both bounds in the same histogram. Ok, we've to decide if we need standard histogram. In some cases it can be used for more accurate estimation of and operators. But I think it is not so important. So, we can replace standard histogram with histograms of lower and upper bounds? -- With best regards, Alexander Korotkov.
[HACKERS] Don't allow relative path for copy from file
As described in the reference manual for COPY, we should to check file's path format not to allow relative path. Please find attached a patch. Thanks, Best regards, Etsuro Fujita copy_from_pathcheck.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [COMMITTERS] pgsql: Revert commit_delay change; just add comment that we don't hav
On 15 August 2012 05:15, Tom Lane t...@sss.pgh.pa.us wrote: Peter Geoghegan pe...@2ndquadrant.com writes: On 14 August 2012 21:26, Bruce Momjian br...@momjian.us wrote: Revert commit_delay change; just add comment that we don't have a microsecond specification. I think that if we eventually decide to change the name of commit_delay for 9.3 (you previously suggested that that might be revisited), it will be reasonable to have the new GUC in units of milliseconds. Well, the reason why it's like that at all is the thought that values of less than 1 millisecond might be useful. Are we prepared to suppose that that is not and never will be true? I think that the need for sub-millisecond granularity had more to do with historic quirks of our implementation. Slight tweaks accidentally greatly improved throughput, if only for the synthetic benchmark in question. I personally have not seen any need for a sub-millisecond granularity when experimenting with the setting on 9.3-devel. However, I am not sure that sub-millisecond granularity could never be of any use, in squeezing the last small increase in throughput made possible by commit_delay. Importantly, feedback as the GUC is tuned is far more predictable than it was with the prior implementation, so this does seem quite unimportant. Why does commit_delay have to be an integer? Can't we devise a way of manipulating it in units of milliseconds, but have the internal representation be a double, as with pg_stat_statements' total_time column? That would allow very fine tuning of the delay. As I've outlined, I'm not sure that it's worth supporting such fine-tuning with the new implementation. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] sha1, sha2 functions into core?
On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? There is still open ToDecide here: 1) Status quo - md5() in core, everything else in pgcrypto 2) Start moving other hashes as separate functions into core: eg. sha1() Problems: - sha1 will be obsolete soon, like md5 - many newer hashes: sha2 family, upcoming sha3 family - hex vs. binary api issue - hex-by-default in not good when you actually need cryptographic hash (eg. indexes) - does not solve the password storage problem. 3) Move digest() from pgcrypto into core, with same api. Problems: - does not solve the password storage problem. 4) Move both digest() and crypt() into core, with same api. Password problem - the cryptographic hashes are meant for universal usage, thus they need to be usable even on megabytes of data. This means they are easily bruteforceable, when the amount of data is microscopic (1..16 chars). Also they are unsalted, thus making cracking even easier. crypt() is better api for passwords. So when the main need to have hashes is password storage, then 2) is bad choice. My vote: 4), 1) -- marko -- 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: Revert commit_delay change; just add comment that we don't hav
Peter Geoghegan pe...@2ndquadrant.com writes: Why does commit_delay have to be an integer? Can't we devise a way of manipulating it in units of milliseconds, but have the internal representation be a double, as with pg_stat_statements' total_time column? If you wanted to re-implement all the guc.c logic for supporting unit-ified values such that it would also work with floats, we could do that. It seems like way more mechanism than the problem is worth however. 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] sha1, sha2 functions into core?
Marko Kreen mark...@gmail.com writes: On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? There is still open ToDecide here: [snip] The argument against moving crypto code into core remains the same as it was, ie export regulations. I don't see that that situation has changed at all. Thus, I think we should leave all the pgcrypto code where it is, in an extension that's easily separated out by anybody who's concerned about legal restrictions. The recent improvements in the ease of installing extensions have made it even less interesting than it used to be to merge extension-supported code into core --- if anything, we ought to be trying to move functionality the other way. If anybody's concerned about the security of our password storage, they'd be much better off working on improving the length and randomness of the salt string than replacing the md5 hash per se. 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] Don't allow relative path for copy from file
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: As described in the reference manual for COPY, we should to check file's path format not to allow relative path. Please find attached a patch. The argument for disallowing writing to a relative path is to make it harder to accidentally overwrite a database file. That argument does not apply to COPY IN, so I'm not convinced we should impose an additional restriction. It's not out of the question that this would break real-world use-cases --- imagine someone whose workflow involves copying data files across a network to a directory accessible to the server (and quite possibly specified by a relative path) and then doing COPY IN. In any case, this patch is missing documentation updates, specifically the paragraph in the COPY reference page that it falsifies. 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] sha1, sha2 functions into core?
On 08/15/2012 06:48 AM, Tom Lane wrote: On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? If anybody's concerned about the security of our password storage, they'd be much better off working on improving the length and randomness of the salt string than replacing the md5 hash per se. Or change to an md5 HMAC rather than straight md5 with salt. Last I checked (which admittedly was a while ago) there were still no known cryptographic weaknesses associated with an HMAC based on md5. Joe -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 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] pg_stat_replication vs StandbyReplyMessage
On Mon, Aug 15, 2011 at 01:03:35PM +0200, Magnus Hagander wrote: The pg_stat_replication view exposes all the fields in StandbyReplyMessage *except* for the timestamp when the message was generated. On an active system this is not all that interesting, but on a mostly idle system that allows the monitoring to react faster than the timeout that actually kicks the other end off - and could be useful in manual debugging scenarios. Any particular reason why this was not exposed as it's own column? Did this ever get done? I don't think so, though everyone wanted it. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [COMMITTERS] pgsql: Revert commit_delay change; just add comment that we don't hav
On 15 August 2012 14:39, Tom Lane t...@sss.pgh.pa.us wrote: If you wanted to re-implement all the guc.c logic for supporting unit-ified values such that it would also work with floats, we could do that. It seems like way more mechanism than the problem is worth however. Fair enough. I'm not quite comfortable recommending a switch to milliseconds if that implies a loss of sub-millisecond granularity. I know that someone is going to point out that in some particularly benchmark, they can get another relatively modest increase in throughput (perhaps 2%-3%) by splitting the difference between two adjoining millisecond integer values. In that scenario, I'd be tempted to point out that that increase is quite unlikely to carry over to real-world benefits, because the setting is then right on the cusp of where increasing commit_delay stops helping throughput and starts hurting it. The improvement is likely to get lost in the noise in the context of a real-world application, where for example the actually cost of an fsync is more variable. I'm just not sure that that's the right attitude. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and 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] sha1, sha2 functions into core?
On 08/15/2012 11:22 AM, Joe Conway wrote: On 08/15/2012 06:48 AM, Tom Lane wrote: On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? If anybody's concerned about the security of our password storage, they'd be much better off working on improving the length and randomness of the salt string than replacing the md5 hash per se. Or change to an md5 HMAC rather than straight md5 with salt. Last I checked (which admittedly was a while ago) there were still no known cryptographic weaknesses associated with an HMAC based on md5. Possibly. I still think the right time to revisit this whole area will be when the NIST Hash Function competition ends supposedly later this year. See http://csrc.nist.gov/groups/ST/hash/timeline.html. At that time we should probably consider moving our password handling to use the new standard function. 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] sha1, sha2 functions into core?
On Wed, Aug 15, 2012 at 11:37:04AM -0400, Andrew Dunstan wrote: On 08/15/2012 11:22 AM, Joe Conway wrote: On 08/15/2012 06:48 AM, Tom Lane wrote: On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? If anybody's concerned about the security of our password storage, they'd be much better off working on improving the length and randomness of the salt string than replacing the md5 hash per se. Or change to an md5 HMAC rather than straight md5 with salt. Last I checked (which admittedly was a while ago) there were still no known cryptographic weaknesses associated with an HMAC based on md5. Possibly. I still think the right time to revisit this whole area will be when the NIST Hash Function competition ends supposedly later this year. See http://csrc.nist.gov/groups/ST/hash/timeline.html. At that time we should probably consider moving our password handling to use the new standard function. Are we really going to be comforable with a algorithm that is new? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] sha1, sha2 functions into core?
On Wed, Aug 15, 2012 at 10:22 AM, Joe Conway m...@joeconway.com wrote: On 08/15/2012 06:48 AM, Tom Lane wrote: On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? If anybody's concerned about the security of our password storage, they'd be much better off working on improving the length and randomness of the salt string than replacing the md5 hash per se. Or change to an md5 HMAC rather than straight md5 with salt. Last I checked (which admittedly was a while ago) there were still no known cryptographic weaknesses associated with an HMAC based on md5. There is no cryptographic with md5 either really. The best known attack IIRC is 2^123 (well outside of any practical brute force search) and the algorithm is very well studied. The main issue with md5 is that it's fast enough that you can search low entropy passwords (rainbow tables etc) which does not depend on the strength of the hashing algorithm. If the hacker has access to the salt, then it will only slow him/her down somewhat because the search will be have to be restarted for each password. The sha family are engineered to be fast and are therefore not meaningfully safer IMO. Ditto NIST hash function (upcoming sha-3) that Andrew is mentioning downthread (that might be a good idea for other reasons but I don't really think it's better in terms of securing user password). If you want to give the user good password security, I think you have only two choices: 1) allow use hmac as you suggest (but this forces user to maintain additional password or some token) 2) force or at least strongly encourage user to choose high entropy password A lot of people argue for 3) use a purposefully slow hashing function like bcrypt. but I disagree: I don't like any scheme that encourages use of low entropy passwords. merlin -- 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: Revert commit_delay change; just add comment that we don't hav
Peter Geoghegan pe...@2ndquadrant.com writes: I'm not quite comfortable recommending a switch to milliseconds if that implies a loss of sub-millisecond granularity. I know that someone is going to point out that in some particularly benchmark, they can get another relatively modest increase in throughput (perhaps 2%-3%) by splitting the difference between two adjoining millisecond integer values. In that scenario, I'd be tempted to point out that that increase is quite unlikely to carry over to real-world benefits, because the setting is then right on the cusp of where increasing commit_delay stops helping throughput and starts hurting it. The improvement is likely to get lost in the noise in the context of a real-world application, where for example the actually cost of an fsync is more variable. I'm just not sure that that's the right attitude. To me it's more about future-proofing. commit_delay is the only time-interval setting we've got where reasonable values today are in the single-digit-millisecond range. So it seems to me not hard to infer that in a few years sub-millisecond values will be important, whether or not there's any real argument for them today. 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] sha1, sha2 functions into core?
On 08/15/2012 11:48 AM, Bruce Momjian wrote: On Wed, Aug 15, 2012 at 11:37:04AM -0400, Andrew Dunstan wrote: On 08/15/2012 11:22 AM, Joe Conway wrote: On 08/15/2012 06:48 AM, Tom Lane wrote: On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? If anybody's concerned about the security of our password storage, they'd be much better off working on improving the length and randomness of the salt string than replacing the md5 hash per se. Or change to an md5 HMAC rather than straight md5 with salt. Last I checked (which admittedly was a while ago) there were still no known cryptographic weaknesses associated with an HMAC based on md5. Possibly. I still think the right time to revisit this whole area will be when the NIST Hash Function competition ends supposedly later this year. See http://csrc.nist.gov/groups/ST/hash/timeline.html. At that time we should probably consider moving our password handling to use the new standard function. Are we really going to be comforable with a algorithm that is new? The only thing that will be new about it will be that it's the new standard. There is a reason these crypto function competitions runs for quite a few years. 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] sha1, sha2 functions into core?
On 08/15/2012 08:49 AM, Merlin Moncure wrote: 1) allow use hmac as you suggest (but this forces user to maintain additional password or some token) Not really. You would store a token and the HMAC of the token using the user password on the server side. You would need access to the hash function on the client side as well. On authentication the server sends the token to the client, and the client calculates the HMAC using the user provided password. The result is sent back to the server for comparison. This way the user's password is never actually sent over the wire. Now this is still susceptible to a replay attack, but you can fix that by adding another layer. On authentication the server generates a new nonce (random token) and sends it to the client along with the stored token, as well as calculating the HMAC of the nonce using the stored user HMAC as the key. On the client side the the process is repeated -- HMAC(nonce,HMAC(token,password)). This provides a one time calculation preventing replay and does not expose the user's password or token-HMAC over the wire. The final problem as you stated is weak passwords and some kind of dictionary attack against a stolen set of tokens and HMACs. Didn't we add a hook some time ago for user provided password checker? Joe -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, 24x7 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] text search: restricting the number of parsed words in headline generation
Is this a TODO? --- On Tue, Aug 23, 2011 at 10:31:42PM -0400, Tom Lane wrote: Sushant Sinha sushant...@gmail.com writes: Doesn't this force the headline to be taken from the first N words of the document, independent of where the match was? That seems rather unworkable, or at least unhelpful. In headline generation function, we don't have any index or knowledge of where the match is. We discover the matches by first tokenizing and then comparing the matches with the query tokens. So it is hard to do anything better than first N words. After looking at the code in wparser_def.c a bit more, I wonder whether this patch is doing what you think it is. Did you do any profiling to confirm that tokenization is where the cost is? Because it looks to me like the match searching in hlCover() is at least O(N^2) in the number of tokens in the document, which means it's probably the dominant cost for any long document. I suspect that your patch helps not so much because it saves tokenization costs as because it bounds the amount of effort spent in hlCover(). I haven't tried to do anything about this, but I wonder whether it wouldn't be possible to eliminate the quadratic blowup by saving more state across the repeated calls to hlCover(). At the very least, it shouldn't be necessary to find the last query-token occurrence in the document from scratch on each and every call. Actually, this code seems probably flat-out wrong: won't every successful call of hlCover() on a given document return exactly the same q value (end position), namely the last token occurrence in the document? How is that helpful? 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 -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] sha1, sha2 functions into core?
On Wed, Aug 15, 2012 at 09:18:48AM -0700, Joe Conway wrote: The final problem as you stated is weak passwords and some kind of dictionary attack against a stolen set of tokens and HMACs. Didn't we add a hook some time ago for user provided password checker? Yes, contrib/passwordcheck: http://www.postgresql.org/docs/9.2/static/passwordcheck.html -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] text search: restricting the number of parsed words in headline generation
This might indicate that the hlCover() item is resolved. --- On Wed, Aug 24, 2011 at 10:08:11AM +0530, Sushant Sinha wrote: Actually, this code seems probably flat-out wrong: won't every successful call of hlCover() on a given document return exactly the same q value (end position), namely the last token occurrence in the document? How is that helpful? regards, tom lane There is a line that saves the computation state from the previous call and search only starts from there: int pos = *p; -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Statistics and selectivity estimation for ranges
Alexander Korotkov aekorot...@gmail.com writes: Ok, we've to decide if we need standard histogram. In some cases it can be used for more accurate estimation of and operators. But I think it is not so important. So, we can replace standard histogram with histograms of lower and upper bounds? You should assign a new pg_statistic kind value (see pg_statistic.h) rather than mislabel this as being a standard histogram. However, there's nothing wrong with a data-type-specific stats collection function choosing to gather only this type of histogram and not the standard one. 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] text search: restricting the number of parsed words in headline generation
Bruce Momjian br...@momjian.us writes: Is this a TODO? AFAIR nothing's been done about the speed issue, so yes. I didn't like the idea of creating a user-visible knob when the speed issue might be fixable with internal algorithm improvements, but we never followed up on this in either fashion. regards, tom lane --- On Tue, Aug 23, 2011 at 10:31:42PM -0400, Tom Lane wrote: Sushant Sinha sushant...@gmail.com writes: Doesn't this force the headline to be taken from the first N words of the document, independent of where the match was? That seems rather unworkable, or at least unhelpful. In headline generation function, we don't have any index or knowledge of where the match is. We discover the matches by first tokenizing and then comparing the matches with the query tokens. So it is hard to do anything better than first N words. After looking at the code in wparser_def.c a bit more, I wonder whether this patch is doing what you think it is. Did you do any profiling to confirm that tokenization is where the cost is? Because it looks to me like the match searching in hlCover() is at least O(N^2) in the number of tokens in the document, which means it's probably the dominant cost for any long document. I suspect that your patch helps not so much because it saves tokenization costs as because it bounds the amount of effort spent in hlCover(). I haven't tried to do anything about this, but I wonder whether it wouldn't be possible to eliminate the quadratic blowup by saving more state across the repeated calls to hlCover(). At the very least, it shouldn't be necessary to find the last query-token occurrence in the document from scratch on each and every call. Actually, this code seems probably flat-out wrong: won't every successful call of hlCover() on a given document return exactly the same q value (end position), namely the last token occurrence in the document? How is that helpful? 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 -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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] text search: restricting the number of parsed words in headline generation
I will do the profiling and present the results. On Wed, 2012-08-15 at 12:45 -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: Is this a TODO? AFAIR nothing's been done about the speed issue, so yes. I didn't like the idea of creating a user-visible knob when the speed issue might be fixable with internal algorithm improvements, but we never followed up on this in either fashion. regards, tom lane --- On Tue, Aug 23, 2011 at 10:31:42PM -0400, Tom Lane wrote: Sushant Sinha sushant...@gmail.com writes: Doesn't this force the headline to be taken from the first N words of the document, independent of where the match was? That seems rather unworkable, or at least unhelpful. In headline generation function, we don't have any index or knowledge of where the match is. We discover the matches by first tokenizing and then comparing the matches with the query tokens. So it is hard to do anything better than first N words. After looking at the code in wparser_def.c a bit more, I wonder whether this patch is doing what you think it is. Did you do any profiling to confirm that tokenization is where the cost is? Because it looks to me like the match searching in hlCover() is at least O(N^2) in the number of tokens in the document, which means it's probably the dominant cost for any long document. I suspect that your patch helps not so much because it saves tokenization costs as because it bounds the amount of effort spent in hlCover(). I haven't tried to do anything about this, but I wonder whether it wouldn't be possible to eliminate the quadratic blowup by saving more state across the repeated calls to hlCover(). At the very least, it shouldn't be necessary to find the last query-token occurrence in the document from scratch on each and every call. Actually, this code seems probably flat-out wrong: won't every successful call of hlCover() on a given document return exactly the same q value (end position), namely the last token occurrence in the document? How is that helpful? 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 -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] CREATE SCHEMA IF NOT EXISTS
Hackers, Is there any reason not to add $subject? Would it be difficult? Would save me having to write a DO statement every time I needed it (which in migration scripts is fairly often). Thanks, David -- 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] CREATE SCHEMA IF NOT EXISTS
2012/8/15 David E. Wheeler da...@justatheory.com Hackers, Is there any reason not to add $subject? Would it be difficult? Looking to the source code I think this feature isn't hard to implement... I'm writing a little path to do that and I'll send soon... Would save me having to write a DO statement every time I needed it (which in migration scripts is fairly often). I understand your difficulty. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
[HACKERS] Bogus rescan logic in ExecReScanCteScan
I looked into the misbehavior reported by Adam Mackler in
http://archives.postgresql.org/pgsql-bugs/2012-08/msg00073.php
What it seems to boil down to is a design error in ExecReScanCteScan.
Given the query
WITH RECURSIVE
tab(id_key,link) AS ( VALUES (1,17), (2,17), (3,17), (4,17), (6,17), (5,17) ),
iter (id_key, row_type, link) AS (
SELECT 0, 'base', 17
UNION(
WITH remaining(id_key, row_type, link, min) AS (
SELECT tab.id_key, 'true'::text, iter.link, MIN(tab.id_key) OVER ()
FROM tab INNER JOIN iter USING (link)
WHERE tab.id_key iter.id_key
),
first_remaining AS (
SELECT id_key, row_type, link
FROM remaining
WHERE id_key=min
),
effect AS (
SELECT tab.id_key, 'new'::text, tab.link
FROM first_remaining e INNER JOIN tab ON e.id_key=tab.id_key
/* Try changing this WHERE clause to other false expressions */
WHERE e.row_type='false'
)
SELECT * FROM first_remaining
/* Try uncommenting the next line */
UNION SELECT * FROM effect
)
)
SELECT * FROM iter;
we get a plan like this:
CTE Scan on iter (cost=9.06..9.48 rows=21 width=40)
CTE tab
- Values Scan on *VALUES* (cost=0.00..0.08 rows=6 width=8)
CTE iter
- Recursive Union (cost=0.00..8.98 rows=21 width=40)
- Result (cost=0.00..0.01 rows=1 width=0)
- Unique (cost=0.84..0.86 rows=2 width=40)
CTE remaining
- WindowAgg (cost=0.20..0.53 rows=2 width=8)
- Hash Join (cost=0.20..0.51 rows=2 width=8)
Hash Cond: (iter.link = tab.link)
Join Filter: (tab.id_key iter.id_key)
- WorkTable Scan on iter (cost=0.00..0.20
rows=10 width=8)
- Hash (cost=0.12..0.12 rows=6 width=8)
- CTE Scan on tab (cost=0.00..0.12
rows=6 width=8)
CTE first_remaining
- CTE Scan on remaining (cost=0.00..0.04 rows=1 width=44)
Filter: (id_key = min)
CTE effect
- Hash Join (cost=0.04..0.19 rows=1 width=8)
Hash Cond: (tab.id_key = e.id_key)
- CTE Scan on tab (cost=0.00..0.12 rows=6 width=8)
- Hash (cost=0.02..0.02 rows=1 width=4)
- - CTE Scan on first_remaining e
(cost=0.00..0.02 rows=1 width=4)
Filter: (row_type = 'false'::text)
- Sort (cost=0.07..0.08 rows=2 width=40)
Sort Key: first_remaining.id_key,
first_remaining.row_type, first_remaining.link
- Append (cost=0.00..0.06 rows=2 width=40)
- - CTE Scan on first_remaining (cost=0.00..0.02
rows=1 width=40)
- CTE Scan on effect (cost=0.00..0.02 rows=1
width=40)
where there are two CTEScan nodes (marked for effect) on the output of
the first_remaining CTE. Now the first of these (the one inside the
effect CTE) is initialized first and thus becomes the leader of the
group of CTEScans reading this CTE. However, because node ReScans
happen basically in execution order, the one under the Append is the one
that actually gets a rescan call first; in fact, that one will get run
to completion before effect ever gets rescanned. So what happens
while re-executing the right-hand side of the RecursiveUnion is that
the second scan of first_remaining just regurgitates what
first_remaining already output on the previous cycle, because its
rescan only rewound its own tuple pointer and didn't change the
tuplestore contents. That leads to totally bogus results of course.
Another way to say this is that ExecReScanCteScan only works right if
the CTE nodes of a group are ReScanned in the same order they were
initialized in, which might be true some of the time but is not to be
relied on. It's a bit surprising that we've not had any reports about
this in the nearly 4 years that code has been in there.
After reflecting on this for awhile I think that ExecReScanCteScan
is just overly complicated, and there's no reason not to let the first
arrival reset the tuplestore. The attached patch seems to fix Adam's
problem (at least, variants of his query that seem like they should give
the same answer do), and it passes our regression tests.
Comments?
regards, tom lane
diff --git a/src/backend/executor/nodeCtescan.c b/src/backend/executor/nodeCtescan.c
index 7ab15ac63e1020dd00b3ef27950615dc108b795b..d45352896b09b7580f1518450366d53cbcfdbc1e 100644
*** a/src/backend/executor/nodeCtescan.c
--- b/src/backend/executor/nodeCtescan.c
*** ExecReScanCteScan(CteScanState *node)
*** 312,337
ExecScanReScan(node-ss);
! if (node-leader == node)
{
! /*
! * The leader is
Re: [HACKERS] CREATE SCHEMA IF NOT EXISTS
On Aug 15, 2012, at 11:31 AM, Fabrízio de Royes Mello wrote: Is there any reason not to add $subject? Would it be difficult? Looking to the source code I think this feature isn't hard to implement... I'm writing a little path to do that and I'll send soon... Cool, thanks! David -- 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] sha1, sha2 functions into core?
On Wed, Aug 15, 2012 at 4:48 PM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Kreen mark...@gmail.com writes: On Wed, Aug 15, 2012 at 6:11 AM, Bruce Momjian br...@momjian.us wrote: Is there a TODO here? There is still open ToDecide here: [snip] The argument against moving crypto code into core remains the same as it was, ie export regulations. I don't see that that situation has changed at all. Thus, I think we should leave all the pgcrypto code where it is, in an extension that's easily separated out by anybody who's concerned about legal restrictions. The recent improvements in the ease of installing extensions have made it even less interesting than it used to be to merge extension-supported code into core --- if anything, we ought to be trying to move functionality the other way. Ok. If anybody's concerned about the security of our password storage, they'd be much better off working on improving the length and randomness of the salt string than replacing the md5 hash per se. Sorry, I was not clear enough - by password storage I meant password storage for application-specific passwords, not postgres users. Postgres own usage of md5 is kind of fine, as: - only superusers can see the hashes (pg_shadow). - if somebody sees contents of pg_shadow, they don't need to crack them, they can use hashes to log in immediately. But for storage of application passwords, the hash needs to be one-way and hard to crack, to decrease any damage caused by leaks. -- marko -- 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] timestamptz parsing bug?
I assume we want to apply this patch based on discussion that we should
allow a wider range of date/time formats.
---
On Mon, Aug 29, 2011 at 06:40:07PM +0100, Dean Rasheed wrote:
On 29 August 2011 15:40, Andrew Dunstan and...@dunslane.net wrote:
Why do we parse this as a correct timestamptz literal:
2011-08-29T09:11:14.123 CDT
but not this:
2011-08-29T09:11:14.123 America/Chicago
Replace the ISO-8601 style T between the date and time parts of the latter
with a space and the parser is happy again.
cheers
andrew
Funny, I've just recently been looking at this code.
I think that the issue is in the DTK_TIME handling code in DecodeDateTime().
For this input string the T is recognised as the start of an ISO
time, and the ptype variable is set to DTK_TIME. The next field is a
DTK_TIME, however, when it is handled it doesn't reset the ptype
variable.
When it gets to the timezone America/Chicago at the end, this is
handled in the DTK_DATE case, because of the /. But because ptype is
still set, it is expecting this to be an ISO time, so it errors out.
The attached patch seems to fix it. Could probably use a new
regression test though.
Regards,
Dean
diff --git a/src/backend/utils/adt/datetime.c
b/src/backend/utils/adt/datetime.c
new file mode 100644
index 3d320cc..a935d98
*** a/src/backend/utils/adt/datetime.c
--- b/src/backend/utils/adt/datetime.c
*** DecodeDateTime(char **field, int *ftype,
*** 942,947
--- 942,957
break;
case DTK_TIME:
+ /*
+ * This might be an ISO time following a t
field.
+ */
+ if (ptype != 0)
+ {
+ /* Sanity check; should not fail this
test */
+ if (ptype != DTK_TIME)
+ return DTERR_BAD_FORMAT;
+ ptype = 0;
+ }
dterr = DecodeTime(field[i], fmask,
INTERVAL_FULL_RANGE,
tmask, tm,
fsec);
if (dterr)
--
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
+ It's impossible for everything to be true. +
--
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] [BUGS] BUG #6184: Inconsistencies in log messages
[ moved to hackers]
Would someone review this report for me? I think there are few
reasonable changes in here, but I am not familiar enough with all the
code areas.
---
On Tue, Aug 30, 2011 at 07:15:19AM +, Ding Yuan wrote:
The following bug has been logged online:
Bug reference: 6184
Logged by: Ding Yuan
Email address: ding.yuan...@hotmail.com
PostgreSQL version: 8.4.8
Operating system: All
Description:Inconsistencies in log messages
Details:
Hi Postgres developers,
(I reported this a day ago and for some reason did not see my message appear
in the archive, so I am resending it again and I apologize if it seems
duplicated).
I am a student doing research on improving software's diagnosability by
improving the error messages. Recently I run a static checker on
postgres-8.4.8 and found that some inconsistencies in log messages in
similar situations. While we suspect some of them should be fixed, we are
not 100% sure since we do not have domain expertise in Squid code.
I am reporting 10 such cases in this report. We truly appreciate your
feedback on whether our findings make sense.
Thanks,
Ding
== Report 1
The two elog messages are of similar situations but with different verbosity
levels.
Pattern: 7478
src/backend/executor/nodeResult.c -
@@ line: 171 @@
171:void
172:ExecResultMarkPos(ResultState *node)
173:{
174:PlanState *outerPlan = outerPlanState(node);
175:
176:if (outerPlan != NULL)
177:ExecMarkPos(outerPlan);
178:else
179:elog(DEBUG2, Result nodes do not support
mark/restore);
src/backend/executor/nodeResult.c -
@@ line: 186 @@
186:void
187:ExecResultRestrPos(ResultState *node)
188:{
189:PlanState *outerPlan = outerPlanState(node);
190:
191:if (outerPlan != NULL)
192:ExecRestrPos(outerPlan);
193:else
194:elog(ERROR, Result nodes do not support
mark/restore);
Report 2
==
The verbosity level for error return for CreateEvent is not consistent!
Pattern: 6490
src/backend/port/win32/socket.c -
@@ line: 128 @@
128:waitevent = CreateEvent(NULL, TRUE, FALSE, NULL);
129:
130:if (waitevent == INVALID_HANDLE_VALUE)
131:ereport(ERROR,
132:(errmsg_internal(Failed to
create socket waiting event: %i, (int) GetLastError(;
src/backend/port/win32/signal.c -
@@ line: 83 @@
83: pgwin32_signal_event = CreateEvent(NULL, TRUE, FALSE,
NULL);
84: if (pgwin32_signal_event == NULL)
85: ereport(FATAL,
86: (errmsg_internal(failed to create
signal event: %d, (int) GetLastError(;
Report 3
==
The log message in the first snippet (line 3487) has ERROR level, while
PANIC
is logged in other similar situations.
Pattern: 4207
src/backend/access/heap/heapam.c -
@@ line: 3483 @@
3483: if (PageGetMaxOffsetNumber(page) = offnum)
3484: lp = PageGetItemId(page, offnum);
3485:
3486: if (PageGetMaxOffsetNumber(page) offnum ||
!ItemIdIsNormal(lp))
3487: elog(ERROR, heap_inplace_update: invalid lp);
3488:
3489: htup = (HeapTupleHeader) PageGetItem(page, lp);
3490:
3491: oldlen = ItemIdGetLength(lp) - htup-t_hoff;
src/backend/access/heap/heapam.c -
@@ line: 4732 @@
4732: if (PageGetMaxOffsetNumber(page) = offnum)
4733: lp = PageGetItemId(page, offnum);
4734:
4735: if (PageGetMaxOffsetNumber(page) offnum ||
!ItemIdIsNormal(lp))
4736: elog(PANIC, heap_inplace_redo: invalid lp);
4737:
4738: htup = (HeapTupleHeader) PageGetItem(page, lp);
4739:
4740: oldlen = ItemIdGetLength(lp) - htup-t_hoff;
src/backend/access/heap/heapam.c -
@@ line: 4268 @@
4268: if (PageGetMaxOffsetNumber(page) = offnum)
4269: lp = PageGetItemId(page, offnum);
4270:
4271: if (PageGetMaxOffsetNumber(page) offnum ||
!ItemIdIsNormal(lp))
4272: elog(PANIC, heap_delete_redo: invalid lp);
4273:
4274: htup = (HeapTupleHeader) PageGetItem(page, lp);
src/backend/access/heap/heapam.c -
@@ line: 4469 @@
Re: [HACKERS] B-tree parent pointer and checkpoints
Has this been addressed? A TODO? --- On Tue, Sep 6, 2011 at 09:49:39AM -0400, Robert Haas wrote: On Tue, Sep 6, 2011 at 9:45 AM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Do you really need to hold the page locks for all that time, or could you cheat? Like... release the locks on the split pages but then go back and reacquire them to clear the flag... Hmm, there's two issues with that: 1. While you're not holding the locks on the child pages, someone can step onto the page and see that the MISSING_DOWNLINK flag is set, and try to finish the split for you. 2. If you don't hold the page locked while you clear the flag, someone can start and finish a checkpoint after you've inserted the downlink, and before you've cleared the flag. You end up in a scenario where the flag is set, but the page in fact *does* have a downlink in the parent. It seems like both of these could be handled by making the code that repairs the damage insert the downlink into the parent only if it's not already present. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Planner avoidance of index only scans for partial indexes
If you create an index like this: create index on foo(a,b,v) where d = some_constant; there is no way to get an IOS on the index: you have to supply a the partial index exclusionary value to get the value of the index and that fools the IOS chooser because it doesn't see the value in the explicit list of index columns. The workaround is to include the 'partial value' (d) in the index, but often that's unnecessary. In other words, if you have a partial index that is based on a constant, a query that is filtering on the constant is an exception to the rule that all columns must be in the index to get the IOS. Not a bug, but it's worth noting. Aside: the performance gains I'm seeing for IOS are nothing short of spectacular. merlin -- 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: Clean up the #include mess a little.
On Wed, Sep 7, 2011 at 01:20:17AM +0300, Ants Aasma wrote: On Tue, Sep 6, 2011 at 10:18 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: I wonder what happens if files in the same subdir are grouped in a subgraph. Is that possible? Possible, and done. Also added possivility to add .c files to the graph, coloring by subdir and possibility exclude nodes from the graph. I didn't yet bother to clean up the code - to avoid eye damage, don't look at the source. Bad news is that it doesn't significantly help readability for the all nodes case. See all_with_subgraphs.svgz. It does help for other cases. For example parsenodes.h.svgz has the result for render_includes.py --select='nodes/parsenodes.h+*-*' --subgraphs and execnodes.h.svgz for --subgraphs --select='nodes/execnodes.h+*-*' Should we add this script and instructions to src/tools/pginclude? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] [BUGS] BUG #6184: Inconsistencies in log messages
Bruce Momjian br...@momjian.us writes: [ moved to hackers] Would someone review this report for me? I think there are few reasonable changes in here, but I am not familiar enough with all the code areas. I see one place where we forgot to sync a comment, but otherwise this all looks OK. Details below ... == Report 1 The two elog messages are of similar situations but with different verbosity levels. The inconsistency here is intentional: it's okay for Mark to be a no-op, as long as you don't ever do a Restore. Report 2 == The verbosity level for error return for CreateEvent is not consistent! I'm not too familiar with the Windows-specific code, but I don't see anything particularly wrong here. Failing to initialize in a wait for an individual socket is perhaps recoverable, whereas failing to set up the signal-handling emulation at all is not. Report 3 == The log message in the first snippet (line 3487) has ERROR level, while PANIC is logged in other similar situations. They're not really similar since one case is mainline code while the others are log replay. Actually, I think the mainline case is inside a critical section so it's not going to matter too much anyway; and for that matter there's no practical difference between ERROR and PANIC in replay either. But by and large we've coded PANIC explicitly in log replay, but not in initial creation/application of WAL records. Report 4 == There is no error message in the 2nd snippet, when: 3089: if (!SplitIdentifierString(rawname, ',', namelist)) occurred, while there is one at line 62 under similar situations. [ shrug... ] It's not always the case that errors are to be reported immediately where detected. In this particular case, the second instance is inside a GUC check_hook, which is specifically required to not throw errors, but instead return a failure indication to its caller. Report 5 == The first log message at line 2221 is of ERROR level, while the other log messages in similar situations are FATAL! I think probably the point here is that erroring out of transaction start is recoverable (you are still not in a transaction, and you could try again) but erroring out of a later state transition is not going to leave the transaction mechanism in a sane state. In any case, all of those are can't happen errors so it's hard to get very excited about them. Report 6 == The verbosity level of log messages are inconsistent! Again, one of these is mainline code while the others are in recovery context. Report 7 == The log message at line 3050 and line 3142 have inconsistent verbosity levels! The inconsistency here is a consequence of commit 82748bc253e00a07f0fcce3959af711b509a4e69, so it's entirely intentional, and presumably a result of discussion in the mailing lists (I'm too lazy to go look for the thread right now). I notice that that commit failed to adjust the adjacent comment, though --- tut tut, Bruce. Report 8 == The log message at 2292 is ERROR, inconsistent with the line @line 2521 (PANIC). Failure of advance creation of an xlog file is recoverable, not having one when we need it is not. Also see header comment for XLogFileInit. Report 9 == The 2 log messages: 624@comment.c and 663@comment.c have different verbosity levels! Again, note the comment, which shows pretty clearly that this choice was intentional. I'm not going to do a git blame for this one but you could certainly track back where the code was changed to make it a WARNING. Report 10 == The first log message has WARNING while the others have ERROR! Yup. The first one is in postgresql.conf parsing and the others are in command line option parsing. We don't start up if command line options are bad ... but once running, a mistake in postgresql.conf shouldn't bring down the database. On the whole, my impression is that this type of analysis might be productive, but it would require a great deal more context awareness (and knowledge of project-specific coding rules) to get it to the point where it's not almost all false positives. I doubt that there are very many mistakes in assignment of NOTICE/WARNING/ERROR levels in our code. There's a certain lack of consistency in use of ERROR/FATAL/PANIC in contexts where the coder knows they're all the same anyway, namely just about anything that
Re: [HACKERS] [BUGS] BUG #6184: Inconsistencies in log messages
On Wed, Aug 15, 2012 at 06:42:51PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: [ moved to hackers] Would someone review this report for me? I think there are few reasonable changes in here, but I am not familiar enough with all the code areas. Report 7 == The log message at line 3050 and line 3142 have inconsistent verbosity levels! The inconsistency here is a consequence of commit 82748bc253e00a07f0fcce3959af711b509a4e69, so it's entirely intentional, and presumably a result of discussion in the mailing lists (I'm too lazy to go look for the thread right now). I notice that that commit failed to adjust the adjacent comment, though --- tut tut, Bruce. Oops, thanks, fixed, and thanks for reviewing this report. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] Planner avoidance of index only scans for partial indexes
Merlin Moncure mmonc...@gmail.com writes: If you create an index like this: create index on foo(a,b,v) where d = some_constant; there is no way to get an IOS on the index: you have to supply a the partial index exclusionary value to get the value of the index and that fools the IOS chooser because it doesn't see the value in the explicit list of index columns. Yeah, this is a known limitation that we'll probably try to improve someday. Per the comment in check_index_only(): /* * Check that all needed attributes of the relation are available from the * index. * * XXX this is overly conservative for partial indexes, since we will * consider attributes involved in the index predicate as required even * though the predicate won't need to be checked at runtime. (The same is * true for attributes used only in index quals, if we are certain that * the index is not lossy.) However, it would be quite expensive to * determine that accurately at this point, so for now we take the easy * way out. */ 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] xlog file naming
Are there any TODO items here? --- On Mon, Sep 12, 2011 at 09:36:02PM +0300, Peter Eisentraut wrote: Isn't the naming of the xlog files slightly bogus? We have the following sequence: 000108D000FD 000108D000FE 000108D1 Ignoring that we skip FF for some obscure reason (*), these are effectively supposed to be 64-bit numbers, chunked into 16MB pieces, so shouldn't the naming be 000108D0FD00 000108D0FE00 000108D1 Then a lot of other things would also start making more sense: The backup file 000108D100C9.00013758.backup should really be 000108D1C9013758.backup (At least conceptually. It's debatable whether we'd want to change that, since as it is, it's convenient to detect the preceding WAL file name by cutting off the end. OTOH, it's safer to actually look into the backup file for that information.) The return value of pg_start_backup that currently looks something like pg_start_backup - 8D1/C9013758 should really be 08D1C9013758 (perhaps the timeline should be included?) and similarly, log output and pg_controldata output like Latest checkpoint location: 8D3/5A1BB578 should be Latest checkpoint location: 08D35A1BB578 Then all instances of xlog location would look the same. Also, the documentation offers this example: For example, if the starting WAL file is 0001123455CD the backup history file will be named something like 0001123455CD.007C9330.backup. Both the WAL and the backup file names used here are actually impossible, and are not helping to clarify the issue. It seems to me that this is all uselessly complicated and confused. (*) - That idea appears to come from the same aboriginal confusion about WAL files vs segments vs WAL position. Unless we support WAL segments of size 1 or 2 bytes, there shouldn't be any risk of overflowing the segment counter. PS2: While we're discussing the cleanup of xlog.c, someone daring could replace XLogRecPtr by a plain uint64 and possibly save hundres of lines of code and eliminate a lot of the above confusion. -- 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 + It's impossible for everything to be true. + -- 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 to improve reliability of postgresql on linux nfs
On Tue, Sep 13, 2011 at 10:32:01AM -0400, Aidan Van Dyk wrote: On Tue, Sep 13, 2011 at 10:14 AM, Florian Pflug f...@phlo.org wrote: Personally, I'ld think that's ripe for bugs. If the contract is that ret != amount is the error case, then don't return -1 for an error *sometimes*. Hm, but isn't that how write() works also? AFAIK (non-interruptible) write() will return the number of bytes written, which may be less than the requested number if there's not enough free space, or -1 in case of an error like an invalid fd being passed. Looking through the code, it appears as if all the write calls I've seen are checking ret != amount, so it's probably not as big a deal for PG as I fear... But the subtle change in semantics (from system write ret != amount not necessarily a real error, hence no errno set) of pg_write ret != amount only happening after a real error (errno should be set) is one that could yet lead to confusion. I assume there is no TODO here. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] SSL key with passphrase
On Wed, Sep 14, 2011 at 02:40:15AM +0100, Thom Brown wrote: On 13 September 2011 15:17, Tom Lane t...@sss.pgh.pa.us wrote: Thom Brown t...@linux.com writes: There appears to be a problem with starting Postgres if the SSL key has a passphrase on it. It's documented that that's unsupported. Given the number of ways to start a postmaster, and the fact that many of them are noninteractive, I don't think it's very productive for us to worry about it. I've managed to get pg_ctl to accept the passphrase with the -w option. Works fine like that. Since that works, perhaps the page referring to SSL could mention this. I have added a documention mention as you suggested for PG 9.3 in the '-w' option section. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/doc/src/sgml/ref/pg_ctl-ref.sgml b/doc/src/sgml/ref/pg_ctl-ref.sgml new file mode 100644 index 37826e9..90725d9 *** a/doc/src/sgml/ref/pg_ctl-ref.sgml --- b/doc/src/sgml/ref/pg_ctl-ref.sgml *** PostgreSQL documentation *** 383,388 --- 383,389 attempts to connect to the server. When waiting for shutdown, commandpg_ctl/command waits for the server to remove its acronymPID/acronym file. + This option allows the entry of an acronymSSL/ passphrase on startup. commandpg_ctl/command returns an exit code based on the success of the startup or shutdown. /para -- 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] feature request: auto savepoint for interactive psql when in transaction.
On Mon, Nov 14, 2011 at 04:19:30PM -0600, Ross Reedstrom wrote: On Wed, Sep 28, 2011 at 11:47:51AM -0700, David Fetter wrote: On Wed, Sep 28, 2011 at 02:25:44PM -0400, Gurjeet Singh wrote: On Wed, Sep 28, 2011 at 1:51 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Alvaro Herrera alvhe...@commandprompt.com wrote: See ON_ERROR_ROLLBACK http://www.postgresql.org/docs/9.0/static/app-psql.html I had missed that. Dang, this database product is rich with nice features! :-) +1 I would like it to be on/interactive by default, though. You can have it by putting it in your .psqlrc. If we were just starting out, I'd be all for changing the defaults, but we're not. We'd break things unnecessarily if we changed this default. This discussion died out with a plea for better documentation, and perhaps some form of discoverability. I've scanned ahead and see no further discussion. However, I'm wondering, what use-cases would be broken by setting the default to 'interactive'? Running a non-interactive script by piping it to psql? Reading the code, I see that case is covered: the definition of 'interactive' includes both stdin and stdout are a tty, and the source of commands is stdin. Seems this functionality appeared in version 8.1. Was there discussion re: making it the default at that time? I'm all for backward compatibility, but I'm having trouble seeing what would break. I see that Peter blogged about this from a different angle over a year ago (http://petereisentraut.blogspot.com/2010/03/running-sql-scripts-with-psql.html) which drew a comment from Tom Lane that perhaps we need a better/different tool for running scripts. That would argue the defaults for psql proper should favor safe interactive use (autocommit off, anyone?) Peter mentioned the traditional method unix shells use to handle this: different config files are read for interactive vs. non-interactive startup. Seems we have that, just for the one setting ON_ERROR_ROLLBACK. What documentation improvement are you suggesting? The docs seem clear to me. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- 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] ToDo: allow to get a number of processed rows by COPY statement
What ever happened to this patch? I don't see it on any of the
commit-fests, though someone was asked for it to be added:
http://archives.postgresql.org/pgsql-hackers/2011-10/msg00381.php
---
On Tue, Oct 4, 2011 at 12:22:19PM +0200, Pavel Stehule wrote:
Hello
There is not possible to get a number of processed rows when COPY is
evaluated via SPI. Client can use a tag, but SPI doesn't use a tag.
I propose a small change a ProcessUtility to return a processed rows.
Note: I found a small inconsistency between SPI and Utility interface.
SPI still use a 4 byte unsign int for storing a number of processed
rows. Utility use a 8bytes unsign int.
Motivation:
postgres=# \sf fx
CREATE OR REPLACE FUNCTION public.fx(tablename text, filename text)
RETURNS integer
LANGUAGE plpgsql
AS $function$
declare r int;
begin
execute format('COPY %s FROM %s', quote_ident(tablename),
quote_literal(filename));
get diagnostics r = row_count;
return r;
end;
$function$
Regards
Pavel Stehule
diff --git a/src/backend/executor/functions.c
b/src/backend/executor/functions.c
index 398bc40..a7c2b8f 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -600,6 +600,7 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr
fcache)
es-qd-params,
false, /* not top level */
es-qd-dest,
+NULL,
NULL);
result = true; /* never stops early */
}
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 688279c..21cabcc 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1838,6 +1838,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo
paramLI,
{
Node *stmt = (Node *) lfirst(lc2);
boolcanSetTag;
+ boolisCopyStmt = false;
DestReceiver *dest;
_SPI_current-processed = 0;
@@ -1857,6 +1858,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo
paramLI,
{
CopyStmt *cstmt = (CopyStmt *) stmt;
+ isCopyStmt = true;
if (cstmt-filename == NULL)
{
my_res = SPI_ERROR_COPY;
@@ -1911,16 +1913,23 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo
paramLI,
}
else
{
+ uint32 processed;
+
ProcessUtility(stmt,
plansource-query_string,
paramLI,
false, /* not
top level */
dest,
-NULL);
+NULL,
+processed);
/* Update processed if stmt returned tuples */
+
if (_SPI_current-tuptable)
_SPI_current-processed =
_SPI_current-tuptable-alloced -
_SPI_current-tuptable-free;
+ else if (canSetTag isCopyStmt)
+ _SPI_current-processed = processed;
+
res = SPI_OK_UTILITY;
}
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 466727b..1a861ee 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1184,7 +1184,8 @@ PortalRunUtility(Portal portal, Node *utilityStmt, bool
isTopLevel,
portal-portalParams,
isTopLevel,
dest,
-completionTag);
+completionTag,
+NULL);
/* Some utility statements may change context on us */
MemoryContextSwitchTo(PortalGetHeapMemory(portal));
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 0749227..35db28c 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -319,6 +319,9 @@ CheckRestrictedOperation(const char *cmdname)
* completionTag is only set nonempty if we want to return a
Re: [HACKERS] ToDo: allow to get a number of processed rows by COPY statement
Hello
I can reassign it again
Regards
Pavel
2012/8/16 Bruce Momjian br...@momjian.us:
What ever happened to this patch? I don't see it on any of the
commit-fests, though someone was asked for it to be added:
http://archives.postgresql.org/pgsql-hackers/2011-10/msg00381.php
---
On Tue, Oct 4, 2011 at 12:22:19PM +0200, Pavel Stehule wrote:
Hello
There is not possible to get a number of processed rows when COPY is
evaluated via SPI. Client can use a tag, but SPI doesn't use a tag.
I propose a small change a ProcessUtility to return a processed rows.
Note: I found a small inconsistency between SPI and Utility interface.
SPI still use a 4 byte unsign int for storing a number of processed
rows. Utility use a 8bytes unsign int.
Motivation:
postgres=# \sf fx
CREATE OR REPLACE FUNCTION public.fx(tablename text, filename text)
RETURNS integer
LANGUAGE plpgsql
AS $function$
declare r int;
begin
execute format('COPY %s FROM %s', quote_ident(tablename),
quote_literal(filename));
get diagnostics r = row_count;
return r;
end;
$function$
Regards
Pavel Stehule
diff --git a/src/backend/executor/functions.c
b/src/backend/executor/functions.c
index 398bc40..a7c2b8f 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -600,6 +600,7 @@ postquel_getnext(execution_state *es,
SQLFunctionCachePtr fcache)
es-qd-params,
false, /* not top level */
es-qd-dest,
+NULL,
NULL);
result = true; /* never stops early */
}
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 688279c..21cabcc 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1838,6 +1838,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo
paramLI,
{
Node *stmt = (Node *) lfirst(lc2);
boolcanSetTag;
+ boolisCopyStmt = false;
DestReceiver *dest;
_SPI_current-processed = 0;
@@ -1857,6 +1858,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo
paramLI,
{
CopyStmt *cstmt = (CopyStmt *) stmt;
+ isCopyStmt = true;
if (cstmt-filename == NULL)
{
my_res = SPI_ERROR_COPY;
@@ -1911,16 +1913,23 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo
paramLI,
}
else
{
+ uint32 processed;
+
ProcessUtility(stmt,
plansource-query_string,
paramLI,
false, /* not
top level */
dest,
-NULL);
+NULL,
+processed);
/* Update processed if stmt returned tuples
*/
+
if (_SPI_current-tuptable)
_SPI_current-processed =
_SPI_current-tuptable-alloced -
_SPI_current-tuptable-free;
+ else if (canSetTag isCopyStmt)
+ _SPI_current-processed = processed;
+
res = SPI_OK_UTILITY;
}
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 466727b..1a861ee 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1184,7 +1184,8 @@ PortalRunUtility(Portal portal, Node *utilityStmt,
bool isTopLevel,
portal-portalParams,
isTopLevel,
dest,
-completionTag);
+completionTag,
+NULL);
/* Some utility statements may change context on us */
MemoryContextSwitchTo(PortalGetHeapMemory(portal));
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 0749227..35db28c 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -319,6 +319,9 @@
Re: [HACKERS] pg_prewarm
I hope it's not too late for another reviewer to pitch in :) I have let some time pass between previous reviews so that I can give this patch a fresh look, and not be tainted by what the other reviews said, so I may be repeating a few things already covered by other reviewers. I haven't performed any tests on this (yet) because I have seen a few other posts which show that other people have already used this utility. When I get time next, I will try to develop some useful scripts around this function to help in hibernation-like feature, and also the speeding-up of recovery when combined with xlogdump as previously suggested in this thread. This is my first review of a patch, and I just realized after finishing the review that this does not qualify as proper review as documented in Reviewing a patch wiki page. But this is an ungodly hour for me, so cannot spend more time on it right now. These are just the notes I took while doing the code review. Hope it helps in improving the patch. Applying the patch on master HEAD needs some hunk adjustments, but I didn't see anything out of place during the review. snip patching file doc/src/sgml/contrib.sgml Hunk #1 succeeded at 128 with fuzz 2 (offset 12 lines). patching file doc/src/sgml/filelist.sgml Hunk #1 succeeded at 125 (offset 1 line). /snip I think it'll be useful to provide some overloaded functions, or provide some defaults. Here's what I think are sensible defaults. Note that I have moved prefetch_type parameter from position 3 to 2; I think prefetch_type will see more variations in the field than fork (which will be 'main' most of the times). pg_prewarm(relation) defaults: type (prefetch/read), fork (main), first_block(null), last_block(null) pg_prewarm(relation, type) defaults: fork (main), first_block(null), last_block(null) pg_prewarm(relation, type, fork) defaults: first_block(null), last_block(null) pg_prewarm(relation, type, fork, first_block) defaults: last_block(null) pg_prewarm(relation, type, fork, first_block, last_block) -- already provided in the patch. Should we provide a capability to prewarm all forks of a relation by allowing a pseudo fork by the name 'all'. I don't see much use for it, but others might. We should consider making this error 'fork \%s\ does not exist for this relation' into a warning, unless we can guarantee that forks always exist for a relation; for eg. if Postgres delays creating the 'vm' fork until first vacuum on a relation, then a script that simply tries to prewarm all forks of a relation will encounter errors, which may stop the script processing altogether and lead to not prewarming the rest of the relations the user might have wanted to. Does the regclass conversion of first parameter respects the USAGE privileges on the schema? Does it need to if the user has SELECT privilege on it? Do not raise error when the provided last_block number is larger than total blocks in the relation. This may have been caused by a truncate operation since the user initiated the function. Just raise a warning and use total blocks as last_block. Make this error prefetch is not supported by this build into a warning. This will let the scripts developed on one build at least complete successfully on a different build. Check for last_block 0. Better yet, raise an error if last_block first_block. In PREWARM_BUFFER case, raise a warning and load only (end_buffer - begin_buffer) number of buffers if ((end_buffer - begin_buffer) shared_buffers). This will help prewarming complete quicker if the relation is too big for the shared_buffers to accommodate, and also let the user know that she needs to tweak the prewarming method. It may help to perform PREWARM_READ on the rest of the buffers. In the docs, where it says so it is possible + for other system activity may evict the newly prewarmed the word 'for' seems out of place. Replace it with 'that'. Best regards, On Sat, Jul 14, 2012 at 5:33 AM, Cédric Villemain ced...@2ndquadrant.comwrote: c) isn't necessarily safe in production (I've crashed Linux with Fincore in the recent past). fincore is another soft, please provide a bugreport if you hit issue with pgfincore, I then be able to fix it and all can benefit. -- Cédric Villemain +33 (0)6 20 30 22 52 http://2ndQuadrant.fr/ PostgreSQL: Support 24x7 - Développement, Expertise et Formation -- Gurjeet Singh
