Re: [HACKERS] Improving avg performance for numeric
Hello, I updated the patch by taking ideas from your patch, and unifying the transition struct and update function for different aggregates. The speed of avg improved even more. It now has 60% better performance than the current committed version. This patch optimizes numeric/int8 sum, avg, stddev_pop, stddev_samp, var_pop, var_samp. I also noticed that this patch makes matview test fail. It seems that it just changes the ordering of rows for queries like "SELECT * FROM tv;". Does this seem like a bug in my patch, or should we add "ORDER BY" clauses to this test to make it more deterministic? I also agree with you that adding sum functions to use preallocated buffers will make even more optimization. I'll try to see if I can find a simple way to do this. Thanks, -- Hadi On Mon, Mar 18, 2013 at 5:25 PM, Pavel Stehule wrote: > Hello > > I played with sum(numeric) optimization > > Now it is based on generic numeric_add function - this code is > relative old - now we can design aggregates with internal transition > buffers, and probably we can do this work more effective. > > I just removed useles palloc/free operations and I got a 30% better > performance! My patch is ugly - because I used a generic add_var > function. Because Sum, Avg and almost all aggregates functions is > limited by speed of sum calculation I thing so we need a new numeric > routines optimized for calculation "sum", that use a only preallocated > buffers. A speed of numeric is more important now, because there are > more and more warehouses, where CPU is botleneck. > > Regards > > Pavel > > > 2013/3/18 Hadi Moshayedi : > > Hi Pavel, > > > > Thanks a lot for your feedback. > > > > I'll work more on this patch this week, and will send a more complete > patch > > later this week. > > > > I'll also try to see how much is the speed up of this method for other > > types. > > > > Thanks, > > -- Hadi > > > > > > On Mon, Mar 18, 2013 at 10:36 AM, Pavel Stehule > > > wrote: > >> > >> 2013/3/16 Hadi Moshayedi : > >> > Revisiting: > >> > http://www.postgresql.org/message-id/45661be7.4050...@paradise.net.nz > >> > > >> > I think the reasons which the numeric average was slow were: > >> > (1) Using Numeric for count, which is slower than int8 to increment, > >> > (2) Constructing/deconstructing arrays at each transition step. > >> > > >> > This is also discussed at: > >> > > >> > > http://www.citusdata.com/blog/53-postgres-performance-to-avg-or-to-sum-divided-by-count > >> > > >> > So, I think we can improve the speed of numeric average by keeping the > >> > transition state as an struct in the aggregate context, and just > passing > >> > the > >> > pointer to that struct from/to the aggregate transition function. > >> > > >> > The attached patch uses this method. > >> > > >> > I tested it using the data generated using: > >> > CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d > FROM > >> > generate_series(1, 1000) s; > >> > > >> > After applying this patch, run time of "SELECT avg(d) FROM avg_test;" > >> > improves from 10.701 seconds to 5.204 seconds, which seems to be a > huge > >> > improvement. > >> > > >> > I think we may also be able to use a similar method to improve > >> > performance > >> > of some other numeric aggregates (like stddev). But I want to know > your > >> > feedback first. > >> > > >> > Is this worth working on? > >> > >> I checked this patch and it has a interesting speedup - and a price of > >> this methoud should not be limited to numeric type only > >> > >> Pavel > >> > >> > > >> > Thanks, > >> > -- Hadi > >> > > >> > > >> > > >> > -- > >> > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > >> > To make changes to your subscription: > >> > http://www.postgresql.org/mailpref/pgsql-hackers > >> > > > > > > numeric-optimize-v2.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
Re: [HACKERS] Trust intermediate CA for client certificates
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 03/19/2013 01:46 PM, Stephen Frost wrote: > If you're using a public CA as your > root, then you need to make sure you know how to ensure only the right > people have access, typically be storing in the mapping table the unique > ID issued by the CA for your users. Yep, in most applications I've seen you usually store a list of authorized SubjectDNs or you just use your own self-signed root and issue certs from it. I'm pretty sure I've seen tools match on part of the DN, like the organisation field, but since I can't remember *where* I'm not sure that's all that useful. > It's very rare, from what I've > seen, for public CAs to issue intermediate CAs to organizations to > generate their own certs off of, so I'm a bit confused about how we got > to this point. I don't know about "very rare" but it's certainly not common outside very large orgs. I tried to find a CA that'd let me issue intermediate client certs for 2ndQuadrant but found nobody that'd do it for certificate volumes less than several thousand new certs a month. I'd been using intermediate CAs based on my own self-signed CA root quite heavily in infrastructure elsewhere I was rather surprised that the same sort of thing wasn't easily available for public CAs. I get the impression it's fairly common in internal infrastructure, especially with the cert management tools offered by Microsoft Active Directory servers, but have no strong information to substantiate this. Nor do I know whether we need to support this mode of operation. BTW, This discussion has made me realise that I know less about SSL/TLS and X.509 certificate extensions than I'd like to when dealing with this topic. In particular, I don't know whether a CA can issue an intermediate CA with extensions that restrict it to validly signing only host certificates for hosts under a particular domain or user-identifying client certs with CNs under a particular organisation - and whether, if such extensions exist, applications actually check them when verifying the certificate trust chain. > What I *have* seen is cross-root-cert trusts (known as the Federal > Bridge in the US government), but that's quite a different thing as you > have multiple self-signed root CAs involved and need to know how to > properly traverse between them based on the trusts which have been > built. Ugh, that's not something I've ever had the ... privilege ... to deal with before. > > I'm no longer convinced that this really makes sense and I'm a bit > worried about the simple authentication issue which I thought was at the > heart of this concern. Is there anything there that you see as being an > issue with what we're doing currently..? Only for using intermediate certs as authorization roots, and it may be reasonable to say "we don't support that, use an authorized DN list". Or come up with a better solution like checking attributes of the SubjectDN for authorization purposes after validating the signature chain to prove authenticity. > I do think we want to figure out a way to improve our mapping table to > be able to use more than just the CN, since that can be repeated in > multiple certs issued from a root CA, particularly when there are > intermediary CAs. One option might be to provide a way to map against a > specific issuing CA, or to a CA in the chain, but there's a lot of risk > to that due to CA churn (in large setups, you're going to have lots of > users who have certs issued from a bunch of different CAs, and those > user certs will roll to new CAs as new badges are issued, for > example..). It can get to be a real nightmare to try and keep up with > all of the changes at that level. Certificate fingerprint? Easily obtained via most client UIs and via openssl x509 -in cert.crt -fingerprint, eg: SHA1 Fingerprint=DA:03:9B:FB:81:69:AB:48:64:3D:35:B4:90:56:CF:F1:24:FE:89:B0 However, if I was managing a group large enough to want cert auth I'd want to be able to specify something like: SubjectDNMatches: C=*, ST=*, L=*, O=MyCompany, CN=* ... in which case there'd no longer be a need to restrict trust to intermediate CAs, you'd just trust the root and restrict the authorized SubjectDNs. If you don't trust your own root CA not to issue certs in your company's name to 3rd parties you shouldn't be using it. (Whether it's actually sane to trust a CA is another argument). - -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRSAJZAAoJELBXNkqjr+S2JrcIALZebfEW4FbfFI6WOs6qDutr tz486SlnPV+cf29ex242evSUgNQTz38uFKMIs9EIRfe7sVKz3whn0MARmQY9dKph CusbXNqcPIBbZIZM1hObaKOnMvNnGk5sxnRh4iKjzcMjqCULG5LVX7bXAXn3PcjA u3lYlNWONWdmz708QOCgvpui4wEv5+bVuik/CnRdPu+BWAcndJHUMuxZMxkUC/rs 4OjLlEg6BPiXRgIKTFBNsa0vvCyVBUd5ri0RCtxUr5T/L/ORWdM+Ic0nqCEPTqyI EOtDKu
Re: [HACKERS] Trust intermediate CA for client certificates
Craig, * Craig Ringer (cr...@2ndquadrant.com) wrote: > They are intermediary, but we're dealing with the case where trust and > authorization are not the same thing. Trust stems from the trusted root > in the SSL CA model, but that's a chain of trust for *identity* > (authentication), not *authorization*. Oh, I see. > The usual SSL terminology doesn't consider this, because it's a simple > back and white trust model where authenticated = authorized. That's not entirely accurate on a couple of levels. First, basic SSL is only concerned with authentication, authorization has historically been left up to the application. The more typical approach with the situation you're describing is to have an organization-level root CA which you only issue certs against. If you're using a public CA as your root, then you need to make sure you know how to ensure only the right people have access, typically be storing in the mapping table the unique ID issued by the CA for your users. It's very rare, from what I've seen, for public CAs to issue intermediate CAs to organizations to generate their own certs off of, so I'm a bit confused about how we got to this point. What I *have* seen is cross-root-cert trusts (known as the Federal Bridge in the US government), but that's quite a different thing as you have multiple self-signed root CAs involved and need to know how to properly traverse between them based on the trusts which have been built. Regarding cross-CAS authorization, there are extended attributes which are listed in the certificates that applications are expected to look at when considering authorization for the client. It's been taken further than that however, where inter-CA trusts have been defined with actual mappings between these extended attributes, including 'null' mappings (indicating that CA 'A' doesn't trust attribute 'q' from CA 'B'). > I guess that suggests we should be calling this something like > 'ssl_authorized_client_roots'. I'm no longer convinced that this really makes sense and I'm a bit worried about the simple authentication issue which I thought was at the heart of this concern. Is there anything there that you see as being an issue with what we're doing currently..? I do think we want to figure out a way to improve our mapping table to be able to use more than just the CN, since that can be repeated in multiple certs issued from a root CA, particularly when there are intermediary CAs. One option might be to provide a way to map against a specific issuing CA, or to a CA in the chain, but there's a lot of risk to that due to CA churn (in large setups, you're going to have lots of users who have certs issued from a bunch of different CAs, and those user certs will roll to new CAs as new badges are issued, for example..). It can get to be a real nightmare to try and keep up with all of the changes at that level. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Trust intermediate CA for client certificates
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 03/18/2013 08:55 PM, Stephen Frost wrote: > Makes sense to me. I'm not particular about the names, but isn't this > set of CAs generally considered intermediary? Eg: 'trusted', ' > intermediate', etc? They are intermediary, but we're dealing with the case where trust and authorization are not the same thing. Trust stems from the trusted root in the SSL CA model, but that's a chain of trust for *identity* (authentication), not *authorization*. Bob J. Criminal might well have a client certificate from a trusted authority proving that he's who he says he is (he's authenticated) but we sure as hell don't want to authorize his access to anything. That's where the intermediate certs come in. We might say "Only users with certificates issued by our corporate HR team are authorized to connect to our servers". This is a root of trust, but this time it's a root of trust to *authorize*, not just to authenticate. The usual SSL terminology doesn't consider this, because it's a simple back and white trust model where authenticated = authorized. I guess that suggests we should be calling this something like 'ssl_authorized_client_roots'. - -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRR/dqAAoJELBXNkqjr+S2TV4H/3f9Hnf9JhSuGhWblh2adgTJ Rkdx/9RbByJDMJP0s0c8C1sXaWZGJmKmLhJoes4IIvOVW85SVUa9WoT+UBJPdx9P esUNsSLFokLqom3TxNRZOHaloyZ+OZafSUnKCwMOIvD0hIehrS3Wcg70QMSj06tX h22BVhA8bzO1Wdg9UdD98jcuWdEbLgWzVtvIXjICcMJ1azgiF1VY4zwUUbBJBfLG UIA7+2TtVaXQuge6qWgId0RTKKrb6cLHXCSQ/rigy0mRH9m/G5jKmqENvLAnafI4 4lSBPyDzNj2fBfP9YgIiAe/EGjnJMWQfBBghQI3QrK2kjOZXtzZoOb4XEjfn3FI= =u+2j -END PGP SIGNATURE- -- 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] Optimizing pglz compressor
On Wed, Mar 6, 2013 at 6:32 AM, Joachim Wieland wrote: > On Tue, Mar 5, 2013 at 8:32 AM, Heikki Linnakangas > wrote: >> With these tweaks, I was able to make pglz-based delta encoding perform >> roughly as well as Amit's patch. > > Out of curiosity, do we know how pglz compares with other algorithms, e.g. > lz4 ? This one is for the archives, as I thought it surprising: there can be a surprisingly huge magnitude of performance difference of these algorithms depending on architecture. Here's a table reproduced from: http://www.reddit.com/r/programming/comments/1aim6s/lz4_extremely_fast_compression_algorithm/c8y0ew9 """ testdata/alice29.txt : ZLIB:[b 1M] bytes 152089 -> 54404 35.8% comp 0.8 MB/s uncomp 8.1 MB/s LZO: [b 1M] bytes 152089 -> 82721 54.4% comp 14.5 MB/s uncomp 43.0 MB/s CSNAPPY: [b 1M] bytes 152089 -> 90965 59.8% comp 2.1 MB/s uncomp 4.4 MB/s SNAPPY: [b 4M] bytes 152089 -> 90965 59.8% comp 1.8 MB/s uncomp 2.8 MB/s testdata/asyoulik.txt: ZLIB:[b 1M] bytes 125179 -> 48897 39.1% comp 0.8 MB/s uncomp 7.7 MB/s LZO: [b 1M] bytes 125179 -> 73224 58.5% comp 15.3 MB/s uncomp 42.4 MB/s CSNAPPY: [b 1M] bytes 125179 -> 80207 64.1% comp 2.0 MB/s uncomp 4.2 MB/s SNAPPY: [b 4M] bytes 125179 -> 80207 64.1% comp 1.7 MB/s uncomp 2.7 MB/s LZO was ~8x faster compressing and ~16x faster decompressing. Only on uncompressible data was Snappy was faster: testdata/house.jpg : ZLIB:[b 1M] bytes 126958 -> 126513 99.6% comp 1.2 MB/s uncomp 9.6 MB/s LZO: [b 1M] bytes 126958 -> 127173 100.2% comp 4.2 MB/s uncomp 74.9 MB/s CSNAPPY: [b 1M] bytes 126958 -> 126803 99.9% comp 24.6 MB/s uncomp 381.2 MB/s SNAPPY: [b 4M] bytes 126958 -> 126803 99.9% comp 22.8 MB/s uncomp 354.4 MB/s """ So that's one more gotcha to worry about, since I surmise most numbers are being taken on x86. Apparently this has something to do with alignment of accesses. Some of it may be fixable by tweaking the implementation rather than the compression encoding, although I am no expert in the matter. -- fdr -- 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] Enabling Checksums
On Mon, Mar 18, 2013 at 7:13 PM, Greg Smith wrote: > I wasn't trying to flog EBS as any more or less reliable than other types of > storage. What I was trying to emphasize, similarly to your "quite a > stretch" comment, was the uncertainty involved when such deployments fail. > Failures happen due to many causes outside of just EBS itself. But people > are so far removed from the physical objects that fail, it's harder now to > point blame the right way when things fail. I didn't mean to imply you personally were going out of your way to flog EBS, but there is a sufficient vacuum in the narrative that someone could reasonably interpereted it that way, so I want to set it straight. The problem is the quantity of databases per human. The Pythons said it best: 'A simple question of weight ratios.' > A quick example will demonstrate what I mean. Let's say my server at home > dies. There's some terrible log messages, it crashes, and when it comes > back up it's broken. Troubleshooting and possibly replacement parts follow. > I will normally expect an eventual resolution that includes data like "the > drive showed X SMART errors" or "I swapped the memory with a similar system > and the problem followed the RAM". I'll learn something about what failed > that I might use as feedback to adjust my practices. But an EC2+EBS failure > doesn't let you get to the root cause effectively most of the time, and that > makes people nervous. Yes, the layering makes it tougher to do vertical treatment of obscure issues. Redundancy has often been the preferred solution here: bugs come and go all the time, and everyone at each level tries to fix what they can without much coordination from the layer above or below. There are hopefully benefits in throughput of progress at each level from this abstraction, but predicting when any one particular issue will go understood top to bottom is even harder than it already was. Also, I think the line of reasoning presented is biased towards a certain class of database: there are many, many databases with minimal funding and oversight being run in the traditional way, and the odds they'll get a vigorous root cause analysis in event of an obscure issue is already close to nil. Although there are other considerations at play (like not just leaving those users with nothing more than a "bad block" message), checksums open some avenues gradually benefit those use cases, too. > I can already see "how do checksums alone help narrow the blame?" as the > next question. I'll post something summarizing how I use them for that > tomorrow, just out of juice for that tonight. Not from me. It seems pretty intuitive from here how database maintained checksums assist in partitioning the problem. -- fdr -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] backward incompatible pg_basebackup and pg_receivexlog
pg_basebackup and pg_receivexlog from 9.3 won't work with earlier servers anymore. I wonder if this has been fully thought through. We have put in a lot of effort to make client programs compatible with many server versions as well as keeping the client/server protocol compatible across many versions. Both of these assumptions are now being broken, which will result in all kinds of annoyances. It seems to me that these tools could probably be enhanced to understand both old and new formats. Also, using the old tools against new server versions either behaves funny or silently appears to work, both of which might be a problem. I think if we are documenting the replication protocol as part of the frontend/backend protocol and are exposing client tools that use it, changes need to be done with the same rigor as other protocol changes. As far as I can tell, there is no separate version number for the replication part of the protocol, so either there needs to be one or the protocol as a whole needs to be updated. -- 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] machine-parseable object descriptions
On 2013.03.18 1:03 PM, Alvaro Herrera wrote: For Dimitri's patch to add support for dropped objects in event triggers, there's an open question about how to report objects that are being dropped in a tabular format. Now that JSON is a built-in type with 9.2+, could we not perhaps use that to represent some things in a more natural manner than a tabular format? JSON is designed to be machine-parseable, and some objects such as routine definitions are naturally trees of arbitrary depth. -- Darren Duncan -- 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] Enabling Checksums
On 3/18/13 5:36 PM, Daniel Farina wrote: Clarification, because I think this assessment as delivered feeds some unnecessary FUD about EBS: EBS is quite reliable. Presuming that all noticed corruptions are strictly EBS's problem (that's quite a stretch), I'd say the defect rate falls somewhere in the range of volume-centuries. I wasn't trying to flog EBS as any more or less reliable than other types of storage. What I was trying to emphasize, similarly to your "quite a stretch" comment, was the uncertainty involved when such deployments fail. Failures happen due to many causes outside of just EBS itself. But people are so far removed from the physical objects that fail, it's harder now to point blame the right way when things fail. A quick example will demonstrate what I mean. Let's say my server at home dies. There's some terrible log messages, it crashes, and when it comes back up it's broken. Troubleshooting and possibly replacement parts follow. I will normally expect an eventual resolution that includes data like "the drive showed X SMART errors" or "I swapped the memory with a similar system and the problem followed the RAM". I'll learn something about what failed that I might use as feedback to adjust my practices. But an EC2+EBS failure doesn't let you get to the root cause effectively most of the time, and that makes people nervous. I can already see "how do checksums alone help narrow the blame?" as the next question. I'll post something summarizing how I use them for that tomorrow, just out of juice for that tonight. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.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_upgrade segfaults when given an invalid PGSERVICE value
On Mon, Mar 18, 2013 at 12:08:09PM -0400, Steve Singer wrote: > If you try running pg_upgrade with the PGSERVICE environment > variable set to some invalid/non-existent service pg_upgrade > segfaults > > Program received signal SIGSEGV, Segmentation fault. > 0x0040bdd1 in check_pghost_envvar () at server.c:304 > 304 for (option = start; option->keyword != NULL; option++) > (gdb) p start > $5 = (PQconninfoOption *) 0x0 > > > PQconndefaults can return NULL if it has issues. > > The attached patch prints a minimally useful error message. I don't > a good way of getting a more useful error message out of > PQconndefaults() > > I checked this against master but it was reported to me as a issue in 9.2 Well, that's interesting. There is no mention of PQconndefaults() returning NULL except for out of memory: Returns a connection options array. This can be used to determine all possible PQconnectdb options and their current default values. The return value points to an array of PQconninfoOption structures, which ends with an entry having a null keyword pointer. The -->null pointer is returned if memory could not be allocated. Note that the current default values (val fields) will depend on environment variables and other context. Callers must treat the connection options data as read-only. Looking at libpq/fe-connect.c::PQconndefaults(), it calls conninfo_add_defaults(), which has this: /* * If there's a service spec, use it to obtain any not-explicitly-given * parameters. */ if (parseServiceInfo(options, errorMessage) != 0) return false; so it is clearly possible for PQconndefaults() to return NULL for service file failures. The questions are: * Is this what we want? * Should we document this? * Should we change this to just throw a warning? Also, it seems pg_upgrade isn't the only utility that is confused: contrib/postgres_fdw/option.c and contrib/dblink/dblink.c think PQconndefaults() returning NULL means out of memory and report that as the error string. bin/scripts/pg_isready.c and contrib/pg_upgrade/server.c have no check for NULL return. libpq/test/uri-regress.c knows to throw a generic error message. So, we have some decisions and work to do. -- Bruce Momjian http://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] Enabling Checksums
On Mon, Mar 18, 2013 at 1:31 PM, Greg Smith wrote: > On 3/18/13 10:52 AM, Bruce Momjian wrote: >> >> With a potential 10-20% overhead, I am unclear who would enable this at >> initdb time. > > > If you survey people who are running PostgreSQL on "cloud" hardware, be it > Amazon's EC2 or similar options from other vendors, you will find a high > percentage of them would pay quite a bit of performance to make their > storage more reliable. To pick one common measurement for popularity, a > Google search on "ebs corruption" returns 17 million hits. To quote one of > those, Baron Schwartz of Percona talking about MySQL on EC2:> > "BTW, I have seen data corruption on EBS volumes. It’s not clear whether it > was InnoDB’s fault (extremely unlikely IMO), the operating system’s fault, > EBS’s fault, or something else." Clarification, because I think this assessment as delivered feeds some unnecessary FUD about EBS: EBS is quite reliable. Presuming that all noticed corruptions are strictly EBS's problem (that's quite a stretch), I'd say the defect rate falls somewhere in the range of volume-centuries. I want to point this out because I think EBS gets an outsized amount of public flogging, and not all of it is deserved. My assessment of the caustion at hand: I care about this feature not because EBS sucks more than anything else by a large degree, but because there's an ever mounting number of EBS volumes whose defects are under the responsibility of comparatively few individuals. -- fdr -- 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] Enabling Checksums
On Mon, Mar 18, 2013 at 2:04 AM, Greg Smith wrote: > On 3/15/13 5:32 AM, Ants Aasma wrote: >> >> Best case using the CRC32 instruction would be 6.8 bytes/cycle [1]. >> But this got me thinking about how to do this faster... >> [1] >> http://www.drdobbs.com/parallel/fast-parallelized-crc-computation-using/229401411 > > > The optimization work you went through here looked very nice. Unfortunately, > a few things seem pushing toward using a CRC16 instead of the Fletcher > approach. It seems possible to execute a CRC16 in a reasonable enough time, > in the same neighborhood as the Fletcher one. And there is some hope that > hardware acceleration for CRCs will be available in a system API/compiler > feature one day, making them even cheaper. > > Ants, do you think you could take a similar look at optimizing a CRC16 > calculation? I'm back to where I can do a full performance comparison run > again starting tomorrow, with the latest version of this patch, and I'd like > to do that with a CRC16 implementation or two. I'm not sure if it's > possible to get a quicker implementation because the target is a CRC16, or > whether it's useful to consider truncating a CRC32 into a CRC16. I looked for fast CRC implementations on the net. The fastest plain C variant I could find was one produced by Intels R&D department (available with a BSD license [1], requires some porting). It does 8 x 8bit table lookups in parallel, requiring a 8*256*4 = 8kB lookup table. Using the table lookup method CRC16 would run at exactly the same speed but the table would be 2x smaller. There is also an option to do 4 lookup tables, this approach is said to be about 2x slower for 2x less data. I took a look at assembly generated for the slice-by-8 algorithm. It seems to me that GCC for some mysterious reason decides to accumulate the xor's in a serial chain, losing superscalar execution possibilities. If it could be coaxed into accumulating xor's in a tree pattern the performance should improve somewhere between 1.5 and 2x. For CRC32C there is also an option to use the crc32 instructions available on newer Intel machines and run 3 parallel CRC calculations to cover for the 3 cycle latency on that instruction, combining them in the end [2]. Skimming the paper it looks like there are some patents in this area, so if we wish to implement this, we would have to see how we can navigate around them. The other issue is that crc32 instruction is Intel only so far. The cited performance is 6.8 bytes/cycle. There is also an option to use pclmulqdq instruction to do generic CRC's in 16byte blocks. This is available on Intel Westmere and up (2010+) and AMD Bulldozer and up (2011+). Sample ASM code is available in the Intel paper. [3] Cited speed is 0.88 bytes/cycle. I lifted the benchmark framework of the 8 byte slicing method from the Intel code and ran some tests on the implementations I had available - the 8 byte slicing CRC from Intel, fletcher from the checksum patch, my parallel 16bit checksums approach and a hand coded 32bit parallel checksum I had (requires SSE4.1 as implemented but on sandy bridge platform the performance should be equivalent to a 16bit one that requires only SSE2). So here come the results: gcc4.7 -O2, 8192byte buffer: CRC32 slicing by 8 Algorithm (bytes/cycle), 0.524249 Fletcher Algorithm: (bytes/cycle), 1.930567 SIMD Algorithm (gcc): (bytes/cycle), 0.575617 SIMD Algorithm (hand coded): (bytes/cycle), 9.196853 gcc4.7 -O2 -ftree-vectorize -funroll-loops, 8192byte buffer: CRC32 slicing by 8 Algorithm (bytes/cycle), 0.523573 Fletcher Algorithm: (bytes/cycle), 3.316269 SIMD Algorithm (gcc): (bytes/cycle), 7.866682 SIMD Algorithm (hand coded): (bytes/cycle), 9.114214 Notes: * As you can see, CRC based approach would have 4x larger performance overhead compared to the Fletcher algorithm as implemented in the current patch. * This benchmark is the best case for the slicing CRC algorithm. Real world uses might not have the lookup table in cache. * We should probably check what the code path length from read syscall to checksum calculation is. We don't want it to contain something that would push the page out from cache. * Even a pclmulqdq based implementation would be a lot slower than Fletcher. * The Fletcher algorithm benefits greatly from unrolling as the loop body is so cheap and the algorithm is ALU bound. * As predicted the SIMD algorithm is quite slow if the compiler won't vectorize it. But notice that the performance is comparable to unaccelerated CRC. * The vectorized SIMD gcc variant is outperforming the claimed performance of hardware accelerated crc32 using only SSE2 features (available in the base x86-64 instruction set). The gap isn't large though. * Vectorized SIMD code performance is surprisingly close to handcoded. Not sure if there is something holding back the handcoded version or if the measurement overhead is coming into play here. This would require further investigation. perf accounted 25% of execution
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Tue, Mar 19, 2013 at 8:54 AM, Michael Paquier wrote: > > > On Tue, Mar 19, 2013 at 3:03 AM, Fujii Masao wrote: > >> On Sun, Mar 17, 2013 at 9:24 PM, Michael Paquier >> wrote: >> > Please find attached the patches wanted: >> > - 20130317_dump_only_valid_index.patch, a 1-line patch that makes >> pg_dump >> > not take a dump of invalid indexes. This patch can be backpatched to >> 9.0. >> >> Don't indisready and indislive need to be checked? >> >> The patch seems to change pg_dump so that it ignores an invalid index only >> when the remote server version >= 9.0. But why not when the remote server >> version < 9.0? >> >> I think that you should start new thread to get much attention about this >> patch >> if there is no enough feedback. >> > Yeah... Will send a message about that... > > >> >> > Note that there have been some recent discussions about that. This >> *problem* >> > also concerned pg_upgrade. >> > >> http://www.postgresql.org/message-id/20121207141236.gb4...@alvh.no-ip.org >> >> What's the conclusion of this discussion? pg_dump --binary-upgrade also >> should >> ignore an invalid index? pg_upgrade needs to be changed together? >> > The conclusion is that pg_dump should not need to include invalid indexes > if it is > to create them as valid index during restore. However I haven't seen any > patch... > The fix has been done inside pg_upgrade: http://momjian.us/main/blogs/pgblog/2012.html#December_14_2012 Nothing has been done for pg_dump. -- Michael
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Tue, Mar 19, 2013 at 3:24 AM, Fujii Masao wrote: > On Wed, Mar 13, 2013 at 9:04 PM, Michael Paquier > wrote: > > I have been working on improving the code of the 2 patches: > > 1) reltoastidxid removal: > > > - Fix a bug with pg_dump and binary upgrade. One valid index is necessary > > for a given toast relation. > > Is this bugfix related to the following? > > appendPQExpBuffer(upgrade_query, > - "SELECT c.reltoastrelid, > t.reltoastidxid " > + "SELECT c.reltoastrelid, > t.indexrelid " > "FROM pg_catalog.pg_class c LEFT > JOIN " > - "pg_catalog.pg_class t ON > (c.reltoastrelid = t.oid) " > - "WHERE c.oid = > '%u'::pg_catalog.oid;", > + "pg_catalog.pg_index t ON > (c.reltoastrelid = t.indrelid) " > + "WHERE c.oid = > '%u'::pg_catalog.oid AND t.indisvalid " > + "LIMIT 1", > Yes. > Don't indisready and indislive need to be checked? > An index is valid if it is already ready and line. We could add such check for safely but I don't think it is necessary. Why is LIMIT 1 required? The toast table can have more than one toast > indexes? > It cannot have more than one VALID index, so yes as long as a check on indisvalid is here there is no need to worry about a LIMIT condition. I only thought of that as a safeguard. The same thing applies to the addition of a condition based on indislive and indisready. -- Michael
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Tue, Mar 19, 2013 at 3:03 AM, Fujii Masao wrote: > On Sun, Mar 17, 2013 at 9:24 PM, Michael Paquier > wrote: > > Please find attached the patches wanted: > > - 20130317_dump_only_valid_index.patch, a 1-line patch that makes pg_dump > > not take a dump of invalid indexes. This patch can be backpatched to 9.0. > > Don't indisready and indislive need to be checked? > > The patch seems to change pg_dump so that it ignores an invalid index only > when the remote server version >= 9.0. But why not when the remote server > version < 9.0? > > I think that you should start new thread to get much attention about this > patch > if there is no enough feedback. > Yeah... Will send a message about that... > > > Note that there have been some recent discussions about that. This > *problem* > > also concerned pg_upgrade. > > > http://www.postgresql.org/message-id/20121207141236.gb4...@alvh.no-ip.org > > What's the conclusion of this discussion? pg_dump --binary-upgrade also > should > ignore an invalid index? pg_upgrade needs to be changed together? > The conclusion is that pg_dump should not need to include invalid indexes if it is to create them as valid index during restore. However I haven't seen any patch... -- Michael
Re: [HACKERS] Add some regression tests for SEQUENCE
Duh. Apologies. That's what happens when you make that 1 last change. Please find an updated patch. -- Robins Tharakan On 19 March 2013 04:07, Josh Kupershmidt wrote: > On Mon, Mar 18, 2013 at 3:10 PM, Robins Tharakan > wrote: > > Hi, > > > > Please find an updated patch (reworked on the names of SEQUENCES / ROLES > / > > SCHEMA etc.) > > Takes code-coverage of 'make check' for SEQUENCE to ~95%. > > There is a typo difference between sequence.out and sequence.sql > causing the test to fail: > > +-- Should fail since seq5 shouldn't exist > ... > +-- Should fail since seq5 shouldn't exit > > Josh > regress_sequence_v3.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
Re: [HACKERS] Add some regression tests for SEQUENCE
On Mon, Mar 18, 2013 at 3:10 PM, Robins Tharakan wrote: > Hi, > > Please find an updated patch (reworked on the names of SEQUENCES / ROLES / > SCHEMA etc.) > Takes code-coverage of 'make check' for SEQUENCE to ~95%. There is a typo difference between sequence.out and sequence.sql causing the test to fail: +-- Should fail since seq5 shouldn't exist ... +-- Should fail since seq5 shouldn't exit Josh -- 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] machine-parseable object descriptions
Tom Lane writes: > I could also live with keeping the schema column as proposed, if people > think it has a use, but letting it be redundant with a schema name > included in the identity string. But it seems like a bad idea to try to > shear schema off of identity. +1 Use case for keeping the extra column: replication to a federated database where you want to tweak the target schema. 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] Add some regression tests for SEQUENCE
Hi, Please find an updated patch (reworked on the names of SEQUENCES / ROLES / SCHEMA etc.) Takes code-coverage of 'make check' for SEQUENCE to ~95%. -- Robins Tharakan On 16 March 2013 02:03, robins wrote: > Hi, > > I've added some regression tests for SEQUENCE. A cumulative patch is > attached. > > Barring a (still to decipher) function seq_redo() and trying to learn how > to actually test it, this takes care of most branches of ( > src/backend/commands/sequence.c) taking code-coverage (of 'make check') > to ~95%. > > Any feedback is more than welcome. > regress_sequence_v2.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
Re: [HACKERS] Trust intermediate CA for client certificates
On 03/18/2013 02:01 AM, Craig Ringer wrote:
> This appears to match Ian's description of having a validation-only cert
> list and a separate list of certs used to verify clients. I'd like to
> follow Apache's model:
Ready for some more good news?
It's possible that I'm missing something, but Apache (mod_ssl) appears
to exhibit the exact same behavior.
Tested on Fedora 18, with the following packages:
httpd-2.4.3-15.fc18.x86_64
mod_ssl-2.4.3-15.fc18.x86_64
openssl-1.0.1e-3.fc18.x86_64
I have set the following in /etc/httpd/conf.d/ssl.conf:
Listen 3 https
ServerName postgres.example.com
SSLCertificateFile /etc/pki/tls/certs/postgres.crt
SSLCertificateKeyFile /etc/pki/tls/private/postgres.key
SSLCACertificateFile /etc/pki/tls/certs/client-ca.chain
SSLCADNRequestFile /etc/pki/tls/certs/client-ca.crt
SSLVerifyClient require
SSLVerifyDepth 10
Notes:
* The port is set to 3, because that's hard-coded into the
(attached) test client.
* I am using the certificates that I previously sent.
* ServerName is set to postgres.example.com to match its certificate.
* postgres.crt contains its entire chain (postgres.crt + server-ca.crt
+ root-ca.crt), so that I don't have to specify a
SSLCertificateChainFile.
* client-ca.chain is client-ca.crt + root-ca.crt. As with PostgreSQL,
I found that I have to provide the root CA certificate in order for
any client to connect.
With this configuration, the test client is able to connect with the
"good" client certificate, but it is also able to connect with the "bad"
client certificate when it presents a certificate chain that includes
the server CA certificate.
--
Ian Pilcher arequip...@gmail.com
Sometimes there's nothing left to do but crash and burn...or die trying.
#include
#include
#include
#include
#include
#include
#include
#include
int main(int argc, char *argv[])
{
SSL_CTX *ctx;
SSL *ssl;
struct sockaddr_in server_addr;
int sock_fd;
if (argc != 3) {
fprintf(stderr, "USAGE: %s \n", argv[0]);
exit(__LINE__);
}
SSL_load_error_strings();
SSL_library_init();
if ((ctx = SSL_CTX_new(SSLv23_client_method())) == NULL) {
ERR_print_errors_fp(stderr);
exit(__LINE__);
}
if (SSL_CTX_use_certificate_chain_file(ctx, argv[1]) != 1) {
ERR_print_errors_fp(stderr);
exit(__LINE__);
}
if (SSL_CTX_use_PrivateKey_file(ctx, argv[2], SSL_FILETYPE_PEM) != 1) {
ERR_print_errors_fp(stderr);
exit(__LINE__);
}
if (SSL_CTX_check_private_key(ctx) != 1) {
ERR_print_errors_fp(stderr);
exit(__LINE__);
}
memset(&server_addr, 0, sizeof server_addr);
server_addr.sin_family = AF_INET;
server_addr.sin_port = htons(3);
server_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
if ((sock_fd = socket(AF_INET, SOCK_STREAM, 0)) == -1) {
perror("socket");
exit(__LINE__);
}
if (connect(sock_fd, (struct sockaddr *)&server_addr,
sizeof server_addr) == -1) {
perror("connect");
exit(__LINE__);
}
puts("Connected. Starting SSL handshake.");
if ((ssl = SSL_new(ctx)) == NULL) {
ERR_print_errors_fp(stderr);
exit(__LINE__);
}
if (SSL_set_fd(ssl, sock_fd) == 0) {
ERR_print_errors_fp(stderr);
exit(__LINE__);
}
if (SSL_connect(ssl) != 1) {
ERR_print_errors_fp(stderr);
exit(__LINE__);
}
puts("SSL handshake successful. Shutting down.");
return 0;
}
--
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] Materialized view assertion failure in HEAD
Tom Lane wrote: > Kevin Grittner writes: >> Tom Lane wrote: >>> [ why not allow matviews to have OID columns? ] > >> An oid column in a materialized view will not be significantly more >> stable than its ctid. The same "logical" row could easily have a >> different OID on a REFRESH or even an incremental update (due to >> the probable need to handle some incremental updates by deleting >> and re-inserting portions of the MV). Allowing an "Object ID" to >> be generated when it is not as stable as the name or its usage in a >> table might suggest seems likely to lead to all kinds of trouble >> and false bug reports down the road. > > Meh. I don't find that argument all that convincing. It'd pass as a > rationale for not supporting OIDs if we'd found it was hard to do so; > but I don't find it strong enough to justify introducing dirty kluges to > prevent people from creating matviews with OIDs. OIDs in regular tables > aren't a sure thing as a row identifier either. Moreover, if we are > afraid of people expecting OIDs to act in a particular way, why are > we exposing other system columns in matviews? ctid, xmin, etc all might > have behaviors that differ from a "pure view", or that we find it > convenient to change from release to release. > >>> Anyway, I think it makes more sense to go in the direction of >>> making the case work than to introduce messy kluges to prevent >>> matviews from having OIDs. > >> If you can come up with a single use case where having OIDs for >> them makes sense and is useful, rather than being a loaded foot-gun >> for users, it would go a long way toward convincing me that is the >> way to go. > > Arguably, OIDs in regular tables are a loaded foot-gun already: they > aren't unique unless you go out of your way to make them so, and they > aren't wide enough to be reliably unique for modern volumes of data. > There's a reason why they're deprecated. That being the case, I'm > entirely fine with taking the whats-the-easiest-implementation approach > to whether they can be in matviews or not. > > IOW, I'd not object to prohibiting them if it were possible to do so > with a small, clean patch in an unsurprising place. But it appears > to me that it's messier to prohibit them than to allow them. Well, we agree on that, anyway. :-/ >> Granted, my first go at switching away from that didn't >> touch all the bases, but at this point it's probably at least as >> much of a change to revert to supporting OIDs as to fix the >> omissions. I will also grant that I haven't been able to see a >> really pretty way to do it, but that is mostly because the whole >> area of handling this is full of grotty special cases which need to >> be handled already. > > Quite, which is why I'm objecting to adding more special cases; and > I think that disallowing this for matviews is going to end up requiring > exactly that. I'd rather see the code just treating matviews > identically to regular relations for OID-related purposes. I would really like to hear the opinions of others here. Basically, I think Tom and I are agreeing that OIDs already require some ugly hacks and they are not of any practical use in a materialized view, but that there's no way to prohibit their use in MVs without additional ugly hacks. (Please correct me if I'm mis-characterizing your POV.) We're coming down on opposite sides of the fence on whether it is worth compounding the source code hackery to keep this away from MV users who will probably occasionally (mis)use it and then tell us we have a bug. The most common form of such a "bug" report would probably be that they used the oid to identify a row, and due to a REFRESH or due to updates to an underlying table the same "logical" row in the MV suddenly has a new oid and their link is broken. To dance around the existing hacks requires that the new hacks violate modularity to some degree. I've come around to the POV that the least invasive and least fragile form of hack for this is to essentially mock up a "WITH (oids = false)" at CREATE MATERIALIZED VIEW parse time and let that ripple through the rest of the layers. >>> ISTM that suppressing this check during a matview refresh is >>> rather broken, because then it won't complain if the matview >>> reads some other matview that's in a nonscannable state. Is that >>> really the behavior we want? > >> If that is what you got out of the comment, the comment probably >> needs some work. > > [ looks a bit more... ] Indeed, the comment appears to be entirely > unrelated to reality. I'll wait until the other issues are sorted to fix that. > I still think that implementing this check in a rangetable scan > is probably broken or at least overly complicated, though. The > fact that a rel is in the rangetable is not evidence that it's > going to be read, so you're going to need some fragile logic to > avoid throwing undesirable errors. Moreover, > ExecCheckRelationsScannable has added an additional relcache > ope
Re: [HACKERS] machine-parseable object descriptions
Alvaro Herrera writes: > For Dimitri's patch to add support for dropped objects in event > triggers, there's an open question about how to report objects that are > being dropped in a tabular format. What I proposed last had three > columns: (type, schema, identity). The "type" is a description of the > object class; I propose the following list: > > ... > > Ellipses hide uninteresting examples. Opinions on this please? In my view, it needs to be the same thing as the second part of the command tag used for commands on the given type of objects. > Then we have the identity, which is a texualt representation of the > other stuff needed to uniquely identify the object being referred to. > For most stuff it's just the name, but for other things it is a bit more > complex. For example, for a function it'd be the function signature; > for a table (and for most other object classes) it's the table name. > For columns, it's the qualified column name i.e. > .. Note the schema name never appears here, even > if the object is not in path; the whole point of this is to be > machine-parseable and we have the schema separately anyway. So some > tweaks to functions such as format_type_internal are necessary, to > supress schema-qualifying when not wanted. As long as we keep also the name in a separate field, I'm in. Also, that needs to be properly quoted by the backend (MixedCase, extra dots, etc) as we just said on an external communication channel. > I think this kind of machine-parseable object representation would be > useful for cases other than event triggers, so I am thinking in > committing this part before the pg_dropped_objects patch. I haven't > split it out yet from the event triggers patch; I will be sending a > patch later, in the meantime I welcome comments on the above. +1 Maybe that can also set the debate for which information to pass down in Event Trigger User Functions and how. Still don't want to make that a datatype so that we expose a single variable per object? 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] machine-parseable object descriptions
Alvaro Herrera writes: > For Dimitri's patch to add support for dropped objects in event > triggers, there's an open question about how to report objects that are > being dropped in a tabular format. What I proposed last had three > columns: (type, schema, identity). The "type" is a description of the > object class; I propose the following list: I'm okay with the proposed type names. > After the object type we have the schema, which would be NULL for > objects that don't belong to schemas (extensions, access methods, casts, > languages, etc). > Then we have the identity, which is a texualt representation of the > other stuff needed to uniquely identify the object being referred to. I think you should seriously consider dropping the separate "schema" column and instead plugging the schema in at the appropriate place in the identity string (which, evidently, is not always going to be at the front). Otherwise, how will client code know how to assemble the schema onto the identity to make a usable name? It's also pretty weird that some of the names appearing in an identity will be fully qualified but the most important one isn't. I could also live with keeping the schema column as proposed, if people think it has a use, but letting it be redundant with a schema name included in the identity string. But it seems like a bad idea to try to shear schema off of identity. 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] Enabling Checksums
On 3/18/13 10:52 AM, Bruce Momjian wrote: With a potential 10-20% overhead, I am unclear who would enable this at initdb time. If you survey people who are running PostgreSQL on "cloud" hardware, be it Amazon's EC2 or similar options from other vendors, you will find a high percentage of them would pay quite a bit of performance to make their storage more reliable. To pick one common measurement for popularity, a Google search on "ebs corruption" returns 17 million hits. To quote one of those, Baron Schwartz of Percona talking about MySQL on EC2: "BTW, I have seen data corruption on EBS volumes. It’s not clear whether it was InnoDB’s fault (extremely unlikely IMO), the operating system’s fault, EBS’s fault, or something else." http://www.mysqlperformanceblog.com/2011/08/04/mysql-performance-on-ec2ebs-versus-rds/ *That* uncertainty is where a lot of the demand for this feature is coming from. People deploy into the cloud, their data gets corrupted, and no one call tell them what/why/how it happened. And that means they don't even know what to change to make it better. The only people I see really doing something about this problem all seem years off, and I'm not sure they are going to help--especially since some of them are targeting "enterprise" storage rather than the cloud-style installations. I assume a user would wait until they suspected corruption to turn it on, and because it is only initdb-enabled, they would have to dump/reload their cluster. The open question is whether this is a usable feature as written, or whether we should wait until 9.4. The reliability issues of both physical and virtual hardware are so widely known that many people will deploy with this on as their default configuration. If you don't trust your existing data, you can't retroactively check it. A checksum of an already corrupt block is useless. Accordingly, there is no use case for converting an installation with real or even suspected problems to a checksummed one. If you wait until you suspect corruption to care about checksums, it's really too late. There is only one available next step: you must do a dump to figure out what's readable. That is the spot that all of the incoming data recovery customers we see at 2ndQuadrant are already in when we're called. The cluster is suspicious, sometimes they can get data out of it with a dump, and if we hack up their install we can usually recover a bit more than they could. After the data from a partially corrupted database is dumped, someone who has just been through that pain might decide they should turn checksums on when they restore the dump. When it's on, they can access future damage easily at the block level when it happens, and possibly repair it without doing a full dump/reload. What's implemented in the feature we're talking about has a good enough UI to handle this entire cycle I see damaged installations go through. Good questions, Bruce, I don't think the reasons behind this feature's demand have been highlighted very well before. I try not to spook the world by talking regularly about how many corrupt PostgreSQL databases I've seen, but they do happen. Right now we have two states: "believed good" and "believed corrupted"--and the transitions between them are really nasty. Just being able to quantify corruption would be a huge improvement. Related aside, most of my regular ranting on crappy SSDs that lie about writes comes from a TB scale PostgreSQL install that got corrupted due to the write-cache flaws of the early Intel SSDs--twice. They would have happily lost even the worst-case 20% of regular performance to avoid going down for two days each time they saw corruption, where we had to dump/reload to get them going again. If the install had checksums, I could have figured out which blocks were damaged and manually fixed them, basically go on a hunt for torn pages and the last known good copy via full-page write. Without checksums, there really was nowhere to go with them except dump/reload. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.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] JSON Function Bike Shedding
On 03/18/2013 12:29 PM, Andrew Dunstan wrote: > > One wrinkle in this picture is the variadic forms of extraction which > don't lend themselves nicely to use with an operator. We could decide to > do away with those altogether, or come up with a better name. I'm loath > to use "json_path" since it's a name used for something similar but > different elsewhere. I do think it's valuable to have the variadic form, > though, and I'd be sad to see it go. Given that the variadic form is meant to be the foundation of future tree-based indexing of JSON values, I really don't want to do without it. Plus, I'd be forced to reimplement it in my own code. But the name does need work. json_tree? Hmmm, no good ... -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] JSON Function Bike Shedding
Andrew Dunstan writes:
> I've been sitting here for a while mulling none too happily over the
> debate on the names for the proposed JSON extraction functions. I
> haven't really been happy with any of the suggestions, much, not least
> my own original function names which were really only intended as
> placeholders. Last night in the still watches I decided I just couldn't
> go with a function name as almost totally content-free as get(), or even
> get_text(). And I don't think prepending "json_'" to the name helps much
> either.
Agreed.
> Just concentrating to start with on those get() functions, in the simple
> case we really don't need them at all. hstore has the "->" operator
> without documenting the underlying function ("fetchval"). So maybe we
> should just do that.
Well, not documenting the underlying function does not relieve you from
having to name it in a reasonably sane fashion. It still wouldn't do
to call it "get()".
> * I'd be inclined to stick with json_array_length() and
> json_object_keys() - I think they describe pretty well what they do.
> hstore's skeys() does more or less the same as json_object_keys(),
> so we could use that if we want to be consistent. I don't think it's
> a terribly good name though.
> * json_unnest() should certainly be renamed. Alternatives that come to
> mind are json_unfold() or json_elements() or json_array_elements().
> * json_each(), json_each_as_text(), json_populate_record() and
> json_populate_recordset() - to be consistent with hstore we could
> remove the "json_". We probably should remove the "_as_ from
> json_each_as_text().
I don't particularly have a dog in this fight, but do we really want
some of these to have a json_ prefix and others not?
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
[HACKERS] machine-parseable object descriptions
For Dimitri's patch to add support for dropped objects in event triggers, there's an open question about how to report objects that are being dropped in a tabular format. What I proposed last had three columns: (type, schema, identity). The "type" is a description of the object class; I propose the following list: table view materialized view foreign table sequence ... function aggregate type cast collation table constraint domain constraint default value ... operator of access method function of access method ... table column view column materialized view column ... default acl for relations default acl for sequences default acl for functions default acl for types Ellipses hide uninteresting examples. Opinions on this please? After the object type we have the schema, which would be NULL for objects that don't belong to schemas (extensions, access methods, casts, languages, etc). Then we have the identity, which is a texualt representation of the other stuff needed to uniquely identify the object being referred to. For most stuff it's just the name, but for other things it is a bit more complex. For example, for a function it'd be the function signature; for a table (and for most other object classes) it's the table name. For columns, it's the qualified column name i.e. .. Note the schema name never appears here, even if the object is not in path; the whole point of this is to be machine-parseable and we have the schema separately anyway. So some tweaks to functions such as format_type_internal are necessary, to supress schema-qualifying when not wanted. So the main thing here is about the particular format for each specific object class. For example, it seems to me we need to schema-qualify type names in function signatures. For casts I propose "(%s as %s)", where each %s is a schema-qualified type name. For constraints we can use "%s on %s" where there's no schema qualification (the schema column carries the table's schema, for the constraint must always belong to the same schema). For operators, we use "%s(%s, %s)" where the first %s is the operator name and the other two are schema-qualified type names. For default values (pg_attrdef tuples) we use "for %s" where the %s is the column's identity (i.e. .; again not schema-qualified because the attrdef "is" in the same schema as the table)). For operator classes and families we use "%s for %s" where the first one is the opclass/opfam name, and the second is the AM name. For pg_amop and pg_amproc objects we use "%d (%s, %s) of %s": strategy number, qualified type names, and AM name. For rules and triggers, "%s on %s": rule name and relation name (unqualified). For all the rest, we simply use the unqualified object name. pg_default_acl objects are the funniest of all, and they end up as "belonging to role %s in schema %s". I am especially open to suggestions in this one. I think this kind of machine-parseable object representation would be useful for cases other than event triggers, so I am thinking in committing this part before the pg_dropped_objects patch. I haven't split it out yet from the event triggers patch; I will be sending a patch later, in the meantime I welcome comments on the above. -- Á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] Materialized view assertion failure in HEAD
Kevin Grittner writes: > Tom Lane wrote: >> [ why not allow matviews to have OID columns? ] > An oid column in a materialized view will not be significantly more > stable than its ctid. The same "logical" row could easily have a > different OID on a REFRESH or even an incremental update (due to > the probable need to handle some incremental updates by deleting > and re-inserting portions of the MV). Allowing an "Object ID" to > be generated when it is not as stable as the name or its usage in a > table might suggest seems likely to lead to all kinds of trouble > and false bug reports down the road. Meh. I don't find that argument all that convincing. It'd pass as a rationale for not supporting OIDs if we'd found it was hard to do so; but I don't find it strong enough to justify introducing dirty kluges to prevent people from creating matviews with OIDs. OIDs in regular tables aren't a sure thing as a row identifier either. Moreover, if we are afraid of people expecting OIDs to act in a particular way, why are we exposing other system columns in matviews? ctid, xmin, etc all might have behaviors that differ from a "pure view", or that we find it convenient to change from release to release. >> Anyway, I think it makes more sense to go in the direction of >> making the case work than to introduce messy kluges to prevent >> matviews from having OIDs. > If you can come up with a single use case where having OIDs for > them makes sense and is useful, rather than being a loaded foot-gun > for users, it would go a long way toward convincing me that is the > way to go. Arguably, OIDs in regular tables are a loaded foot-gun already: they aren't unique unless you go out of your way to make them so, and they aren't wide enough to be reliably unique for modern volumes of data. There's a reason why they're deprecated. That being the case, I'm entirely fine with taking the whats-the-easiest-implementation approach to whether they can be in matviews or not. IOW, I'd not object to prohibiting them if it were possible to do so with a small, clean patch in an unsurprising place. But it appears to me that it's messier to prohibit them than to allow them. > Granted, my first go at switching away from that didn't > touch all the bases, but at this point it's probably at least as > much of a change to revert to supporting OIDs as to fix the > omissions. I will also grant that I haven't been able to see a > really pretty way to do it, but that is mostly because the whole > area of handling this is full of grotty special cases which need to > be handled already. Quite, which is why I'm objecting to adding more special cases; and I think that disallowing this for matviews is going to end up requiring exactly that. I'd rather see the code just treating matviews identically to regular relations for OID-related purposes. >> ISTM that suppressing this check during a matview refresh is >> rather broken, because then it won't complain if the matview >> reads some other matview that's in a nonscannable state. Is that >> really the behavior we want? > If that is what you got out of the comment, the comment probably > needs some work. [ looks a bit more... ] Indeed, the comment appears to be entirely unrelated to reality. I still think that implementing this check in a rangetable scan is probably broken or at least overly complicated, though. The fact that a rel is in the rangetable is not evidence that it's going to be read, so you're going to need some fragile logic to avoid throwing undesirable errors. Moreover, ExecCheckRelationsScannable has added an additional relcache open/close cycle per relation per query, which is undesirable and quite unnecessary overhead. It'd be worth thinking about getting rid of EXEC_FLAG_WITH_NO_DATA and instead using EXEC_FLAG_EXPLAIN_ONLY to suppress unwanted checks during CREATE TABLE AS WITH NO DATA. Or at least implementing both flags in the same places. 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] JSON Function Bike Shedding
On 03/01/2013 11:09 AM, Merlin Moncure wrote:
On Fri, Feb 22, 2013 at 11:50 AM, David E. Wheeler
wrote:
On Feb 22, 2013, at 9:37 AM, Robert Haas wrote:
What I think is NOT tolerable is choosing a set of short but arbitrary
names which are different from anything that we have now and
pretending that we'll want to use those again for the next data type
that comes along. That's just wishful thinking. Programmers who
believe that their decisions will act as precedent for all future code
are almost inevitably disappointed. Precedent grows organically out
of what happens; it's very hard to create it ex nihilo, especially
since we have no clear idea what future data types we'll likely want
to add. Sure, if we add something that's just like JSON but with a
few extra features, we'll be able to reuse the names no problem. But
that's unlikely, because we typically resist the urge to add things
that are too much like what we already have. The main reason we're
adding JSON when we already have hstore is because JSON has become
something of a standard. We probably WILL add more "container" types
in the future, but I'd guess that they are likely to be as different
from JSON as JSON is from XML, or from arrays. I'm not convinced we
can define a set of semantics that are going to sweep that broadly.
Maybe. I would argue, however, that a key/value-oriented data type will always call those things
"keys" and "values". So keys() and vals() (or get_keys() and get_vals()) seems
pretty reasonable to me.
Anyway, back to practicalities, Andrew last posted:
I am going to go the way that involves the least amount of explicit casting or
array construction. So get_path() stays, but becomes non-variadic. get() can
take an int or variadic text[], so you can do:
get(myjson,0)
get(myjson,'f1')
get(myjson,'f1','2','f3')
get_path(myjson,'{f1,2,f3}')
I would change these to mention the return types:
get_json(myjson,0)
get_json(myjson,'f1')
get_json(myjson,'f1','2','f3')
get_path_json(myjson,'{f1,2,f3}')
And then the complementary text-returning versions:
get_text(myjson,0)
get_text(myjson,'f1')
get_text(myjson,'f1','2','f3')
get_path_text(myjson,'{f1,2,f3}')
I do think that something like length() has pretty good semantics across data
types, though. So to update the proposed names, taking in the discussion, I now
propose:
Existing Name Proposed Name
-- ---
json_array_length() length()
json_each() each_json()
json_each_as_text() each_text()
json_get() get_json()
json_get_as_text() get_text()
json_get_path() get_path_json()
json_get_path_as_text() get_path_text()
json_object_keys() get_keys()
json_populate_record() to_record()
json_populate_recordset() to_records()
json_unnest() get_values()
json_agg() json_agg()
I still prefer to_record() and to_records() to populate_record(). It just feels
more like a cast to me. I dislike json_agg(), but assume we're stuck with it.
But at this point, I’m happy to leave Andrew to it. The functionality is
awesome.
Agreed: +1 to your thoughts here. But also +1 to the originals and +1
to Robert's point of view also. This feature is of huge strategic
importance to the project and we need to lock this down and commit it.
There is a huge difference between "i slightly prefer some different
names" and "the feature has issues".
So, i think the various positions are clear: this is one argument i'd
be happy to lose (or win).
I've been sitting here for a while mulling none too happily over the
debate on the names for the proposed JSON extraction functions. I
haven't really been happy with any of the suggestions, much, not least
my own original function names which were really only intended as
placeholders. Last night in the still watches I decided I just couldn't
go with a function name as almost totally content-free as get(), or even
get_text(). And I don't think prepending "json_'" to the name helps much
either.
Just concentrating to start with on those get() functions, in the simple
case we really don't need them at all. hstore has the "->" operator
without documenting the underlying function ("fetchval"). So maybe we
should just do that. We could have documented, simply:
myjson -> 'fname'
myjson -> 1
myjson ->> 'fname'
myjson ->> 1
myjson #> '{fname,1}'
myjson #>> '{fname,1}'
and leave the underlying functions undocumented.
One wrinkle in this picture is the variadic forms of extraction which
don't lend themselves nicely to use with an operator. We could decide to
do away with those altogether, or come up with a better name. I'm loath
to use "json_path" since it's a name used for something similar but
different elsewhere. I do think it's valuable
Re: [HACKERS] Enabling Checksums
On 18 March 2013 19:02, Jeff Davis wrote: > On Sun, 2013-03-17 at 22:26 -0700, Daniel Farina wrote: >> as long as I am able to turn them off easily > > To be clear: you don't get the performance back by doing > "ignore_checksum_failure = on". You only get around the error itself, > which allows you to dump/reload the good data. Given that the worst pain point comes from setting hint bits during a large SELECT, it makes sense to offer an option to simply skip hint bit setting when we are reading data (SELECT, not INSERT/UPDATE/DELETE). That seems like a useful option even without checksums. I know I have seen cases across many releases where setting that would have been good, since it puts the cleanup back onto VACUUM/writers, rather than occasional SELECTs. -- 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] Re: Doc patch making firm recommendation for setting the value of commit_delay
On Fri, Mar 15, 2013 at 05:47:30PM -0400, Tom Lane wrote: > Noah Misch writes: > > I'm marking this patch Ready for Committer, qualified with a recommendation > > to > > adopt only the wal.sgml changes. > > I've committed this along with some further wordsmithing. I kept > Peter's change to pg_test_fsync's default -s value; I've always felt > that 2 seconds was laughably small. It might be all right for very > quick-and-dirty tests, but as a default value, it seems like a poor > choice, because it's at the very bottom of the credible range of > choices. Agreed, 2 seconds was at the bottom. The old behavior was very slow so I went low. Now that we are using it, 5 secs makes sense. -- Bruce Momjian http://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] Enabling Checksums
On Mon, Mar 18, 2013 at 06:24:37PM +, Simon Riggs wrote: > On 18 March 2013 17:52, Bruce Momjian wrote: > > On Sun, Mar 17, 2013 at 05:50:11PM -0700, Greg Smith wrote: > >> As long as the feature is off by default, so that people have to > >> turn it on to hit the biggest changed code paths, the exposure to > >> potential bugs doesn't seem too bad. New WAL data is no fun, but > >> it's not like this hasn't happened before. > > > > With a potential 10-20% overhead, > > ... for some workloads. > > > > I am unclear who would enable this at initdb time. > > Anybody that cares a lot about their data. > > > I assume a user would wait until they suspected corruption to turn it > > on, and because it is only initdb-enabled, they would have to > > dump/reload their cluster. The open question is whether this is a > > usable feature as written, or whether we should wait until 9.4. > > When two experienced technical users tell us this is important and > that they will use it, we should listen. > > > > In fact, this feature is going to need > > pg_upgrade changes to detect from pg_controldata that the old/new > > clusters have the same checksum setting. > > I don't see any way they can differ. > > pg_upgrade and checksums don't mix, in this patch, at least. Jeff has already addressed the issue in the patch, e.g. if someone initdb's the new cluster with checksums. I am now fine with the patch based on the feedback I received. I needed to hear that the initdb limitation and the new performance numbers still produced a useful feature. -- Bruce Momjian http://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] Enabling Checksums
On Mon, Mar 18, 2013 at 11:42:23AM -0700, Jeff Davis wrote: > On Mon, 2013-03-18 at 13:52 -0400, Bruce Momjian wrote: > > In fact, this feature is going to need > > pg_upgrade changes to detect from pg_controldata that the old/new > > clusters have the same checksum setting. > > I believe that has been addressed in the existing patch. Let me know if > you see any problems. Oh, I see it now, right at the top. I didn't realize anyone else would have been looking to address this. Nice! -- Bruce Momjian http://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] Enabling Checksums
On Sun, 2013-03-17 at 22:26 -0700, Daniel Farina wrote: > as long as I am able to turn them off easily To be clear: you don't get the performance back by doing "ignore_checksum_failure = on". You only get around the error itself, which allows you to dump/reload the good data. Regards, Jeff Davis -- 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] Enabling Checksums
On Mon, 2013-03-18 at 13:52 -0400, Bruce Momjian wrote: > In fact, this feature is going to need > pg_upgrade changes to detect from pg_controldata that the old/new > clusters have the same checksum setting. I believe that has been addressed in the existing patch. Let me know if you see any problems. Regards, Jeff Davis -- 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] Materialized view assertion failure in HEAD
Tom Lane wrote: > I wrote: >> BTW, is there a really solid reason why a matview couldn't be >> allowed to have OIDs on demand, and thereby dodge this whole >> problem? I'm thinking that the analogy to regular views not >> having OIDs is not a very good argument, because certainly >> matview rows are going to need all the other system columns. > > Some experimentation says that actually it works just fine to > create matviews with OIDs; the only part of the backend that has > a problem with that is REFRESH MATERIALIZED VIEW, which is easily > fixed as per attached. Sure that works. Initial versions of the patch did it that way, and the paint is not as dry in the "no OIDS" area because it is a recent change to the patch which I haven't (yet) tested as thoroughly. What happened was this: in his review Noah asked why WITH OIDS and WITHOUT OIDS were supported, meaning (as I later found out) that he questioned support for the *old syntax* without the parentheses. I misunderstood and thought he was questioning whether OIDs were actually useful in materialized views, and came to the conclusion that they were not. An oid column in a materialized view will not be significantly more stable than its ctid. The same "logical" row could easily have a different OID on a REFRESH or even an incremental update (due to the probable need to handle some incremental updates by deleting and re-inserting portions of the MV). Allowing an "Object ID" to be generated when it is not as stable as the name or its usage in a table might suggest seems likely to lead to all kinds of trouble and false bug reports down the road. That said, there is no doubt that it would be the easiest thing to do; but once that genie is out of the bottle, do we take it away later, or deal with the headaches forever? Neither seems appealing to me. > If we go in this direction it would probably also be a good idea > to revert the hack in psql/describe.c to prevent it from printing > Has OIDs for matviews. Sure, although I fail to see what that "if" any more of a hack than 100 others in that area of the code. > Also, the fact that Joachim saw this problem in a dump/reload > implies that pg_dump is failing to preserve the state of > relhasoids for matviews, because tvmm hasn't got OIDs in the > regression database, but his stack trace says it did after dump > and reload. I haven't looked into how come, but whichever way we > jump as far as the backend behavior, pg_dump is clearly confused. Yeah, that was the bug I was fixing here. The MV was created between two SETs of default_with_oids based on table names with different settings of this. If we change back to allowing OIDs for MVs we need to reinstate the test for whether to omit SETs for default_with_oids for them instead of having an assert that they don't have OIDs. > Anyway, I think it makes more sense to go in the direction of > making the case work than to introduce messy kluges to prevent > matviews from having OIDs. If you can come up with a single use case where having OIDs for them makes sense and is useful, rather than being a loaded foot-gun for users, it would go a long way toward convincing me that is the way to go. Granted, my first go at switching away from that didn't touch all the bases, but at this point it's probably at least as much of a change to revert to supporting OIDs as to fix the omissions. I will also grant that I haven't been able to see a really pretty way to do it, but that is mostly because the whole area of handling this is full of grotty special cases which need to be handled already. > Also ... while I was nosing around I noticed this bit in > InitPlan(): > > /* > + * Unless we are creating a view or are creating a materialized view WITH > + * NO DATA, ensure that all referenced relations are scannable. > + */ > + if ((eflags & EXEC_FLAG_WITH_NO_DATA) == 0) > + ExecCheckRelationsScannable(rangeTable); > > ISTM that suppressing this check during a matview refresh is > rather broken, because then it won't complain if the matview > reads some other matview that's in a nonscannable state. Is that > really the behavior we want? It isn't, nor is it the behavior we get: test=# create materialized view x as select 1; SELECT 1 test=# create materialized view y as select * from x; SELECT 1 test=# refresh materialized view x with no data; REFRESH MATERIALIZED VIEW test=# refresh materialized view y; ERROR: materialized view "x" has not been populated HINT: Use the REFRESH MATERIALIZED VIEW command. If that is what you got out of the comment, the comment probably needs some work. > (Scanning the rangetable is probably wrong for this anyway > --- it'd be better to put the responsibility into the > table-scan-node initialization functions.) Don't we want to lock the appropriate relations at the point where I have this? Would it be right to wait until the point you suggest from the locking POV? -- Kevin Grittner EnterpriseDB
Re: [HACKERS] Enabling Checksums
> With a potential 10-20% overhead, I am unclear who would enable this at > initdb time. People who know they have a chronic issue with bad disks/cards/drivers would. Or anyone with enough machines that IO corruption is an operational concern worth more than 10% overhead. Or, in a word: Heroku, Enova and Aster Data, by their own admission. This seems like a sufficiently rsignificant user group to make it worthwhile to get something in, as long as it's something we can build on. Also, Simon, Greg and I discussed this feature while at PyCon last week. We went over it to discuss whether the poor performance now was a permanent result of the checksum design, or whether it would be possible to improve performance in future versions of PostgreSQL without an incompatible change. We concluded that it would be possible to improve it substantially while using the same file & checksum format. Some of the performance improvements require finally doing something to clean up hint bits, though, so it's not something we want to do for 9.3 at this stage. As such, I'm recommending that we go ahead with committing this feature. > I assume a user would wait until they suspected corruption to turn it > on, and because it is only initdb-enabled, they would have to > dump/reload their cluster. The open question is whether this is a > usable feature as written, or whether we should wait until 9.4. "release early, release often". We just need to document that the feature has substantial performance overhead, and the limitations around it. Right now it's useful to a minority of our users, but in the future it can be made useful to a larger group. And, importantly, for that minority, there really is no other solution. > pg_upgrade can't handle this because the old/new clusters would have the > same catalog version number and the tablespace directory names would > conflict. Even if they are not using tablespaces, the old heap/index > files would not have checksums and therefore would throw an error as > soon as you accessed them. In fact, this feature is going to need > pg_upgrade changes to detect from pg_controldata that the old/new > clusters have the same checksum setting. Better get cracking, then! ;-) -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Enabling Checksums
* Bruce Momjian (br...@momjian.us) wrote: > With a potential 10-20% overhead, I am unclear who would enable this at > initdb time. I'd expect that quite a few people would, myself included on a brand new DB that I didn't have any reason to think would need to be super-performant. > I assume a user would wait until they suspected corruption to turn it > on, and because it is only initdb-enabled, they would have to > dump/reload their cluster. The open question is whether this is a > usable feature as written, or whether we should wait until 9.4. It's absolutely useful as an initdb-only option. If we want to worry about users who will see corruption and who will wait until then to want to turn on this feature, then we should just enable it by default. > pg_upgrade can't handle this because the old/new clusters would have the > same catalog version number and the tablespace directory names would > conflict. pg_upgrade would just need to complain and exit if someone tried to go from a non-checksum DB to a DB which was initdb'd with checksums, right? I don't see pg_upgrade being able to convert from one to the other. Users can use pg_dump/restore for that.. > Even if they are not using tablespaces, the old heap/index > files would not have checksums and therefore would throw an error as > soon as you accessed them. In fact, this feature is going to need > pg_upgrade changes to detect from pg_controldata that the old/new > clusters have the same checksum setting. Right, but that's it, imv. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Enabling Checksums
On 18 March 2013 17:52, Bruce Momjian wrote: > On Sun, Mar 17, 2013 at 05:50:11PM -0700, Greg Smith wrote: >> As long as the feature is off by default, so that people have to >> turn it on to hit the biggest changed code paths, the exposure to >> potential bugs doesn't seem too bad. New WAL data is no fun, but >> it's not like this hasn't happened before. > > With a potential 10-20% overhead, ... for some workloads. > I am unclear who would enable this at initdb time. Anybody that cares a lot about their data. > I assume a user would wait until they suspected corruption to turn it > on, and because it is only initdb-enabled, they would have to > dump/reload their cluster. The open question is whether this is a > usable feature as written, or whether we should wait until 9.4. When two experienced technical users tell us this is important and that they will use it, we should listen. > In fact, this feature is going to need > pg_upgrade changes to detect from pg_controldata that the old/new > clusters have the same checksum setting. I don't see any way they can differ. pg_upgrade and checksums don't mix, in this patch, at least. -- 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] Support for REINDEX CONCURRENTLY
On Wed, Mar 13, 2013 at 9:04 PM, Michael Paquier wrote: > I have been working on improving the code of the 2 patches: > 1) reltoastidxid removal: > - Fix a bug with pg_dump and binary upgrade. One valid index is necessary > for a given toast relation. Is this bugfix related to the following? appendPQExpBuffer(upgrade_query, - "SELECT c.reltoastrelid, t.reltoastidxid " + "SELECT c.reltoastrelid, t.indexrelid " "FROM pg_catalog.pg_class c LEFT JOIN " - "pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) " - "WHERE c.oid = '%u'::pg_catalog.oid;", + "pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) " + "WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid " + "LIMIT 1", Don't indisready and indislive need to be checked? Why is LIMIT 1 required? The toast table can have more than one toast indexes? Regards, -- Fujii Masao -- 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] Enabling Checksums
2013/3/18 Bruce Momjian : > On Sun, Mar 17, 2013 at 05:50:11PM -0700, Greg Smith wrote: >> As long as the feature is off by default, so that people have to >> turn it on to hit the biggest changed code paths, the exposure to >> potential bugs doesn't seem too bad. New WAL data is no fun, but >> it's not like this hasn't happened before. > > With a potential 10-20% overhead, I am unclear who would enable this at > initdb time. everybody who has no 100% loaded server. I can see on almost all PostgreSQL instances load to 5 on 8CPU core instances. It is similar to PostgreSQL statistics - I remember so it did 20% slowdown too Regards Pavel > > I assume a user would wait until they suspected corruption to turn it > on, and because it is only initdb-enabled, they would have to > dump/reload their cluster. The open question is whether this is a > usable feature as written, or whether we should wait until 9.4. > > pg_upgrade can't handle this because the old/new clusters would have the > same catalog version number and the tablespace directory names would > conflict. Even if they are not using tablespaces, the old heap/index > files would not have checksums and therefore would throw an error as > soon as you accessed them. In fact, this feature is going to need > pg_upgrade changes to detect from pg_controldata that the old/new > clusters have the same checksum setting. > > -- > Bruce Momjian http://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] Support for REINDEX CONCURRENTLY
On Sun, Mar 17, 2013 at 9:24 PM, Michael Paquier wrote: > Please find attached the patches wanted: > - 20130317_dump_only_valid_index.patch, a 1-line patch that makes pg_dump > not take a dump of invalid indexes. This patch can be backpatched to 9.0. Don't indisready and indislive need to be checked? The patch seems to change pg_dump so that it ignores an invalid index only when the remote server version >= 9.0. But why not when the remote server version < 9.0? I think that you should start new thread to get much attention about this patch if there is no enough feedback. > Note that there have been some recent discussions about that. This *problem* > also concerned pg_upgrade. > http://www.postgresql.org/message-id/20121207141236.gb4...@alvh.no-ip.org What's the conclusion of this discussion? pg_dump --binary-upgrade also should ignore an invalid index? pg_upgrade needs to be changed together? Regards, -- Fujii Masao -- 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] transforms
On 03/13/2013 09:54 AM, Dimitri Fontaine wrote: > Peter Eisentraut writes: >> At run time, this will sort itself out, because all the required modules >> will be loaded. The problem is when you create the "glue" extension and >> haven't invoked any code from any of the dependent extension in the >> session. Abstractly, the possible solutions are either not to check the >> functions when the extension is created (possibly settable by a flag) or >> to somehow force a load of all dependent extensions when the new >> extension is created. (I say extension here even though dynamically >> loadable modules are attached to functions, which makes this even more >> confusing.) > > I think CREATE EXTENSION should be LOADing all distinct modules > referenced from all required extensions functions. It does sound easy > enough to code, and I can't imagine why we would want to not do that. Where are we with this? Will the loading issue be fixed, or should we bump this feature to 9.4? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] Enabling Checksums
On Sun, Mar 17, 2013 at 05:50:11PM -0700, Greg Smith wrote: > As long as the feature is off by default, so that people have to > turn it on to hit the biggest changed code paths, the exposure to > potential bugs doesn't seem too bad. New WAL data is no fun, but > it's not like this hasn't happened before. With a potential 10-20% overhead, I am unclear who would enable this at initdb time. I assume a user would wait until they suspected corruption to turn it on, and because it is only initdb-enabled, they would have to dump/reload their cluster. The open question is whether this is a usable feature as written, or whether we should wait until 9.4. pg_upgrade can't handle this because the old/new clusters would have the same catalog version number and the tablespace directory names would conflict. Even if they are not using tablespaces, the old heap/index files would not have checksums and therefore would throw an error as soon as you accessed them. In fact, this feature is going to need pg_upgrade changes to detect from pg_controldata that the old/new clusters have the same checksum setting. -- Bruce Momjian http://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] Materialized view assertion failure in HEAD
I wrote:
> BTW, is there a really solid reason why a matview couldn't be allowed to
> have OIDs on demand, and thereby dodge this whole problem? I'm thinking
> that the analogy to regular views not having OIDs is not a very good
> argument, because certainly matview rows are going to need all the other
> system columns.
Some experimentation says that actually it works just fine to create
matviews with OIDs; the only part of the backend that has a problem with
that is REFRESH MATERIALIZED VIEW, which is easily fixed as per
attached.
If we go in this direction it would probably also be a good idea to
revert the hack in psql/describe.c to prevent it from printing Has OIDs
for matviews.
Also, the fact that Joachim saw this problem in a dump/reload implies
that pg_dump is failing to preserve the state of relhasoids for
matviews, because tvmm hasn't got OIDs in the regression database, but
his stack trace says it did after dump and reload. I haven't looked
into how come, but whichever way we jump as far as the backend behavior,
pg_dump is clearly confused.
Anyway, I think it makes more sense to go in the direction of making
the case work than to introduce messy kluges to prevent matviews from
having OIDs.
Also ... while I was nosing around I noticed this bit in InitPlan():
/*
+* Unless we are creating a view or are creating a materialized view
WITH
+* NO DATA, ensure that all referenced relations are scannable.
+*/
+ if ((eflags & EXEC_FLAG_WITH_NO_DATA) == 0)
+ ExecCheckRelationsScannable(rangeTable);
ISTM that suppressing this check during a matview refresh is rather
broken, because then it won't complain if the matview reads some other
matview that's in a nonscannable state. Is that really the behavior
we want? (Scanning the rangetable is probably wrong for this anyway
--- it'd be better to put the responsibility into the table-scan-node
initialization functions.)
regards, tom lane
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index e20fedaeaf2a47b34f423ddeb062b429bf1f..dbcbf332539b00516afbb28be56857a5f9c2402f 100644
*** a/src/backend/commands/matview.c
--- b/src/backend/commands/matview.c
*** typedef struct
*** 44,55
BulkInsertState bistate; /* bulk insert state */
} DR_transientrel;
static void transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo);
static void transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
static void transientrel_shutdown(DestReceiver *self);
static void transientrel_destroy(DestReceiver *self);
- static void refresh_matview_datafill(DestReceiver *dest, Query *query,
- const char *queryString);
/*
* SetRelationIsScannable
--- 44,55
BulkInsertState bistate; /* bulk insert state */
} DR_transientrel;
+ static void refresh_matview_datafill(DestReceiver *dest, bool hasoids,
+ Query *query, const char *queryString);
static void transientrel_startup(DestReceiver *self, int operation, TupleDesc typeinfo);
static void transientrel_receive(TupleTableSlot *slot, DestReceiver *self);
static void transientrel_shutdown(DestReceiver *self);
static void transientrel_destroy(DestReceiver *self);
/*
* SetRelationIsScannable
*** ExecRefreshMatView(RefreshMatViewStmt *s
*** 138,145
*/
Assert(!IsSystemRelation(matviewRel));
- Assert(!matviewRel->rd_rel->relhasoids);
-
/*
* Check that everything is correct for a refresh. Problems at this point
* are internal errors, so elog is sufficient.
--- 138,143
*** ExecRefreshMatView(RefreshMatViewStmt *s
*** 185,198
tableSpace = matviewRel->rd_rel->reltablespace;
- heap_close(matviewRel, NoLock);
-
/* Create the transient table that will receive the regenerated data. */
OIDNewHeap = make_new_heap(matviewOid, tableSpace);
dest = CreateTransientRelDestReceiver(OIDNewHeap);
if (!stmt->skipData)
! refresh_matview_datafill(dest, dataQuery, queryString);
/*
* Swap the physical files of the target and transient tables, then
--- 183,197
tableSpace = matviewRel->rd_rel->reltablespace;
/* Create the transient table that will receive the regenerated data. */
OIDNewHeap = make_new_heap(matviewOid, tableSpace);
dest = CreateTransientRelDestReceiver(OIDNewHeap);
if (!stmt->skipData)
! refresh_matview_datafill(dest, matviewRel->rd_rel->relhasoids,
! dataQuery, queryString);
!
! heap_close(matviewRel, NoLock);
/*
* Swap the physical files of the target and transient tables, then
*** ExecRefreshMatView(RefreshMatViewStmt *s
*** 208,215
* refresh_matview_datafill
*/
static void
! refresh_matview_datafill(DestReceiver *dest, Query *query,
! const char *queryString)
{
List *rewritten;
PlannedStmt *plan;
--- 207,214
* refresh_matview_dat
[HACKERS] pg_upgrade segfaults when given an invalid PGSERVICE value
If you try running pg_upgrade with the PGSERVICE environment variable
set to some invalid/non-existent service pg_upgrade segfaults
Program received signal SIGSEGV, Segmentation fault.
0x0040bdd1 in check_pghost_envvar () at server.c:304
304 for (option = start; option->keyword != NULL; option++)
(gdb) p start
$5 = (PQconninfoOption *) 0x0
PQconndefaults can return NULL if it has issues.
The attached patch prints a minimally useful error message. I don't a
good way of getting a more useful error message out of PQconndefaults()
I checked this against master but it was reported to me as a issue in 9.2
Steve
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index ed67759..f2e4d63 100644
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*** check_pghost_envvar(void)
*** 300,306
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
!
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
--- 300,309
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
! if (start == NULL)
! {
! pg_log(PG_FATAL,"can not get default connection options\n");
! }
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
--
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] Improving avg performance for numeric
Hello I played with sum(numeric) optimization Now it is based on generic numeric_add function - this code is relative old - now we can design aggregates with internal transition buffers, and probably we can do this work more effective. I just removed useles palloc/free operations and I got a 30% better performance! My patch is ugly - because I used a generic add_var function. Because Sum, Avg and almost all aggregates functions is limited by speed of sum calculation I thing so we need a new numeric routines optimized for calculation "sum", that use a only preallocated buffers. A speed of numeric is more important now, because there are more and more warehouses, where CPU is botleneck. Regards Pavel 2013/3/18 Hadi Moshayedi : > Hi Pavel, > > Thanks a lot for your feedback. > > I'll work more on this patch this week, and will send a more complete patch > later this week. > > I'll also try to see how much is the speed up of this method for other > types. > > Thanks, > -- Hadi > > > On Mon, Mar 18, 2013 at 10:36 AM, Pavel Stehule > wrote: >> >> 2013/3/16 Hadi Moshayedi : >> > Revisiting: >> > http://www.postgresql.org/message-id/45661be7.4050...@paradise.net.nz >> > >> > I think the reasons which the numeric average was slow were: >> > (1) Using Numeric for count, which is slower than int8 to increment, >> > (2) Constructing/deconstructing arrays at each transition step. >> > >> > This is also discussed at: >> > >> > http://www.citusdata.com/blog/53-postgres-performance-to-avg-or-to-sum-divided-by-count >> > >> > So, I think we can improve the speed of numeric average by keeping the >> > transition state as an struct in the aggregate context, and just passing >> > the >> > pointer to that struct from/to the aggregate transition function. >> > >> > The attached patch uses this method. >> > >> > I tested it using the data generated using: >> > CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d FROM >> > generate_series(1, 1000) s; >> > >> > After applying this patch, run time of "SELECT avg(d) FROM avg_test;" >> > improves from 10.701 seconds to 5.204 seconds, which seems to be a huge >> > improvement. >> > >> > I think we may also be able to use a similar method to improve >> > performance >> > of some other numeric aggregates (like stddev). But I want to know your >> > feedback first. >> > >> > Is this worth working on? >> >> I checked this patch and it has a interesting speedup - and a price of >> this methoud should not be limited to numeric type only >> >> Pavel >> >> > >> > Thanks, >> > -- Hadi >> > >> > >> > >> > -- >> > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) >> > To make changes to your subscription: >> > http://www.postgresql.org/mailpref/pgsql-hackers >> > > > numeric-sum-optimize.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
Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)
Alvaro Herrera writes: > Here's a rebased version; there were some merge conflicts with master. Thanks! > I also fixed some compiler warnings. I haven't reviewed the patch in > any detail yet. One thing that jump at me from the code style > perspective is the strange way it deals with "isnull" from heap_getattr. > (I think most of these should just elog(ERROR) if a null attr is found). I think you're right here, yes. > Another thing is that I don't find the name "uptmpl" very clear. Any suggestion is welcome, I don't like it very much either. > Keeping the "template.c" file name seems wrong -- exttemplate.c maybe? > (I renamed the parse nodes to ExtTemplate) I've been wondering about that. My thinking is that we're providing a new set of TEMPLATE objects and commands and the commands are designed so that it's easy to add more objects in there. The implementation too is quite open to that, with "routing" functions in template.c. Now I've choosen to implement the Extension templates in the same file because currently those are the only kind of "Templates" that we manage, and this project seems to prefer big files rather than too many files. That said, I'm not wedded to this choice. > There was a strange bug in pg_dump; it used "qto" where I thought > qversion was appropriate. I changed it (I looked at this hunk more > closely than most others because there was a compiler warning here, but > I didn't verify that it works.) It's quite hard for me to spot the hunk you're talking about without setting up a whole new branch to extract the work you did in the new diff, but maybe I'm just missing a diff-between-diffs tool? > You seem to love using Capitalized Letters for some things in error > messages; I don't find these very pretty, and anyway they violate our > style guidelines. (I think these are in elog() not ereport() calls, but > still) I'll make sure to remember about that, thanks. Regards, -- Dimitri Fontaine06 63 07 10 78 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] Strange Windows problem, lock_timeout test request
Boszormenyi Zoltan writes: > How about the attached patch over current GIT? In other words, > why I am wrong with this idea? Because it's wrong. Removing "volatile" means that the compiler is permitted to optimize away stores (and fetches!) on the basis of their being unnecessary according to straight-line analysis of the code. Write barriers don't fix that, they only say that stores that the compiler chooses to issue at all have to be ordered a certain way. (There are also pretty serious questions as to whether pg_write_barrier can be trusted yet, but it doesn't really matter here. Removing volatile would break the code.) 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] Enabling Checksums
On 18 March 2013 00:50, Greg Smith wrote: > On 3/17/13 1:41 PM, Simon Riggs wrote: >> >> So I'm now moving towards commit using a CRC algorithm. I'll put in a >> feature to allow algorithm be selected at initdb time, though that is >> mainly a convenience to allow us to more easily do further testing on >> speedups and whether there are any platform specific regressions >> there. > > That sounds reasonable. As I just posted, I'm hoping Ants can help make a > pass over a CRC16 version, since his one on the Fletcher one seemed very > productive. If you're spending time looking at this, I know I'd prefer to > see you poking at the WAL related aspects instead. There are more of us who > are capable of crunching CRC code than the list of people who have practice > at WAL changes like you do. Just committed the first part, which was necessary refactoring. I see at least 2 further commits here: * Next part is the checksum patch itself, with some checksum calc or other (mostly unimportant from a code perspective, since the actual algorithm is just a small isolated piece of code. * Further commit(s) to set the agreed checksum algorithm and/or tune it. -- 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] Trust intermediate CA for client certificates
Craig, all, * Craig Ringer (cr...@2ndquadrant.com) wrote: > PROBLEM VERIFIED Let me just say "ugh". I've long wondered why we have things set up in such a way that the whole chain has to be in one file, but it didn't occur to me that it'd actually end up causing this issue. In some ways, I really wonder about this being OpenSSL's fault as much as ours, but I doubt they'd see it that way. :) > What we need to happen instead is for root.crt to contain only the > trusted certificates and have a *separate* file or directory for > intermediate certificates that OpenSSL can look up to get the > intermediates it needs to validate client certs, like > `ssl_ca_chain_file` or `ssl_ca_chain_path` if we want to support > OpenSSL's hashed certificate directories. Makes sense to me. I'm not particular about the names, but isn't this set of CAs generally considered intermediary? Eg: 'trusted', ' intermediate', etc? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Patch to add regression tests for SCHEMA
Hi, Attached is an updated patch that uses better schema / role names. -- Robins Tharakan On 18 March 2013 12:59, robins wrote: > Thanks Alvaro. > > Since the tests were running fine, I didn't bother with elaborate names, > but since you're concerned guess I'll make that change and re-submit. > > And yes, I've already submitted (to Commitfest) another patch related to > regression tests for SEQUENCE. > Would submit the SCHEMA patch once the above change is done. > -- > Robins > > > On 18 March 2013 09:42, Alvaro Herrera wrote: > >> robins escribió: >> > Hi, >> > >> > Please find attached a patch to take 'make check' code-coverage of >> SCHEMA >> > from 33% to 98%. >> > >> > Any feedback is more than welcome. >> >> I think you should use more explicit names for shared objects such as >> roles -- i.e. not "r1" but "regression_user_1" and so on. (But be >> careful about role names used by other tests). The issue is that these >> tests might be run in a database that contains other stuff; certainly we >> don't want to drop or otherwise affect previously existing roles. >> >> > p.s.: I am currently working on more regression tests (USER / VIEW / >> > DISCARD etc). Please let me know if I need to post these as one large >> > patch, instead of submitting one patch at a time. >> >> I think separate patches is better. Are you adding these patches to the >> upcoming commitfest, for evaluation? https://commitfest.postgresql.org >> >> -- >> Álvaro Herrerahttp://www.2ndQuadrant.com/ >> PostgreSQL Development, 24x7 Support, Training & Services >> > > regress_schema_v2.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
Re: [HACKERS] Strange Windows problem, lock_timeout test request
2013-03-18 06:47 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan writes:
The volatile marking shouldn't even be necessary there.
The signal handler doesn't writes to it, only the main code.
Well, (a) that's not the case in the patch as committed, and (b) even if
it were true, the volatile marking is still *necessary*, because that's
what tells the compiler it can't optimize away changes to the variable,
say on the grounds of there being another store with no intervening
fetches in the main-line code. Without that, a compiler that had
aggressively inlined the smaller functions could well deduce that the
disable_alarm() assignment was useless.
Also, since the the variable assignment doesn't depend on other code
in the same function (or vice-versa) the compiler is still free to
reorder it.
Volatile is about not caching the variable in a CPU register since
it's "volatile"...
I don't believe you understand what volatile is about at all. Go read
the C standard: it's about requiring objects' values to actually match
the nominal program-specified values at sequence points. A more
accurate heuristic is that volatile tells the compiler there may be
other forces reading or writing the variable besides the ones it can see
in the current function's source code, and so it can't drop or reorder
accesses to the variable.
regards, tom lane
After reading up on a lot of volatile and memory barriers,
I am still not totally convinced.
For the record, sig_atomit_t is a plain int without any special
treatment from the compiler. It's atomic by nature on every 32-bit
and 64-bit CPU.
How about the attached patch over current GIT? In other words,
why I am wrong with this idea?
The problem that may arise if it's wrong is that transactions are
left waiting for the lock when the interrupt triggers and the variable
is still seen as false from the interrupt handler. But this doesn't happen.
FWIW, this small patch seems to give a 0,7 percent performance
boost for my settings:
shared_buffers = 256MB
work_mem = 8MB
effective_io_concurrency = 8
wal_level = hot_standby
checkpoint_segments = 64
random_page_cost = 1.8
Everything else is the default. I tested the patch on a 8-core
AMD FX-8120 CPU with this pgbench script below. Basically, it's
the default transaction prepended with "SET lock_timeout = 1;"
I have used the attached quick-and-dirty patch to pgbench to
ignore SQL errors coming from statements. "-s 100" was used
to initialize the test database.
\set nbranches 1 * :scale
\set ntellers 10 * :scale
\set naccounts 10 * :scale
\setrandom aid 1 :naccounts
\setrandom bid 1 :nbranches
\setrandom tid 1 :ntellers
\setrandom delta -5000 5000
BEGIN;
SET lock_timeout = 1;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid,
:delta, CURRENT_TIMESTAMP);
END;
Results of "pgbench -c 32 -j 32 -t 1 -e -f script.sql" are
for the GIT version:
tps = 3366.844023 (including connections establishing)
tps = 3367.645454 (excluding connections establishing)
tps = 3379.784707 (including connections establishing)
tps = 3380.622317 (excluding connections establishing)
tps = 3385.198238 (including connections establishing)
tps = 3386.132433 (excluding connections establishing)
and with the barrier patch:
tps = 3412.799044 (including connections establishing)
tps = 3413.670832 (excluding connections establishing)
tps = 3389.796287 (including connections establishing)
tps = 3390.602187 (excluding connections establishing)
tps = 3405.924548 (including connections establishing)
tps = 3406.726997 (excluding connections establishing)
Best regards,
Zoltán Böszörményi
--
--
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 3c3e41e..8737a86 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -16,6 +16,7 @@
#include
+#include "storage/barrier.h"
#include "storage/proc.h"
#include "utils/timeout.h"
#include "utils/timestamp.h"
@@ -58,10 +59,10 @@ static timeout_params *volatile active_timeouts[MAX_TIMEOUTS];
* since the probability of the interrupt actually occurring while we have
* it disabled is low. See comments in schedule_alarm() about that.
*/
-static volatile sig_atomic_t alarm_enabled = false;
+static sig_atomic_t alarm_enabled = false;
-#define disable_alarm() (alarm_enabled = false)
-#define enable_alarm() (alarm_enabled = true)
+#define disable_alarm() do { alarm_enabled = false; pg_write_barrier(); } while (0
Re: [HACKERS] Improving avg performance for numeric
Hi Pavel, Thanks a lot for your feedback. I'll work more on this patch this week, and will send a more complete patch later this week. I'll also try to see how much is the speed up of this method for other types. Thanks, -- Hadi On Mon, Mar 18, 2013 at 10:36 AM, Pavel Stehule wrote: > 2013/3/16 Hadi Moshayedi : > > Revisiting: > > http://www.postgresql.org/message-id/45661be7.4050...@paradise.net.nz > > > > I think the reasons which the numeric average was slow were: > > (1) Using Numeric for count, which is slower than int8 to increment, > > (2) Constructing/deconstructing arrays at each transition step. > > > > This is also discussed at: > > > http://www.citusdata.com/blog/53-postgres-performance-to-avg-or-to-sum-divided-by-count > > > > So, I think we can improve the speed of numeric average by keeping the > > transition state as an struct in the aggregate context, and just passing > the > > pointer to that struct from/to the aggregate transition function. > > > > The attached patch uses this method. > > > > I tested it using the data generated using: > > CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d FROM > > generate_series(1, 1000) s; > > > > After applying this patch, run time of "SELECT avg(d) FROM avg_test;" > > improves from 10.701 seconds to 5.204 seconds, which seems to be a huge > > improvement. > > > > I think we may also be able to use a similar method to improve > performance > > of some other numeric aggregates (like stddev). But I want to know your > > feedback first. > > > > Is this worth working on? > > I checked this patch and it has a interesting speedup - and a price of > this methoud should not be limited to numeric type only > > Pavel > > > > > Thanks, > > -- Hadi > > > > > > > > -- > > 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] Improving avg performance for numeric
2013/3/16 Hadi Moshayedi : > Revisiting: > http://www.postgresql.org/message-id/45661be7.4050...@paradise.net.nz > > I think the reasons which the numeric average was slow were: > (1) Using Numeric for count, which is slower than int8 to increment, > (2) Constructing/deconstructing arrays at each transition step. > > This is also discussed at: > http://www.citusdata.com/blog/53-postgres-performance-to-avg-or-to-sum-divided-by-count > > So, I think we can improve the speed of numeric average by keeping the > transition state as an struct in the aggregate context, and just passing the > pointer to that struct from/to the aggregate transition function. > > The attached patch uses this method. > > I tested it using the data generated using: > CREATE TABLE avg_test AS SELECT (random() * 999)::decimal(5,2) as d FROM > generate_series(1, 1000) s; > > After applying this patch, run time of "SELECT avg(d) FROM avg_test;" > improves from 10.701 seconds to 5.204 seconds, which seems to be a huge > improvement. > > I think we may also be able to use a similar method to improve performance > of some other numeric aggregates (like stddev). But I want to know your > feedback first. > > Is this worth working on? I checked this patch and it has a interesting speedup - and a price of this methoud should not be limited to numeric type only Pavel > > Thanks, > -- Hadi > > > > -- > 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] Patch to add regression tests for SCHEMA
Thanks Alvaro. Since the tests were running fine, I didn't bother with elaborate names, but since you're concerned guess I'll make that change and re-submit. And yes, I've already submitted (to Commitfest) another patch related to regression tests for SEQUENCE. Would submit the SCHEMA patch once the above change is done. -- Robins On 18 March 2013 09:42, Alvaro Herrera wrote: > robins escribió: > > Hi, > > > > Please find attached a patch to take 'make check' code-coverage of SCHEMA > > from 33% to 98%. > > > > Any feedback is more than welcome. > > I think you should use more explicit names for shared objects such as > roles -- i.e. not "r1" but "regression_user_1" and so on. (But be > careful about role names used by other tests). The issue is that these > tests might be run in a database that contains other stuff; certainly we > don't want to drop or otherwise affect previously existing roles. > > > p.s.: I am currently working on more regression tests (USER / VIEW / > > DISCARD etc). Please let me know if I need to post these as one large > > patch, instead of submitting one patch at a time. > > I think separate patches is better. Are you adding these patches to the > upcoming commitfest, for evaluation? https://commitfest.postgresql.org > > -- > Álvaro Herrerahttp://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services >
Re: [HACKERS] Trust intermediate CA for client certificates
On 03/18/2013 02:27 PM, Ian Pilcher wrote: > On 03/18/2013 12:07 AM, Craig Ringer wrote: >> So this problem is verified. > * Trusted certificates - What currently goes in the (unfortunately > named) root.crt file. Well, a little unfortunate. It contains roots of *client authentication* trust, which is fair enough, they just aren't necessarily self-signed certificates that are roots of *certificate validity* trust (root CA certs). This list is set by SSL_CTX_set_client_CA_list . The examples section of its man page contains: Scan all certificates in CAfile and list them as acceptable CAs: SSL_CTX_set_client_CA_list(ctx,SSL_load_client_CA_file(CAfile)); > * Validation-only certificates - CA certificates that are used only to > complete the chain from a trusted certificate to a self-signed root. > I haven't been able to come up with a particularly good name for a > file containing this type of certificate(s) -- validate.crt? We should probably take advantage of the fact that 9.2 made these filenames configurable to deprecate root.crt and choose two descriptive filenames, something like trusted_cert_roots.crt and trusted_client_cert_signers.crt . > This is conceptually simple, and I've been fiddling with it for the last > week or so. Unfortunately, the OpenSSL documentation has made this far > more challenging that it should be. Simple things like reading multiple > certificates from a file, checking whether an X509_STORE contains a > particular certificate, etc. are all proving to be unexpectedly > difficult. (I never thought that I'd miss the Java SSL API!) Apache's sources are useful there. When working with OpenSSL sometimes the sanest option is to find something you know already does it right, work out how, *understand why it works* and then apply that approach to your code. Blindly copying their approach is stupid and guaranteed to lead to security holes, but others' code remains some of the best documentation for OpenSSL if used for hints rather than blindly copied. -- 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] Trust intermediate CA for client certificates
On 03/18/2013 01:07 PM, Craig Ringer wrote:
> System wide installation of the root may allow OpenSSL to discover it
> and use it for verification back to the root without having to trust it
> to sign clients. I'll do some more checking to see if this is possible
> with how Pg uses OpenSSL but I'm inclined to doubt it.
It looks like we aren't reading the system-wide certs or looking them up
when certs aren't resolved in the files Pg explicitly passes to OpenSSL,
so a system-wide install doesn't look like it'll work.
I've had a look at how Apache handles this and it took me a while to
work out what's going on. Apache uses SSLCACertificateFile (concatenated
certs) / SSLCACertificatePath (hashed dir) to look up trusted
certificate signers. It According to the docs it doesn't appear to make
any provision there for trusting intermediate certificates but not their
parents as signers of client certificates, but it looks like support is
there, the docs just don't explain it well.
Apache has SSLCADNRequestFile / SSLCADNRequestPath which are described
as controlling the acceptable certificate names sent to clients in a
client cert request. The docs don't make it clear if Apache will trust
only client certs with these certs in their chains or whether this only
controls the list of certificate DNs presented to the client rather than
what's accepted in response. The code suggests that they control trust
not just the cert list presented.
In Apache's modules/ssl/ssl_engine_init.c it calls
SSL_CTX_load_verify_locations on the SSLCACertificateFile and
SSLCACertificatePath.
It then calls ssl_init_FindCAList with the
SSLCADNRequestFile/SSLCADNRequestPath if they're specified in the
configuration, otherwise it calls it with the
SSLCACertificateFile/SSLCACertificatePath . That calls
ssl_init_PushCAList on all of the certs it finds in the File and Path
variants. For each cert file that calls SSL_load_client_CA_file and for
each cert within each file pushes the cert onto a STACK_OF(X509_NAME) if
a cert with the same DN isn't already in the stack. It passes the stack
to OpenSSL's SSL_CTX_set_client_CA_list .
So what Apache does appears to boil down to:
SSL_CTX_load_verify_locations(ca_file,ca_path);
if (ca_dn_file || ca_dn_path) {
SSL_CTX_set_client_CA_list( ... STACK_OF unique certs on ca_dn_file
and ca_dn_path ... );
} else {
SSL_CTX_set_client_CA_list( ... STACK_OF unique certs on ca_file and
ca_path );
}
This appears to match Ian's description of having a validation-only cert
list and a separate list of certs used to verify clients. I'd like to
follow Apache's model:
in postgresql.conf, if ssl_ca_file is set then pass it to
SSL_CTX_load_verify_locations . If the proposed new parameter
ssl_ca_valid_client_signers_file is set then pass the certs in that to
SSL_CTX_set_client_CA_list ; otherwise pass the certs in ssl_ca_file to
SSL_CTX_set_client_CA_list and thus retain the current behaviour.
Hopefully we can avoid the ugly read-and-deduplicate stuff Apache has to
do because we currently only support a certfile anyway, we don't read
certdirs, so I'll look for helper functions that wrap
SSL_CTX_set_client_CA_list.
--
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
