Re: [HACKERS] PostgreSQL doesn't stop propley when --slot option is specified with pg_receivexlog.
On Wed, Nov 19, 2014 at 6:16 AM, Fujii Masao masao.fu...@gmail.com wrote: On Mon, Nov 17, 2014 at 12:30 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Nov 17, 2014 at 10:02 AM, Fujii Masao masao.fu...@gmail.com wrote: On Sat, Nov 15, 2014 at 9:10 PM, Michael Paquier michael.paqu...@gmail.com wrote: Yep, sounds a good thing to do if master requested answer from the client in the keepalive message. Something like the patch attached would make the deal. Isn't it better to do this only when replication slot is used? Makes sense. What about a check using reportFlushPosition then? Sounds reasonable. Thanks for updating the patch! But the patch could not already be applied to the master cleanly because c4f99d2 heavily changed the code that the patch also touches... I rewrote the patch and pushed it to both master and REL9_4_STABLE. Anyway, thanks! Is this: + if (reportFlushPosition lastFlushPosition blockpos + walfile != 1) really correct? Shouldn't that walfile test be against -1 (minus one)? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.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] Customized Options Threshold Error
When we specify a value which exceeds valid range in Customized Options , its behavior is different from Parameter Interaction via Configuration File behavior. In case of Parameter Interaction via Configuration File, it finish with FATAL error when it get threshold error. But in Customized Options, it just printout WARNING and continue the process with default value. I think this is based on a misunderstanding. Bad values in postgresql.conf only result in a FATAL error at initial postmaster startup; if you change them and SIGHUP the server, an erroneous new value just results in a WARNING. This is because we don't really want the server crashing once it's up. The case you're showing with registering a new background worker is something that happens after the server is up, so it's consistent for it to produce a WARNING not a fatal error. Thanks for the reply. registering a new background worker is something that happens after the server is up I understand that parameters which are set by Customized Options are loaded after server is up, same as like pg_ctl reload, so even if invalid parameters are specified postmaster just ignore them and results in a WARNING. However, Customized Options always treated as pg_ctl reload is little bit strange to me. Regards, -- Furuya Osamu -- 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] postgres_fdw behaves oddly
Ok. I added that comment to the commitfest and changed the status to ready for commiter. On Wed, Nov 19, 2014 at 1:10 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/11/19 15:56), Ashutosh Bapat wrote: On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/11/19 14:58), Ashutosh Bapat wrote: May be we should modify use_physical_tlist() to return false in case of RELKIND_FOREIGN_TABLE, so that we can use tlist in create_foreignscan_plan(). I do not see any create_*_plan() function using reltargetlist directly. Yeah, I think we can do that, but I'm not sure that we should use tlist in create_foreignscan_plan(), not rel-reltargetlist. How about leaving this for committers to decide. I am fine with that. May be you want to add an XXX comment there to bring it to the committer's notice. It's ok, but I'm not convinced with your idea. So, I think the comment can be adequately described by you, not by me. So, my proposal is for you to add the comment to the CF app. Could you do that? Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] PostgreSQL doesn't stop propley when --slot option is specified with pg_receivexlog.
On Wed, Nov 19, 2014 at 5:15 PM, Magnus Hagander mag...@hagander.net wrote: On Wed, Nov 19, 2014 at 6:16 AM, Fujii Masao masao.fu...@gmail.com wrote: On Mon, Nov 17, 2014 at 12:30 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Nov 17, 2014 at 10:02 AM, Fujii Masao masao.fu...@gmail.com wrote: On Sat, Nov 15, 2014 at 9:10 PM, Michael Paquier michael.paqu...@gmail.com wrote: Yep, sounds a good thing to do if master requested answer from the client in the keepalive message. Something like the patch attached would make the deal. Isn't it better to do this only when replication slot is used? Makes sense. What about a check using reportFlushPosition then? Sounds reasonable. Thanks for updating the patch! But the patch could not already be applied to the master cleanly because c4f99d2 heavily changed the code that the patch also touches... I rewrote the patch and pushed it to both master and REL9_4_STABLE. Anyway, thanks! Is this: + if (reportFlushPosition lastFlushPosition blockpos + walfile != 1) really correct? Shouldn't that walfile test be against -1 (minus one)? Ooops... You're right. That's really mistake... Just fixed and pushed. Thanks! 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] Patch to support SEMI and ANTI join removal
On Sun, Nov 16, 2014 at 12:19 PM, David Rowley dgrowle...@gmail.com wrote: On Sun, Nov 16, 2014 at 10:09 AM, Simon Riggs si...@2ndquadrant.com wrote: I propose that we keep track of whether there are any potentially skippable joins at the top of the plan. When we begin execution we do a single if test to see if there is run-time work to do. If we pass the run-time tests we then descend the tree and prune the plan to completely remove unnecessary nodes. We end with an EXPLAIN and EXPLAIN ANALYZE that looks like this QUERY PLAN -- Aggregate (actual rows=1 loops=1) - Seq Scan on t1 (actual rows=100 loops=1) Doing that removes all the overheads and complexity; it also matches how join removal currently works. This sounds much cleaner than what I have at the moment, although, you say EXPLAIN would look like that... I don't think that's quite true as the EXPLAIN still would have the un-pruned version, as the pruning would be done as executor start-up. Would it cause problems to have the EXPLAIN have a different looking plan than EXPLAIN ANALYZE? Oops, It seems you're right about the EXPLAIN output. I had not previously realised that plain old EXPLAIN would initialise the plan. It's nice to see that I'll get my old tests working again! I've been hacking away at this, and I've now got a function which implodes the plan down to just what is required, I'm just calling this function is there are no pending foreign key triggers. Writing this has made me realise that I may need to remove the functionality that I've added to the planner which, after it removes 1 inner join, it puts that relation in an ignore list and tries again to remove other relations again, but this time ignoring any vars from ignored relations. The problem I see with this is that, with a plan such as: Hash Join Hash Cond: (t1.id = t4.id) - Hash Join Hash Cond: (t1.id = t3.id) - Hash Join Hash Cond: (t1.id = t2.id) - Seq Scan on t1 - Hash - Seq Scan on t2 - Hash - Seq Scan on t3 - Hash - Seq Scan on t4 If t1 and t4 are marked as can remove, then the code that implodes plan to remove the nodes which are no longer required would render the plan a bit useless as there's no join between t2 and t3, we'd need to keep t1 in this case, even though non of it's Vars are required. Perhaps I could fix this by writing some more intelligent code which would leave joins in place in this situation, and maybe I could coerce the planner into not producing plans like this by lowering the costs of joins where 1 of the relations could be removed. Andres did mention lowering costs previously, but at the time I'd not realised why it was required. I'm also a little concerned around Merge Joins, as if I removed a Merge Join, because one of the relations was not required, and just left, say the SeqScan node for the other relation in place of the Merge Join, then I'd need to somehow check that none of the parent nodes were expecting some specific sort order. Perhaps I could just always leave any Sort node in place, if it existed, and just put the scan below that, but it all feels a bit like executor performing voodoo on the plan... i.e. just feels like a little bit more than the executor should know about plans. I'm a bit worried that I could spend a week on this and Tom or someone else then comes along and throws it out. So I'm really just looking for some confirmation to if this is a good or bad idea, based on the discoveries I've explained above. I really want to see this stuff working, but at the same time don't want to waste time on it if it's never going to be committed. Regards David Rowley
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/19 18:21), Ashutosh Bapat wrote: Ok. I added that comment to the commitfest and changed the status to ready for commiter. Many thanks! PS: the link to the review emails that discuss the issue doesn't work properly, so I re-added the link. Best regards, Etsuro Fujita -- 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] tracking commit timestamps
On 19 November 2014 02:12, Petr Jelinek p...@2ndquadrant.com wrote: Maybe we need better explanation of the LSN use-case(s) to understand why it should be stored here and why the other solutions are significantly worse. We should apply the same standard that has been applied elsewhere. If someone can show some software that could actually make use of LSN and there isn't a better way, then we can include it. PostgreSQL isn't a place where we speculate about possible future needs. I don't see why it should take 2+ years of prototypes, designs and discussions to get something in from BDR, but then we simply wave a hand and include something else at last minute without careful thought. Even if that means that later additions might need to think harder about upgrades. Timestamp and nodeid are useful for a variety of cases; LSN doesn't meet the same standard and should not be included now. We still have many months before even beta for people that want LSN to make a *separate* case for its inclusion as a separate feature. -- 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
[HACKERS] Unlogged tables can vanish after a crash
I observed an interesting (and I think buggy) behaviour today after one of our clusters crashed due to an out of space condition in the data directory. Five databases in that cluster have each one unlogged table. The log reads as follows: PANIC could not write to file pg_xlog/xlogtemp.1820: No space left on device ... LOGterminating any other active server processes ... LOGall server processes terminated; reinitializing LOGdatabase system was interrupted; last known up at 2014-11-18 18:04:28 CET LOGdatabase system was not properly shut down; automatic recovery in progress LOGredo starts at C9/50403B20 LOGredo done at C9/5A98 LOGcheckpoint starting: end-of-recovery immediate LOGcheckpoint complete: ... LOGautovacuum launcher started LOGdatabase system is ready to accept connections ... PANIC could not write to file pg_xlog/xlogtemp.4417: No space left on device ... LOGterminating any other active server processes ... LOGall server processes terminated; reinitializing LOGdatabase system was interrupted; last known up at 2014-11-18 18:04:38 CET LOGdatabase system was not properly shut down; automatic recovery in progress LOGredo starts at C9/5B70 LOGredo done at C9/5FFFE4E0 LOGcheckpoint starting: end-of-recovery immediate LOGcheckpoint complete: ... FATAL could not write to file pg_xlog/xlogtemp.4442: No space left on device LOGstartup process (PID 4442) exited with exit code 1 LOGaborting startup due to startup process failure After the problem was removed, the cluster was restarted. The log reads as follows: LOGending log output to stderr Future log output will go to log destination csvlog. LOGdatabase system was shut down at 2014-11-18 18:05:03 CET LOGautovacuum launcher started LOGdatabase system is ready to accept connections So no crash recovery was performed, probably because the startup process failed *after* it completed the end-of-recovery checkpoint. Now the main fork files for all five unlogged tables are gone; the init fork files are still there. Obviously the main fork got nuked during recovery, but the startup process died before it could recreate them: /* * Preallocate additional log files, if wanted. */ PreallocXlogFiles(EndOfLog); /* * Reset initial contents of unlogged relations. This has to be done * AFTER recovery is complete so that any unlogged relations created * during recovery also get picked up. */ if (InRecovery) ResetUnloggedRelations(UNLOGGED_RELATION_INIT); It seems to me that the right fix would be to recreate the unlogged relations *before* the checkpoint. Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
On 18 November 2014 21:19, Petr Jelinek p...@2ndquadrant.com wrote: Personally, I see this as natural extension of the conditional block control which we already have for loops with CONTINUE WHEN and EXIT WHEN. This basically extends it to any block and it seems quite natural to have it for me... That's a reasonable argument to include it. I seem to share the same opinion with Andrew: its not going to hurt to include this, but its not gonna cause dancing in the streets either. I would characterize that as 2 very neutral and unimpressed people, plus 3 in favour. Which seems enough to commit. Perhaps I misunderstand, Andrew? Any objectors, say so now or I will commit tomorrow. -- 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] tracking commit timestamps
On 19/11/14 12:20, Simon Riggs wrote: On 19 November 2014 02:12, Petr Jelinek p...@2ndquadrant.com wrote: Maybe we need better explanation of the LSN use-case(s) to understand why it should be stored here and why the other solutions are significantly worse. We should apply the same standard that has been applied elsewhere. If someone can show some software that could actually make use of LSN and there isn't a better way, then we can include it. ... We still have many months before even beta for people that want LSN to make a *separate* case for its inclusion as a separate feature. This is good point, we are not too late in the cycle that LSN couldn't be added later if we find that it is indeed needed (and we don't have to care about pg_upgrade until beta). -- Petr Jelinek 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] Unlogged tables can vanish after a crash
Hi, On 2014-11-19 11:26:56 +, Albe Laurenz wrote: I observed an interesting (and I think buggy) behaviour today after one of our clusters crashed due to an out of space condition in the data directory. Hah, just a couple days I pushed a fix for that ;) http://archives.postgresql.org/message-id/20140912112246.GA4984%40alap3.anarazel.de and http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d3586fc8aa5d9365a5c50cb5e555971eb633a4ec So no crash recovery was performed, probably because the startup process failed *after* it completed the end-of-recovery checkpoint. Now the main fork files for all five unlogged tables are gone; the init fork files are still there. You can recover them by restarting with -m immediate or so again. It seems to me that the right fix would be to recreate the unlogged relations *before* the checkpoint. Yep, that's what we're doing now. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unlogged tables can vanish after a crash
Andres Freund wrote: On 2014-11-19 11:26:56 +, Albe Laurenz wrote: I observed an interesting (and I think buggy) behaviour today after one of our clusters crashed due to an out of space condition in the data directory. Hah, just a couple days I pushed a fix for that ;) http://archives.postgresql.org/message-id/20140912112246.GA4984%40alap3.anarazel.de and http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d3586fc8aa5d9365a5c50cb5e555971eb633a4ec Thanks, I didn't see that. PostgreSQL, the database system where your bugs get fixed before you report them! Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SSL information view
On Tue, Nov 11, 2014 at 1:04 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Nov 11, 2014 at 1:43 AM, Magnus Hagander mag...@hagander.net wrote: Right now it just truncates the dn at NAMEDATALEN - so treating it the same as we do with hostnames. My guess is this is not a big problem because in the case of long DNs, most of the time the important stuff is at the beginning anyway... (And it's not like it's actually used for authentication, in which case it would of course be a problem). You should add it to the next CF for proper tracking, there are already many patches in the queue waiting for reviews :) Absolutely - I just wanted those that were already involved in the thread to get a chance to look at it early :) didn't want to submit it until it was complete. Which it is now - attached is a new version that includes docs. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ *** a/doc/src/sgml/monitoring.sgml --- b/doc/src/sgml/monitoring.sgml *** *** 300,305 postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser --- 300,313 /entry /row + row + entrystructnamepg_stat_ssl/indextermprimarypg_stat_ssl/primary/indexterm/entry + entryOne row per connection (regular and replication), showing statistics about +SSL used on this connection. +See xref linkend=pg-stat-ssl-view for details. + /entry + /row + /tbody /tgroup /table *** *** 825,830 postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser --- 833,907 listed; no information is available about downstream standby servers. /para + table id=pg-stat-ssl-view xreflabel=pg_stat_ssl +titlestructnamepg_stat_ssl/structname View/title +tgroup cols=3 + thead + row + entryColumn/entry + entryType/entry + entryDescription/entry + /row + /thead + +tbody + row + entrystructfieldpid//entry + entrytypeinteger//entry + entryProcess ID of a backend or WAL sender process/entry + /row + row + entrystructfieldssl//entry + entrytypeboolean//entry + entryTrue if SSL is used on this connection/entry + /row + row + entrystructfieldbits//entry + entrytypeinteger//entry + entryNumber of bits in the encryption algorithm used, or NULL + if SSL is not used on this connection/entry + /row + row + entrystructfieldcompression//entry + entrytypeboolean//entry + entryTrue if SSL compression is in use, false if not, + or NULL if SSL is not in use on this connection/entry + /row + row + entrystructfieldversion//entry + entrytypetext//entry + entryVersion of SSL in use, or NULL if SSL is not in use + on this connection/entry + /row + row + entrystructfieldcipher//entry + entrytypetext//entry + entryName of SSL cipher in use, or NULL if SSL is not in use + on this connection/entry + /row + row + entrystructfieldclientdn//entry + entrytypetext//entry + entryDistinguished Name (DN) field from the client certificate + used, or NULL if no client certificate was supplied or if SSL + is not in use on this connection. This field is truncated if the + DN field is longer than symbolNAMEDATALEN/symbol (64 characters + in a standard build) + /entry + /row +/tbody +/tgroup + /table + + para +The structnamepg_stat_ssl/structname view will contain one row per +backend or WAL sender process, showing statistics about SSL usage on +this connection. It can be joined to structnamepg_stat_activity/structname +or structnamepg_stat_replication/structname on the +structfieldpid/structfield column to get more details about the +connection. + /para + table id=pg-stat-archiver-view xreflabel=pg_stat_archiver titlestructnamepg_stat_archiver/structname View/title *** a/src/backend/catalog/system_views.sql --- b/src/backend/catalog/system_views.sql *** *** 648,653 CREATE VIEW pg_stat_replication AS --- 648,664 WHERE S.usesysid = U.oid AND S.pid = W.pid; + CREATE VIEW pg_stat_ssl AS + SELECT + I.pid, + I.ssl, + I.bits, + I.compression, + I.version, + I.cipher, + I.clientdn + FROM pg_stat_get_sslstatus() AS I; + CREATE VIEW pg_replication_slots AS SELECT L.slot_name, *** a/src/backend/libpq/be-secure-openssl.c --- b/src/backend/libpq/be-secure-openssl.c *** *** 88,93 static void info_cb(const SSL *ssl, int type, int args); --- 88,95 static void initialize_ecdh(void); static const char *SSLerrmessage(void); + static char *X509_NAME_to_cstring(X509_NAME *name); + /* are we in the middle
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On 18 November 2014 22:05, Petr Jelinek p...@2ndquadrant.com wrote: OK, promote works for me as well, I attached patch that changes continue to promote so you don't have to find and replace everything yourself. The changed doc wording probably needs to be checked. I've reworded docs a little. Which led me to think about shutdown more. If we ask for PAUSE and we're not in HotStandby it just ignores it, which means it changes into PROMOTE. My feeling is that we should change that into a SHUTDOWN, not a PROMOTE. I can raise that separately if anyone objects. Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? That gives a clean, fast shutdown rather than what looks like a crash. -- 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] tracking commit timestamps
Petr Jelinek wrote: This is good point, we are not too late in the cycle that LSN couldn't be added later if we find that it is indeed needed (and we don't have to care about pg_upgrade until beta). I think we're overblowing the pg_upgrade issue. Surely we don't need to preserve commit_ts data when upgrading across major versions; and pg_upgrade is perfectly prepared to remove old data when upgrading (actually it just doesn't copy it; consider pg_subtrans or pg_serial, for instance.) If we need to change binary representation in a future major release, we can do so without any trouble. -- Á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] Doing better at HINTing an appropriate column within errorMissingColumn()
On Tue, Nov 18, 2014 at 8:03 PM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Geoghegan p...@heroku.com writes: On Tue, Nov 18, 2014 at 3:29 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Nov 17, 2014 at 3:04 PM, Peter Geoghegan p...@heroku.com wrote: postgres=# select qty from orderlines ; ERROR: 42703: column qty does not exist HINT: Perhaps you meant to reference the column orderlines.quantity. I don't buy this example, because it would give you the same hint if you told it you wanted to access a column called ant, or uay, or tit. And that's clearly ridiculous. The reason why quantity looks like a reasonable suggestion for qty is because it's a conventional abbreviation, but an extremely high percentage of comparable cases won't be. I maintain that omission of part of the correct spelling should be weighed less. I would say that omission of the first letter should completely disqualify suggestions based on this heuristic; but it might make sense to weight omissions less after the first letter. I think we would be well-advised not to start inventing our own approximate matching algorithm. Peter's suggestion boils down to a guess that the default cost parameters for Levenshtein suck, and your suggestion boils down to a guess that we can fix the problems with Peter's suggestion by bolting another heuristic on top of it - and possibly running Levenshtein twice with different sets of cost parameters. Ugh. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] tracking commit timestamps
On Wed, Nov 19, 2014 at 8:22 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Petr Jelinek wrote: This is good point, we are not too late in the cycle that LSN couldn't be added later if we find that it is indeed needed (and we don't have to care about pg_upgrade until beta). I think we're overblowing the pg_upgrade issue. Surely we don't need to preserve commit_ts data when upgrading across major versions; and pg_upgrade is perfectly prepared to remove old data when upgrading (actually it just doesn't copy it; consider pg_subtrans or pg_serial, for instance.) If we need to change binary representation in a future major release, we can do so without any trouble. Actually, that's a good point. I still don't understand what the resistance is to add something quite inexpensive that multiple people obviously want, but at least if we don't, we still have the option to change it later. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On Wed, Nov 19, 2014 at 8:13 AM, Simon Riggs si...@2ndquadrant.com wrote: If we ask for PAUSE and we're not in HotStandby it just ignores it, which means it changes into PROMOTE. My feeling is that we should change that into a SHUTDOWN, not a PROMOTE. To me, that seems like a definite improvement. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
On 11/19/2014 06:35 AM, Simon Riggs wrote: On 18 November 2014 21:19, Petr Jelinek p...@2ndquadrant.com wrote: Personally, I see this as natural extension of the conditional block control which we already have for loops with CONTINUE WHEN and EXIT WHEN. This basically extends it to any block and it seems quite natural to have it for me... That's a reasonable argument to include it. I seem to share the same opinion with Andrew: its not going to hurt to include this, but its not gonna cause dancing in the streets either. I would characterize that as 2 very neutral and unimpressed people, plus 3 in favour. Which seems enough to commit. Perhaps I misunderstand, Andrew? That's about right, although I would put it a bit stronger than that. But if we're the only people unimpressed I'm not going to object further. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] Performance issue with libpq prepared queries on 9.3 and 9.4
On Mon, Nov 17, 2014 at 4:27 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Nov 13, 2014 at 7:34 PM, Tom Lane t...@sss.pgh.pa.us wrote: One thing that occurs to me is that if the generic plan estimate comes out much cheaper than the custom one, maybe we should assume that the generic's cost estimate is bogus. Right offhand I can't think of a reason for a custom plan to look worse than a generic one, unless there's a statistical quirk like this one. That's an interesting idea, but what do we do after deciding that it's bogus? Keep using custom plans. It's possible that the estimate that's in error is the custom one, but that's not the way to bet IMO, since the custom plan estimate is based on better information. The generic plan really can't be cheaper than the custom plan, but it could be the same price, or as close as makes no difference. Right, and what we want to do is use the generic plan as long as it's close to the same cost (close enough to not justify replanning effort). The trick here is to not be fooled by estimation errors. Can we assume that generic cost custom cost is always an estimation error? Maybe. It seems like kind of a fragile bet to me. There's going to be some qual selectivity below which an index scan on a particular table outperforms a sequential scan, but the selectivity estimated for a generic plan can be either higher or lower than the selectivity we'd estimate for some particular value. And once one of the two plans decides on an index scan while the other one divides on a sequential scan, it can cascade through and change the whole plan - e.g. because it affects whether the tuples emerge with usable pathkeys. I don't feel very confident about assuming that applying to the result of all that is going to tell us anything useful. I think what's really going on here is that the generic plan will be optimal for some range of possible qual selectivities. Typically, the qual is of the form col = val, so the plan will be optimal for all values where the estimated frequency is between some values A and B. What would be nice is to notice when we see a value that is outside that range, and switch to a custom plan in that case. I realize that the planner isn't really well set up to deliver the information we'd need to test that for each new supplied value, but that's really what we need to do. The current system wastes CPU cycles by replanning up to 5 times even when there is no benefit to be gained by it, but can also cause big performance problems when it settles into a generic plan and then a value with different characteristics shows up later on. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Increasing test coverage of WAL redo functions
To test WAL replay, I often set up a master-standby system with streaming replication and run make installcheck on the master. However, the regression suite doesn't generate all WAL record types. I spent some time looking at the lcov report (make coverage-html), and crafted new tests to test those redo functions that were not otherwise covered. All the new test cases are related to indexes, mostly vacuuming them. See attached. With this patch, all WAL record types are tested, although there are still a few codepaths within the redo functions (aside from can't happen error checks) that are not exercised. There are a couple of problems with these new tests: 1. Whether the vacuum tests test what they're supposed to, depends on what else is going on in the system. If there's another backend present that holds onto an snapshot, vacuum won't be able to remove any rows, so that code will go untested. Running those tests in parallel with other tests makes it quite likely that nothing can be vacuumed. 2. These make the regression database larger. The following tables and indexes are added: postgres=# \d+ List of relations Schema | Name | Type | Owner | Size | Description +--+---++-+- public | btree_tall_tbl | table | heikki | 24 kB | public | btree_test_tbl | table | heikki | 392 kB | public | gin_test_tbl | table | heikki | 588 MB | public | gist_point_tbl | table | heikki | 1056 kB | public | spgist_point_tbl | table | heikki | 1056 kB | public | spgist_text_tbl | table | heikki | 1472 kB | (6 rows) postgres=# \di+ List of relations Schema | Name | Type | Owner | Table | Size | Descri ption +--+---++--+-+--- -- public | btree_tall_idx | index | heikki | btree_tall_tbl | 1176 kB | public | btree_test_idx | index | heikki | btree_test_tbl | 472 kB | public | gin_test_idx | index | heikki | gin_test_tbl | 220 MB | public | gist_pointidx| index | heikki | gist_point_tbl | 1744 kB | public | spgist_point_idx | index | heikki | spgist_point_tbl | 1120 kB | public | spgist_text_idx | index | heikki | spgist_text_tbl | 440 kB | (6 rows) The GIN test needs to create a huge table, to cover the case of splitting an internal posting tree page. That 588MB table plus index is obviously too much to include in the regular regression suite. I'm not sure how much smaller it could be made, but it's going to be in that ballpark anyway. It also takes a long time to run. I think the rest are tolerable, they make the regression database about 9 MB larger, from 45 MB to 53 MB, and only take a few seconds to run, on my laptop. My plan is to leave out that large GIN test for now, and commit the rest. I'll add comments to the vacuum tests explaining that it's a hit and miss whether they manage to vacuum anything. It's still better to have the tests even if they sometimes fail to test vacuum as intended, than not have the tests at all. In either case, they won't fail unless there's a bug somewhere, and they will still exercise some code that's not otherwise tested at all. Thoughts? PS. The brin test case is currently in a funny position in serial_schedule, compared to parallel_schedule. This patch moves it to where I think it belongs. - Heikki diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out index 74d47be..c8e51fc 100644 --- a/src/test/regress/expected/btree_index.out +++ b/src/test/regress/expected/btree_index.out @@ -127,3 +127,32 @@ select proname from pg_proc where proname like E'RI\\_FKey%del' order by 1; RI_FKey_setnull_del (5 rows) +-- +-- Test B-tree insertion with a metapage update (XLOG_BTREE_INSERT_META +-- WAL record type). This happens when a fast-root page is split. +-- +-- First create a tree that's at least two levels deep. +create table btree_test_tbl(id int4); +insert into btree_test_tbl select generate_series(1, 1); +create index btree_test_idx on btree_test_tbl (id); +-- Delete most entries, and vacuum. This turns the leaf page into a fast root. +delete from btree_test_tbl where id 9990; +vacuum btree_test_tbl; +-- Now do more insertions, creating a sibling for the fast root, so that +-- it's not the fast root anymore. +insert into btree_test_tbl select generate_series(1, 9900); +-- +-- Test B-tree page deletion. In particular, deleting a non-leaf page. +-- +-- First create a tree that's at least four levels deep. The text inserted +-- is long and poorly compressible. That way only a few index tuples fit on +-- each page, allowing us to get a tall tree with fewer pages. +CREATE TABLE btree_tall_tbl(id int4, t text); +CREATE INDEX btree_tall_idx on btree_tall_tbl (id, t) WITH (fillfactor = 10); +INSERT INTO btree_tall_tbl +SELECT g,
[HACKERS] Functions used in index definitions shouldn't be changed
Currently it is possible to change the behaviour of a function with CREATE OR REPLACE FUNCTION even if the function is part of an index definition. I think that should be forbidden, because it is very likely to corrupt the index. I expect the objection that this would break valid use cases where people know exactly what they are doing, but I believe that this is a footgun for inexperienced users that should be disarmed. I'd also opt for forbidding behaviour changing modifications with ALTER FUNCTION for functions used in index definitions, specifically altering strictness. Attached is a patch implementing a fix. Yours, Laurenz Albe alter_index_function.patch Description: alter_index_function.patch -- 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] Increasing test coverage of WAL redo functions
Hi, On 2014-11-19 16:27:56 +0200, Heikki Linnakangas wrote: To test WAL replay, I often set up a master-standby system with streaming replication and run make installcheck on the master. However, the regression suite doesn't generate all WAL record types. I spent some time looking at the lcov report (make coverage-html), and crafted new tests to test those redo functions that were not otherwise covered. That sounds good. A next step would be to have them run in some automated fashion... All the new test cases are related to indexes, mostly vacuuming them. See attached. With this patch, all WAL record types are tested, although there are still a few codepaths within the redo functions (aside from can't happen error checks) that are not exercised. There are a couple of problems with these new tests: 1. Whether the vacuum tests test what they're supposed to, depends on what else is going on in the system. If there's another backend present that holds onto an snapshot, vacuum won't be able to remove any rows, so that code will go untested. Running those tests in parallel with other tests makes it quite likely that nothing can be vacuumed. Yes, that's annoying, but not prohibitive imo. Even having the routines only triggered every couple runs is better than them not being triggered at all. 2. These make the regression database larger. The following tables and indexes are added: postgres=# \d+ List of relations Schema | Name | Type | Owner | Size | Description +--+---++-+- public | btree_tall_tbl | table | heikki | 24 kB | public | btree_test_tbl | table | heikki | 392 kB | public | gin_test_tbl | table | heikki | 588 MB | public | gist_point_tbl | table | heikki | 1056 kB | public | spgist_point_tbl | table | heikki | 1056 kB | public | spgist_text_tbl | table | heikki | 1472 kB | (6 rows) postgres=# \di+ List of relations Schema | Name | Type | Owner | Table | Size | Descri ption +--+---++--+-+--- -- public | btree_tall_idx | index | heikki | btree_tall_tbl | 1176 kB | public | btree_test_idx | index | heikki | btree_test_tbl | 472 kB | public | gin_test_idx | index | heikki | gin_test_tbl | 220 MB | public | gist_pointidx| index | heikki | gist_point_tbl | 1744 kB | public | spgist_point_idx | index | heikki | spgist_point_tbl | 1120 kB | public | spgist_text_idx | index | heikki | spgist_text_tbl | 440 kB | (6 rows) The GIN test needs to create a huge table, to cover the case of splitting an internal posting tree page. That 588MB table plus index is obviously too much to include in the regular regression suite. I'm not sure how much smaller it could be made, but it's going to be in that ballpark anyway. It also takes a long time to run. How long? I think the rest are tolerable, they make the regression database about 9 MB larger, from 45 MB to 53 MB, and only take a few seconds to run, on my laptop. My plan is to leave out that large GIN test for now, and commit the rest. How about putting the gin test in a separate schedule? expensive_parallel_schedule or something? I'd be more comfortable if the tests were actually there, even if not run by default. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Functions used in index definitions shouldn't be changed
On 11/19/14 3:38 PM, Albe Laurenz wrote: I think that should be forbidden, because it is very likely to corrupt the index. I expect the objection that this would break valid use cases where people know exactly what they are doing, but I believe that this is a footgun for inexperienced users that should be disarmed. Yes, I believe that objection to be completely valid. I can't begin to describe how frustrating it is to be in the position where my hammer is made out of plastic because someone might hurt themself if it was a real one. I'd at least like to see a workaround for the less inexperienced users. .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Increasing test coverage of WAL redo functions
Heikki Linnakangas wrote: 2. These make the regression database larger. The following tables and indexes are added: postgres=# \d+ List of relations Schema | Name | Type | Owner | Size | Description +--+---++-+- public | btree_tall_tbl | table | heikki | 24 kB | public | btree_test_tbl | table | heikki | 392 kB | public | gin_test_tbl | table | heikki | 588 MB | public | gist_point_tbl | table | heikki | 1056 kB | public | spgist_point_tbl | table | heikki | 1056 kB | public | spgist_text_tbl | table | heikki | 1472 kB | (6 rows) I think it's good to have these tests, though Tom was complaining earlier about the size of the regression test database. Would it work to have this in a separate test suite, like the numeric_big stuff? We can have it run optionally, and perhaps set up a few buildfarm members to exercise them on a regular basis. Also I'm surprised that BRIN did not turn up here. At least the page evacuation protocol to obtain a new revmap page is not exercised by the current tests. I suppose it's because all WAL records are covered by other activity, and page evacuation does not emit a specialized WAL record. If we have the big test for this, maybe we can enlarge the table for the brin index too to ensure we cover this. BTW looking at the lcov reports the other day I noticed that the lines PG_FUNCTION_INFO_V1 do not get marked as ran, which decreases the coverage percentages ... in one of the BRIN files this was quite noticeable, bringing the function coverage count down to about 50-60% when it should have been 100%. -- Á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] Increasing test coverage of WAL redo functions
On 2014-11-19 11:54:47 -0300, Alvaro Herrera wrote: Heikki Linnakangas wrote: Schema | Name | Type | Owner | Size | Description +--+---++-+- public | btree_tall_tbl | table | heikki | 24 kB | public | btree_test_tbl | table | heikki | 392 kB | public | gin_test_tbl | table | heikki | 588 MB | public | gist_point_tbl | table | heikki | 1056 kB | public | spgist_point_tbl | table | heikki | 1056 kB | public | spgist_text_tbl | table | heikki | 1472 kB | (6 rows) I think it's good to have these tests, though Tom was complaining earlier about the size of the regression test database. Would it work to have this in a separate test suite, like the numeric_big stuff? We can have it run optionally, and perhaps set up a few buildfarm members to exercise them on a regular basis. I think the tests except the gin one are resonably sized - I'd much rather run them all the time. We shouldn't make the buildfarm configuration unnecessarily complex. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Functions used in index definitions shouldn't be changed
Albe Laurenz laurenz.a...@wien.gv.at wrote: Currently it is possible to change the behaviour of a function with CREATE OR REPLACE FUNCTION even if the function is part of an index definition. I think that should be forbidden, because it is very likely to corrupt the index. I expect the objection that this would break valid use cases where people know exactly what they are doing, but I believe that this is a footgun for inexperienced users that should be disarmed. I'd also opt for forbidding behaviour changing modifications with ALTER FUNCTION for functions used in index definitions, specifically altering strictness. Attached is a patch implementing a fix. Instead of adding extra check, shouldn't you just ensure that DEPENDENCY_INTERNAL is the dependency type and let the existing logic do the work? -- Antonin Houska Cybertec Schönig Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt Web: http://www.postgresql-support.de, http://www.cybertec.at -- 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 shutdown_at_recovery_target option to recovery.conf
On 19/11/14 14:13, Simon Riggs wrote: On 18 November 2014 22:05, Petr Jelinek p...@2ndquadrant.com wrote: OK, promote works for me as well, I attached patch that changes continue to promote so you don't have to find and replace everything yourself. The changed doc wording probably needs to be checked. I've reworded docs a little. Which led me to think about shutdown more. If we ask for PAUSE and we're not in HotStandby it just ignores it, which means it changes into PROMOTE. My feeling is that we should change that into a SHUTDOWN, not a PROMOTE. I can raise that separately if anyone objects. Ok that seems reasonable, I can write updated patch which does that. Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? That gives a clean, fast shutdown rather than what looks like a crash. My first (unsubmitted) version did that, there was some issue with latches when doing that, but I think that's no longer problem as the point at which the shutdown happens was moved away from the problematic part of code. Other than that, it's just child killing postmaster feels bit weird, but I don't have strong objection to it. -- Petr Jelinek 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] alternative model for handling locking in parallel groups
On Tue, Nov 18, 2014 at 8:53 AM, Amit Kapila amit.kapil...@gmail.com wrote: Won't this be addressed because both updates issued from myfunc() are considered as separate commands, so w.r.t lock it should behave as 2 different updates in same transaction. I think there may be more things to make updates possible via parallel workers apart from tuple lock. Hmm. It won't work to have different backends with different command counters anyway. If backend 1 is using command counter 5 and backend 2 is using command counter 6, then backend 2 will see its snapshot changing under it as backend 1 makes changes. That's not OK. But that's not really the point. Heavyweight locks are sometimes used to protect pretty low-level things, like the right to update a tuple, or physically extend a relation by a block on disk, or split a bucket in a hash index. It won't be OK to just ignore the requirement of mutual exclusion in those cases, I think. We could of course decide that the parallel processes must have their own mechanism to prevent such conflicts apart from the lock manager, but they've got to be prevented somehow. After thinking about these cases for a bit, I came up with a new possible approach to this problem. Suppose that, at the beginning of parallelism, when we decide to start up workers, we grant all of the locks already held by the master to each worker (ignoring the normal rules for lock conflicts). Thereafter, we do everything the same as now, with no changes to the deadlock detector. That allows the lock conflicts to happen normally in the first two cases above, while preventing the unwanted lock conflicts in the second two cases. Here I think we have to consider how to pass the information about all the locks held by master to worker backends. Also I think assuming we have such an information available, still it will be considerable work to grant locks considering the number of locks we acquire [1] (based on Simon's analysis) and the additional memory they require. Finally I think deadlock detector work might also be increased as there will be now more procs to visit. In general, I think this scheme will work, but I am not sure it is worth at this stage (considering initial goal to make parallel workers will be used for read operations). I think this scheme might be quite easy to implement - we just need the user backend to iterate over the locks it holds and serialize them (similar to what pg_background does for GUCs) and store that in the DSM; the parallel worker calls some function on that serialized representation to put all those locks back in place. The actual deadlock detector should need few or no changes, which seems like a major advantage in comparison with the approach dicussed on the other thread. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] add ssl_protocols configuration option
Dag-Erling Smørgrav d...@des.no writes: The attached patches add an ssl_protocols configuration option which control which versions of SSL or TLS the server will use. The syntax is similar to Apache's SSLProtocols directive, except that the list is colon-separated instead of whitespace-separated, although that is easy to change if it proves unpopular. Hello, Here is my review of the patch against master branch: * The patch applies and compiles cleanly, except for sgml docs: postgres.xml:66141: element varlistentry: validity error : Element varlistentry content does not follow the DTD, expecting (term+ , listitem), got (term indexterm listitem) /para/listitem/varlistentryvarlistentry ^ The fix is to move indexterm under the term tag in the material added to config.sgml by the patch. * The patch works as advertised, though the only way to verify that connections made with the protocol disabled by the GUC are indeed rejected is to edit fe-secure-openssl.c to only allow specific TLS versions. Adding configuration on the libpq side as suggested in the original discussion might help here. * The code allows specifying SSLv2 and SSLv3 in the GUC, but removes them forcibly after parsing the complete string (a warning is issued). Should we also add a note about this to the documentation? * In case the list of allowed protocols comes empty a FATAL error message is logged: could not set the protocol list (no valid protocols available). I think it's worth changing that to could not set SSL protocol list... All other related error/warning logs include SSL. * The added code follows conventions and looks good to me. I didn't spot any problems in the parser. I've tried a good deal of different values and all seem to be parsed correctly. I would try to apply patches for older branches if there is consensus that we really need this patch and we want to back-patch it. Thanks. -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
On 18 November 2014 21:19, Petr Jelinek p...@2ndquadrant.com wrote: Personally, I see this as natural extension of the conditional block control which we already have for loops with CONTINUE WHEN and EXIT WHEN. This basically extends it to any block and it seems quite natural to have it for me... This seems to me like a step in the direction of APL, where every statement is a conditional. Perl has the option of putting the conditional on the end of a statement as suggested here for ASSERT. My experience has been that while it may look prettier to some, the conditional is overlooked in reviews, etc., more often than one would expect, giving a net loss in the overall risk/productivity analysis. As a code maintainer, I would be opposed to adding something like this for no other reason than perceived aesthetics. Your mileage may, of course, vary.
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On 19 November 2014 13:13, Simon Riggs si...@2ndquadrant.com wrote: I've reworded docs a little. Done If we ask for PAUSE and we're not in HotStandby it just ignores it, which means it changes into PROMOTE. My feeling is that we should change that into a SHUTDOWN, not a PROMOTE. Done Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? Done Other plan is to throw a FATAL message. That gives a clean, fast shutdown rather than what looks like a crash. I've also changed the location of where we do RECOVERY_TARGET_ACTION_SHUTDOWN, so its in the same place as where we pause. I've also moved the check to see if we should throw FATAL because aren't yet consistent to *before* we do any actionOnRecoveryTarget stuff. It seems essential that we know that earlier rather than later. Thoughts? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services shutdown_at_recovery_target-v4.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] What exactly is our CRC algorithm?
At 2014-11-11 16:56:00 +0530, a...@2ndquadrant.com wrote:
I'm working on this (first speeding up the default calculation using
slice-by-N, then adding support for the SSE4.2 CRC instruction on
top).
I've done the first part in the attached patch, and I'm working on the
second (especially the bits to issue CPUID at startup and decide which
implementation to use).
As a benchmark, I ran pg_xlogdump --stats against 11GB of WAL data (674
segments) generated by running a total of 2M pgbench transactions on a
db initialised with scale factor 25. The tests were run on my i5-3230
CPU, and the code in each case was compiled with -O3 -msse4.2 (and
without --enable-debug). The profile was dominated by the CRC
calculation in ValidXLogRecord.
With HEAD's CRC code:
bin/pg_xlogdump --stats wal/00010001 29.81s user 3.56s system
77% cpu 43.274 total
bin/pg_xlogdump --stats wal/00010001 29.59s user 3.85s system
75% cpu 44.227 total
With slice-by-4 (a minor variant of the attached patch; the results are
included only for curiosity's sake, but I can post the code if needed):
bin/pg_xlogdump --stats wal/00010001 13.52s user 3.82s system
48% cpu 35.808 total
bin/pg_xlogdump --stats wal/00010001 13.34s user 3.96s system
48% cpu 35.834 total
With slice-by-8 (i.e. the attached patch):
bin/pg_xlogdump --stats wal/00010001 7.88s user 3.96s system
34% cpu 34.414 total
bin/pg_xlogdump --stats wal/00010001 7.85s user 4.10s system
34% cpu 35.001 total
(Note the progressive reduction in user time from ~29s to ~8s.)
Finally, just for comparison, here's what happens when we use the
hardware instruction via gcc's __builtin_ia32_crc32xx intrinsics
(i.e. the additional patch I'm working on):
bin/pg_xlogdump --stats wal/00010001 3.33s user 4.79s system
23% cpu 34.832 total
There are a number of potential micro-optimisations, I just wanted to
submit the obvious thing first and explore more possibilities later.
-- Abhijit
diff --git a/src/include/utils/pg_crc.h b/src/include/utils/pg_crc.h
index f871cba..55934e5 100644
--- a/src/include/utils/pg_crc.h
+++ b/src/include/utils/pg_crc.h
@@ -41,6 +41,8 @@
typedef uint32 pg_crc32;
+extern pg_crc32 pg_comp_crc32c(pg_crc32 crc, const void *data, size_t len);
+
/*
* CRC calculation using the CRC-32C (Castagnoli) polynomial.
*
@@ -51,7 +53,7 @@ typedef uint32 pg_crc32;
#define INIT_CRC32C(crc) ((crc) = 0x)
#define FIN_CRC32C(crc) ((crc) ^= 0x)
#define COMP_CRC32C(crc, data, len) \
- COMP_CRC32_NORMAL_TABLE(crc, data, len, pg_crc32c_table)
+ ((crc) = pg_comp_crc32c((crc), (char *) (data), (len)))
#define EQ_CRC32C(c1, c2) ((c1) == (c2))
/*
@@ -115,7 +117,7 @@ do { \
} while (0)
/* Constant tables for CRC-32C and CRC-32 polynomials */
-extern CRCDLLIMPORT const uint32 pg_crc32c_table[];
+extern CRCDLLIMPORT const uint32 pg_crc32c_table[8][256];
extern CRCDLLIMPORT const uint32 pg_crc32_table[];
#endif /* PG_CRC_H */
diff --git a/src/include/utils/pg_crc_tables.h b/src/include/utils/pg_crc_tables.h
index cb6b470..f814c91 100644
--- a/src/include/utils/pg_crc_tables.h
+++ b/src/include/utils/pg_crc_tables.h
@@ -28,71 +28,519 @@
* This table is based on the so-called Castagnoli polynomial (the same
* that is used e.g. in iSCSI).
*/
-const uint32 pg_crc32c_table[256] = {
- 0x, 0xF26B8303, 0xE13B70F7, 0x1350F3F4,
- 0xC79A971F, 0x35F1141C, 0x26A1E7E8, 0xD4CA64EB,
- 0x8AD958CF, 0x78B2DBCC, 0x6BE22838, 0x9989AB3B,
- 0x4D43CFD0, 0xBF284CD3, 0xAC78BF27, 0x5E133C24,
- 0x105EC76F, 0xE235446C, 0xF165B798, 0x030E349B,
- 0xD7C45070, 0x25AFD373, 0x36FF2087, 0xC494A384,
- 0x9A879FA0, 0x68EC1CA3, 0x7BBCEF57, 0x89D76C54,
- 0x5D1D08BF, 0xAF768BBC, 0xBC267848, 0x4E4DFB4B,
- 0x20BD8EDE, 0xD2D60DDD, 0xC186FE29, 0x33ED7D2A,
- 0xE72719C1, 0x154C9AC2, 0x061C6936, 0xF477EA35,
- 0xAA64D611, 0x580F5512, 0x4B5FA6E6, 0xB93425E5,
- 0x6DFE410E, 0x9F95C20D, 0x8CC531F9, 0x7EAEB2FA,
- 0x30E349B1, 0xC288CAB2, 0xD1D83946, 0x23B3BA45,
- 0xF779DEAE, 0x05125DAD, 0x1642AE59, 0xE4292D5A,
- 0xBA3A117E, 0x4851927D, 0x5B016189, 0xA96AE28A,
- 0x7DA08661, 0x8FCB0562, 0x9C9BF696, 0x6EF07595,
- 0x417B1DBC, 0xB3109EBF, 0xA0406D4B, 0x522BEE48,
- 0x86E18AA3, 0x748A09A0, 0x67DAFA54, 0x95B17957,
- 0xCBA24573, 0x39C9C670, 0x2A993584, 0xD8F2B687,
- 0x0C38D26C, 0xFE53516F, 0xED03A29B, 0x1F682198,
- 0x5125DAD3, 0xA34E59D0, 0xB01EAA24, 0x42752927,
- 0x96BF4DCC, 0x64D4CECF, 0x77843D3B, 0x85EFBE38,
- 0xDBFC821C, 0x2997011F, 0x3AC7F2EB, 0xC8AC71E8,
- 0x1C661503, 0xEE0D9600, 0xFD5D65F4, 0x0F36E6F7,
- 0x61C69362, 0x93AD1061, 0x80FDE395, 0x72966096,
- 0xA65C047D, 0x5437877E, 0x4767748A, 0xB50CF789,
- 0xEB1FCBAD, 0x197448AE, 0x0A24BB5A, 0xF84F3859,
- 0x2C855CB2, 0xDEEEDFB1, 0xCDBE2C45, 0x3FD5AF46,
- 0x7198540D, 0x83F3D70E, 0x90A324FA, 0x62C8A7F9,
- 0xB602C312, 0x44694011, 0x5739B3E5, 0xA55230E6,
- 0xFB410CC2, 0x092A8FC1, 0x1A7A7C35, 0xE811FF36,
-
Re: [HACKERS] group locking: incomplete patch, just for discussion
On Tue, Nov 18, 2014 at 4:40 AM, Jeff Davis pg...@j-davis.com wrote: To reiterate the basic problem here, if we do nothing at all about the lock manager, a parallel backend can stall trying to grab an AccessExclusiveLock that the user backend alread holds, either because the user backend holds an AccessExclusiveLock as well, or because some other process is waiting for one, we'll deadlock and not notice. My feeling is that we should keep the concept of a group and group leader from your patch, and improve the deadlock detector to make use of that information (looking at the code, it looks doable but not trivial). But unless I am missing something, we should separate out the lock sharing, and defer that until later. Doing as you propose solves this problem: 1. Backend A-1 acquires AccessShareLock on relation R. 2. Backend B waits for AccessExclusiveLock on relation R. 3. Backend A-2 waits for AccessShareLock on relation R. Knowing that A-1 and A-2 are related, the deadlock detector can move A-2 ahead of B and grant the lock. All is well. But your proposal does not solve this problem: 1. Backend A-1 acquires AccessExclusiveLock on relation R. 2. Backend A-2 waits for AccessShareLock on relation R. The good news is that the deadlock detector should realize that since A-1 and A-2 are in the same group, this is a deadlock. And it can abort either A-1 or A-2, which will eventually abort them both. The bad news, to borrow a phrase from Peter Geoghegan, is that it's an unprincipled deadlock; a user confronted with the news that her parallel scan has self-deadlocked will be justifiably dismayed. It's been proposed by Andres, and maybe a few other people, that we can detect this situation and just not use parallelism in these cases, but that's actually quite hard to do. Of course, it's pretty easy for a backend A-1 contemplating parallel scan of relation R to notice that it holds a lock that conflicts with the AccessShareLock it expects a prospective parallel backend A-2 to attempt to acquire. I don't think there's a lock manager method for that today, but we could easily add one. However, A-1 might incidentally hold other locks (from earlier statements in the transaction) that conflict with locks that will be sought by A-2, and as discussed *extensively* upthread, it is not easy to predict all of the locks a parellel backend might try to take: even seemingly-trivial code like the equality operators for built-in data types can sometimes take relation locks, and we have no infrastructure for predicting which ones. Even if you could somehow solve that problem, there's also the issue of plan caching. If we make a parallel plan for an SQL statement inside a PL/pgsql procedure because our backend holds no strong locks, and then we acquire a strong lock on a relevant relation, we have to invalidate that plan. I think that will add complexity inside the plan cache machinery. It seems to me that it would be better to handle these issues inside the lock manager rather than letting them creep into other parts of the system. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On 2014-11-19 15:47:05 +, Simon Riggs wrote: Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? Done I don't think that's ok. The postmaster is the one that should be in control, not some subprocess. I fail to see the win in simplicity over using exit (like we already do for the normal end of recovery!) is. The issue with the log line seems perfectly easily to avoid by just checking the exit code in postmaster.c. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On 19 November 2014 15:57, Andres Freund and...@2ndquadrant.com wrote: On 2014-11-19 15:47:05 +, Simon Riggs wrote: Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? Done I don't think that's ok. The postmaster is the one that should be in control, not some subprocess. I fail to see the win in simplicity over using exit (like we already do for the normal end of recovery!) is. The issue with the log line seems perfectly easily to avoid by just checking the exit code in postmaster.c. We need to be able to tell the difference between a crashed Startup process and this usage. As long as we can tell, I don't mind how we do it. Suggestions please. -- 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] Add shutdown_at_recovery_target option to recovery.conf
On 19/11/14 17:04, Simon Riggs wrote: On 19 November 2014 15:57, Andres Freund and...@2ndquadrant.com wrote: On 2014-11-19 15:47:05 +, Simon Riggs wrote: Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? Done I don't think that's ok. The postmaster is the one that should be in control, not some subprocess. I fail to see the win in simplicity over using exit (like we already do for the normal end of recovery!) is. The issue with the log line seems perfectly easily to avoid by just checking the exit code in postmaster.c. We need to be able to tell the difference between a crashed Startup process and this usage. As long as we can tell, I don't mind how we do it. Suggestions please. Different exit code? -- Petr Jelinek 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] Add shutdown_at_recovery_target option to recovery.conf
On 2014-11-19 16:04:49 +, Simon Riggs wrote: On 19 November 2014 15:57, Andres Freund and...@2ndquadrant.com wrote: On 2014-11-19 15:47:05 +, Simon Riggs wrote: Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? Done I don't think that's ok. The postmaster is the one that should be in control, not some subprocess. Just as an example why I think this is wrong: Some user could just trigger replication to resume and we'd be in some awkward state. I fail to see the win in simplicity over using exit (like we already do for the normal end of recovery!) is. The issue with the log line seems perfectly easily to avoid by just checking the exit code in postmaster.c. We need to be able to tell the difference between a crashed Startup process and this usage. Exit code, as suggested above. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
Andrew Dunstan and...@dunslane.net writes: On 11/19/2014 06:35 AM, Simon Riggs wrote: I seem to share the same opinion with Andrew: its not going to hurt to include this, but its not gonna cause dancing in the streets either. I would characterize that as 2 very neutral and unimpressed people, plus 3 in favour. Which seems enough to commit. That's about right, although I would put it a bit stronger than that. But if we're the only people unimpressed I'm not going to object further. FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) 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] Add shutdown_at_recovery_target option to recovery.conf
On 19/11/14 16:47, Simon Riggs wrote: On 19 November 2014 13:13, Simon Riggs si...@2ndquadrant.com wrote: Also, for the Shutdown itself, why are we not using kill(PostmasterPid, SIGINT)? Done Other plan is to throw a FATAL message. That gives a clean, fast shutdown rather than what looks like a crash. I've also changed the location of where we do RECOVERY_TARGET_ACTION_SHUTDOWN, so its in the same place as where we pause. Another problem with how you did these two changes is that if you just pause when you send kill then during clean shutdown the recovery will be resumed and will finish renaming the recovery.conf to recovery.done, bumping timeline etc, and we don't want that since that prevents resuming recovery in the future. -- Petr Jelinek 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] Functions used in index definitions shouldn't be changed
Antonin Houska a...@cybertec.at writes: Albe Laurenz laurenz.a...@wien.gv.at wrote: Currently it is possible to change the behaviour of a function with CREATE OR REPLACE FUNCTION even if the function is part of an index definition. I think that should be forbidden, because it is very likely to corrupt the index. I expect the objection that this would break valid use cases where people know exactly what they are doing, but I believe that this is a footgun for inexperienced users that should be disarmed. I'd also opt for forbidding behaviour changing modifications with ALTER FUNCTION for functions used in index definitions, specifically altering strictness. Attached is a patch implementing a fix. Instead of adding extra check, shouldn't you just ensure that DEPENDENCY_INTERNAL is the dependency type and let the existing logic do the work? The dependency logic only prohibits DROP, not ALTER, so changing the dependency type wouldn't be enough to accomplish that. But I dislike the entire concept, so implementation details are not that interesting. A comparable issue is that alteration of text search configurations may partially invalidate precomputed tsvector columns and indexes based on tsvector data. We discussed whether we should prohibit that somehow and explicitly decided that it would be a bad idea. In the text search case, changes like adding stop words are common and don't meaningfully invalidate indexes. In some cases you may decide that you need to recompute the tsvector data, but that decision should be left to the user. I think that pretty much the same thing applies to functions used in indexes. There are too many use-cases where alterations don't meaningfully impact the stored data for us to get away with a blanket prohibition. (I'm pretty sure that this has been discussed in the past, too. If Albe really wants to push the point he should look up the previous discussions and explain why the decision was wrong.) 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] tracking commit timestamps
On 11/19/2014 08:22 AM, Alvaro Herrera wrote: I think we're overblowing the pg_upgrade issue. Surely we don't need to preserve commit_ts data when upgrading across major versions; and pg_upgrade is perfectly prepared to remove old data when upgrading (actually it just doesn't copy it; consider pg_subtrans or pg_serial, for instance.) If we need to change binary representation in a future major release, we can do so without any trouble. That sounds reasonable. I am okay with Petr removing the LSN portion this patch. -- 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 shutdown_at_recovery_target option to recovery.conf
On Wed, Nov 19, 2014 at 10:49 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Nov 19, 2014 at 8:13 AM, Simon Riggs si...@2ndquadrant.com wrote: If we ask for PAUSE and we're not in HotStandby it just ignores it, which means it changes into PROMOTE. My feeling is that we should change that into a SHUTDOWN, not a PROMOTE. To me, that seems like a definite improvement. But changing the default will force us to set action_at_recovery_target to 'promote' when we want to just recover the database to the specified point. This extra step is not necessary so far, but required in 9.5, which would surprise the users and may cause some troubles like Oh, in 9.5, PITR failed and the server shut down unexpectedly even though I just ran the PITR procedures that I used to use so far. Of course probably I can live with the change of the default if it's really worthwhile and we warn the users about that, though. 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] proposal: plpgsql - Assert statement
On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: Andrew Dunstan and...@dunslane.net writes: On 11/19/2014 06:35 AM, Simon Riggs wrote: I seem to share the same opinion with Andrew: its not going to hurt to include this, but its not gonna cause dancing in the streets either. I would characterize that as 2 very neutral and unimpressed people, plus 3 in favour. Which seems enough to commit. That's about right, although I would put it a bit stronger than that. But if we're the only people unimpressed I'm not going to object further. FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) What I liked about this syntax was that we could eventually have: RAISE ASSERT WHEN stuff; ...and if assertions are disabled, we can skip evaluating the condition. If you just write an IF .. THEN block you can't do that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
On 11/19/2014 05:58 PM, Abhijit Menon-Sen wrote: At 2014-11-11 16:56:00 +0530, a...@2ndquadrant.com wrote: I'm working on this (first speeding up the default calculation using slice-by-N, then adding support for the SSE4.2 CRC instruction on top). I've done the first part in the attached patch, and I'm working on the second (especially the bits to issue CPUID at startup and decide which implementation to use). As a benchmark, I ran pg_xlogdump --stats against 11GB of WAL data (674 segments) generated by running a total of 2M pgbench transactions on a db initialised with scale factor 25. That's an interesting choice of workload. That sure is heavy on the CRC calculation, but the speed of pg_xlogdump hardly matters in real life. With HEAD's CRC code: bin/pg_xlogdump --stats wal/00010001 29.81s user 3.56s system 77% cpu 43.274 total bin/pg_xlogdump --stats wal/00010001 29.59s user 3.85s system 75% cpu 44.227 total With slice-by-4 (a minor variant of the attached patch; the results are included only for curiosity's sake, but I can post the code if needed): bin/pg_xlogdump --stats wal/00010001 13.52s user 3.82s system 48% cpu 35.808 total bin/pg_xlogdump --stats wal/00010001 13.34s user 3.96s system 48% cpu 35.834 total With slice-by-8 (i.e. the attached patch): bin/pg_xlogdump --stats wal/00010001 7.88s user 3.96s system 34% cpu 34.414 total bin/pg_xlogdump --stats wal/00010001 7.85s user 4.10s system 34% cpu 35.001 total (Note the progressive reduction in user time from ~29s to ~8s.) Finally, just for comparison, here's what happens when we use the hardware instruction via gcc's __builtin_ia32_crc32xx intrinsics (i.e. the additional patch I'm working on): bin/pg_xlogdump --stats wal/00010001 3.33s user 4.79s system 23% cpu 34.832 total Impressive! It would be good to see separate benchmarks on WAL generation, and WAL replay. pg_xlogdump is probably close to WAL replay, but the WAL generation case is somewhat different, as WAL is generated in small pieces, and each piece is accumulated to the CRC separately. The slice-by-X algorithm might be less effective in that scenario. I have no doubt that it's still a lot faster than the byte-at-a-time operation, but would be nice to have numbers on it. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] What exactly is our CRC algorithm?
Robert Haas robertmh...@gmail.com writes: On Wed, Nov 19, 2014 at 11:44 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: That's an interesting choice of workload. That sure is heavy on the CRC calculation, but the speed of pg_xlogdump hardly matters in real life. But isn't a workload that is heavy on CRC calculation exactly what we want here? That way we can see clearly how much benefit we're getting on that particular part of the computation. It'll still speed up other workloads, too, just not as much. Heikki's point is that it's an unrealistic choice of CRC chunk size. Maybe that doesn't matter very much, but it's unproven. 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] Increasing test coverage of WAL redo functions
On 11/19/2014 04:54 PM, Alvaro Herrera wrote: Also I'm surprised that BRIN did not turn up here. At least the page evacuation protocol to obtain a new revmap page is not exercised by the current tests. I suppose it's because all WAL records are covered by other activity, and page evacuation does not emit a specialized WAL record. If we have the big test for this, maybe we can enlarge the table for the brin index too to ensure we cover this. Yeah, all of BRIN's redo functions are fully covered by current regression tests. Well done :-). The evacuation code indeed isn't covered. That would be pretty easy to fix; just make a table that has a brin index on it large enough. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Move Deprecated configure.in to configure.ac
configure.in was deprecated some years ago. We have a bug at Gentoo [1] to move it to configure.ac. I've done so in my git-clone of the postgresql repo, and ran autoupdate to move away from the deprecated functions as well. I generated the configure script again, but that didn't change. make make check all succeeded without intervention. [1] https://bugs.gentoo.org/show_bug.cgi?id=529680 -- Mr. Aaron W. Swenson Gentoo Linux Developer PostgreSQL Herd Bull Email : titanof...@gentoo.org GnuPG FP : 2C00 7719 4F85 FB07 A49C 0E31 5713 AA03 D1BB FDA0 GnuPG ID : D1BBFDA0 From 60a76e24def32e7206e6d76999e6dca3575656da Mon Sep 17 00:00:00 2001 From: Aaron W. Swenson aaron.w.swen...@gmail.com Date: Wed, 19 Nov 2014 11:44:53 -0500 Subject: [PATCH] Move Deprecated configure.in to configure.ac configure.in was deprecated some years ago. Moved to configure.ac and autoupdate run to move away from deprecated functions. --- configure.ac | 2010 + configure.in | 2036 -- 2 files changed, 2010 insertions(+), 2036 deletions(-) create mode 100644 configure.ac delete mode 100644 configure.in diff --git a/configure.ac b/configure.ac new file mode 100644 index 000..68ecc0e --- /dev/null +++ b/configure.ac @@ -0,0 +1,2010 @@ +dnl Process this file with autoconf to produce a configure script. +dnl configure.in +dnl +dnl Developers, please strive to achieve this order: +dnl +dnl 0. Initialization and options processing +dnl 1. Programs +dnl 2. Libraries +dnl 3. Header files +dnl 4. Types +dnl 5. Structures +dnl 6. Compiler characteristics +dnl 7. Functions, global variables +dnl 8. System services +dnl +dnl Read the Autoconf manual for details. +dnl +m4_pattern_forbid(^PGAC_)dnl to catch undefined macros + +AC_INIT([PostgreSQL],[9.5devel],[pgsql-b...@postgresql.org]) + +m4_if(m4_defn([m4_PACKAGE_VERSION]), [2.69], [], [m4_fatal([Autoconf version 2.69 is required. +Untested combinations of 'autoconf' and PostgreSQL versions are not +recommended. You can remove the check from 'configure.in' but it is then +your responsibility whether the result works or not.])]) +AC_COPYRIGHT([Copyright (c) 1996-2014, PostgreSQL Global Development Group]) +AC_CONFIG_SRCDIR([src/backend/access/common/heaptuple.c]) +AC_CONFIG_AUX_DIR(config) +AC_PREFIX_DEFAULT(/usr/local/pgsql) +AC_SUBST(configure_args, [$ac_configure_args]) + +[PG_MAJORVERSION=`expr $PACKAGE_VERSION : '\([0-9][0-9]*\.[0-9][0-9]*\)'`] +AC_SUBST(PG_MAJORVERSION) +AC_DEFINE_UNQUOTED(PG_MAJORVERSION, $PG_MAJORVERSION, [PostgreSQL major version as a string]) + +PGAC_ARG_REQ(with, extra-version, [STRING], [append STRING to version], + [PG_VERSION=$PACKAGE_VERSION$withval], + [PG_VERSION=$PACKAGE_VERSION]) +AC_DEFINE_UNQUOTED(PG_VERSION, $PG_VERSION, [PostgreSQL version as a string]) + +AC_CANONICAL_HOST + +template= +AC_MSG_CHECKING([which template to use]) + +PGAC_ARG_REQ(with, template, [NAME], [override operating system template], +[ + case $withval in +list) echo; ls $srcdir/src/template; exit;; +*) if test -f $srcdir/src/template/$with_template ; then + template=$withval +else + AC_MSG_ERROR(['$withval' is not a valid template name. Use 'list' for a list.]) +fi;; + esac +], +[ +# --with-template not given + +case $host_os in + aix*) template=aix ;; + cygwin*) template=cygwin ;; + darwin*) template=darwin ;; +dragonfly*) template=netbsd ;; + freebsd*) template=freebsd ;; +hpux*) template=hpux ;; + linux*|gnu*|k*bsd*-gnu) + template=linux ;; + mingw*) template=win32 ;; + netbsd*) template=netbsd ;; + openbsd*) template=openbsd ;; + sco*) template=sco ;; + solaris*) template=solaris ;; + sysv5*) template=unixware ;; +esac + + if test x$template = x ; then +AC_MSG_ERROR([[ +*** +PostgreSQL has apparently not been ported to your platform yet. +To try a manual configuration, look into the src/template directory +for a similar platform and use the '--with-template=' option. + +Please also contact pgsql-b...@postgresql.org to see about +rectifying this. Include the above 'checking host system type...' +line. +*** +]]) + fi + +]) + +AC_MSG_RESULT([$template]) + +PORTNAME=$template +AC_SUBST(PORTNAME) + +# Initialize default assumption that we do not need separate assembly code +# for TAS (test-and-set). This can be overridden by the template file +# when it's executed. +need_tas=no +tas_file=dummy.s + + + +## +## Command line options +## + +# +# Add non-standard directories to the include path +# +PGAC_ARG_REQ(with, includes, [DIRS], [look for additional header files in DIRS]) + + +# +# Add non-standard directories to the library search path +# +PGAC_ARG_REQ(with, libraries, [DIRS], [look for additional libraries in
Re: [HACKERS] What exactly is our CRC algorithm?
On 11/19/2014 06:49 PM, Robert Haas wrote: On Wed, Nov 19, 2014 at 11:44 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: That's an interesting choice of workload. That sure is heavy on the CRC calculation, but the speed of pg_xlogdump hardly matters in real life. But isn't a workload that is heavy on CRC calculation exactly what we want here? That way we can see clearly how much benefit we're getting on that particular part of the computation. It'll still speed up other workloads, too, just not as much. Sure. But pg_xlogdump's way of using the CRC isn't necessarily representative of how the backend uses it. It's probably pretty close to WAL replay in the server, but even there the server might be hurt more by the extra cache used by the lookup tables. And a backend generating the WAL computes the CRC on smaller pieces than pg_xlogdump and WAL redo does. That said, the speedup is so large that I'm sure this is a big win in the server too, despite those factors. - Heikki -- 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] Move Deprecated configure.in to configure.ac
Aaron W. Swenson titanof...@gentoo.org writes: configure.in was deprecated some years ago. We have a bug at Gentoo [1] to move it to configure.ac. I see no advantage to this. It'll mess up our git history to do so, not to mention complicate back-patching. If we were planning to adopt additional tools from the autoconf suite then we might have to change the file suffix, but I have heard no suggestions that we might want to do that. In particular, we really do not care what automake thinks. 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] What exactly is our CRC algorithm?
On 2014-11-19 19:12:22 +0200, Heikki Linnakangas wrote: On 11/19/2014 06:49 PM, Robert Haas wrote: On Wed, Nov 19, 2014 at 11:44 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: That's an interesting choice of workload. That sure is heavy on the CRC calculation, but the speed of pg_xlogdump hardly matters in real life. But isn't a workload that is heavy on CRC calculation exactly what we want here? That way we can see clearly how much benefit we're getting on that particular part of the computation. It'll still speed up other workloads, too, just not as much. Sure. But pg_xlogdump's way of using the CRC isn't necessarily representative of how the backend uses it. It's probably pretty close to WAL replay in the server, but even there the server might be hurt more by the extra cache used by the lookup tables. And a backend generating the WAL computes the CRC on smaller pieces than pg_xlogdump and WAL redo does. Right. Although it hugely depends on the checkpoint settings - if there's many FPWs it doesn't matter much. Obviously it won't be a fourfold performance improvement in the server, but given the profiles I've seen in the past I'm pretty sure it'll bee a benefit. That said, the speedup is so large that I'm sure this is a big win in the server too, despite those factors. Yep. I've done some fast and loose benchmarking in the past and it was quite noticeable. Made XLogInsert() nearly entirely drop from profiles. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_test_fsync file descriptor leak
On Tue, Nov 18, 2014 at 3:41 PM, Jeff Janes jeff.ja...@gmail.com wrote: The open_datasync code opens the output file as a test to make sure the flags are accepted by the OS, and if it succeeds it then immediately opens the file again with the same flags, overwriting and so leaking the descriptor from the previous open. On Windows MinGW, this prevents the final unlink from working, as the file is still open. Trivial fix attached. Committed and back-patched to 9.1. Thanks. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 5:43 AM, Robert Haas robertmh...@gmail.com wrote: I think we would be well-advised not to start inventing our own approximate matching algorithm. Peter's suggestion boils down to a guess that the default cost parameters for Levenshtein suck, and your suggestion boils down to a guess that we can fix the problems with Peter's suggestion by bolting another heuristic on top of it - and possibly running Levenshtein twice with different sets of cost parameters. Ugh. I agree. While I am perfectly comfortable with the fact that we are guessing here, my guesses are based on what I observed to work well with real schemas, and simulated errors that I thought were representative of human error. Obviously it's possible that another scheme will do better sometimes, including for example a scheme that picks a match entirely at random. But on average, I think that what I have here will do better than anything else proposed so far. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
On Wed, Nov 19, 2014 at 12:01 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) What I liked about this syntax was that we could eventually have: RAISE ASSERT WHEN stuff; ...and if assertions are disabled, we can skip evaluating the condition. If you just write an IF .. THEN block you can't do that. Well, if that's what you want, let's just invent ASSERT condition and not tangle RAISE into it. The analogy to EXIT WHEN is a lot cleaner in this form: no order-of-evaluation issues, no questions of whether a sub-clause results in totally changing the meaning of the command. And if your argument is partially based on how much you have to type, doesn't this way dominate all others? That doesn't bother me any. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 12:33 PM, Peter Geoghegan p...@heroku.com wrote: On Wed, Nov 19, 2014 at 5:43 AM, Robert Haas robertmh...@gmail.com wrote: I think we would be well-advised not to start inventing our own approximate matching algorithm. Peter's suggestion boils down to a guess that the default cost parameters for Levenshtein suck, and your suggestion boils down to a guess that we can fix the problems with Peter's suggestion by bolting another heuristic on top of it - and possibly running Levenshtein twice with different sets of cost parameters. Ugh. I agree. While I am perfectly comfortable with the fact that we are guessing here, my guesses are based on what I observed to work well with real schemas, and simulated errors that I thought were representative of human error. Obviously it's possible that another scheme will do better sometimes, including for example a scheme that picks a match entirely at random. But on average, I think that what I have here will do better than anything else proposed so far. If you agree, then I'm not being clear enough. I don't think think that tinkering with the Levenshtein cost factors is a good idea, and I think it's unhelpful to suggest something when the suggestion and the original word differ by more than 50% of the number characters in the shorter word. Suggesting col for oid or x for xmax, as crops up in the regression tests with this patch applied, shows the folly of this: the user didn't mean the other named column; rather, the user was confused about whether a particular system column existed for that table. If we had a large database of examples showing what the user typed and what they intended, we could try different algorithms against it and see which one performs best with fewest false positives. But if we don't have that, we should do things that are like the things that other people have done before. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New Event Trigger: table_rewrite
Alvaro Herrera alvhe...@2ndquadrant.com writes: Almost the whole of that function is conditions to bail out clustering the relation if things have changed since the relation list was collected. It seems wrong to invoke the event trigger in all those cases; it's going to fire spuriously. I think you should move the invocation of the event trigger at the end, just before rebuild_relation is called. Not sure where relative to the predicate lock stuff therein; probably before, so that we avoid doing that dance if the event trigger function decides to jump ship. Actually when you do a CLUSTER or a VACUUM FULL you know that the table is going to be rewritten on disk, because that's about the only purpose of the command. Given the complexity involved here, the new version of the patch (attached) has removed support for those statements. In ATRewriteTables, it seems wrong to call it after make_new_heap. If the event trigger function aborts, we end up with useless work done there; so I think it should be called before that. Also, why do you have the evt_table_rewrite_fired stuff? I think you should fire one event per table, no? Fixed in the attached version of the patch. The second ATRewriteTable call in ATRewriteTables does not actually rewrite the table; it only scans it to verify constraints. So I'm thinking you shouldn't call this event trigger there. Or, if we decide we want this, we probably also need something for the table scans in ALTER DOMAIN too. Fixed in the attached version of the patch. You still have the ANALYZE thing in docs, which now should be removed. Fixed in the attached version of the patch. -- Dimitri Fontaine06 63 07 10 78 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml index 6f71a27..78ec27b 100644 --- a/doc/src/sgml/event-trigger.sgml +++ b/doc/src/sgml/event-trigger.sgml @@ -65,6 +65,16 @@ /para para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal. While other +control statements are available to rewrite a table, +like literalCLUSTER/literal and literalVACUUM/literal, +the literaltable_rewrite/ event is currently only triggered by +the literalALTER TABLE/literal command, which might or might not need +to rewrite the table. + /para + + para Event triggers (like other functions) cannot be executed in an aborted transaction. Thus, if a DDL command fails with an error, any associated literalddl_command_end/ triggers will not be executed. Conversely, @@ -120,6 +130,7 @@ entryliteralddl_command_start/literal/entry entryliteralddl_command_end/literal/entry entryliteralsql_drop/literal/entry +entryliteraltable_rewrite/literal/entry /row /thead tbody @@ -128,510 +139,595 @@ entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER COLLATION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER CONVERSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER DOMAIN/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER EXTENSION/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN DATA WRAPPER/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteral-/literal/entry +entry align=centerliteral-/literal/entry /row row entry align=leftliteralALTER FOREIGN TABLE/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry entry align=centerliteralX/literal/entry +entry align=centerliteral-/literal/entry /row row entry
Re: [HACKERS] New Event Trigger: table_rewrite
On Tue, Nov 18, 2014 at 5:14 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@gmail.com writes: It seems pretty weird, also, that the event trigger will fire after we've taken AccessExclusiveLock when you cluster a particular relation, and before we've taken AccessExclusiveLock when you cluster database-wide. That's more or less an implementation artifact of the current code that we're exposing to the use for, really, no good reason. In the CLUSTER implementation we have only one call site for invoking the Event Trigger, in cluster_rel(). While it's true that in the single relation case, the relation is opened in cluster() then cluster_rel() is called, the opening is done with NoLock in cluster(): rel = heap_open(tableOid, NoLock); My understanding is that the relation locking only happens in cluster_rel() at this line: OldHeap = try_relation_open(tableOid, AccessExclusiveLock); Please help me through the cluster locking strategy here, I feel like I'm missing something obvious, as my conclusion from re-reading the code in lights of your comment is that your comment is not accurate with respect to the current state of the code. Unless I'm missing something, when you cluster a particular relation, cluster() does this: /* Find and lock the table */ rel = heap_openrv(stmt-relation, AccessExclusiveLock); I don't see the rel = heap_open(tableOid, NoLock); line you quoted anywhere. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New Event Trigger: table_rewrite
On Tue, Nov 18, 2014 at 5:34 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Almost the whole of that function is conditions to bail out clustering the relation if things have changed since the relation list was collected. It seems wrong to invoke the event trigger in all those cases; it's going to fire spuriously. I think you should move the invocation of the event trigger at the end, just before rebuild_relation is called. Not sure where relative to the predicate lock stuff therein; probably before, so that we avoid doing that dance if the event trigger function decides to jump ship. I can see two problems with that: 1. What if the conditions aren't true any more after the event trigger has run? Then it's unsafe. 2. If we do it that way, then we'll unnecessarily wait for a lock on the relation even if the event trigger is just going to bail out. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Increasing test coverage of WAL redo functions
On 11/19/2014 05:01 PM, Andres Freund wrote: On 2014-11-19 11:54:47 -0300, Alvaro Herrera wrote: Heikki Linnakangas wrote: Schema | Name | Type | Owner | Size | Description +--+---++-+- public | btree_tall_tbl | table | heikki | 24 kB | public | btree_test_tbl | table | heikki | 392 kB | public | gin_test_tbl | table | heikki | 588 MB | public | gist_point_tbl | table | heikki | 1056 kB | public | spgist_point_tbl | table | heikki | 1056 kB | public | spgist_text_tbl | table | heikki | 1472 kB | (6 rows) I think it's good to have these tests, though Tom was complaining earlier about the size of the regression test database. Would it work to have this in a separate test suite, like the numeric_big stuff? We can have it run optionally, and perhaps set up a few buildfarm members to exercise them on a regular basis. I think the tests except the gin one are resonably sized - I'd much rather run them all the time. We shouldn't make the buildfarm configuration unnecessarily complex. Agreed. Committed, except for the large GIN test. I kept a smaller version of the GIN test, which exercises vacuum and page deletion, but not the internal page split. I also slimmed down the other tests down a little bit. This grew pg_dumpall | wc -c from 5505689 to 6926066 bytes. The size of the regression database grew, according to psql's \l+ command grew from 45 MB to 57 MB. The amount of WAL generated by make installcheck grew from 75 MB to 104 MB. That's a quite big increase in all those measures, but I think it's still acceptable, and I would really like these to run as part of the regular regression suite. - Heikki -- 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] New Event Trigger: table_rewrite
On Wed, Nov 19, 2014 at 1:01 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Nov 18, 2014 at 5:14 PM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Robert Haas robertmh...@gmail.com writes: It seems pretty weird, also, that the event trigger will fire after we've taken AccessExclusiveLock when you cluster a particular relation, and before we've taken AccessExclusiveLock when you cluster database-wide. That's more or less an implementation artifact of the current code that we're exposing to the use for, really, no good reason. In the CLUSTER implementation we have only one call site for invoking the Event Trigger, in cluster_rel(). While it's true that in the single relation case, the relation is opened in cluster() then cluster_rel() is called, the opening is done with NoLock in cluster(): rel = heap_open(tableOid, NoLock); My understanding is that the relation locking only happens in cluster_rel() at this line: OldHeap = try_relation_open(tableOid, AccessExclusiveLock); Please help me through the cluster locking strategy here, I feel like I'm missing something obvious, as my conclusion from re-reading the code in lights of your comment is that your comment is not accurate with respect to the current state of the code. Unless I'm missing something, when you cluster a particular relation, cluster() does this: /* Find and lock the table */ rel = heap_openrv(stmt-relation, AccessExclusiveLock); I don't see the rel = heap_open(tableOid, NoLock); line you quoted anywhere. ...which is because I have the 9.1 branch checked out. Genius. But what I said originally is still true, because the current code looks like this: /* Find, lock, and check permissions on the table */ tableOid = RangeVarGetRelidExtended(stmt-relation, AccessExclusiveLock, false, false, RangeVarCallbackOwnsTable, NULL); rel = heap_open(tableOid, NoLock); It's true that the heap_open() is not acquiring any lock. But the RangeVarGetRelidExtended() call right before it is. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup vs. Windows and tablespaces
On Tue, Nov 18, 2014 at 9:19 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Right, but they provide same functionality as symlinks and now we are even planing to provide this feature for both linux and windows as both Tom and Robert seems to feel, it's better that way. Anyhow, I think naming any entity generally differs based on individual's perspective, so we can go with the name which appeals to more people. In case, nobody else has any preference, I will change it to what both of us can agree upon (either 'tablespace catalog', 'tablespace_info' ...). Well, I have made my argument. Since you're the submitter, feel free to select what you think is the best name. For what it's worth, I, too, dislike having symlink in the name. Maybe tablespace_map? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 9:52 AM, Robert Haas robertmh...@gmail.com wrote: If you agree, then I'm not being clear enough. I don't think think that tinkering with the Levenshtein cost factors is a good idea, and I think it's unhelpful to suggest something when the suggestion and the original word differ by more than 50% of the number characters in the shorter word. I agree - except for very short strings, where there is insufficient information to go on. I was talking about the difficulty of bolting something on top of Levenshtein distance that looked at the first character. That would not need two costings, but would require an encoding aware matching of the first code point. Suggesting col for oid or x for xmax, as crops up in the regression tests with this patch applied, shows the folly of this: the user didn't mean the other named column; rather, the user was confused about whether a particular system column existed for that table. Those are all very terse strings. What you're overlooking is what is broken by using straight Levenshtein distance, which includes things in the regression test that are reasonable and helpful. As I mentioned before, requiring a greater than 50% of total string size distance breaks this, just within the regression tests: ERROR: column f1 does not exist LINE 1: select f1 from c1; ^ - HINT: Perhaps you meant to reference the column c1.f2. And: ERROR: column atts.relid does not exist LINE 1: select atts.relid::regclass, s.* from pg_stats s join ^ - HINT: Perhaps you meant to reference the column atts.indexrelid. Those are really useful suggestions! And, they're much more representative of real user error. The downside of weighing deletion less than substitution and insertion is much smaller than the upside. It's worth it. The downside is only that the user gets to see the best suggestion that isn't all that good in an absolute sense (which we have a much harder time concluding using simple tests for short misspellings). If we had a large database of examples showing what the user typed and what they intended, we could try different algorithms against it and see which one performs best with fewest false positives. But if we don't have that, we should do things that are like the things that other people have done before. That seems totally impractical. No one has that kind of data that I'm aware of. How about git as a kind of precedent? It is not at all conservative about showing *some* suggestion: $ git aa git: 'aa' is not a git command. See 'git --help'. Did you mean this? am $ git d git: 'd' is not a git command. See 'git --help'. Did you mean one of these? diff add $ git ddd git: 'ddd' is not a git command. See 'git --help'. Did you mean this? add And why wouldn't git be? As far as its concerned, you can only have meant one of those small number of things. Similarly, with the patch, the number of things we can pick from is fairly limited at this stage, since we are actually fairly far along with parse analysis. Now, this won't give a suggestion: $ git git: '' is not a git command. See 'git --help'. So it looks like git similarly weighs deletion less than insertion/substitution. Are its suggestions any less ridiculous than your examples of questionable hints from the modified regression test expected output? This is just a guidance mechanism, and at worst we'll show the best match (on the mechanisms own terms, which isn't too bad). -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Increasing test coverage of WAL redo functions
On 2014-11-19 19:59:33 +0200, Heikki Linnakangas wrote: On 11/19/2014 05:01 PM, Andres Freund wrote: On 2014-11-19 11:54:47 -0300, Alvaro Herrera wrote: Heikki Linnakangas wrote: Schema | Name | Type | Owner | Size | Description +--+---++-+- public | btree_tall_tbl | table | heikki | 24 kB | public | btree_test_tbl | table | heikki | 392 kB | public | gin_test_tbl | table | heikki | 588 MB | public | gist_point_tbl | table | heikki | 1056 kB | public | spgist_point_tbl | table | heikki | 1056 kB | public | spgist_text_tbl | table | heikki | 1472 kB | (6 rows) I think it's good to have these tests, though Tom was complaining earlier about the size of the regression test database. Would it work to have this in a separate test suite, like the numeric_big stuff? We can have it run optionally, and perhaps set up a few buildfarm members to exercise them on a regular basis. I think the tests except the gin one are resonably sized - I'd much rather run them all the time. We shouldn't make the buildfarm configuration unnecessarily complex. Agreed. Committed, except for the large GIN test. I kept a smaller version of the GIN test, which exercises vacuum and page deletion, but not the internal page split. I also slimmed down the other tests down a little bit. This grew pg_dumpall | wc -c from 5505689 to 6926066 bytes. The size of the regression database grew, according to psql's \l+ command grew from 45 MB to 57 MB. The amount of WAL generated by make installcheck grew from 75 MB to 104 MB. Why not just drop some of these slightly larger tables after the test? Then the maximum size of the database/the size of the dump doesn't increase as much? I don't think there's these are that interesting to look into after the test has finished. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 10:22 AM, Peter Geoghegan p...@heroku.com wrote: Those are all very terse strings. What you're overlooking is what is broken by using straight Levenshtein distance, which includes things in the regression test that are reasonable and helpful. As I mentioned before, requiring a greater than 50% of total string size distance breaks this, just within the regression tests: Maybe you'd prefer if there was a more gradual ramp-up to requiring a distance of no greater than 50% of the string size (normalized to take account of my non-default costings). Right now it's a step function of the number of characters in the string - there is no absolute quality requirement for strings of 6 or fewer requirements. Otherwise, there is the 50% distance absolute quality test (the test that you want to be applied generally). I think that would be better, without being much more complicated. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 10:33 AM, Peter Geoghegan p...@heroku.com wrote: there is no absolute quality requirement for strings of 6 or fewer requirements. I meant 6 or fewer *characters*, obviously. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On 19 November 2014 16:11, Petr Jelinek p...@2ndquadrant.com wrote: We need to be able to tell the difference between a crashed Startup process and this usage. As long as we can tell, I don't mind how we do it. Suggestions please. Different exit code? Try this one. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services shutdown_at_recovery_target-v5.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] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 10:33 AM, Peter Geoghegan p...@heroku.com wrote:
Maybe you'd prefer if there was a more gradual ramp-up to requiring a
distance of no greater than 50% of the string size (normalized to take
account of my non-default costings)
I made this modification:
diff --git a/src/backend/parser/parse_relation.c
b/src/backend/parser/parse_relation.c
index 40c69d7..cca075f 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -929,7 +929,8 @@ searchRangeTableForCol(ParseState *pstate, const
char *alias, char *colname,
* seen when 6 deletions are required against actual attribute
name, or 3
* insertions/substitutions.
*/
- if (state-distance 6 state-distance strlen(colname) / 2)
+ if ((state-distance 3 state-distance strlen(colname)) ||
+ (state-distance 6 state-distance strlen(colname) / 2))
{
state-rsecond = state-rfirst = NULL;
state-second = state-first = InvalidAttrNumber;
When I run the regression tests now, then all the cases that you found
objectionable in the regression tests' previous expected output
disappear, while all the cases I think are useful that were previously
removed by applying a broad 50% standard remain. While I'm not 100%
sure that this exact formulation is the best one, I think that we can
reach a compromise on this point, that allows the costing to remain
the same without offering particularly bad suggestions for short
strings.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 1:22 PM, Peter Geoghegan p...@heroku.com wrote: On Wed, Nov 19, 2014 at 9:52 AM, Robert Haas robertmh...@gmail.com wrote: If you agree, then I'm not being clear enough. I don't think think that tinkering with the Levenshtein cost factors is a good idea, and I think it's unhelpful to suggest something when the suggestion and the original word differ by more than 50% of the number characters in the shorter word. I agree - except for very short strings, where there is insufficient information to go on. That's precisely the time I think it's *most* important. In a very long string, the threshold should be LESS than 50%. My original proposal was no more than 3 characters of difference, but in any event not more than half the length of the shorter string. Suggesting col for oid or x for xmax, as crops up in the regression tests with this patch applied, shows the folly of this: the user didn't mean the other named column; rather, the user was confused about whether a particular system column existed for that table. Those are all very terse strings. What you're overlooking is what is broken by using straight Levenshtein distance, which includes things in the regression test that are reasonable and helpful. As I mentioned before, requiring a greater than 50% of total string size distance breaks this, just within the regression tests: ERROR: column f1 does not exist LINE 1: select f1 from c1; ^ - HINT: Perhaps you meant to reference the column c1.f2. That's exactly 50%, not more than 50%. (I'm also on the fence about whether the hint is actually helpful in that case, but the rule I proposed wouldn't prohibit it.) And: ERROR: column atts.relid does not exist LINE 1: select atts.relid::regclass, s.* from pg_stats s join ^ - HINT: Perhaps you meant to reference the column atts.indexrelid. Those are really useful suggestions! And, they're much more representative of real user error. That one's right at 50% too, but it's certainly more than 3 characters of difference. I think it's going to be pretty hard to emit a suggestion in that case but not in a whole lot of cases that don't make any sense. How about git as a kind of precedent? It is not at all conservative about showing *some* suggestion: $ git aa git: 'aa' is not a git command. See 'git --help'. Did you mean this? am $ git d git: 'd' is not a git command. See 'git --help'. Did you mean one of these? diff add $ git ddd git: 'ddd' is not a git command. See 'git --help'. Did you mean this? add And why wouldn't git be? As far as its concerned, you can only have meant one of those small number of things. Similarly, with the patch, the number of things we can pick from is fairly limited at this stage, since we are actually fairly far along with parse analysis. I went and found the actual code git uses for this. It's here: https://github.com/git/git/blob/d29e9c89dbbf0876145dc88615b99308cab5f187/help.c And the underlying Levenshtein implementation is here: https://github.com/git/git/blob/398dd4bd039680ba98497fbedffa415a43583c16/levenshtein.c Apparently what they're doing is charging 0 for a transposition (which we don't have as a separate concept), 2 for a substitution, 1 for an insertion, and 3 for a deletion, with the constraint that anything with a total distance of more than 6 isn't considered. And that does overall seem to give pretty good suggestions. However, an interesting point about the git algorithm is that it's not hard to make it do stupid things on short strings: [rhaas pgsql]$ git xy git: 'xy' is not a git command. See 'git --help'. Did you mean one of these? am gc mv rm Maybe they should adopt my idea of a lower cutoff for short strings. :-) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 11:13 AM, Robert Haas robertmh...@gmail.com wrote: Apparently what they're doing is charging 0 for a transposition (which we don't have as a separate concept), 2 for a substitution, 1 for an insertion, and 3 for a deletion, with the constraint that anything with a total distance of more than 6 isn't considered. And that does overall seem to give pretty good suggestions. The git people know that no git command is longer than 4 or 5 characters. That doesn't apply to us. I certainly would not like to have an absolute distance test of n characters. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 11:13 AM, Robert Haas robertmh...@gmail.com wrote: That's precisely the time I think it's *most* important. In a very long string, the threshold should be LESS than 50%. My original proposal was no more than 3 characters of difference, but in any event not more than half the length of the shorter string. We can only hint based on the information given by the user. If they give a lot of badly matching information, we have something to go on. That one's right at 50% too, but it's certainly more than 3 characters of difference. I think it's going to be pretty hard to emit a suggestion in that case but not in a whole lot of cases that don't make any sense. I don't think that's the case. Other RTEs are penalized for having non-matching aliases here. In general, I think the cost of a bad suggestion is much lower than the benefit of a good one. You seem to be suggesting that they're equal. Or that they're equally likely in an organic situation. In my estimation, this is not the case at all. I'm curious about your thoughts on the compromise of a ramped up distance threshold to apply a test for the absolute quality of a match. I think that the fact that git gives bad suggestions with terse strings tells us a lot, though. Note that unlike git, with terse strings we may well have a good deal more equidistant matches, and as soon as the number of would-be matches exceeds 2, we actually give no matches at all. So that's an additional protection against poor matches with terse strings. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
Robert Haas wrote: And the underlying Levenshtein implementation is here: https://github.com/git/git/blob/398dd4bd039680ba98497fbedffa415a43583c16/levenshtein.c Apparently what they're doing is charging 0 for a transposition (which we don't have as a separate concept), 2 for a substitution, 1 for an insertion, and 3 for a deletion, with the constraint that anything with a total distance of more than 6 isn't considered. 0 for a transposition, wow. I suggested adding transpositions but there was no support for that idea. I suggested it because I thikn it's the most common form of typo, and charging 2 for a deletion plus 1 for an insertion makes a single transposition mistaek count as 3, which seems wrong -- particularly seeing the git precedent. -- Á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] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 11:34 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 0 for a transposition, wow. Again, they're optimizing for short strings (git commands) only. There just isn't that many transposition errors possible with a 4 character string. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
Peter Geoghegan wrote: On Wed, Nov 19, 2014 at 11:34 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 0 for a transposition, wow. Again, they're optimizing for short strings (git commands) only. There just isn't that many transposition errors possible with a 4 character string. If there's logic in your statement, I can't see it. -- Á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] Doing better at HINTing an appropriate column within errorMissingColumn()
On Wed, Nov 19, 2014 at 11:54 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Peter Geoghegan wrote: On Wed, Nov 19, 2014 at 11:34 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 0 for a transposition, wow. Again, they're optimizing for short strings (git commands) only. There just isn't that many transposition errors possible with a 4 character string. If there's logic in your statement, I can't see it. The point is that transposition errors should not have no cost. If git did not have an absolute quality test of a distance of 6, which they can only have because all git commands are terse, then you could construct a counter example. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
Peter Geoghegan wrote: On Wed, Nov 19, 2014 at 11:54 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Peter Geoghegan wrote: On Wed, Nov 19, 2014 at 11:34 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: 0 for a transposition, wow. Again, they're optimizing for short strings (git commands) only. There just isn't that many transposition errors possible with a 4 character string. If there's logic in your statement, I can't see it. The point is that transposition errors should not have no cost. If git did not have an absolute quality test of a distance of 6, which they can only have because all git commands are terse, then you could construct a counter example. Okay. My point is just that whatever the string length, I think we'd do well to regard transpositions as cheap in terms of error cost. -- Á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] Add shutdown_at_recovery_target option to recovery.conf
On 19 November 2014 16:41, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Nov 19, 2014 at 10:49 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Nov 19, 2014 at 8:13 AM, Simon Riggs si...@2ndquadrant.com wrote: If we ask for PAUSE and we're not in HotStandby it just ignores it, which means it changes into PROMOTE. My feeling is that we should change that into a SHUTDOWN, not a PROMOTE. To me, that seems like a definite improvement. But changing the default will force us to set action_at_recovery_target to 'promote' when we want to just recover the database to the specified point. If you explicitly request pause and then can't pause, ISTM the action we actually perform shouldn't be the exact opposite of what was requested. So if the user explicitly requests pause and we aren't in HS then they should get Shutdown, which is closest action. If the user doesn't request anything at all then we can default to Promote, just like we do now. -- 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] Additional role attributes superuser review
All,
If we're going to change the catalog representation for the existing
capabilities, I'd recommend that the first patch change the catalog
representation and add the new syntax without adding any more
capabilities; and then the second and subsequent patches can add
additional capabilities.
Attached is an initial patch for review and discussion that takes a swing
at changing the catalog representation.
This patch includes:
* int64 (C) to int8 (SQL) mapping for genbki.
* replace all role attributes columns in pg_authid with single int64 column
named rolattr.
* update CreateRole and AlterRole to use rolattr.
* update all has_*_privilege functions to check rolattr.
* builtin SQL function 'has_role_attribute' that takes a role oid and text
name of the attribute as input and returns a boolean.
Items not currently addressed:
* New syntax - more discussion needs to occur around these potential
changes. Specifically, how would CREATE USER/ROLE be affected? I suppose
it is OK to keep it as WITH attribute_or_capability, though if ALTER ROLE
is modified to have ADD | DROP CAPABILITY for consistency would WITH
CAPABILITY value,..., make more sense for CREATE? At any rate, I think
there is obviously much more discussion that needs to be had.
* Documentation - want to gain feedback on implementation prior to making
changes.
* Update regression tests, rules test for system_views - want to gain
feedback on approach to handling pg_roles, etc. before updating.
Also, what would be the community preference on tracking these
attached/proposed changes? Should I create a separate entry in the next CF
for this item (seems prudent) or would it be preferred to keep it attached
to the current 'new attributes' CF entry?
Thanks,
Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
new file mode 100644
index eb91c53..523b379
*** a/src/backend/catalog/Catalog.pm
--- b/src/backend/catalog/Catalog.pm
*** sub Catalogs
*** 33,38
--- 33,39
my %RENAME_ATTTYPE = (
'int16' = 'int2',
'int32' = 'int4',
+ 'int64' = 'int8',
'Oid' = 'oid',
'NameData' = 'name',
'TransactionId' = 'xid');
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
new file mode 100644
index d30612c..93eb2e6
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** aclcheck_error_type(AclResult aclerr, Oi
*** 3423,3448
}
- /* Check if given user has rolcatupdate privilege according to pg_authid */
- static bool
- has_rolcatupdate(Oid roleid)
- {
- bool rolcatupdate;
- HeapTuple tuple;
-
- tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
- if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg(role with OID %u does not exist, roleid)));
-
- rolcatupdate = ((Form_pg_authid) GETSTRUCT(tuple))-rolcatupdate;
-
- ReleaseSysCache(tuple);
-
- return rolcatupdate;
- }
-
/*
* Relay for the various pg_*_mask routines depending on object kind
*/
--- 3423,3428
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3630,3636
if ((mask (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
IsSystemClass(table_oid, classForm)
classForm-relkind != RELKIND_VIEW
! !has_rolcatupdate(roleid)
!allowSystemTableMods)
{
#ifdef ACLDEBUG
--- 3610,3616
if ((mask (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE))
IsSystemClass(table_oid, classForm)
classForm-relkind != RELKIND_VIEW
! !role_has_attribute(roleid, ROLE_ATTR_CATUPDATE)
!allowSystemTableMods)
{
#ifdef ACLDEBUG
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5051,5056
--- 5031,5058
}
/*
+ * Check whether the specified role has a specific role attribute.
+ */
+ bool
+ role_has_attribute(Oid roleid, RoleAttr attribute)
+ {
+ RoleAttr attributes;
+ HeapTuple tuple;
+
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg(role with OID %u does not exist, roleid)));
+
+ attributes = ((Form_pg_authid) GETSTRUCT(tuple))-rolattr;
+ ReleaseSysCache(tuple);
+
+ return ((attributes attribute) 0);
+ }
+
+ /*
* Check whether specified role has CREATEROLE privilege (or is a superuser)
*
* Note: roles do not have owners per se; instead we use this test in
*** pg_extension_ownercheck(Oid ext_oid, Oid
*** 5064,5102
bool
has_createrole_privilege(Oid roleid)
{
- bool result = false;
- HeapTuple utup;
-
/* Superusers bypass all permission checking. */
if (superuser_arg(roleid))
return true;
! utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
! if
Re: [HACKERS] pg_multixact not getting truncated
On 11/12/2014 06:57 PM, Alvaro Herrera wrote: How did template0 even get a MultiXact? That sounds like they're really abusing the template databases. :( (Do keep in mind that MXID 1 is a special value.) No, it's normal -- template0 does not have a multixact in any tuple's xmax, but datminxid is set to the value that is current when it is frozen. So, to follow up on this: it seems to me that we shouldn't be requiring freezing for databases where allowconn=false. This seems like a TODO to me, even possibly a backpatchable bug fix. -- 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] pg_multixact not getting truncated
Josh Berkus wrote: On 11/12/2014 06:57 PM, Alvaro Herrera wrote: How did template0 even get a MultiXact? That sounds like they're really abusing the template databases. :( (Do keep in mind that MXID 1 is a special value.) No, it's normal -- template0 does not have a multixact in any tuple's xmax, but datminxid is set to the value that is current when it is frozen. So, to follow up on this: it seems to me that we shouldn't be requiring freezing for databases where allowconn=false. This seems like a TODO to me, even possibly a backpatchable bug fix. Why do we need this for pg_multixact but not for pg_clog? -- Á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] pg_multixact not getting truncated
On 11/19/2014 01:03 PM, Alvaro Herrera wrote: Josh Berkus wrote: On 11/12/2014 06:57 PM, Alvaro Herrera wrote: How did template0 even get a MultiXact? That sounds like they're really abusing the template databases. :( (Do keep in mind that MXID 1 is a special value.) No, it's normal -- template0 does not have a multixact in any tuple's xmax, but datminxid is set to the value that is current when it is frozen. So, to follow up on this: it seems to me that we shouldn't be requiring freezing for databases where allowconn=false. This seems like a TODO to me, even possibly a backpatchable bug fix. Why do we need this for pg_multixact but not for pg_clog? I think we want it for both. -- 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] Increasing test coverage of WAL redo functions
Andres Freund and...@2ndquadrant.com writes: On 2014-11-19 19:59:33 +0200, Heikki Linnakangas wrote: This grew pg_dumpall | wc -c from 5505689 to 6926066 bytes. The size of the regression database grew, according to psql's \l+ command grew from 45 MB to 57 MB. The amount of WAL generated by make installcheck grew from 75 MB to 104 MB. Why not just drop some of these slightly larger tables after the test? Then the maximum size of the database/the size of the dump doesn't increase as much? I don't think there's these are that interesting to look into after the test has finished. While the post-run database size is interesting, I think the peak in-run size is probably the more critical number, since that affects whether buildfarm critters with limited disk space are going to run out. Still, I agree that we could drop any such tables that don't add new cases for pg_dump to think about. Another thought, if we drop large tables at completion of their tests, is that these particular tests ought not be run in parallel with each other. That's just uselessly concentrating the peak space demand. 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] amcheck prototype
Attached is a revision of what I previously called btreecheck, which is now renamed to amcheck. This is not 9.5 material - I already have 3 bigger patches in the queue, 2 of which are large and complex and have major controversies, and one of which has details that need to be worked out, which is currently consuming a lot of reviewer time. There seems to be little point in trying to get amcheck into shape for 9.5. The goals for this as a real patch need to be worked out in greater detail. At some point we'll need to have a discussion around both stress-testing (as a way of finding bugs) and allowing users to verify indexes on production systems when corruption is suspected. Since, as far as I know, no one else has so much as applied and compiled my ON CONFLICT UPDATE patch, it would be pretty senseless of me to add another patch to the queue. Reviewers are clearly more overburdened than ever. Anyway, this revision adds the ability to check invariants across pages (that a page's right-link comports with the target page's last item, since when targeting a particular page there is no locally available next item to check the last item against, other than the page highkey). This even occurs for the index check user callable SQL function that only acquire an AccessShareLock (bt_index_verify() and bt_page_verify()). As before, it also exhaustively tests certain other related invariants previously described [1], without really considering their plausibility as either bugs in the B-Tree code, or things likely to be violated in the event of organic data corruption. In other words, I could probably stand to be considerably more selective in what I'm testing, but in order to do that I'd need to make up my mind about my exact goals for this tool. amcheck is something that I thought might find bugs in approach #1 to value locking [2] (for the ON CONFLICT UPDATE patch). However, extensive stress testing while constantly using the tool has not revealed any bugs. That doesn't mean that they're not there, of course, and it doesn't really alter our understanding of approach #1, but it's worth mentioning. Anyway, this is presented here in the hope that it will be useful for testing other patches, and perhaps even in testing corruption on production systems (with appropriate precautions taken - this is still a prototype patch - but it's also still the only thing of its kind). I post this with the expectation that it won't make it into contrib until PostgreSQL 9.6, or whatever we end up calling it. It might be that someone has some feedback that allows me to build a better temporary prototype (certainly, some testing tools were maintained out of git for a while in the past, such as the precursor to isolation tester), but I don't expect even that. If no one wants to do anything with this patch in the foreseeable future (probably the current cycle), there may still be some value in dumping my progress here. As I said, I tend to think that its biggest problem right now is that it's just too scatter gun, but that's probably appropriate for an early iteration. In general, I think we could prevent a lot of bugs by performing targeted stress-testing with custom tools. Ideally, this tool would go on to provide a way of doing for several different areas of the code. [1] http://www.postgresql.org/message-id/cam3swzrtv+xmrwlwq6c-x7czvwavfdwfi4st1zz4ddgfh4y...@mail.gmail.com [2] https://wiki.postgresql.org/wiki/Value_locking#.231._Heavyweight_page_locking_.28Peter_Geoghegan.29 -- Peter Geoghegan diff --git a/contrib/amcheck/Makefile b/contrib/amcheck/Makefile new file mode 100644 index ...07b18d8 *** a/contrib/amcheck/Makefile --- b/contrib/amcheck/Makefile *** *** 0 --- 1,18 + # contrib/amcheck/Makefile + + MODULE_big = amcheck + OBJS = amcheck.o + + EXTENSION = amcheck + DATA = amcheck--1.0.sql + + ifdef USE_PGXS + PG_CONFIG = pg_config + PGXS := $(shell $(PG_CONFIG) --pgxs) + include $(PGXS) + else + subdir = contrib/amcheck + top_builddir = ../.. + include $(top_builddir)/src/Makefile.global + include $(top_srcdir)/contrib/contrib-global.mk + endif diff --git a/contrib/amcheck/amcheck--1.0.sql b/contrib/amcheck/amcheck--1.0.sql new file mode 100644 index ...e25fad1 *** a/contrib/amcheck/amcheck--1.0.sql --- b/contrib/amcheck/amcheck--1.0.sql *** *** 0 --- 1,44 + /* contrib/amcheck/amcheck--1.0.sql */ + + -- complain if script is sourced in psql, rather than via CREATE EXTENSION + \echo Use CREATE EXTENSION amcheck to load this file. \quit + + -- + -- bt_page_verify() + -- + CREATE FUNCTION bt_page_verify(relname text, blkno int4) + RETURNS VOID + AS 'MODULE_PATHNAME', 'bt_page_verify' + LANGUAGE C STRICT; + + -- + -- bt_parent_page_verify() + -- + CREATE FUNCTION bt_parent_page_verify(relname text, blkno int4) + RETURNS VOID + AS 'MODULE_PATHNAME', 'bt_parent_page_verify' + LANGUAGE C STRICT; + + -- + -- bt_index_verify() + -- + CREATE FUNCTION bt_index_verify(relname text)
Re: [HACKERS] proposal: plpgsql - Assert statement
2014-11-19 17:13 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Andrew Dunstan and...@dunslane.net writes: On 11/19/2014 06:35 AM, Simon Riggs wrote: I seem to share the same opinion with Andrew: its not going to hurt to include this, but its not gonna cause dancing in the streets either. I would characterize that as 2 very neutral and unimpressed people, plus 3 in favour. Which seems enough to commit. That's about right, although I would put it a bit stronger than that. But if we're the only people unimpressed I'm not going to object further. FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) last query looks clean for me. First we evaluate WHEN expression, next (if previous expression is true) we evaluate a expressions inside RAISE statement. Regards Pavel regards, tom lane
Re: [HACKERS] proposal: plpgsql - Assert statement
2014-11-19 17:43 GMT+01:00 Robert Haas robertmh...@gmail.com: On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: Andrew Dunstan and...@dunslane.net writes: On 11/19/2014 06:35 AM, Simon Riggs wrote: I seem to share the same opinion with Andrew: its not going to hurt to include this, but its not gonna cause dancing in the streets either. I would characterize that as 2 very neutral and unimpressed people, plus 3 in favour. Which seems enough to commit. That's about right, although I would put it a bit stronger than that. But if we're the only people unimpressed I'm not going to object further. FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) What I liked about this syntax was that we could eventually have: RAISE ASSERT WHEN stuff; ...and if assertions are disabled, we can skip evaluating the condition. If you just write an IF .. THEN block you can't do that. I share this idea. It is possible next step Pavel -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] proposal: plpgsql - Assert statement
2014-11-19 18:01 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) What I liked about this syntax was that we could eventually have: RAISE ASSERT WHEN stuff; ...and if assertions are disabled, we can skip evaluating the condition. If you just write an IF .. THEN block you can't do that. Well, if that's what you want, let's just invent ASSERT condition there was this proposal .. ASSERT statement .. related discuss was finished, because it needs a reserved keyword ASSERT. and not tangle RAISE into it. The analogy to EXIT WHEN is a lot cleaner in this form: no order-of-evaluation issues, no questions of whether a sub-clause results in totally changing the meaning of the command. And if your argument is partially based on how much you have to type, doesn't this way dominate all others? regards, tom lane
Re: [HACKERS] amcheck prototype
On Wed, Nov 19, 2014 at 2:09 PM, Peter Geoghegan p...@heroku.com wrote:
Attached is a revision of what I previously called btreecheck, which
is now renamed to amcheck.
Whoops. I left in modifications to pg_config_manual.h to build with
Valgrind support. Here is a version without that.
--
Peter Geoghegan
diff --git a/contrib/amcheck/Makefile b/contrib/amcheck/Makefile
new file mode 100644
index ...07b18d8
*** a/contrib/amcheck/Makefile
--- b/contrib/amcheck/Makefile
***
*** 0
--- 1,18
+ # contrib/amcheck/Makefile
+
+ MODULE_big = amcheck
+ OBJS = amcheck.o
+
+ EXTENSION = amcheck
+ DATA = amcheck--1.0.sql
+
+ ifdef USE_PGXS
+ PG_CONFIG = pg_config
+ PGXS := $(shell $(PG_CONFIG) --pgxs)
+ include $(PGXS)
+ else
+ subdir = contrib/amcheck
+ top_builddir = ../..
+ include $(top_builddir)/src/Makefile.global
+ include $(top_srcdir)/contrib/contrib-global.mk
+ endif
diff --git a/contrib/amcheck/amcheck--1.0.sql b/contrib/amcheck/amcheck--1.0.sql
new file mode 100644
index ...e25fad1
*** a/contrib/amcheck/amcheck--1.0.sql
--- b/contrib/amcheck/amcheck--1.0.sql
***
*** 0
--- 1,44
+ /* contrib/amcheck/amcheck--1.0.sql */
+
+ -- complain if script is sourced in psql, rather than via CREATE EXTENSION
+ \echo Use CREATE EXTENSION amcheck to load this file. \quit
+
+ --
+ -- bt_page_verify()
+ --
+ CREATE FUNCTION bt_page_verify(relname text, blkno int4)
+ RETURNS VOID
+ AS 'MODULE_PATHNAME', 'bt_page_verify'
+ LANGUAGE C STRICT;
+
+ --
+ -- bt_parent_page_verify()
+ --
+ CREATE FUNCTION bt_parent_page_verify(relname text, blkno int4)
+ RETURNS VOID
+ AS 'MODULE_PATHNAME', 'bt_parent_page_verify'
+ LANGUAGE C STRICT;
+
+ --
+ -- bt_index_verify()
+ --
+ CREATE FUNCTION bt_index_verify(relname text)
+ RETURNS VOID
+ AS 'MODULE_PATHNAME', 'bt_index_verify'
+ LANGUAGE C STRICT;
+
+ --
+ -- bt_parent_index_verify()
+ --
+ CREATE FUNCTION bt_parent_index_verify(relname text)
+ RETURNS VOID
+ AS 'MODULE_PATHNAME', 'bt_parent_index_verify'
+ LANGUAGE C STRICT;
+
+ --
+ -- bt_leftright_verify()
+ --
+ CREATE FUNCTION bt_leftright_verify(relname text)
+ RETURNS VOID
+ AS 'MODULE_PATHNAME', 'bt_leftright_verify'
+ LANGUAGE C STRICT;
diff --git a/contrib/amcheck/amcheck.c b/contrib/amcheck/amcheck.c
new file mode 100644
index ...8776fbe
*** a/contrib/amcheck/amcheck.c
--- b/contrib/amcheck/amcheck.c
***
*** 0
--- 1,1009
+ /*-
+ *
+ * amcheck.c
+ * Verifies the integrity of access methods based on invariants.
+ *
+ * Currently, only checks for the nbtree AM are provided. Provides
+ * SQL-callable functions for verifying that various invariants in the
+ * structure of nbtree indexes are respected. This includes for example the
+ * invariant that each page with a high key has data items bound by the high
+ * key. Some functions also check invariant conditions that must hold across
+ * multiple pages, such as the requirement each left link within a page (if
+ * any) should point to a page that has as its right link a pointer to the
+ * page.
+ *
+ *
+ * Copyright (c) 2014, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/amcheck/amcheck.c
+ *
+ *-
+ */
+ #include postgres.h
+
+ #include access/nbtree.h
+ #include catalog/index.h
+ #include catalog/namespace.h
+ #include commands/tablecmds.h
+ #include funcapi.h
+ #include miscadmin.h
+ #include storage/lmgr.h
+ #include utils/builtins.h
+ #include utils/lsyscache.h
+ #include utils/rel.h
+
+
+ PG_MODULE_MAGIC;
+
+ #define CHECK_SUPERUSER() { \
+ if (!superuser()) \
+ ereport(ERROR, \
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), \
+ (errmsg(must be superuser to use verification functions; }
+ #define CHECK_RELATION_BTREE(r) { \
+ if ((r)-rd_rel-relkind != RELKIND_INDEX || (r)-rd_rel-relam != BTREE_AM_OID) \
+ elog(ERROR, relation \%s\ is not a btree index, \
+ RelationGetRelationName(r)); }
+ /* reject attempts to read non-local temporary relations */
+ #define CHECK_RELATION_IS_NOT_OTHER_TEMP(rel) { \
+ if (RELATION_IS_OTHER_TEMP(rel)) \
+ ereport(ERROR, \
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), \
+ errmsg(cannot access temporary tables of other sessions))); }
+ /* note: BlockNumber is unsigned, hence can't be negative */
+ #define CHECK_RELATION_BLOCK_RANGE(rel, blkno) { \
+ if (blkno == 0) \
+ elog(ERROR, block 0 is a meta page); \
+ if (RelationGetNumberOfBlocks(rel) = (BlockNumber) (blkno) ) \
+ elog(ERROR, block number out of range); }
+
+ /*
+ * Callers to verification functions can reasonably expect to never receive a
+ * warning. Therefore, when using amcheck functions for stress testing it
+ * may be useful to temporally change CHCKELEVEL to PANIC, to immediately halt
+ * the server in the event of detecting an invariant condition
Re: [HACKERS] proposal: plpgsql - Assert statement
On 2014-11-19 23:18, Pavel Stehule wrote: 2014-11-19 18:01 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) What I liked about this syntax was that we could eventually have: RAISE ASSERT WHEN stuff; ...and if assertions are disabled, we can skip evaluating the condition. If you just write an IF .. THEN block you can't do that. Well, if that's what you want, let's just invent ASSERT condition there was this proposal .. ASSERT statement .. related discuss was finished, because it needs a reserved keyword ASSERT. Finished? Really? This was Heikki's take on the discussion that took place a good while ago: http://www.postgresql.org/message-id/5405ff73.1010...@vmware.com. And in the same thread you also said you like it: http://www.postgresql.org/message-id/CAFj8pRAC-ZWDrbU-uj=xqowqtbaqr5oxsm1xyoyhzmyeuvz...@mail.gmail.co. But perhaps you've changed your mind since then (which is fine.) Or maybe that was only in the case where we'd have a special mode where you could opt-in if you're willing to accept the backwards compatibility issue? I also went back to the original thread, and I think Heikki's summary dismissed e.g. Robert's criticism quite lightly: http://www.postgresql.org/message-id/ca+tgmobwossrncv_ijk3xhsytxb7dv0awgvwkmeurntovez...@mail.gmail.com .marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add shutdown_at_recovery_target option to recovery.conf
On 19/11/14 19:51, Simon Riggs wrote: On 19 November 2014 16:11, Petr Jelinek p...@2ndquadrant.com wrote: We need to be able to tell the difference between a crashed Startup process and this usage. As long as we can tell, I don't mind how we do it. Suggestions please. Different exit code? Try this one. Ok this seems ok, I did couple of fixes - used exit code 3 as 2 is used in some places - given the if (pid == StartupPID) it would probably never conflict in practice, but better be safe than sorry in this case IMHO. And you forgot to actually set the postmaster into one of the Shutdown states so I added that. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/doc/src/sgml/recovery-config.sgml b/doc/src/sgml/recovery-config.sgml index 0f1ff34..a145a3f 100644 --- a/doc/src/sgml/recovery-config.sgml +++ b/doc/src/sgml/recovery-config.sgml @@ -289,12 +289,39 @@ restore_command = 'copy C:\\server\\archivedir\\%f %p' # Windows /term listitem para -Specifies whether recovery should pause when the recovery target -is reached. The default is true. -This is intended to allow queries to be executed against the -database to check if this recovery target is the most desirable -point for recovery. The paused state can be resumed by using -functionpg_xlog_replay_resume()/ (See +Alias for action_at_recovery_target, literaltrue/ is same as +action_at_recovery_target = literalpause/ and literalfalse/ +is same as action_at_recovery_target = literalpromote/. + /para + para +This setting has no effect if xref linkend=guc-hot-standby is not +enabled, or if no recovery target is set. + /para + /listitem + /varlistentry + + /variablelist + + varlistentry id=action-at-recovery-target + xreflabel=action_at_recovery_target + termvarnameaction_at_recovery_target/varname (typeenum/type) + indexterm +primaryvarnameaction_at_recovery_target/ recovery parameter/primary + /indexterm + /term + listitem + para +Specifies what action the server should take once the recovery target is +reached. The default is literalpause/, which means recovery will +be paused. literalpromote/ means recovery process will finish and +the server will start to accept connections. +Finally literalshutdown/ will stop the server after reaching the +recovery target. + /para +The intended use of literalpause/ setting is to allow queries to be +executed against the database to check if this recovery target is the +most desirable point for recovery. The paused state can be resumed by +using functionpg_xlog_replay_resume()/ (See xref linkend=functions-recovery-control-table), which then causes recovery to end. If this recovery target is not the desired stopping point, then shutdown the server, change the @@ -302,8 +329,23 @@ restore_command = 'copy C:\\server\\archivedir\\%f %p' # Windows continue recovery. /para para -This setting has no effect if xref linkend=guc-hot-standby is not -enabled, or if no recovery target is set. +The literalshutdown/ setting is useful to have instance ready at +exact replay point desired. +The instance will still be able to replay more WAL records (and in fact +will have to replay WAL records since last checkpoint next time it is +started). + /para + para +Note that because filenamerecovery.conf/ will not be renamed when +varnameaction_at_recovery_target/ is set to literalshutdown/, +any subsequent start will end with immediate shutdown unless the +configuration is changed or the filenamerecovery.conf/ is removed +manually. + /para + para +This setting has no effect if no recovery target is set. +If xref linkend=guc-hot-standby is not enabled, a setting of +literalpause/ will act the same as literalshutdown/. /para /listitem /varlistentry diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 99f702c..6747ea6 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -228,7 +228,7 @@ static char *recoveryEndCommand = NULL; static char *archiveCleanupCommand = NULL; static RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET; static bool recoveryTargetInclusive = true; -static bool recoveryPauseAtTarget = true; +static RecoveryTargetAction actionAtRecoveryTarget = RECOVERY_TARGET_ACTION_PAUSE; static TransactionId recoveryTargetXid; static TimestampTz recoveryTargetTime; static char *recoveryTargetName; @@ -4643,6 +4643,9 @@ readRecoveryCommandFile(void)
Re: [HACKERS] proposal: plpgsql - Assert statement
Marko Tiikkaja ma...@joh.to writes: I also went back to the original thread, and I think Heikki's summary dismissed e.g. Robert's criticism quite lightly: http://www.postgresql.org/message-id/ca+tgmobwossrncv_ijk3xhsytxb7dv0awgvwkmeurntovez...@mail.gmail.com The core of that complaint is that we'd have to make ASSERT a plpgsql reserved word, which is true enough as things stand today. However, why is it that plpgsql statement-introducing keywords need to be reserved? The only reason for that AFAICS is to allow us to distinguish the statements from assignments. But it seems like that could possibly be gotten around with some work. The first thing that comes to mind is for the lexer to do one-token lookahead and assume that the second token of an assignment must be := (aka =), ., or [. (Have I missed any cases?) Then, any statement for which the second token can't be one of those, which is surely most if not all of them, could have an unreserved leading keyword. This would at a stroke dereserve about half of plpgsql's existing reserved words, as well as remove a roadblock for ASSERT and other new statements. 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] proposal: plpgsql - Assert statement
2014-11-19 23:38 GMT+01:00 Marko Tiikkaja ma...@joh.to: On 2014-11-19 23:18, Pavel Stehule wrote: 2014-11-19 18:01 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Robert Haas robertmh...@gmail.com writes: On Wed, Nov 19, 2014 at 11:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: FWIW, I would vote against it also. I do not find this to be a natural extension of RAISE; it adds all sorts of semantic issues. (In particular, what is the evaluation order of the WHEN versus the other subexpressions of the RAISE?) What I liked about this syntax was that we could eventually have: RAISE ASSERT WHEN stuff; ...and if assertions are disabled, we can skip evaluating the condition. If you just write an IF .. THEN block you can't do that. Well, if that's what you want, let's just invent ASSERT condition there was this proposal .. ASSERT statement .. related discuss was finished, because it needs a reserved keyword ASSERT. Finished? Really? This was Heikki's take on the discussion that took place a good while ago: http://www.postgresql.org/message-id/5405ff73.1010...@vmware.com. And in the same thread you also said you like it: http://www.postgresql.org/ message-id/CAFj8pRAC-ZWDrbU-uj=xQOWQtbAqR5oXsM1xYOyhZmyeuvZvQ a...@mail.gmail.co. But perhaps you've changed your mind since then (which is fine.) Or maybe that was only in the case where we'd have a special mode where you could opt-in if you're willing to accept the backwards compatibility issue? I also went back to the original thread, and I think Heikki's summary dismissed e.g. Robert's criticism quite lightly: http://www.postgresql.org/message-id/CA+TgmobWoSSRNcV_ ijk3xhsytxb7dv0awgvwkmeurntovez...@mail.gmail.com this discuss is too long. I shouldn't remember all details well. Proposal of plpgsql statement ASSERT was there, but there was not a agreement of syntax (as statement X as function call) and one objective disadvantage was request of new keyword. So I throw this idea as unacceptable. I have no objections against a statement ASSERT still - but there was not a strong agreement, so my next proposal (and some common agreement was on RAISE WHEN). .marko
Re: [HACKERS] proposal: plpgsql - Assert statement
2014-11-19 23:54 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: Marko Tiikkaja ma...@joh.to writes: I also went back to the original thread, and I think Heikki's summary dismissed e.g. Robert's criticism quite lightly: http://www.postgresql.org/message-id/ca+tgmobwossrncv_ijk3xhsytxb7dv0awgvwkmeurntovez...@mail.gmail.com The core of that complaint is that we'd have to make ASSERT a plpgsql reserved word, which is true enough as things stand today. However, why is it that plpgsql statement-introducing keywords need to be reserved? The only reason for that AFAICS is to allow us to distinguish the statements from assignments. But it seems like that could possibly be gotten around with some work. The first thing that comes to mind is for the lexer to do one-token lookahead and assume that the second token of an assignment must be := (aka =), ., or [. (Have I missed any cases?) Then, any statement for which the second token can't be one of those, which is surely most if not all of them, could have an unreserved leading keyword. This would at a stroke dereserve about half of plpgsql's existing reserved words, as well as remove a roadblock for ASSERT and other new statements. Doesn't it close a doors to implement a procedures call without explicit CALL statement (like PL/SQL) ? Personally I doesn't feel to introduce lot of new keywords (statements) to plpgsql. Probably only two - ASSERT (assertions), PRAGMA (some cooperation with plpgsql extensions). regards, tom lane
[HACKERS] Merging recovery.conf into PostgreSQL.conf -- where did this go?
Hackers, One patch that got deferred from 9.4 was the merger of recovery.conf and postgresql.conf, due to conflicts with ALTER SYSTEM SET. However, this is still a critical TODO, because recovery.conf is still an ongoing major management problem for PostgreSQL DBAs. So, what happened to this? I kinda expected it to get committed right after we opened 9.5. -- 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] Merging recovery.conf into PostgreSQL.conf -- where did this go?
Hi, On 2014-11-19 15:09:10 -0800, Josh Berkus wrote: One patch that got deferred from 9.4 was the merger of recovery.conf and postgresql.conf, due to conflicts with ALTER SYSTEM SET. However, this is still a critical TODO, because recovery.conf is still an ongoing major management problem for PostgreSQL DBAs. So, what happened to this? I kinda expected it to get committed right after we opened 9.5. Nobody did the remaining work. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: plpgsql - Assert statement
Pavel Stehule pavel.steh...@gmail.com writes: 2014-11-19 23:54 GMT+01:00 Tom Lane t...@sss.pgh.pa.us: The core of that complaint is that we'd have to make ASSERT a plpgsql reserved word, which is true enough as things stand today. However, why is it that plpgsql statement-introducing keywords need to be reserved? Doesn't it close a doors to implement a procedures call without explicit CALL statement (like PL/SQL) ? So, in order to leave the door open for implicit CALL (which nobody's ever proposed or tried to implement AFAIR), you're willing to see every other proposal for new plpgsql statements go down the drain due to objections to new reserved words? I think your priorities are skewed. (If we did want to allow implicit CALL, I suspect that things could be hacked so that it worked for any function name that wasn't already an unreserved keyword, anyway. So you'd be no worse off.) Personally I doesn't feel to introduce lot of new keywords (statements) to plpgsql. Probably only two - ASSERT (assertions), PRAGMA (some cooperation with plpgsql extensions). I can't say that either of those excite me particularly, so the idea that those two are the only new statements we'd ever want to add seems improbable. 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] RLS with check option - surprised design
On Sun, Oct 5, 2014 at 5:16 AM, Stephen Frost sfr...@snowman.net wrote: next a message: ERROR: new row violates WITH CHECK OPTION for data DETAIL: Failing row contains (2014-10-05 12:28:30.79652, petr, 1000). Doesn't inform about broken policy. I'm guessing this is referring to the above policies and so my comments there apply.. One thing to note about this is that there is an active discussion about removing the 'DETAIL' part of that error message as it may be an information leak. I should point out that there is an issue with the ON CONFLICT UPDATE patch and RLS, as described here: https://wiki.postgresql.org/wiki/UPSERT#RLS I think it'll be possible to prevent the current information leak that my example illustrates (by making sure that there is an appropriate predicate associated with the auxiliary UPDATE plan, like any other UPDATE). After all, the auxiliary UPDATE accepts a WHERE clause, subject only to a few restrictions that are not relevant for the purposes of appending security quals. I actually spent over a day trying to figure out how to make this work, but gave up before the most recent revision, V1.4 was submitted. I guess I'll have to look at the problem again soon. I don't grok RLS, but offhand I think simply not including the DETAIL message might be good enough to fix my case. Maybe you have an opinion on that. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Mon, Nov 10, 2014 at 3:33 PM, Peter Geoghegan p...@heroku.com wrote: Attached is V1.4. Someone mentioned to me privately that they weren't sure that the question of whether or not RETURNING only projected actually inserted tuples was the right one. Also, I think someone else mentioned this a few months back. I'd like to address this question directly sooner rather than later, and so I've added a note on the Wiki page in relation to this [1]. It's a possible area of concern at this point. Anyway, it wouldn't require much implementation effort to change the behavior so that updated tuples were also projected. In addition, we might also consider the necessity of inventing a mechanism to make apparent whether the tuple was inserted or updated. The discussion needs to happen first, though. [1] https://wiki.postgresql.org/wiki/UPSERT#RETURNING_behavior -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New Event Trigger: table_rewrite
On Thu, Nov 20, 2014 at 2:57 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Fixed in the attached version of the patch. Thanks! Things are moving nicely for this patch. Patch compiles and passes check-world. Some minor comments about the latest version: 1) Couldn't this paragraph be reworked? para +The literaltable_rewrite/ event occurs just before a table is going to +get rewritten by the commands literalALTER TABLE/literal. While other +control statements are available to rewrite a table, +like literalCLUSTER/literal and literalVACUUM/literal, +the literaltable_rewrite/ event is currently only triggered by +the literalALTER TABLE/literal command, which might or might not need +to rewrite the table. + /para CLUSTER and VACUUM are not part of the supported commands anymore, so I think that we could replace that by the addition of a reference number in the cell of ALTER TABLE for the event table_rewrite and write at the bottom of the table a description of how this event behaves with ALTER TABLE. Note as well that might or might not is not really helpful for the user. 2) The examples of SQL queries provided are still in lower case in the docs, that's contrary to the rest of the docs where upper case is used for reserved keywords. + para +Here's an example implementing such a policy. +programlisting +create or replace function no_rewrite() + returns event_trigger + language plpgsql as [...] 3) This reference can be completely removed: /* * Otherwise, command should be CREATE, ALTER, or DROP. +* Or one of ANALYZE, CLUSTER, VACUUM. */ 4) In those places as well CLUSTER and VACUUM should be removed: + else if (pg_strncasecmp(tag, ANALYZE, 7) == 0 || +pg_strncasecmp(tag, CLUSTER, 7) == 0 || +pg_strncasecmp(tag, VACUUM, 6) == 0) + return EVENT_TRIGGER_COMMAND_TAG_OK; else And here: + if (pg_strcasecmp(tag, ALTER TABLE) == 0 || + pg_strcasecmp(tag, CLUSTER) == 0 || + pg_strcasecmp(tag, VACUUM) == 0 || + pg_strcasecmp(tag, ANALYZE) == 0 ) + return EVENT_TRIGGER_COMMAND_TAG_OK I am noticing that the points raised by Alvaro previously are fixed. Regards, -- Michael -- 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] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On 11/20/2014 01:52 AM, Peter Geoghegan wrote: On Mon, Nov 10, 2014 at 3:33 PM, Peter Geoghegan p...@heroku.com wrote: Also, I think someone else mentioned this a few months back. Yeah, that was me. I think we have three options. 1. Return only inserted tuples 2. Return inserted and updated tuples 3. Return inserted, updated and skipped tuples To me option 1 is surprising and less useful since I imagine in most cases where you do an upsert you do not care if the tuple was inserted or updated as long as it has the right values after the upsert, and these values is also what I would expect to be returned. The possible use case I see for option 3 is when you want the values of automatically generated columns but there is actually no work to do if another transaction had already inserted the same row (same according to the unique constraints). But this behavior even though useful in certain cases might be surprising. Andreas -- 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] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Wed, Nov 19, 2014 at 5:37 PM, Andreas Karlsson andr...@proxel.se wrote: I think we have three options. 1. Return only inserted tuples 2. Return inserted and updated tuples 3. Return inserted, updated and skipped tuples To me option 1 is surprising and less useful since I imagine in most cases where you do an upsert you do not care if the tuple was inserted or updated as long as it has the right values after the upsert, and these values is also what I would expect to be returned. I can see why you'd say that about option 1. That also seems like an argument against surfacing the distinction directly (through a dedicated hidden column or other expressing that RETURNING might reference, say). The possible use case I see for option 3 is when you want the values of automatically generated columns but there is actually no work to do if another transaction had already inserted the same row (same according to the unique constraints). But this behavior even though useful in certain cases might be surprising. I think that 3 is out. It seems hard to justify not RETURNING anything in respect of a slot when there is a before row insert trigger that returns NULL on the one hand, but RETURNING something despite not inserting for ON CONFLICT UPDATE on the other. I think if we do this, we're also going to have to set a command tag. That could look like this: postgres=# INSERT INTO upsert values(1, 'Foo'), (2, 'Bar') ON CONFLICT (key) UPDATE SET val = EXCLUDED.val; INSERT 0 1 UPDATE 1 Or perhaps like this: postgres=# INSERT INTO upsert values(1, 'Foo'), (2, 'Bar') ON CONFLICT (key) UPDATE SET val = EXCLUDED.val; UPSERT 0 2 Maybe the latter is better, because it's less likely to break tools that currently parse the command tag. But if we went with the former command tag format, we'd have to figure out if there should always be an UPDATE part of INSERT command tags generally, even when there was no ON CONFLICT UPDATE clause. I guess in that case it would have to become stable/consistent across INSERTs, so we'd always have an UPDATE part, but I'm not sure. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Wed, Nov 19, 2014 at 6:04 PM, Peter Geoghegan p...@heroku.com wrote: I think that 3 is out. It seems hard to justify not RETURNING anything in respect of a slot when there is a before row insert trigger that returns NULL on the one hand, but RETURNING something despite not inserting for ON CONFLICT UPDATE on the other. I mean: despite not inserting *or updating*. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
