Re: [HACKERS] What exactly is our CRC algorithm?
At 2014-11-20 09:52:01 +0530, a...@2ndquadrant.com wrote: To address (a), I am timing a standby restoring the same 11GB of WAL via restore_command with and without the CRC patch. I ran perf record -F 100 --call-graph=dwarf bin/postgres -D backup, where: (a) bin/postgres was compiled, as before, with CFLAGS=-O3 -msse4.2 and without --enable-debug. (b) backup is a basebackup of the original data directory before I ran pgbench; it has a recovery.conf with a restore_command to fetch the WAL generated during the pgbench run. I ran the test frice (as my daughter would say) with HEAD and the slice-by-8 patch, and counted the time between the first and last «restored log file NNN from archive» log messages. The number in parentheses shows the order in which I ran the tests. HEAD: 6m47s (1), 6m23s (2), 3m12s (6), 3m04s (7) 8CRC: 5m16s (3), 3m13s (4), 2m57s (5), 2m46s (8) In the unpatched server, the profile shows ValidXLogRecord at 50% in every case (54.81%, 59.41%, 58.82%, 59.05%). In the patched server, pg_comp_crc32c was at the top of the profile in three cases, with 10.29%, 12.01%, and 10.99%. In test (4), however, it was at 6.38%, and first place went to copy_user_enhanced_fast_string with 10.03% (which was in second place the other three times). I believe the wallclock runtime in these tests was influenced more by IO than CPU, but that the profile shows a worthwhile improvement anyway. I have the perf.data from each run, and I can rerun the tests if anyone wants to suggest a different strategy. Suggestions for how to address (b) are welcome. I'll think about how to do this, and work on the SSE4.2 patch meanwhile. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GIN code managing entry insertion not able to differentiate fresh and old indexes
Hi all, While playing with the GIN code for an upcoming patch, I noticed that when inserting a new entry in a new index, this code path is not able to make the difference if the index is in a build state or not. Basically, when entering in ginEntryInsert@gininsert.c GinBtree built via ginPrepareEntryScan does not have its flag isBuild set up properly. I think that it should be set as follows to let this code path be aware that index is in build state: btree.isBuild = (buildStats != NULL); Note that the entry insertion code does nothing with isBuild yet, so it does not really impact back-branches. However, if in the future we fix a bug in this area and need to make distinction between a fresh index and an old one well there will be problems. For those reasons, this correctness fix should be perhaps master-only for now (perhaps even 9.4 stuff as well). Patch is attached. Regards, -- Michael diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c index 370884e..229167b 100644 --- a/src/backend/access/gin/gininsert.c +++ b/src/backend/access/gin/gininsert.c @@ -191,6 +191,7 @@ ginEntryInsert(GinState *ginstate, buildStats-nEntries++; ginPrepareEntryScan(btree, attnum, key, category, ginstate); + btree.isBuild = (buildStats != NULL); stack = ginFindLeafPage(btree, false); page = BufferGetPage(stack-buffer); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: GIN code managing entry insertion not able to differentiate fresh and old indexes
On Thu, Nov 20, 2014 at 5:22 PM, Michael Paquier michael.paqu...@gmail.com wrote: Patch is attached. A cleaner fix may be btw to pass the build stats in ginPrepareEntryScan and set the flag directly there. -- 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] [PATCH] add ssl_protocols configuration option
Alex Shulgin a...@commandprompt.com writes: * 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. I can easily do that, but I won't have time until next week or so. DES -- Dag-Erling Smørgrav - d...@des.no -- 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
On Wed, Nov 19, 2014 at 4:34 PM, Alex Shulgin a...@commandprompt.com wrote: 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 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? I see no reason to accept them at all, if we're going to reject them later anyway. We can argue (as was done earlier in this thread) if we can drop SSL 3.0 completely -- but we can *definitely* drop SSLv2, and we should. But anything that we're going to reject at a later stage anyway, we should reject early. -- 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] [PATCH] add ssl_protocols configuration option
Magnus Hagander mag...@hagander.net writes: Alex Shulgin a...@commandprompt.com writes: * 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? I see no reason to accept them at all, if we're going to reject them later anyway. We can argue (as was done earlier in this thread) if we can drop SSL 3.0 completely -- but we can *definitely* drop SSLv2, and we should. But anything that we're going to reject at a later stage anyway, we should reject early. It's not really early or late, but rather within the loop or at the end of it. From the users' perspective, the difference is that they get (to paraphrase) SSLv2 is not allowed instead of syntax error and that they can use constructs such as ALL:-SSLv2. DES -- Dag-Erling Smørgrav - d...@des.no -- 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] group locking: incomplete patch, just for discussion
On Wed, 2014-11-19 at 11:03 -0500, Robert Haas wrote: 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. Right. It can even give a nice error hint to tell the user how to avoid the problem. 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. You seem to be raising this as a show-stopping problem, and I'm not convinced that it is. (a) There are no parallel operators yet, so this is speculative. (b) Out of the parallel operators you imagine (e.g. Scan), we don't expect anything other than AccessShare locks in the normal case. (c) The failure mode is not so bad; it's just an error and the user can probably work around it. You could argue that we know we're headed for this problem, and therefore we should solve it now. I disagree. You are assuming that sharing exclusive heavyweight locks among a group will be a fundamental part of everything postgres does with parallelism; but not every design requires it. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] add ssl_protocols configuration option
On Thu, Nov 20, 2014 at 10:19 AM, Dag-Erling Smørgrav d...@des.no wrote: Magnus Hagander mag...@hagander.net writes: Alex Shulgin a...@commandprompt.com writes: * 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? I see no reason to accept them at all, if we're going to reject them later anyway. We can argue (as was done earlier in this thread) if we can drop SSL 3.0 completely -- but we can *definitely* drop SSLv2, and we should. But anything that we're going to reject at a later stage anyway, we should reject early. It's not really early or late, but rather within the loop or at the end of it. From the users' perspective, the difference is that they get (to paraphrase) SSLv2 is not allowed instead of syntax error and that they can use constructs such as ALL:-SSLv2. Ah, I see now - I hadn't looked at the code, just the review comment. It's a fallout from the reverse logic in openssl. Then it makes a lot more sense. -- 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] Add shutdown_at_recovery_target option to recovery.conf
On 19 November 2014 22:47, Petr Jelinek p...@2ndquadrant.com wrote: 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. ... 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. Like it. Patch looks good now. Will commit shortly. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] add ssl_protocols configuration option
Dag-Erling Smørgrav d...@des.no writes: Alex Shulgin a...@commandprompt.com writes: * 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. I can easily do that, but I won't have time until next week or so. I can do that too, just need a hint where to look at in libpq/psql to add the option. For SSL we have sslmode and sslcompression, etc. in conninfo, so adding sslprotocols seems to be an option. As an aside note: should we also expose a parameter to choose SSL ciphers (would be a separate patch)? -- 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 19 November 2014 23:29, Tom Lane t...@sss.pgh.pa.us wrote: 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.) Implictly CALLed procedures/function-that-return-void would be a great feature for 9.5 Great proposal. -- 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] pg_background (and more parallelism infrastructure patches)
On Wed, Nov 12, 2014 at 11:09 PM, Robert Haas robertmh...@gmail.com wrote:
On Wed, Nov 12, 2014 at 11:36 AM, Robert Haas robertmh...@gmail.com
wrote:
On Wed, Nov 12, 2014 at 11:19 AM, Andres Freund and...@2ndquadrant.com
wrote:
The question is whether the library is actually loaded in that case?
Because that normally only happens early during startup - which is why
it's a PGC_BACKEND guc.
It looks like that does not work.
[rhaas pgsql]$ PGOPTIONS='-c local_preload_libraries=auto_explain' psql
psql (9.5devel)
Type help for help.
rhaas=# select * from pg_background_result(pg_background_launch('show
auto_explain.log_min_duration')) as (x text);
ERROR: unrecognized configuration parameter
auto_explain.log_min_duration
CONTEXT: background worker, pid 31316
So, there's more to be done here. Rats.
It turned out to be quite simple to fix both problems.
Updated patches attached.
Few compilation errors in the patch:
1contrib\postgres_fdw\postgres_fdw.c(2107): error C2198:
'set_config_option' : too few arguments for call
1contrib\postgres_fdw\postgres_fdw.c(2111): error C2198:
'set_config_option' : too few arguments for call
1contrib\postgres_fdw\postgres_fdw.c(2115): error C2198:
'set_config_option' : too few arguments for call
2contrib\dblink\dblink.c(2983): error C2198: 'set_config_option' : too few
arguments for call
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] New Event Trigger: table_rewrite
Alvaro Herrera alvhe...@2ndquadrant.com writes: 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. That's precisely why we have an event trigger here, I think --- for some subcommands, it's not easy to determine whether a rewrite happens or not. (I think SET TYPE is the one). I don't think we want to document precisely under what condition a rewrite takes place. Yeah, the current documentation expands to the following sentence, as browsed in http://www.postgresql.org/docs/9.3/interactive/sql-altertable.html As an exception, if the USING clause does not change the column contents and the old type is either binary coercible to the new type or an unconstrained domain over the new type, a table rewrite is not needed, but any indexes on the affected columns must still be rebuilt. I don't think that “might or might not” is less helpful in the context of the Event Trigger, because the whole point is that the event is only triggered in case of a rewrite. Of course we could cross link the two paragraphs or something. 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. Right, being consistent trumps personal preferences, changed in the attached. Yes please. nitpick Another thing in that sample code is not current_hour between 1 and 6. That reads strange to me. It should be equally correct to spell it as current_hour not between 1 and 6, which seems more natural. / True, fixed in the attached. Regards, -- Dimitri Fontaine 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..0a80993 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
Re: [HACKERS] Merging recovery.conf into PostgreSQL.conf -- where did this go?
Andres Freund and...@2ndquadrant.com writes: 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. I'd like to work on this. AFAICT, this CommitFest entry is the latest on the subject, right? https://commitfest.postgresql.org/action/patch_view?id=1293 Should/may I move it to the next Open fest? -- 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] Merging recovery.conf into PostgreSQL.conf -- where did this go?
On 2014-11-20 17:26:02 +0300, Alex Shulgin wrote: Andres Freund and...@2ndquadrant.com writes: 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. I'd like to work on this. AFAICT, this CommitFest entry is the latest on the subject, right? https://commitfest.postgresql.org/action/patch_view?id=1293 Should/may I move it to the next Open fest? I think before that it needs actual work - no point in adding it to the CF until progress has been made. 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 2:33 PM, Peter Geoghegan p...@heroku.com wrote: I don't think that's the case. Other RTEs are penalized for having non-matching aliases here. The point I made did not, as far as I can see, have anything to do with non-matching aliases; it could arise with just a single RTE. 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. The way I see it, the main cost of a bad suggestion is that it annoys the user with clutter which they may brand as stupid. Think about how much vitriol has been spewed over the years against progress bars (or estimated completion) times that don't turn out to mirror reality. Microsoft has gotten more cumulative flack about their inaccurate progress bars over the years than they would have for dropping an elevator on a cute baby. Or think about how annoying it is when a spell-checker or grammar-checker underlines something you've written that is, in your own opinion, correctly spelled or grammatical. Maybe that kind of thing doesn't annoy you, but it definitely annoys me, and I think probably a lot of other people. My general experience is that people get quite pissed off by bad suggestions from a computer. At least in my experience, users' actual level of agitation is often all out of proportion to what might seem justified, but we are designing this software for actual users, so their likely emotional reactions are relevant. 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. I don't know what you mean by a ramped-up distance threshold, exactly. I think it's good for the distance threshold to be lower for small strings and higher for large ones. I think I'm somewhat open to negotiation on the details, but I think any system that's going to suggest quantity for tit is going too far. If the user types qty when they meant quantity, they probably don't really need the hint, because they're going to say to themselves wait, I guess I didn't abbreviate that. The time when they need the hint is when they typed quanttiy, because it's quite possible to read a query with that sort of typo multiple times and not realize that you've made one. You're sitting there puzzling over where the quantity column went, and asking yourselves how you can be mis-remembering the schema, and saying wait, didn't I just see that column in the \d output ... and you don't even think to check carefully for a spelling mistake. The hint may well clue you in to what the real problem is. In other words, I think there's value in trying to clue somebody in when they've made a typo, but not when they've made a think-o. We won't be able to do the latter accurately enough to make it more useful than annoying. -- 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_background (and more parallelism infrastructure patches)
On Thu, Nov 20, 2014 at 7:30 AM, Amit Kapila amit.kapil...@gmail.com wrote:
Few compilation errors in the patch:
1contrib\postgres_fdw\postgres_fdw.c(2107): error C2198:
'set_config_option' : too few arguments for call
1contrib\postgres_fdw\postgres_fdw.c(2111): error C2198:
'set_config_option' : too few arguments for call
1contrib\postgres_fdw\postgres_fdw.c(2115): error C2198:
'set_config_option' : too few arguments for call
2contrib\dblink\dblink.c(2983): error C2198: 'set_config_option' : too few
arguments for call
Oops. Good catch. Fixed in the attached version.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d77b3ee..18ae318 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2980,7 +2980,7 @@ applyRemoteGucs(PGconn *conn)
/* Apply the option (this will throw error on failure) */
(void) set_config_option(gucName, remoteVal,
PGC_USERSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
}
return nestlevel;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 905a821..76bda73 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -2104,15 +2104,15 @@ set_transmission_modes(void)
if (DateStyle != USE_ISO_DATES)
(void) set_config_option(datestyle, ISO,
PGC_USERSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
if (IntervalStyle != INTSTYLE_POSTGRES)
(void) set_config_option(intervalstyle, postgres,
PGC_USERSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
if (extra_float_digits 3)
(void) set_config_option(extra_float_digits, 3,
PGC_USERSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
return nestlevel;
}
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 9a0afa4..6692bb5 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -814,11 +814,11 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
if (client_min_messages WARNING)
(void) set_config_option(client_min_messages, warning,
PGC_USERSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
if (log_min_messages WARNING)
(void) set_config_option(log_min_messages, warning,
PGC_SUSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
/*
* Set up the search path to contain the target schema, then the schemas
@@ -843,7 +843,7 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
(void) set_config_option(search_path, pathbuf.data,
PGC_USERSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
/*
* Set creating_extension and related variables so that
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index c0156fa..2f02303 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -2422,7 +2422,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
snprintf(workmembuf, sizeof(workmembuf), %d, maintenance_work_mem);
(void) set_config_option(work_mem, workmembuf,
PGC_USERSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0);
+ GUC_ACTION_SAVE, true, 0, false);
if (SPI_connect() != SPI_OK_CONNECT)
elog(ERROR, SPI_connect failed);
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index bb9207a..6dca5df 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -318,7 +318,7 @@ ProcessConfigFile(GucContext context)
/* Now we can re-apply the wired-in default (i.e., the boot_val) */
if (set_config_option(gconf-name, NULL,
context, PGC_S_DEFAULT,
- GUC_ACTION_SET, true, 0) 0)
+ GUC_ACTION_SET, true, 0, false) 0)
{
/* Log the change if appropriate */
if (context == PGC_SIGHUP)
@@ -373,7 +373,7 @@ ProcessConfigFile(GucContext context)
scres = set_config_option(item-name, item-value,
context, PGC_S_FILE,
- GUC_ACTION_SET, true, 0);
+ GUC_ACTION_SET, true, 0, false);
if (scres 0)
{
/* variable was updated, so log the change if appropriate */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 23cbe90..f04757c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -110,6 +110,12 @@
#define S_PER_D (60 * 60 * 24)
#define MS_PER_D (1000 * 60 * 60 * 24)
+/*
+ * Precision with which REAL type guc values are to be printed for GUC
+ * serialization.
+ */
+#define
Re: [HACKERS] pg_multixact not getting truncated
On Wed, Nov 19, 2014 at 4:16 PM, Josh Berkus j...@agliodbs.com wrote: 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. So that we can have two ways to lose data? Forbidding connections to a database doesn't prevent XID or MXID wraparound. -- 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()
Robert Haas robertmh...@gmail.com writes: ... In other words, I think there's value in trying to clue somebody in when they've made a typo, but not when they've made a think-o. We won't be able to do the latter accurately enough to make it more useful than annoying. FWIW, I concur with Robert's analysis that wrong suggestions are likely to be annoying. We should be erring on the side of not making a suggestion rather than making one that's a low-probability guess. I'm not particularly convinced that the f1 - f2 example is a useful behavior, and I'm downright horrified by the qty - quantity case. If the hint mechanism thinks the latter two are close enough together to suggest, it's going to be spewing a whole lot of utterly ridiculous suggestions. I'm going to be annoyed way more times than I'm going to be helped. The big picture is that this is more or less our first venture into heuristic suggestions. I think we should start slow with a very conservative set of heuristics. If it's a success maybe we can get more aggressive over time --- but if we go over the top here, the entire concept will be discredited in this community for the next ten years. 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] On partitioning
On Wed, Nov 19, 2014 at 10:27 PM, Amit Langote
langote_amit...@lab.ntt.co.jp wrote:
Maybe as anyarray, but I think pg_node_tree
might even be better. That can also represent data of some arbitrary
type, but it doesn't enforce that everything is uniform. So you could
have a list of objects of the form {RANGEPARTITION :lessthan {CONST
...} :partition 16982} or similar. The relcache could load that up
and convert the list to a C array, which would then be easy to
binary-search.
As you say, you also need to store the relevant operator somewhere,
and the fact that it's a range partition rather than list or hash,
say.
I'm wondering here if it's better to keep partition values per partition
wherein we have two catalogs, say, pg_partitioned_rel and pg_partition_def.
pg_partitioned_rel stores information like partition kind, key (attribute
number(s)?), key opclass(es). Optionally, we could also say here if a given
record (in pg_partitioned_rel) represents an actual top-level partitioned
table or a partition that is sub-partitioned (wherein this record is just a
dummy for keys of sub-partitioning and such); something like partisdummy...
pg_partition_def stores information of individual partitions
(/sub-partitions, too?) such as its parent (either an actual top level
partitioned table or a sub-partitioning template), whether this is an
overflow/default partition, and partition values.
Yeah, you could do something like this. There's a certain overhead to
adding additional system catalogs, though. It means more inodes on
disk, probably more syscaches, and more runtime spent probing those
additional syscache entries to assemble a relcache entry. On the
other hand, it's got a certain conceptual cleanliness to it.
I do think at a very minimum it's important to have a Boolean flag in
pg_class so that we need not probe what you're calling
pg_partitioned_rel if no partitioning information is present there. I
might be tempted to go further and add the information you are
proposing to put in pg_partitioned_rel in pg_class instead, and just
add one new catalog. But it depends on how many columns we end up
with.
Before going too much further with this I'd mock up schemas for your
proposed catalogs and a list of DDL operations to be supported, with
the corresponding syntax, and float that here for comment.
Such a scheme would be similar to what Greenplum [1] has.
Interesting.
Perhaps this duplicates inheritance and can be argued in that sense, though.
Do you think keeping partition defining values with the top-level partitioned
table would make some partitioning schemes (multikey, sub- , etc.) a bit
complicated to implement? I cannot offhand imagine the actual implementation
difficulties that might be involved myself but perhaps you have a better idea
of such details and would have a say...
I don't think this is a big deal one way or the other. We're all
database folks here, so deciding to normalize for performance or
denormalize for conceptual cleanliness shouldn't tax our powers
unduly.
--
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] Bugfix and new feature for PGXS
On Wed, Nov 19, 2014 at 11:11 PM, Peter Eisentraut pete...@gmx.net wrote: The applicable parts of that patch could be backpatched as a bug fix (but evidently no one cares about building contrib with pgxs (except when I submit a patch to remove it)). Touché. -- 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] group locking: incomplete patch, just for discussion
On Thu, Nov 20, 2014 at 4:21 AM, Jeff Davis pg...@j-davis.com wrote: 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. You seem to be raising this as a show-stopping problem, and I'm not convinced that it is. Well, what I'm saying is that at very minimum we have to be able detect deadlocks, and we have two plausible designs for avoiding that: 1. Modify the deadlock detector to know about lock groups. 2. Propagate pre-existing locks from the user backend to all the workers. I initially proposed #1, but now I think #2 solves more of the problems for less code. -- 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] WAL format and API changes (9.5)
As you may have noticed, I committed this (after some more cleanup). Of course, feel free to still review it, and please point out any issues you may find. - 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] group locking: incomplete patch, just for discussion
On Thu, 2014-11-20 at 11:22 -0500, Robert Haas wrote: 2. Propagate pre-existing locks from the user backend to all the workers. I initially proposed #1, but now I think #2 solves more of the problems for less code. OK. The primary concern with that is unintended consequences. But it's reasonable for you to ask for something more concrete. I will think on this more. A few things I'm thinking about now: * What do you mean by pre-existing? Locks existing prior to what event? (I don't think that's exactly what you meant.) * What's the conceptual difference between granting locks that would otherwise conflict with another process in the group (which is what this proposal is about) and having exactly the same set of locks? Is there any? * Let's say you have processes A1 and A2 in one group, and B. A1 and B both have an AccessShare lock, and A2 tries to acquire an exclusive lock. B is waiting on A2. That's still a deadlock, right? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] group locking: incomplete patch, just for discussion
On 2014-11-20 11:22:37 -0500, Robert Haas wrote: On Thu, Nov 20, 2014 at 4:21 AM, Jeff Davis pg...@j-davis.com wrote: 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. You seem to be raising this as a show-stopping problem, and I'm not convinced that it is. Well, what I'm saying is that at very minimum we have to be able detect deadlocks, and we have two plausible designs for avoiding that: 1. Modify the deadlock detector to know about lock groups. 2. Propagate pre-existing locks from the user backend to all the workers. I initially proposed #1, but now I think #2 solves more of the problems for less code. Except that it opens us up for all kinds of concurrency bugs. I'm pretty strictly set against granting any self exclusive locks en-masse. If we do this by default for all granted locks when starting a worker backend it'll get *so* much harder to reason about correctness. Suddenly locks don't guarantee what they used to anymore. We'll e.g. not be able to rely that a CheckTableNotInUse() + AEL makes it safe to drop/rewrite/whatever a relation - even if that happens in the main backend. 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] T_CustomScan on ExplainTargetRel
Kouhei Kaigai kai...@ak.jp.nec.com writes: The attached obvious patch adds T_CustomScan on case-switch of ExplainTargetRel() that was oversight. Applied, thanks. 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] group locking: incomplete patch, just for discussion
On Thu, Nov 20, 2014 at 12:12 PM, Jeff Davis pg...@j-davis.com wrote: On Thu, 2014-11-20 at 11:22 -0500, Robert Haas wrote: 2. Propagate pre-existing locks from the user backend to all the workers. I initially proposed #1, but now I think #2 solves more of the problems for less code. OK. The primary concern with that is unintended consequences. But it's reasonable for you to ask for something more concrete. I will think on this more. A few things I'm thinking about now: * What do you mean by pre-existing? Locks existing prior to what event? (I don't think that's exactly what you meant.) * What's the conceptual difference between granting locks that would otherwise conflict with another process in the group (which is what this proposal is about) and having exactly the same set of locks? Is there any? * Let's say you have processes A1 and A2 in one group, and B. A1 and B both have an AccessShare lock, and A2 tries to acquire an exclusive lock. B is waiting on A2. That's still a deadlock, right? I think I discussed all of these issues on the other thread already. Am I wrong? -- 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 Thu, Nov 20, 2014 at 8:05 AM, Tom Lane t...@sss.pgh.pa.us wrote: I'm not particularly convinced that the f1 - f2 example is a useful behavior, and I'm downright horrified by the qty - quantity case. If the hint mechanism thinks the latter two are close enough together to suggest, it's going to be spewing a whole lot of utterly ridiculous suggestions. I'm going to be annoyed way more times than I'm going to be helped. I happen to think that that isn't the case, because the number of possible suggestions is fairly low anyway, and people don't tend to make those kind of errors. Robert's examples of ridiculous suggestions of quantity based on three letter strings other than qty (e.g. tit) were rather contrived. In fact, most 3 letter strings will not offer a suggestion. 3 or more Equidistant would-be matches tend to offer a lot of additional protection against bad suggestions for these terse strings. The big picture is that this is more or less our first venture into heuristic suggestions. I think we should start slow with a very conservative set of heuristics. If it's a success maybe we can get more aggressive over time --- but if we go over the top here, the entire concept will be discredited in this community for the next ten years. I certainly see your point here. It's not as if we have an *evolved* understanding of the usability issues. Besides, as Robert pointed out, most of the value of this patch is added by simple cases, like a failure to pluralize or not pluralize, or the omission of an underscore. I still think we should charge half for deletion, but I will concede that it's prudent to apply a more restrictive absolute quality final test. -- 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 Thu, Nov 20, 2014 at 7:32 AM, Robert Haas robertmh...@gmail.com wrote: 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. The way I see it, the main cost of a bad suggestion is that it annoys the user with clutter which they may brand as stupid. Think about how much vitriol has been spewed over the years against progress bars (or estimated completion) times that don't turn out to mirror reality. Well, you can judge the quality of the suggestion immediately. I imagined a mechanism that gives a little bit more than the minimum amount of guidance for things like contractions/abbreviations. Microsoft has gotten more cumulative flack about their inaccurate progress bars over the years than they would have for dropping an elevator on a cute baby. I haven't used a more recent version of Windows than Windows Vista, but I'm pretty sure that they kept it up. 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. I don't know what you mean by a ramped-up distance threshold, exactly. I think it's good for the distance threshold to be lower for small strings and higher for large ones. I think I'm somewhat open to negotiation on the details, but I think any system that's going to suggest quantity for tit is going too far. I mean the suggestion of raising the cost threshold more gradually, not as a step function of the number of characters in the string [1] where it's either over 6 characters and must pass the 50% test, or isn't and has no absolute quality test. The exact modification I described will FWIW remove the quantity for qty suggestion, as well as all the similar suggestions that you found objectionable (like tit also offering a suggestion of quantity). If you look at the regression tests, none of the sensible suggestions are lost (some would be by an across the board 50% absolute quality threshold, as I previously pointed out [2]), but all the bad ones are. I attach failed regression test output showing the difference between the previous expected values, and actual values with that small modification - it looks like most or all bad cases are now fixed. If the user types qty when they meant quantity, they probably don't really need the hint, because they're going to say to themselves wait, I guess I didn't abbreviate that. The time when they need the hint is when they typed quanttiy, because it's quite possible to read a query with that sort of typo multiple times and not realize that you've made one. I agree that that's a more important case. In other words, I think there's value in trying to clue somebody in when they've made a typo, but not when they've made a think-o. We won't be able to do the latter accurately enough to make it more useful than annoying. That's certainly true; I think that we only disagree about the exact point at which we enter the think-o correction business. [1] http://www.postgresql.org/message-id/CAM3SWZT+7hH29Go6ZuY2OrCS40=6ypvm_nt9njfovp3xwji...@mail.gmail.com [2] http://www.postgresql.org/message-id/cam3swztsgoknht8rk+0eed7spnjg4padmbqqyi0fh9bwcnv...@mail.gmail.com -- Peter Geoghegan regression.diffs 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] Functions used in index definitions shouldn't be changed
Tom Lane wrote: 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. 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.) I don't think that there is a universally compelling right or wrong to questions like this, it is more a matter of taste. Is it more important to protect the casual DBA from hurting himself or herself, or is it more important to provide a well honed scalpel for the experienced surgeon? It seems that more people lean in the latter direction, so I'll cease and desist. 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] group locking: incomplete patch, just for discussion
On Thu, Nov 20, 2014 at 12:19 PM, Andres Freund and...@2ndquadrant.com wrote: Except that it opens us up for all kinds of concurrency bugs. I'm pretty strictly set against granting any self exclusive locks en-masse. If we do this by default for all granted locks when starting a worker backend it'll get *so* much harder to reason about correctness. Suddenly locks don't guarantee what they used to anymore. We'll e.g. not be able to rely that a CheckTableNotInUse() + AEL makes it safe to drop/rewrite/whatever a relation - even if that happens in the main backend. Haven't I responded to this a few times already? I see no way, even theoretically, that it can be sane for CheckTableNotInUse() to succeed in a parallel context. Ever. If the deadlock detector would kill the processes anyway, it doesn't matter, because CheckTableNotInUse() should do it first, so that we get a better error and without waiting for deadlock_timeout. So any scenario that's predicated on the assumption that CheckTableNotInUse() will succeed in a parallel context is 100% unconvincing to me as an argument for anything. -- 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 Thu, Nov 20, 2014 at 1:30 PM, Peter Geoghegan p...@heroku.com wrote: I mean the suggestion of raising the cost threshold more gradually, not as a step function of the number of characters in the string [1] where it's either over 6 characters and must pass the 50% test, or isn't and has no absolute quality test. The exact modification I described will FWIW remove the quantity for qty suggestion, as well as all the similar suggestions that you found objectionable (like tit also offering a suggestion of quantity). If you look at the regression tests, none of the sensible suggestions are lost (some would be by an across the board 50% absolute quality threshold, as I previously pointed out [2]), but all the bad ones are. I attach failed regression test output showing the difference between the previous expected values, and actual values with that small modification - it looks like most or all bad cases are now fixed. That does seem to give better results, but it still seems awfully complicated. If we just used Levenshtein with all-default cost factors and a distance cap equal to Max(strlen(what_user_typed), strlen(candidate_match), 3), what cases that you think are important would be harmed? -- 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] Functions used in index definitions shouldn't be changed
On Thu, Nov 20, 2014 at 1:56 PM, Albe Laurenz laurenz.a...@wien.gv.at wrote: I don't think that there is a universally compelling right or wrong to questions like this, it is more a matter of taste. Is it more important to protect the casual DBA from hurting himself or herself, or is it more important to provide a well honed scalpel for the experienced surgeon? +1. I think if we had an already-existing prohibition here and you proposed relaxing it, the howls would be equally loud. We're not entirely consistent about how picky we are. -- 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] Ripping out dead code for mark/restore in some plan types
execAmi.c points out that * (However, since the only present use of mark/restore is in mergejoin, * there is no need to support mark/restore in any plan type that is not * capable of generating ordered output. So the seqscan, tidscan, * and valuesscan support is actually useless code at present.) I don't see any prospect that we'd ever adopt mark/restore for any other purpose than mergejoin, so it seems to me that the code in question is permanently dead. There's not that much of it, but I'm thinking that ripping it out and clarifying the commentary in execAmi.c would still be a net benefit for readability. Any objections? 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] Functions used in index definitions shouldn't be changed
Robert Haas robertmh...@gmail.com writes: On Thu, Nov 20, 2014 at 1:56 PM, Albe Laurenz laurenz.a...@wien.gv.at wrote: I don't think that there is a universally compelling right or wrong to questions like this, it is more a matter of taste. Is it more important to protect the casual DBA from hurting himself or herself, or is it more important to provide a well honed scalpel for the experienced surgeon? +1. I think if we had an already-existing prohibition here and you proposed relaxing it, the howls would be equally loud. We're not entirely consistent about how picky we are. How's that quote about foolish consistency go? In many cases, the reason why we enforce some things and not others is practical utility. 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] Functions used in index definitions shouldn't be changed
On 11/20/2014 02:28 PM, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Thu, Nov 20, 2014 at 1:56 PM, Albe Laurenz laurenz.a...@wien.gv.at wrote: I don't think that there is a universally compelling right or wrong to questions like this, it is more a matter of taste. Is it more important to protect the casual DBA from hurting himself or herself, or is it more important to provide a well honed scalpel for the experienced surgeon? +1. I think if we had an already-existing prohibition here and you proposed relaxing it, the howls would be equally loud. We're not entirely consistent about how picky we are. How's that quote about foolish consistency go? In many cases, the reason why we enforce some things and not others is practical utility. Right. (FTR, the quote from Emerson goes A foolish consistency is the hobgoblin of little minds, adored by little statesmen and philosophers and divines.) 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] Doing better at HINTing an appropriate column within errorMissingColumn()
On Thu, Nov 20, 2014 at 11:03 AM, Robert Haas robertmh...@gmail.com wrote: That does seem to give better results, but it still seems awfully complicated. If we just used Levenshtein with all-default cost factors and a distance cap equal to Max(strlen(what_user_typed), strlen(candidate_match), 3), what cases that you think are important would be harmed? Well, just by plugging in default Levenshtein cost factors, I can see the following regression: *** /home/pg/postgresql/src/test/regress/expected/join.out 2014-11-20 10:17:55.042291912 -0800 --- /home/pg/postgresql/src/test/regress/results/join.out 2014-11-20 11:42:15.670108745 -0800 *** *** 3452,3458 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. Within the catalogs, the names of attributes are prefixed as a form of what you might call internal namespacing. For example, pg_index has attributes that all begin with ind*. You could easily omit something like that, while still more or less knowing what you're looking for. In more concrete terms, this gets no suggestion: postgres=# select key from pg_index; ERROR: 42703: column key does not exist LINE 1: select key from pg_index; ^ Only this does: postgres=# select ikey from pg_index; ERROR: 42703: column ikey does not exist LINE 1: select ikey from pg_index; ^ HINT: Perhaps you meant to reference the column pg_index.indkey. postgres=# The git people varied their Levenshtein costings for a reason. I also think that a one size fits all cap will break things. It will independently break the example above, as well as the more marginal c1.f2. vs c1.f2 case (okay, maybe that case was exactly on the threshold, but others won't be). I don't see that different costings actually saves any complexity. Similarly, the final cap is quite straightforward. Anything with any real complexity happens before 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] Doing better at HINTing an appropriate column within errorMissingColumn()
On Thu, Nov 20, 2014 at 3:00 PM, Peter Geoghegan p...@heroku.com wrote: On Thu, Nov 20, 2014 at 11:03 AM, Robert Haas robertmh...@gmail.com wrote: That does seem to give better results, but it still seems awfully complicated. If we just used Levenshtein with all-default cost factors and a distance cap equal to Max(strlen(what_user_typed), strlen(candidate_match), 3), what cases that you think are important would be harmed? Well, just by plugging in default Levenshtein cost factors, I can see the following regression: *** /home/pg/postgresql/src/test/regress/expected/join.out 2014-11-20 10:17:55.042291912 -0800 --- /home/pg/postgresql/src/test/regress/results/join.out 2014-11-20 11:42:15.670108745 -0800 *** *** 3452,3458 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. Within the catalogs, the names of attributes are prefixed as a form of what you might call internal namespacing. For example, pg_index has attributes that all begin with ind*. You could easily omit something like that, while still more or less knowing what you're looking for. In more concrete terms, this gets no suggestion: postgres=# select key from pg_index; ERROR: 42703: column key does not exist LINE 1: select key from pg_index; ^ Only this does: postgres=# select ikey from pg_index; ERROR: 42703: column ikey does not exist LINE 1: select ikey from pg_index; ^ HINT: Perhaps you meant to reference the column pg_index.indkey. postgres=# Seems fine to me. If you typed relid rather than indexrelid or key rather than indkey, that's a thinko, not a typo. ikey for indkey could plausible be a typo, though you'd have to be having a fairly bad day at the keyboard. -- 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] group locking: incomplete patch, just for discussion
On 2014-11-20 13:57:25 -0500, Robert Haas wrote: On Thu, Nov 20, 2014 at 12:19 PM, Andres Freund and...@2ndquadrant.com wrote: Except that it opens us up for all kinds of concurrency bugs. I'm pretty strictly set against granting any self exclusive locks en-masse. If we do this by default for all granted locks when starting a worker backend it'll get *so* much harder to reason about correctness. Suddenly locks don't guarantee what they used to anymore. We'll e.g. not be able to rely that a CheckTableNotInUse() + AEL makes it safe to drop/rewrite/whatever a relation - even if that happens in the main backend. Haven't I responded to this a few times already? Not in a particularly convincing way at least. I see no way, even theoretically, that it can be sane for CheckTableNotInUse() to succeed in a parallel context. Ever. It's not exactly inconceivable that you'd want a parallel CLUSTER/REINDEX/VACUUM or something similar. That'll require cooperating backends, true, but it's still a pretty sane case. You haven't really made your case why you want to allow access to AEL locked relations in a parallel context in the first place. If the deadlock detector would kill the processes anyway, it doesn't matter, because CheckTableNotInUse() should do it first, so that we get a better error and without waiting for deadlock_timeout. So any scenario that's predicated on the assumption that CheckTableNotInUse() will succeed in a parallel context is 100% unconvincing to me as an argument for anything. Meh, there's plenty cases where CheckTableNotInUse() isn't used where we still rely on locks actually working. And it's not just postgres code, this *will* break user code. Your argument essentially is that we don't actually need to lock objects if the other side only reads. Yes, you add a condition or two ontop (e.g. CheckTableNotInUse() fails), but that's not that much. If we want to do this we really need to forbid *any* modification to catalog/user tables while parallel operations are ongoing. In the primary and worker backends. I think that's going to end up being much more problematic/restrictive than not allowing non-cooperative paralellism after = ShareUpdateExclusive. If we ever want to parallelize writing operations we'll regret this quite a bit. Breaking the locking system - and that's what you're doing by removing the usual guarantees - seems just fundamentally wrong. Yes, I can't name all the dangers that I think are out there. The 'lets grant all the current locks at start of parallel query' approach seems quite a bit safer than always doing that during parallel query, don't get me wrong. Btw, what are your thoughts about SERIALIZABLE and parallel query? Afaics that'd be pretty hard to support? 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 Thu, Nov 20, 2014 at 12:14 PM, Robert Haas robertmh...@gmail.com wrote: Seems fine to me. If you typed relid rather than indexrelid or key rather than indkey, that's a thinko, not a typo. ikey for indkey could plausible be a typo, though you'd have to be having a fairly bad day at the keyboard. I can tell that I have no chance of convincing you otherwise. While I think you're mistaken to go against the precedent set by git, you're the one with the commit bit, and I think we've already spent enough time discussing this. So default costings it is. -- 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 2014-11-20 12:00:51 -0800, Peter Geoghegan wrote: In more concrete terms, this gets no suggestion: postgres=# select key from pg_index; ERROR: 42703: column key does not exist LINE 1: select key from pg_index; ^ I don't think that's a bad thing. Yes, for a human those look pretty similar, but it's easy to construct cases where that gives completely hilarious results. I think something simplistic like levenshtein, even with modified distances, is good to catch typos. But not to find terms that are related in more complex ways. 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()
Peter Geoghegan p...@heroku.com writes: On Thu, Nov 20, 2014 at 11:03 AM, Robert Haas robertmh...@gmail.com wrote: That does seem to give better results, but it still seems awfully complicated. If we just used Levenshtein with all-default cost factors and a distance cap equal to Max(strlen(what_user_typed), strlen(candidate_match), 3), what cases that you think are important would be harmed? Well, just by plugging in default Levenshtein cost factors, I can see the following regression: *** /home/pg/postgresql/src/test/regress/expected/join.out 2014-11-20 10:17:55.042291912 -0800 --- /home/pg/postgresql/src/test/regress/results/join.out 2014-11-20 11:42:15.670108745 -0800 *** *** 3452,3458 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. I do not have a problem with deciding that that is not a regression; in fact, not giving that hint seems like a good conservative behavior here. By your logic, we should also be prepared to suggest supercalifragilisticexpialidocious when the user enters ocious. It's simply a bridge too far for what is supposed to be a hint for minor typos. You sound like you want to turn it into something that will look up column names for people who are too lazy to even try to type the right thing. While I can see the value of such a tool within certain contexts, firing completed queries at a live SQL engine is not one of them. 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] pg_multixact not getting truncated
So that we can have two ways to lose data? Forbidding connections to a database doesn't prevent XID or MXID wraparound. It does prevent the user from doing anything about it, though, since they can't manually vacuum template0 without knowing unpublished hackery. -- 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] Doing better at HINTing an appropriate column within errorMissingColumn()
On Thu, Nov 20, 2014 at 12:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: I do not have a problem with deciding that that is not a regression; in fact, not giving that hint seems like a good conservative behavior here. By your logic, we should also be prepared to suggest supercalifragilisticexpialidocious when the user enters ocious. That's clearly not true. I just want to be a bit more forgiving of omissions. That clearly isn't my logic, since that isn't a suggestion that the implementation will give, or would be anywhere close to giving - my weighing of deletions is only twice that of substitutions or insertions, not ten times. git does not use Levenshtein default costings either. It's simply a bridge too far for what is supposed to be a hint for minor typos. Minor typos and minor omissions. My example was on the edge of what would be tolerable under my proposed cost model. You sound like you want to turn it into something that will look up column names for people who are too lazy to even try to type the right thing. While I can see the value of such a tool within certain contexts, firing completed queries at a live SQL engine is not one of them. It's just a hint; a convenience. Users who imagine that it takes away the need for putting any thought into their SQL queries have bigger problems. Anyway, that's all that needs to be said on that, since I've already given up on a non-default costing. Also, we have default costing, and we always apply a 50% standard (I see no point in doing otherwise with default costings). Robert: Where does that leave us? What about suggestions across RTEs? Alias costing, etc? -- 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] group locking: incomplete patch, just for discussion
On Thu, Nov 20, 2014 at 3:15 PM, Andres Freund and...@2ndquadrant.com wrote: Haven't I responded to this a few times already? Not in a particularly convincing way at least. That's not a very helpful response. I see no way, even theoretically, that it can be sane for CheckTableNotInUse() to succeed in a parallel context. Ever. It's not exactly inconceivable that you'd want a parallel CLUSTER/REINDEX/VACUUM or something similar. That'll require cooperating backends, true, but it's still a pretty sane case. You haven't really made your case why you want to allow access to AEL locked relations in a parallel context in the first place. Well, what the heck was I doing here? http://www.postgresql.org/message-id/ca+tgmoa_+u-qccfz9zavzfk+tgxbqqb1+0ktpzrggpj5pvr...@mail.gmail.com I've *repeatedly* point out that if you *don't* allow this, stuff breaks. And you haven't answered how you'd like to fix that. The only answer you've given so far, that I can see, is that maybe we can foresee all the locks that the child might take and not initiate parallelism if any of them conflict with locks we already hold. But that's not a reasonable approach; we have no reasonable way of predicting what locks the child will need, and even if we did, things can change between plan time and execution time. If the deadlock detector would kill the processes anyway, it doesn't matter, because CheckTableNotInUse() should do it first, so that we get a better error and without waiting for deadlock_timeout. So any scenario that's predicated on the assumption that CheckTableNotInUse() will succeed in a parallel context is 100% unconvincing to me as an argument for anything. Meh, there's plenty cases where CheckTableNotInUse() isn't used where we still rely on locks actually working. And it's not just postgres code, this *will* break user code. It would help if you'd provide some specific examples of the problems you foresee. In the absence of specificity, it's hard to tell come to any conclusions about whether your concerns are well-founded, or propose any way to overcome them. Your argument essentially is that we don't actually need to lock objects if the other side only reads. Yes, you add a condition or two ontop (e.g. CheckTableNotInUse() fails), but that's not that much. I don't think that's my argument at all. What I'm arguing is that there's more than one reason for taking locks. Broadly, I think we take some locks to preserve transaction isolation guarantees and others to preserve data integrity. If you're adding or dropping a column, nobody else had better be looking at that relation, because they might get confused and crash the server or something. Backends, even cooperating parallel backends, won't be able to cope with the tuple descriptor changing under them. But once that operation is complete, there's no reason why the *next* statement in that transaction can't see the relation you just modified, even though it's still now exclusively locked. As long as we make sure that new parallel backends use our snapshot, they'll see the right metadata for that relation and can safely access it. We still can't let *other* people see that relation because the ALTER TABLE is an uncommitted change, but that's no argument against letting our own parallel workers look at it. If we want to do this we really need to forbid *any* modification to catalog/user tables while parallel operations are ongoing. In the primary and worker backends. I think that's going to end up being much more problematic/restrictive than not allowing non-cooperative paralellism after = ShareUpdateExclusive. If we ever want to parallelize writing operations we'll regret this quite a bit. I can't really follow how this follows from anything I said. Breaking the locking system - and that's what you're doing by removing the usual guarantees - seems just fundamentally wrong. Yes, I can't name all the dangers that I think are out there. The 'lets grant all the current locks at start of parallel query' approach seems quite a bit safer than always doing that during parallel query, don't get me wrong. Lets grant all the current locks at start of parallel query is the proposal I'm focused on right now. Based on your arguments and those of others, I have long-since abandoned the original proposal of having locks within a group never conflict. It seemed like a good idea at the time, but it would put way too much burden on the parallel workers to coordinate their activity with each other, so it's dead. What I want to establish at this time is what types of problems, if any, you or others can foresee with Lets grant all the current locks at start of parallel query - because I sense considerable skepticism, but cannot put my finger on anything concretely wrong with it. Btw, what are your thoughts about SERIALIZABLE and parallel query? Afaics that'd be pretty hard to support? Hmm, I haven't thought about that.
Re: [HACKERS] Doing better at HINTing an appropriate column within errorMissingColumn()
On Thu, Nov 20, 2014 at 3:20 PM, Peter Geoghegan p...@heroku.com wrote: On Thu, Nov 20, 2014 at 12:14 PM, Robert Haas robertmh...@gmail.com wrote: Seems fine to me. If you typed relid rather than indexrelid or key rather than indkey, that's a thinko, not a typo. ikey for indkey could plausible be a typo, though you'd have to be having a fairly bad day at the keyboard. I can tell that I have no chance of convincing you otherwise. While I think you're mistaken to go against the precedent set by git, you're the one with the commit bit, and I think we've already spent enough time discussing this. So default costings it is. I've got a few +1s, too, if you notice. I'm willing to be outvoted, but not by a majority of one. -- 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 Thu, Nov 20, 2014 at 12:50 PM, Robert Haas robertmh...@gmail.com wrote: I've got a few +1s, too, if you notice. Then maybe I spoke too soon. -- 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] pg_multixact not getting truncated
On Thu, Nov 20, 2014 at 3:44 PM, Josh Berkus j...@agliodbs.com wrote: So that we can have two ways to lose data? Forbidding connections to a database doesn't prevent XID or MXID wraparound. It does prevent the user from doing anything about it, though, since they can't manually vacuum template0 without knowing unpublished hackery. True. I don't know what to do about that. Do you? -- 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_multixact not getting truncated
Robert Haas wrote: On Thu, Nov 20, 2014 at 3:44 PM, Josh Berkus j...@agliodbs.com wrote: So that we can have two ways to lose data? Forbidding connections to a database doesn't prevent XID or MXID wraparound. It does prevent the user from doing anything about it, though, since they can't manually vacuum template0 without knowing unpublished hackery. True. I don't know what to do about that. Do you? Maybe tweak autovacuum so that it vacuum-freezes the non-connectable template databases when they are multixact_freeze_min_age old -- or something similar. That would cause the multixact age to go down to zero for those databases with enough frequency. -- Á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/20/2014 01:03 PM, Robert Haas wrote: On Thu, Nov 20, 2014 at 3:44 PM, Josh Berkus j...@agliodbs.com wrote: So that we can have two ways to lose data? Forbidding connections to a database doesn't prevent XID or MXID wraparound. It does prevent the user from doing anything about it, though, since they can't manually vacuum template0 without knowing unpublished hackery. True. I don't know what to do about that. Do you? Well, the first thing that comes to mind is that template0 should be permanently frozen. That is, all objects in it should be created with frozen xid and mxids. After all, nobody can modify anything in it. -- 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] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Wed, Nov 19, 2014 at 10:37 PM, Anssi Kääriäinen anssi.kaariai...@thl.fi wrote: I think the biggest problem with the current approach is that there is no way to know if a row was skipped by the where clause when using INSERT ON CONFLICT UPDATE ... WHERE. Well, there could have always been an UPDATE in a trigger or something like that. I am a developer of the Django ORM. Django reports to the user whether a row was inserted or updated. It is possible to know which rows were inserted by returning the primary key value. If something is returned, then it was an insert. If Django implements updated vs inserted checking this way, then if PostgreSQL adds RETURNING for update case later on, that would be a breaking change for Django. How does that actually work at the moment? Do you use RETURNING, or look at the command tag? Would you be happy to just know that certain rows were either inserted or updated in the context of an UPSERT (and not cancelled by a BEFORE ROW INSERT or UPDATE trigger returning NULL), or do you want to specifically know if there was an insert or an update in respect of each row/slot processed? -- 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] group locking: incomplete patch, just for discussion
On 2014-11-20 15:47:39 -0500, Robert Haas wrote: On Thu, Nov 20, 2014 at 3:15 PM, Andres Freund and...@2ndquadrant.com wrote: Haven't I responded to this a few times already? Not in a particularly convincing way at least. That's not a very helpful response. Well... I see no way, even theoretically, that it can be sane for CheckTableNotInUse() to succeed in a parallel context. Ever. It's not exactly inconceivable that you'd want a parallel CLUSTER/REINDEX/VACUUM or something similar. That'll require cooperating backends, true, but it's still a pretty sane case. You haven't really made your case why you want to allow access to AEL locked relations in a parallel context in the first place. Well, what the heck was I doing here? http://www.postgresql.org/message-id/ca+tgmoa_+u-qccfz9zavzfk+tgxbqqb1+0ktpzrggpj5pvr...@mail.gmail.com I don't see anything about actual use cases there. I've *repeatedly* point out that if you *don't* allow this, stuff breaks. You've pointed out some strange corner cases that might break yes. And you haven't answered how you'd like to fix that. The only answer you've given so far, that I can see, is that maybe we can foresee all the locks that the child might take and not initiate parallelism if any of them conflict with locks we already hold. But that's not a reasonable approach; we have no reasonable way of predicting what locks the child will need, and even if we did, things can change between plan time and execution time. We don't need to predict all future locks. We only need to check which self-exclusive locks we are already holding. I don't buy the plan time/execution time argument. We'll need to be able to adapt plans to the availability of bgworker slots *anyway*. Otherwise we either need to to set the number of bgworkers to MaxConnections * MaxParallelism or live with errors as soon as too many processes use parallelism. The former is clearly unrealistic given PG's per-backend overhead and the latter will be a *much* bigger PITA than all the deadlock scenarios talked about here. It'll constantly fail, even if everything is ok. If the deadlock detector would kill the processes anyway, it doesn't matter, because CheckTableNotInUse() should do it first, so that we get a better error and without waiting for deadlock_timeout. So any scenario that's predicated on the assumption that CheckTableNotInUse() will succeed in a parallel context is 100% unconvincing to me as an argument for anything. Meh, there's plenty cases where CheckTableNotInUse() isn't used where we still rely on locks actually working. And it's not just postgres code, this *will* break user code. It would help if you'd provide some specific examples of the problems you foresee. In the absence of specificity, it's hard to tell come to any conclusions about whether your concerns are well-founded, or propose any way to overcome them. How about a frontend process having created a relation that then starts a parallel query. Then the frontend process ERRORs out and, in the course of that, does smgrDoPendingDeletes(). Which will delete the new relation. Boom. The background process might just be accessing it. If you think thats harmless, think e.g. what'd happen with heap cleanup records generated in the background process. They'd not replay. Another one is that the frontend process does, from some function or so, CREATE POLICY. Triggering a relcache flush. And RelationClearRelation() essentially assumes that a referenced relcache entry can't change (which is the reason the keep_* stuff is in RelationClearRelation()). I'm pretty sure there's at least a dozen other such hazards around. Your argument essentially is that we don't actually need to lock objects if the other side only reads. Yes, you add a condition or two ontop (e.g. CheckTableNotInUse() fails), but that's not that much. I don't think that's my argument at all. What I'm arguing is that there's more than one reason for taking locks. Broadly, I think we take some locks to preserve transaction isolation guarantees and others to preserve data integrity. If you're adding or dropping a column, nobody else had better be looking at that relation, because they might get confused and crash the server or something. Backends, even cooperating parallel backends, won't be able to cope with the tuple descriptor changing under them. But once that operation is complete, there's no reason why the *next* statement in that transaction can't see the relation you just modified, even though it's still now exclusively locked. As long as we make sure that new parallel backends use our snapshot, they'll see the right metadata for that relation and can safely access it. We still can't let *other* people see that relation because the ALTER TABLE is an uncommitted change, but that's no argument against letting our own parallel workers look at it. I think the transaction
[HACKERS] Re: Doing better at HINTing an appropriate column within errorMissingColumn()
Andres Freund-3 wrote I think something simplistic like levenshtein, even with modified distances, is good to catch typos. But not to find terms that are related in more complex ways. Tom Lane-2 wrote The big picture is that this is more or less our first venture into heuristic suggestions. I think we should start slow with a very conservative set of heuristics. If it's a success maybe we can get more aggressive over time --- but if we go over the top here, the entire concept will be discredited in this community for the next ten years. +1 for both of these conclusions. The observations regarding standard column prefixes and thinking that abbreviations are in use when in fact the names are spelled out are indeed in-the-wild behaviors that should be considered but a levenshtein distance algorithm is likely not going to be useful in pointing out mistakes in those situations. Limiting the immediate focus to fat/thin-fingering of keys - for which levenshtein is well suited - is useful and will provide data points that can then guide future artificial intelligence endeavors. David J. -- View this message in context: http://postgresql.nabble.com/Doing-better-at-HINTing-an-appropriate-column-within-errorMissingColumn-tp5797700p5827786.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.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 j...@agliodbs.com writes: Well, the first thing that comes to mind is that template0 should be permanently frozen. That is, all objects in it should be created with frozen xid and mxids. After all, nobody can modify anything in it. That sounds about as unsafe as can be. You can't stop superusers from connecting to template0 and modifying it if they want to ... and I don't really want to say ok, the consequence of that is silent disaster many moons later. 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] superuser() shortcuts
On 11/5/14 5:10 PM, Adam Brightwell wrote: Attached is two separate patches to address previous comments/recommendations: * superuser-cleanup-shortcuts_11-5-2014.patch Seeing that the regression tests had to be changed in this patch indicates that there is a change of functionality, even though this patch was previously discussed as merely an internal cleanup. So either there is a user-facing change, which would need to be documented (or at least mentioned, discussed, and dismissed as minor), or there is a fault in the tests, which should be fixed first independently. Which makes me wonder whether the other changes are indeed without effect or just not covered by tests. * has_privilege-cleanup_11-5-2014.patch I don't really see the merit of this patch. A cleanup patch that adds more code than it removes isn't really a cleanup. If you want to make the APIs consistent, then let's make a patch for that. If you want to make the error messages consistent, then let's make a patch for that. There is other work going on replacing these role attributes with something more general, so maybe this cleanup should be done as part of that other work. -- 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] superuser() shortcuts
On 2014-11-05 17:10:17 -0500, Adam Brightwell wrote:
Attached is two separate patches to address previous
comments/recommendations:
* superuser-cleanup-shortcuts_11-5-2014.patch
* has_privilege-cleanup_11-5-2014.patch
-Adam
--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/contrib/test_decoding/expected/permissions.out
b/contrib/test_decoding/expected/permissions.out
new file mode 100644
index 212fd1d..f011955
*** a/contrib/test_decoding/expected/permissions.out
--- b/contrib/test_decoding/expected/permissions.out
*** RESET ROLE;
*** 54,66
-- plain user *can't* can control replication
SET ROLE lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot',
'test_decoding');
! ERROR: must be superuser or replication role to use replication slots
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for relation lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');
! ERROR: must be superuser or replication role to use replication slots
SELECT pg_drop_replication_slot('regression_slot');
! ERROR: must be superuser or replication role to use replication slots
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE lr_superuser;
--- 54,69
-- plain user *can't* can control replication
SET ROLE lr_normal;
SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot',
'test_decoding');
! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.
INSERT INTO lr_test VALUES('lr_superuser_init');
ERROR: permission denied for relation lr_test
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL,
'include-xids', '0', 'skip-empty-xacts', '1');
! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.
SELECT pg_drop_replication_slot('regression_slot');
! ERROR: permission denied to use replication slots
! HINT: You must be superuser or replication role to use replication slots.
RESET ROLE;
-- replication users can drop superuser created slots
SET ROLE lr_superuser;
I still think this change makes the error message more verbose, without
any win in clarity.
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] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Thu, Nov 20, 2014 at 1:42 PM, Peter Geoghegan p...@heroku.com wrote: Would you be happy to just know that certain rows were either inserted or updated in the context of an UPSERT (and not cancelled by a BEFORE ROW INSERT or UPDATE trigger returning NULL) Of course, having the WHERE clause in the auxiliary UPDATE not pass would also be cause to *not* return/project the not-processed row/slot (in a world where we do something with RETURNING in respect of rows actually processed by the auxiliary UPDATE). I mean, you're seeing the final version of the row when RETURNING with an UPDATE, and if the UPDATE is never evaluated, then the would-be final version (which is generally based on the TARGET tuple and EXLCUDED tuple, as processed by the UPDATE) never exists, and so clearly cannot be projected by RETURNING. This explanation a tiny bit misleading, because the rows/slots not affected by the UPDATE (or INSERT) are still *locked*, even when the UPDATE's WHERE clause does not pass - they have been processed to the extent that they were locked. This is also true of postgres_fdw in certain situations, but it seems like a very minor issue. -- 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] 9.5: Better memory accounting, towards memory-bounded HashAgg
On 2014-11-17 21:03:07 +0100, Tomas Vondra wrote:
On 17.11.2014 19:46, Andres Freund wrote:
On 2014-11-17 19:42:25 +0100, Tomas Vondra wrote:
On 17.11.2014 18:04, Andres Freund wrote:
Hi,
On 2014-11-16 23:31:51 -0800, Jeff Davis wrote:
*** a/src/include/nodes/memnodes.h
--- b/src/include/nodes/memnodes.h
***
*** 60,65 typedef struct MemoryContextData
--- 60,66
MemoryContext nextchild;/* next child of same parent */
char *name; /* context name (just
for debugging) */
boolisReset;/* T = no space alloced
since last reset */
+uint64 mem_allocated; /* track memory allocated for
this context */
#ifdef USE_ASSERT_CHECKING
boolallowInCritSection; /* allow palloc in
critical section */
#endif
That's quite possibly one culprit for the slowdown. Right now one
AllocSetContext struct fits precisely into three cachelines. After
your change it doesn't anymore.
I'm no PPC64 expert, but I thought the cache lines are 128 B on that
platform, since at least Power6?
Hm, might be true.
Also, if I'm counting right, the MemoryContextData structure is 56B
without the 'mem_allocated' field (and without the allowInCritSection),
and 64B with it (at that particular place). sizeof() seems to confirm
that. (But I'm on x86, so maybe the alignment on PPC64 is different?).
The MemoryContextData struct is embedded into AllocSetContext.
Oh, right. That makes is slightly more complicated, though, because
AllocSetContext adds 6 x 8B fields plus an in-line array of freelist
pointers. Which is 11x8 bytes. So in total 56+56+88=200B, without the
additional field. There might be some difference because of alignment,
but I still don't see how that one additional field might impact cachelines?
It's actually 196 bytes:
struct AllocSetContext {
MemoryContextData header; /* 056 */
AllocBlock blocks; /*56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
AllocChunk freelist[11]; /*6488 */
/* --- cacheline 2 boundary (128 bytes) was 24 bytes ago --- */
Size initBlockSize;/* 152 8 */
Size maxBlockSize; /* 160 8 */
Size nextBlockSize;/* 168 8 */
Size allocChunkLimit; /* 176 8 */
AllocBlock keeper; /* 184 8 */
/* --- cacheline 3 boundary (192 bytes) --- */
/* size: 192, cachelines: 3, members: 8 */
};
And thus one additional field tipps it over the edge.
pahole is a very useful utility.
But if we separated the freelist, that might actually make it faster, at
least for calls that don't touch the freelist at all, no? Because most
of the palloc() calls will be handled from the current block.
I seriously doubt it. The additional indirection + additional branches
are likely to make it worse.
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] tracking commit timestamps
On 19/11/14 17:30, Steve Singer wrote: 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. I did that then, v9 attached with following changes: - removed lsn info (obviously) - the pg_xact_commit_timestamp and pg_last_committed_xact return NULLs when timestamp data was not found - made the default nodeid crash safe - this also makes use of the do_xlog parameter in TransactionTreeSetCommitTsData if nodeid is set, although that still does not happen without extension actually using the API - added some more regression tests - some small comment and docs adjustments based on Michael's last email I didn't change the pg_last_committed_xact function name and I didn't make nodeid visible from SQL level interfaces and don't plan to in this patch as I think it's very premature to do so before we have some C code using it. Just to explain once more and hopefully more clearly how the crash safety/WAL logging is handled since there was some confusion in last review: We only do WAL logging when nodeid is also logged (when nodeid is not 0) because the timestamp itself can be read from WAL record of transaction commit so it's pointless to log another WAL record just to store the timestamp again. Extension can either set default nodeid which is then logged transparently, or can override the default logging mechanism by calling TransactionTreeSetCommitTsData with whatever data it wants and do_xlog set to true which will then write the WAL record with this overriding information. During WAL replay the commit timestamp is set from transaction commit record and then if committs WAL record is found it's used to overwrite the commit timestamp/nodeid for given xid. Also, there is exactly one record in SLRU for each xid so there is no point in making the SQL interfaces return multiple results. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/contrib/Makefile b/contrib/Makefile index b37d0dd..e331297 100644 --- a/contrib/Makefile +++ b/contrib/Makefile @@ -50,6 +50,7 @@ SUBDIRS = \ spi \ tablefunc \ tcn \ + test_committs \ test_decoding \ test_parser \ test_shm_mq \ diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c index 3b8241b..f0a023f 100644 --- a/contrib/pg_upgrade/pg_upgrade.c +++ b/contrib/pg_upgrade/pg_upgrade.c @@ -423,8 +423,10 @@ copy_clog_xlog_xid(void) /* set the next transaction id and epoch of the new cluster */ prep_status(Setting next transaction ID and epoch for new cluster); exec_prog(UTILITY_LOG_FILE, NULL, true, - \%s/pg_resetxlog\ -f -x %u \%s\, - new_cluster.bindir, old_cluster.controldata.chkpnt_nxtxid, + \%s/pg_resetxlog\ -f -x %u -c %u \%s\, + new_cluster.bindir, + old_cluster.controldata.chkpnt_nxtxid, + old_cluster.controldata.chkpnt_nxtxid, new_cluster.pgdata); exec_prog(UTILITY_LOG_FILE, NULL, true, \%s/pg_resetxlog\ -f -e %u \%s\, diff --git a/contrib/pg_xlogdump/rmgrdesc.c b/contrib/pg_xlogdump/rmgrdesc.c index 9397198..e0af3cf 100644 --- a/contrib/pg_xlogdump/rmgrdesc.c +++ b/contrib/pg_xlogdump/rmgrdesc.c @@ -10,6 +10,7 @@ #include access/brin_xlog.h #include access/clog.h +#include access/committs.h #include access/gin.h #include access/gist_private.h #include access/hash.h diff --git a/contrib/test_committs/.gitignore b/contrib/test_committs/.gitignore new file mode 100644 index 000..1f95503 --- /dev/null +++ b/contrib/test_committs/.gitignore @@ -0,0 +1,5 @@ +# Generated subdirectories +/log/ +/isolation_output/ +/regression_output/ +/tmp_check/ diff --git a/contrib/test_committs/Makefile b/contrib/test_committs/Makefile new file mode 100644 index 000..2240749 --- /dev/null +++ b/contrib/test_committs/Makefile @@ -0,0 +1,45 @@ +# Note: because we don't tell the Makefile there are any regression tests, +# we have to clean those result files explicitly +EXTRA_CLEAN = $(pg_regress_clean_files) ./regression_output ./isolation_output + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = contrib/test_committs +top_builddir = ../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif + +# We can't support installcheck because normally installcheck users don't have +# the required track_commit_timestamp on +installcheck:; + +check: regresscheck + +submake-regress: + $(MAKE) -C
Re: [HACKERS] [v9.5] Custom Plan API
Robert Haas robertmh...@gmail.com writes: I've committed parts 1 and 2 of this, without the documentation, and with some additional cleanup. I am not sure that this feature is sufficiently non-experimental that it deserves to be documented, but if we're thinking of doing that then the documentation needs a lot more work. I think part 3 of the patch is mostly useful as a demonstration of how this API can be used, and is not something we probably want to commit. So I'm not planning, at this point, to spend any more time on this patch series, and will mark it Committed in the CF app. I've done some preliminary cleanup on this patch, but I'm still pretty desperately unhappy about some aspects of it, in particular the way that it gets custom scan providers directly involved in setrefs.c, finalize_primnode, and replace_nestloop_params processing. I don't want any of that stuff exported outside the core, as freezing those APIs would be a very nasty restriction on future planner development. What's more, it doesn't seem like doing that creates any value for custom-scan providers, only a requirement for extra boilerplate code for them to provide. ISTM that we could avoid that by borrowing the design used for FDW plans, namely that any expressions you would like planner post-processing services for should be stuck into a predefined List field (fdw_exprs for the ForeignScan case, perhaps custom_exprs for the CustomScan case?). This would let us get rid of the SetCustomScanRef and FinalizeCustomScan callbacks as well as simplify the API contract for PlanCustomPath. I'm also wondering why this patch didn't follow the FDW lead in terms of expecting private data to be linked from specialized private fields. The design as it stands (with an expectation that CustomPaths, CustomPlans etc would be larger than the core code knows about) is not awful, but it seems just randomly different from the FDW precedent, and I don't see a good argument why it should be. If we undid that we could get rid of CopyCustomScan callbacks, and perhaps also TextOutCustomPath and TextOutCustomScan (though I concede there might be some argument to keep the latter two anyway for debugging reasons). Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism added to ruleutils.c is anything but dead weight that we'll have to maintain forever. It seems somewhat unlikely that anyone will figure out how to use it, or indeed that it can be used for anything very interesting. I suppose the argument for it is that you could stick custom vars into the tlist of a CustomScan plan node, but you cannot, at least not without a bunch of infrastructure that isn't there now; in particular how would such an expression ever get matched by setrefs.c to higher-level plan tlists? I think we should rip that out and wait to see a complete use-case before considering putting it back. Comments? regards, tom lane PS: with no documentation it's arguable that the entire patch is just dead weight. I'm not very happy that it went in without any. -- 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] group locking: incomplete patch, just for discussion
On Thu, Nov 20, 2014 at 4:45 PM, Andres Freund and...@2ndquadrant.com wrote: And you haven't answered how you'd like to fix that. The only answer you've given so far, that I can see, is that maybe we can foresee all the locks that the child might take and not initiate parallelism if any of them conflict with locks we already hold. But that's not a reasonable approach; we have no reasonable way of predicting what locks the child will need, and even if we did, things can change between plan time and execution time. We don't need to predict all future locks. We only need to check which self-exclusive locks we are already holding. I can't follow that logic. Why do you think self-exclusive locks are the only ones that matter? I don't buy the plan time/execution time argument. We'll need to be able to adapt plans to the availability of bgworker slots *anyway*. Otherwise we either need to to set the number of bgworkers to MaxConnections * MaxParallelism or live with errors as soon as too many processes use parallelism. The former is clearly unrealistic given PG's per-backend overhead and the latter will be a *much* bigger PITA than all the deadlock scenarios talked about here. It'll constantly fail, even if everything is ok. I certainly think that any parallel node needs to be able to cope with getting fewer workers than it would ideally like to have. But that's different from saying that when our own backend takes new locks, we have to invalidate plans. Those seem like largely unrelated problems. If the deadlock detector would kill the processes anyway, it doesn't matter, because CheckTableNotInUse() should do it first, so that we get a better error and without waiting for deadlock_timeout. So any scenario that's predicated on the assumption that CheckTableNotInUse() will succeed in a parallel context is 100% unconvincing to me as an argument for anything. Meh, there's plenty cases where CheckTableNotInUse() isn't used where we still rely on locks actually working. And it's not just postgres code, this *will* break user code. It would help if you'd provide some specific examples of the problems you foresee. In the absence of specificity, it's hard to tell come to any conclusions about whether your concerns are well-founded, or propose any way to overcome them. How about a frontend process having created a relation that then starts a parallel query. Then the frontend process ERRORs out and, in the course of that, does smgrDoPendingDeletes(). Which will delete the new relation. Boom. The background process might just be accessing it. If you think thats harmless, think e.g. what'd happen with heap cleanup records generated in the background process. They'd not replay. OK, that's an excellent example. Thanks. I'll think about that. Another one is that the frontend process does, from some function or so, CREATE POLICY. Triggering a relcache flush. And RelationClearRelation() essentially assumes that a referenced relcache entry can't change (which is the reason the keep_* stuff is in RelationClearRelation()). I don't quite follow that one. Are you saying that the frontend would do a CREATE POLICY while there are backend workers running? I seriously doubt that can ever be made safe, but I'd choose to prohibit it using something other than the heavyweight lock manager. I think there's some local state as well, so that's probably not so easy. I guess this needs input from Kevin. Yeah. I'd be OK to have v1 disable parallelism at serializable, and fix that in v2, but of course if we can avoid that it's even better. The heavyweight locking issue is really killing me, though. -- 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] [v9.5] Custom Plan API
On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane t...@sss.pgh.pa.us wrote: I've done some preliminary cleanup on this patch, but I'm still pretty desperately unhappy about some aspects of it, in particular the way that it gets custom scan providers directly involved in setrefs.c, finalize_primnode, and replace_nestloop_params processing. I don't want any of that stuff exported outside the core, as freezing those APIs would be a very nasty restriction on future planner development. What's more, it doesn't seem like doing that creates any value for custom-scan providers, only a requirement for extra boilerplate code for them to provide. ISTM that we could avoid that by borrowing the design used for FDW plans, namely that any expressions you would like planner post-processing services for should be stuck into a predefined List field (fdw_exprs for the ForeignScan case, perhaps custom_exprs for the CustomScan case?). This would let us get rid of the SetCustomScanRef and FinalizeCustomScan callbacks as well as simplify the API contract for PlanCustomPath. Ah, that makes sense. I'm not sure I really understand what's so bad about the current system, but I have no issue with revising it for consistency. I'm also wondering why this patch didn't follow the FDW lead in terms of expecting private data to be linked from specialized private fields. The design as it stands (with an expectation that CustomPaths, CustomPlans etc would be larger than the core code knows about) is not awful, but it seems just randomly different from the FDW precedent, and I don't see a good argument why it should be. If we undid that we could get rid of CopyCustomScan callbacks, and perhaps also TextOutCustomPath and TextOutCustomScan (though I concede there might be some argument to keep the latter two anyway for debugging reasons). OK. Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism added to ruleutils.c is anything but dead weight that we'll have to maintain forever. It seems somewhat unlikely that anyone will figure out how to use it, or indeed that it can be used for anything very interesting. I suppose the argument for it is that you could stick custom vars into the tlist of a CustomScan plan node, but you cannot, at least not without a bunch of infrastructure that isn't there now; in particular how would such an expression ever get matched by setrefs.c to higher-level plan tlists? I think we should rip that out and wait to see a complete use-case before considering putting it back. I thought this was driven by a suggestion from you, but maybe KaiGai can comment. PS: with no documentation it's arguable that the entire patch is just dead weight. I'm not very happy that it went in without any. As I said, I wasn't sure we wanted to commit to the API enough to document it, and by the time you get done whacking the stuff above around, the documentation KaiGai wrote for it (which was also badly in need of editing by a native English speaker) would have been mostly obsolete anyway. But I'm willing to put some effort into it once you get done rearranging the furniture, if that's helpful. -- 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] group locking: incomplete patch, just for discussion
On 2014-11-20 20:21:07 -0500, Robert Haas wrote: On Thu, Nov 20, 2014 at 4:45 PM, Andres Freund and...@2ndquadrant.com wrote: And you haven't answered how you'd like to fix that. The only answer you've given so far, that I can see, is that maybe we can foresee all the locks that the child might take and not initiate parallelism if any of them conflict with locks we already hold. But that's not a reasonable approach; we have no reasonable way of predicting what locks the child will need, and even if we did, things can change between plan time and execution time. We don't need to predict all future locks. We only need to check which self-exclusive locks we are already holding. I can't follow that logic. Why do you think self-exclusive locks are the only ones that matter? All the others can be acquired by jumping in front of the lock's waitqueue? I don't buy the plan time/execution time argument. We'll need to be able to adapt plans to the availability of bgworker slots *anyway*. Otherwise we either need to to set the number of bgworkers to MaxConnections * MaxParallelism or live with errors as soon as too many processes use parallelism. The former is clearly unrealistic given PG's per-backend overhead and the latter will be a *much* bigger PITA than all the deadlock scenarios talked about here. It'll constantly fail, even if everything is ok. I certainly think that any parallel node needs to be able to cope with getting fewer workers than it would ideally like to have. But that's different from saying that when our own backend takes new locks, we have to invalidate plans. Those seem like largely unrelated problems. No need to invalidate them if you have the infrastructure to run with no parallel workers - just use the path that's used if there's no workers available. Another one is that the frontend process does, from some function or so, CREATE POLICY. Triggering a relcache flush. And RelationClearRelation() essentially assumes that a referenced relcache entry can't change (which is the reason the keep_* stuff is in RelationClearRelation()). I don't quite follow that one. Are you saying that the frontend would do a CREATE POLICY while there are backend workers running? Yes. I seriously doubt that can ever be made safe, but I'd choose to prohibit it using something other than the heavyweight lock manager. Right. It's perfectly fine to forbid it imo. But there's lots of places that have assumptions about the locking behaviour baked in, and the relevant bugs will be hard to find. I think there's some local state as well, so that's probably not so easy. I guess this needs input from Kevin. Yeah. I'd be OK to have v1 disable parallelism at serializable, and fix that in v2, but of course if we can avoid that it's even better. I don't have a problem with that either. The heavyweight locking issue is really killing me, though. I don't really understand why you're not content with just detecting deadlocks for now. Everything else seems like bells and whistles for later. 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] [v9.5] Custom Plan API
Robert Haas robertmh...@gmail.com writes: I've committed parts 1 and 2 of this, without the documentation, and with some additional cleanup. I am not sure that this feature is sufficiently non-experimental that it deserves to be documented, but if we're thinking of doing that then the documentation needs a lot more work. I think part 3 of the patch is mostly useful as a demonstration of how this API can be used, and is not something we probably want to commit. So I'm not planning, at this point, to spend any more time on this patch series, and will mark it Committed in the CF app. I've done some preliminary cleanup on this patch, but I'm still pretty desperately unhappy about some aspects of it, in particular the way that it gets custom scan providers directly involved in setrefs.c, finalize_primnode, and replace_nestloop_params processing. I don't want any of that stuff exported outside the core, as freezing those APIs would be a very nasty restriction on future planner development. What's more, it doesn't seem like doing that creates any value for custom-scan providers, only a requirement for extra boilerplate code for them to provide. ISTM that we could avoid that by borrowing the design used for FDW plans, namely that any expressions you would like planner post-processing services for should be stuck into a predefined List field (fdw_exprs for the ForeignScan case, perhaps custom_exprs for the CustomScan case?). This would let us get rid of the SetCustomScanRef and FinalizeCustomScan callbacks as well as simplify the API contract for PlanCustomPath. If core backend can know which field contains expression nodes but processed by custom-scan provider, FinalizedCustomScan might be able to rid. However, rid of SetCustomScanRef makes unavailable a significant use case I intend. In case when tlist contains complicated expression node (thus it takes many cpu cycles) and custom-scan provider has a capability to compute this expression node externally, SetCustomScanRef hook allows to replace this complicate expression node by a simple Var node that references a value being externally computed. Because only custom-scan provider can know how this pseudo varnode is mapped to the original expression, it needs to call the hook to assign correct varno/varattno. We expect it assigns a special vano, like OUTER_VAR, and it is solved with GetSpecialCustomVar. One other idea is, core backend has a facility to translate relationship between the original expression and the pseudo varnode according to the map information given by custom-scan provider. I'm also wondering why this patch didn't follow the FDW lead in terms of expecting private data to be linked from specialized private fields. The design as it stands (with an expectation that CustomPaths, CustomPlans etc would be larger than the core code knows about) is not awful, but it seems just randomly different from the FDW precedent, and I don't see a good argument why it should be. If we undid that we could get rid of CopyCustomScan callbacks, and perhaps also TextOutCustomPath and TextOutCustomScan (though I concede there might be some argument to keep the latter two anyway for debugging reasons). Yep, its original proposition last year followed the FDW manner. It has custom_private field to store the private data of custom-scan provider, however, I was suggested to change the current form because it added a couple of routines to encode / decode Bitmapset that may lead other encode / decode routines for other data types. I'm neutral for this design choice. Either of them people accept is better for me. Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism added to ruleutils.c is anything but dead weight that we'll have to maintain forever. It seems somewhat unlikely that anyone will figure out how to use it, or indeed that it can be used for anything very interesting. I suppose the argument for it is that you could stick custom vars into the tlist of a CustomScan plan node, but you cannot, at least not without a bunch of infrastructure that isn't there now; in particular how would such an expression ever get matched by setrefs.c to higher-level plan tlists? I think we should rip that out and wait to see a complete use-case before considering putting it back. As I described above, as long as core backend has a facility to manage the relationship between a pseudo varnode and complicated expression node, I think we can rid this callback. PS: with no documentation it's arguable that the entire patch is just dead weight. I'm not very happy that it went in without any. I agree with this. Is it a good to write up a wikipage to brush up the documentation draft? Thanks, -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription:
Re: [HACKERS] [v9.5] Custom Plan API
On Thu, Nov 20, 2014 at 7:10 PM, Tom Lane t...@sss.pgh.pa.us wrote: I've done some preliminary cleanup on this patch, but I'm still pretty desperately unhappy about some aspects of it, in particular the way that it gets custom scan providers directly involved in setrefs.c, finalize_primnode, and replace_nestloop_params processing. I don't want any of that stuff exported outside the core, as freezing those APIs would be a very nasty restriction on future planner development. What's more, it doesn't seem like doing that creates any value for custom-scan providers, only a requirement for extra boilerplate code for them to provide. ISTM that we could avoid that by borrowing the design used for FDW plans, namely that any expressions you would like planner post-processing services for should be stuck into a predefined List field (fdw_exprs for the ForeignScan case, perhaps custom_exprs for the CustomScan case?). This would let us get rid of the SetCustomScanRef and FinalizeCustomScan callbacks as well as simplify the API contract for PlanCustomPath. Ah, that makes sense. I'm not sure I really understand what's so bad about the current system, but I have no issue with revising it for consistency. I'm also wondering why this patch didn't follow the FDW lead in terms of expecting private data to be linked from specialized private fields. The design as it stands (with an expectation that CustomPaths, CustomPlans etc would be larger than the core code knows about) is not awful, but it seems just randomly different from the FDW precedent, and I don't see a good argument why it should be. If we undid that we could get rid of CopyCustomScan callbacks, and perhaps also TextOutCustomPath and TextOutCustomScan (though I concede there might be some argument to keep the latter two anyway for debugging reasons). OK. So, the existing form shall be revised as follows? * CustomScan shall not be a base type of custom data-type managed by extension, instead of private data field. * It also eliminates CopyCustomScan and TextOutCustomPath/Scan callback. * Expression nodes that will not be processed by core backend, but processed by extension shall be connected to special field, like fdw_exprs in FDW. * Translation between a pseudo varnode and original expression node shall be informed to the core backend, instead of SetCustomScanRef and GetSpecialCustomVar. Lastly, I'm pretty unconvinced that the GetSpecialCustomVar mechanism added to ruleutils.c is anything but dead weight that we'll have to maintain forever. It seems somewhat unlikely that anyone will figure out how to use it, or indeed that it can be used for anything very interesting. I suppose the argument for it is that you could stick custom vars into the tlist of a CustomScan plan node, but you cannot, at least not without a bunch of infrastructure that isn't there now; in particular how would such an expression ever get matched by setrefs.c to higher-level plan tlists? I think we should rip that out and wait to see a complete use-case before considering putting it back. I thought this was driven by a suggestion from you, but maybe KaiGai can comment. PS: with no documentation it's arguable that the entire patch is just dead weight. I'm not very happy that it went in without any. As I said, I wasn't sure we wanted to commit to the API enough to document it, and by the time you get done whacking the stuff above around, the documentation KaiGai wrote for it (which was also badly in need of editing by a native English speaker) would have been mostly obsolete anyway. But I'm willing to put some effort into it once you get done rearranging the furniture, if that's helpful. For people's convenient, I'd like to set up a wikipage to write up a draft of SGML documentation for easy updates by native English speakers. Thanks, -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.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
On 11/20/2014 01:47 PM, Tom Lane wrote: Josh Berkus j...@agliodbs.com writes: Well, the first thing that comes to mind is that template0 should be permanently frozen. That is, all objects in it should be created with frozen xid and mxids. After all, nobody can modify anything in it. That sounds about as unsafe as can be. You can't stop superusers from connecting to template0 and modifying it if they want to ... and I don't really want to say ok, the consequence of that is silent disaster many moons later. So it would get unfrozen when they modify it, and they'd have to deal with it. Right now we're optimizing for something only 0.1% of users ever do. The harder part of this -- the handwavy part -- is the whole idea of a permanent freeze. Right now there's no way to mark anything as frozen until next modifed, we're just resetting the clock on it. If there were any such thing, it would solve some of the problems around vacuum freeze. -- 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] WAL format and API changes (9.5)
On Thu, Nov 20, 2014 at 10:36 PM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
As you may have noticed, I committed this (after some more cleanup). Of
course, feel free to still review it, and please point out any issues you
may find.
Few minor observations:
1. Readme
void XLogResetInsertion(void)
Clear any currently registered data and buffers from the WAL record
construction workspace. This is only needed if you have already
called XLogBeginInsert(), but decide to not insert the record after all.
I think above sentence could be slightly rephrased as the this function is
also getting called at end of XLogInsert().
2.
shiftList()
{
..
XLogEnsureRecordSpace(data.ndeleted + 1, 0);
..
}
Shouldn't above function call to XLogEnsureRecordSpace() be done
under if (RelationNeedsWAL(rel))?
3.
XLogInsert(RmgrId rmid, uint8 info)
{
XLogRecPtr EndPos;
/* XLogBeginInsert() must have been called.
*/
if (!begininsert_called)
elog(ERROR, XLogBeginInsert was not called);
As we are in critical section at this moment, so is it okay to have
elog(ERROR,). I think this can only happen due to some coding
mistake, but still not sure if elog(ERROR) is okay.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
[HACKERS] Comment header for src/test/regress/regress.c
I thought it might be useful to add a few words at the top of 'src/test/regress/regress.c' to explain what it does and to help differentiate it from 'pg_regress.c' and 'pg_regress_main.c'. Regards Ian Barwick -- Ian Barwick http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c new file mode 100644 index 1487171..be27416 *** a/src/test/regress/regress.c --- b/src/test/regress/regress.c *** *** 1,5 ! /* * src/test/regress/regress.c */ #include postgres.h --- 1,17 ! /* ! * ! * regress.c ! * Code for various C-language functions defined as part of the ! * regression tests. ! * ! * This code is released under the terms of the PostgreSQL License. ! * ! * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group ! * Portions Copyright (c) 1994, Regents of the University of California ! * * src/test/regress/regress.c + * + *- */ #include postgres.h -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fillfactor for GIN indexes
Hi all,
Please find attached a simple patch adding fillfactor as storage parameter
for GIN indexes. The default value is the same as the one currently aka 100
to have the pages completely packed when a GIN index is created.
Note that to have this feature correctly working, the fix I sent yesterday
to set up isBuild for the entry insertion is needed (patch attached as well
here to facilitate the review):
http://www.postgresql.org/message-id/cab7npqsc4vq9mhkqm_yvafcteho-iuy8skbxydnmgnai1xn...@mail.gmail.com
Here are the results of some tests with a simple pg_trgm index on the
English translation of Les Miserables:
CREATE EXTENSION pg_trgm;
CREATE TABLE les_miserables (num serial, line text);
COPY les_miserables (line) FROM '/to/path/pg135.txt';
CREATE INDEX les_miserables_100 ON les_miserables USING gin (line
gin_trgm_ops);
CREATE INDEX les_miserables_40 ON les_miserables USING gin (line
gin_trgm_ops) with (fillfactor = 40);
CREATE INDEX les_miserables_20 ON les_miserables USING gin (line
gin_trgm_ops) with (fillfactor = 20);
CREATE INDEX les_miserables_80 ON les_miserables USING gin (line
gin_trgm_ops) with (fillfactor = 80);
CREATE INDEX les_miserables_10 ON les_miserables USING gin (line
gin_trgm_ops) with (fillfactor = 10);
SELECT relname, pg_size_pretty(pg_relation_size(oid)), reloptions FROM
pg_class where relname like 'les_miserables_%';
relname | pg_size_pretty | reloptions
++-
les_miserables_100 | 8256 kB| null
les_miserables_20 | 14 MB | {fillfactor=20}
les_miserables_40 | 11 MB | {fillfactor=40}
les_miserables_80 | 8712 kB| {fillfactor=80}
les_miserables_num_seq | 8192 bytes | null
(5 rows)
I am adding that to the commit fest of December.
Regards,
--
Michael
From eda0730d991f8b4dfbacc4d7a953ec5bff8b2ffe Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
Date: Fri, 21 Nov 2014 13:40:11 +0900
Subject: [PATCH 1/2] Fix flag marking GIN index as being built for new entries
This was somewhat missing in the current implementation, and leaded
to problems for code that needed special handling with fresh indexes
being built. Note that this does not impact current code as there are
no such operations being done yet but it may be a problem if in the
future a bug fix needs to make this distinction.
---
src/backend/access/gin/gininsert.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index c1ad0fd..c6d8b40 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -191,6 +191,7 @@ ginEntryInsert(GinState *ginstate,
buildStats-nEntries++;
ginPrepareEntryScan(btree, attnum, key, category, ginstate);
+ btree.isBuild = (buildStats != NULL);
stack = ginFindLeafPage(btree, false);
page = BufferGetPage(stack-buffer);
--
2.1.3
From eb48305ac0295fa4a46ffec5f8db447cd4c5f6b2 Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
Date: Fri, 21 Nov 2014 14:08:54 +0900
Subject: [PATCH 2/2] Support fillfactor for GIN indexes
Users can call this new storage parameter to fill in the entry and
leaf pages of a newly-built index as wanted. Fillfactor range varies
between 20 and 100.
---
doc/src/sgml/ref/create_index.sgml | 4 ++--
src/backend/access/common/reloptions.c | 9 +
src/backend/access/gin/gindatapage.c | 22 ++
src/backend/access/gin/ginentrypage.c | 20 +++-
src/backend/access/gin/ginutil.c | 3 ++-
src/include/access/gin_private.h | 3 +++
6 files changed, 53 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 6b2ee28..c0ba24a 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -294,8 +294,8 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] replaceable class=
para
The optional literalWITH/ clause specifies firsttermstorage
parameters/ for the index. Each index method has its own set of allowed
-storage parameters. The B-tree, hash, GiST and SP-GiST index methods all
-accept this parameter:
+storage parameters. The B-tree, hash, GIN, GiST and SP-GiST index methods
+all accept this parameter:
/para
variablelist
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index c16b38e..7137ba9 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -15,6 +15,7 @@
#include postgres.h
+#include access/gin_private.h
#include access/gist_private.h
#include access/hash.h
#include access/htup_details.h
@@ -133,6 +134,14 @@ static relopt_int intRelOpts[] =
},
{
{
+ fillfactor,
+ Packs gin index pages only to this percentage,
+ RELOPT_KIND_GIN
+ },
+ GIN_DEFAULT_FILLFACTOR, GIN_MIN_FILLFACTOR, 100
+ },
+ {
+ {
Re: [HACKERS] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Thu, 2014-11-20 at 13:42 -0800, Peter Geoghegan wrote: I am a developer of the Django ORM. Django reports to the user whether a row was inserted or updated. It is possible to know which rows were inserted by returning the primary key value. If something is returned, then it was an insert. If Django implements updated vs inserted checking this way, then if PostgreSQL adds RETURNING for update case later on, that would be a breaking change for Django. How does that actually work at the moment? Do you use RETURNING, or look at the command tag? Would you be happy to just know that certain rows were either inserted or updated in the context of an UPSERT (and not cancelled by a BEFORE ROW INSERT or UPDATE trigger returning NULL), or do you want to specifically know if there was an insert or an update in respect of each row/slot processed? Django uses the command tag currently to check if a row was updated. We also use RETURNING to get SERIAL values back from the database on insert. The most likely place to use this functionality in Django is Model.save(). This method is best defined as make sure this object's state is either inserted or updated to the database by the primary key of the object. The Model.save() method needs to also report if the model was created or updated. The command tag is sufficient for this case. So, the proposed feature now has everything Django needs for Model.save(). Django might add a bulk_merge(objs) command later on. This is defined as make sure each obj in objs is persisted to the database using the fastest way available. The INSERT ON CONFLICT UPDATE command looks excellent for this case. In this case it will be more problematic to check which rows were inserted, which update, as we need information for each primary key value separately for this case. When I think of this feature outside of Django, it seems it is completely reasonable to return database computed values on UPSERT. This requires two queries with the proposed API. My opinion is that RETURNING for the update case is better than not having it. - Anssi -- 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] WAL format and API changes (9.5)
On Fri, Nov 21, 2014 at 2:06 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: As you may have noticed, I committed this (after some more cleanup). Of course, feel free to still review it, and please point out any issues you may find. This comment on top of XLogRecordAssemble is not adapted as page_writes_omitted does not exist. Perhaps this is a remnant of some older version of the patch? * If there are any registered buffers, and a full-page image was not taken * of all them, *page_writes_omitted is set to true. This signals that the * assembled record is only good for insertion on the assumption that the * RedoRecPtr and doPageWrites values were up-to-date. */ -- Michael
Re: [HACKERS] alternative model for handling locking in parallel groups
On Wed, Nov 19, 2014 at 8:56 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Nov 18, 2014 at 8:53 AM, Amit Kapila amit.kapil...@gmail.com wrote: 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. Today, I have thought about this scheme a bit more and it seems that we can go through the local lock hash table (LockMethodLocalHash) to get the locks which user backend holds and then using Local lock tag we can form the similar hash table in worker. Apart from that, I think we need to get the information for fastpath locks from PGPROC and restore the same information for worker (here before restore we need to ensure that nobody else has moved that fast path lock to shared table, we can do that by checking the available fast path information against the user backend fast path lock information). For non-fastpath locks, I think we can search shared hash table (LockMethodLockHash) to obtain the lock information and assign the same to local lock and finally establish the proclock entry for parallel worker in LockMethodProcLockHash. In the above analysis, one point which slightly worries me is that for non-fastpath locks we need to obtain partitionLock which can be bottleneck as all parallel worker's needs to obtain the same and it can very well contend with any unrelated backend as well. 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. Okay, but there is one downside to this approach as well which is additional overhead of acquiring more number of locks which will be more severe as it has to be taken care every time for a parallel operation for each worker whereas in other approach there will be no such overhead. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
