Re: [HACKERS] Stating the significance of Lehman Yao in the nbtree README
On Fri, Sep 12, 2014 at 10:54 PM, Peter Geoghegan p...@heroku.com wrote: It isn't. It's a minor point, originally raised by Amit. I have observed that this patch is in 'Needs Review' state for next CF. Do you expect any further review from myside? I think we can use text recommended by Heikki and after that if you feel something more is still required, then you can update the same and send an updated patch. I believe it should be in 'Waiting On Author' state, please do let me know if you feel otherwise. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Stating the significance of Lehman Yao in the nbtree README
On Fri, Sep 26, 2014 at 11:34 PM, Amit Kapila amit.kapil...@gmail.com wrote: I have observed that this patch is in 'Needs Review' state for next CF. Do you expect any further review from myside? I think we can use text recommended by Heikki and after that if you feel something more is still required, then you can update the same and send an updated patch. I believe it should be in 'Waiting On Author' state, please do let me know if you feel otherwise. I don't think so. Thanks for the review! -- 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] LIMIT for UPDATE and DELETE
On 09/24/2014 05:22 AM, Etsuro Fujita wrote: (2014/09/17 1:58), Robert Haas wrote: On Tue, Sep 16, 2014 at 11:31 AM, Kevin Grittner kgri...@ymail.com wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/08/15 6:18), Rukh Meski wrote: Based on the feedback on my previous patch, I've separated only the LIMIT part into its own feature. This version plays nicely with inheritance. The intended use is splitting up big UPDATEs and DELETEs into batches more easily and efficiently. IIUC, the patch doesn't support OFFSET with UPDATE/DELETE ... LIMIT. Is that OK? When we support ORDER BY ... LIMIT/OFFSET, we will also be allowing for OFFSET with UPDATE/DELETE ... LIMIT. So, ISTM it would be better for the patch to support OFFSET at this point. No? Without ORDER BY you really would have no idea *which* rows the OFFSET would be skipping. Even more dangerously, you might *think* you do, and get a surprise when you see the results (if, for example, a seqscan starts at a point other than the start of the heap, due to a concurrent seqscan from an unrelated query). It might be better not to provide an illusion of a degree of control you don't have, especially for UPDATE and DELETE operations. Fair point, but I'd lean toward including it. I think we all agree the end goal is ORDER BY .. LIMIT, and there OFFSET certainly has meaning. If we don't include it now, we'll just end up adding it later. It makes for fewer patches, and fewer changes for users, if we do it all at once. I agree with Robert. Rukh, what do you think as an author? I have marked this as returned with feedback in the commitfest app. What I'd like to see happen next is: Rewrite how UPDATEs work, per Tom's suggestion here: http://www.postgresql.org/message-id/1598.1399826...@sss.pgh.pa.us. Then implement ORDER BY ... LIMIT on top of that. A lot of people would also be willing to settle for just implementing LIMIT without ORDER BY, as a stopgap measure. But the UPDATE rewrite is what would make everyone most happy. - 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] Turning off HOT/Cleanup sometimes
This patch has gotten a fair amount of review, and has been rewritten once during the commitfest. I think it's pretty close to being committable, the only remaining question seems to be what to do with system catalogs. I'm marking this as Returned with feedback, I take it that Simon can proceed from here, outside the commitfest. - 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] Scaling shared buffer eviction
Part of this patch was already committed, and the overall patch has had its fair share of review for this commitfest, so I'm marking this as Returned with feedback. The benchmark results for the bgreclaimer showed a fairly small improvement, so it doesn't seem like anyone's going to commit the rest of the patch soon, not without more discussion and testing anyway. Let's not hold up the commitfest for that. - 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] pg_receivexlog --status-interval add fsync feedback
On 09/05/2014 08:51 AM, furu...@pm.nttdata.co.jp wrote: Thanks for the review! I understand the attention message wasn't appropriate. To report the write location, even If you do not specify a replication slot. So the fix only appended messages. There was a description of the flush location section of '-S' option, but I intended to catch eye more and added a message. Is it better to make specification of the -S option indispensable? The patch cannot be applied to HEAD cleanly. Could you update the patch? Thank you for pointing out. Updated the patch. I don't understand what this patch does. When would you want to use the new --reply-fsync option? Is there any reason *not* to use it? In other words, do we need an option for this, couldn't you just always send the feedback message after fsync? - 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] Sloppy thinking about leakproof properties of opclass co-members
On 26 September 2014 15:48, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@gmail.com) wrote: On Wed, Sep 24, 2014 at 5:34 PM, Tom Lane t...@sss.pgh.pa.us wrote: ISTM that the most appropriate solution here is to insist that all or none of the members of an operator class be marked leakproof. (Possibly we could restrict that to btree opclasses, but I'm not sure any exception is needed for other index types.) I looked for existing violations of this precept, and unfortunately found a *lot*. For example, texteq is marked leakproof but its fellow text comparison operators aren't. Is that really sane? Not really. Fortunately, AFAICT, most if not all of these are in the good direction: there are some things not marked leakproof that can be so marked. The reverse direction would be a hard-to-fix security hole. I think at some point somebody went through and tried to mark all of the same-type equality operators as leakproof, and it seems like that got expanded somewhat without fully rationalizing what we had in pg_proc... mostly because I think nobody had a clear idea how to do that. We'll need to investigate and see if there are any which *aren't* safe and probably fix those to be safe rather than trying to deal with this risk in some other way. In other words, I hope it's really all of these rather than just most. In general, I've been hoping that we have more leakproof functions than not as, while it's non-trivial to write them and ensure they're correct, it's better for us to take that on than to ask our users to do so (or have them get some set that someone else wrote off of a website or even the extension network). We can't cover everything, of course, but hopefully we'll cover all reasonable use cases for types we ship. Looking at other functions, ISTM that there's an entire class of functions that can trivially be marked leakproof, and that's no-arg functions which can't possibly leak. There are quite a few no-arg builtin functions and none of them are currently marked leakproof. Rather than (or perhaps as well as) marking all these leakproof, perhaps we should teach contain_leaky_functions() to automatically treat any no-arg function as leakproof, so that we allow user-defined functions too. Taking that one step further, perhaps what it should really be looking for is Vars in the argument list of a leaky function, i.e., contain_leaked_vars() rather than contain_leaky_functions(). Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Last Commitfest patches waiting review
We are down to 13 patch in Needs Review state. Many of these are stragglers that no-one's really even looked at yet. If we want to finish the commitfest, we'll soon have to decide what to do to patches that no-one cares enough about to review them. pg_receivexlog: addition of --create/--drop to create/drop repslots --- Magnus has promised to review this, but hasn't had the time for weeks. Does anyone else want to pick this up? I'm OK to wait for Magnus to handle this - I expect it to be a quick review and commit - but we should not hold up the commitfest for this. Grouping Sets --- There has been a lot of discussion, but no decisions. See http://www.postgresql.org/message-id/87vbodhtvb@news-spur.riddles.org.uk. Would a committer be interested to take responsibility of this? If not, this will just linger indefinitely. Sort support for text with strxfrm() poor man's keys --- Peter: Are you waiting for Robert to review this? Robert, could you review the latest patch, please? Peter: Could you try to get rid of the extra SortSupport object that Robert didn't like? (http://www.postgresql.org/message-id/ca+tgmobde+ydfnhts0gwpt54-er8bpt3vx8rpshd+98ctdo...@mail.gmail.com). I think it would speed up the process if you did that, instead of waiting for Robert to find the time. Compression of Full Page Writes --- This has been a ridiculously long thread, with diversions into different compression and CRC algorithms. Given that, the latest patch is surprisingly small. I don't think this is going to get any better with further discussion, and the patch looks OK at a quick glance, so I've now marked this as Ready for Committer. hash join - dynamic bucket count --- Kevin: Could you review the latest patch, please? INNER JOIN removals --- The latest patch is significantly different from what was originally submitted for the commitfest, so I wouldn't feel bad just bumping this to the next one. I'll do that unless someone picks this up soon. Tom: I know you're busy with the more urgent jsonb patch.. Do you think you would find the time to review this anytime soon? Anyone else? event triggers: more DROP info --- Adam: Are you planning to do more review on this? Alvaro: do you feel comfortable proceeding with the review you got so far? Selectivity estimation for inet operators --- I think there's a bug in the estimation of semi-joins, see http://www.postgresql.org/message-id/5423dc8d.3080...@vmware.com. But I think we could split this patch into two, and commit the non-join selectivity estimator first, as that seems OK. If no-one else steps up to the plate, I can do that.. add latency limit to pgbench throttling (--rate) --- Rukh: Are you planning to review the latest patch version? Escaping from blocked send() by pg_terminate_backend(). --- I've had a look, but I'd like to have a second opinion on this. Fix local_preload_libraries to work as expected. --- Peter: Could you move this forward, please? pg_dump refactor to remove global variables --- Peter: Can you review the latest patch, please? Fix xpath() to return namespace definitions (fixes the issue with nested or repeated xpath()) --- No-one's volunteered to review this. I don't understand XML enough to review this, and Abhijit who was earlier signed up as reviewer said the same thing. Peter: Could you take a look at this too? You've dabbled into XML stuff before.. - 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] Turning off HOT/Cleanup sometimes
On 2014-09-27 10:23:33 +0300, Heikki Linnakangas wrote: This patch has gotten a fair amount of review, and has been rewritten once during the commitfest. I think it's pretty close to being committable, the only remaining question seems to be what to do with system catalogs. I'm marking this as Returned with feedback, I take it that Simon can proceed from here, outside the commitfest. FWIW, I don't think it is, even with that. As is it seems very likely that it's going to regress a fair share of workloads. At the very least it needs a fair amount of benchmarking beforehand. 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] Last Commitfest patches waiting review
On 2014-09-27 11:18:26 +0300, Heikki Linnakangas wrote: pg_receivexlog: addition of --create/--drop to create/drop repslots --- Magnus has promised to review this, but hasn't had the time for weeks. Does anyone else want to pick this up? I'm OK to wait for Magnus to handle this - I expect it to be a quick review and commit - but we should not hold up the commitfest for this. I'll take that one, sometime next week, if Magnus hasn't gotten to it by then. Compression of Full Page Writes --- This has been a ridiculously long thread, with diversions into different compression and CRC algorithms. Given that, the latest patch is surprisingly small. I don't think this is going to get any better with further discussion, and the patch looks OK at a quick glance, so I've now marked this as Ready for Committer. Did you like the patch? On a quick pass I wasn't very enthusiastic about it in its current state. 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] Last Commitfest patches waiting review
On 09/27/2014 11:31 AM, Andres Freund wrote: On 2014-09-27 11:18:26 +0300, Heikki Linnakangas wrote: pg_receivexlog: addition of --create/--drop to create/drop repslots --- Magnus has promised to review this, but hasn't had the time for weeks. Does anyone else want to pick this up? I'm OK to wait for Magnus to handle this - I expect it to be a quick review and commit - but we should not hold up the commitfest for this. I'll take that one, sometime next week, if Magnus hasn't gotten to it by then. Thanks! Compression of Full Page Writes --- This has been a ridiculously long thread, with diversions into different compression and CRC algorithms. Given that, the latest patch is surprisingly small. I don't think this is going to get any better with further discussion, and the patch looks OK at a quick glance, so I've now marked this as Ready for Committer. Did you like the patch? On a quick pass I wasn't very enthusiastic about it in its current state. Now that I look at it closer, there's some ugly things there like putting the xl_compress field to XLogRecord. I guess I should post to the thread with more detailed comments. - 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] Selectivity estimation for inet operators
Thanks. Overall, my impression of this patch is that it works very well. But damned if I understood *how* it works :-). There's a lot of statistics involved, and it's not easy to see why something is multiplied by something else. I'm adding comments as I read through it. Thank you for looking at it. I tried to add more comments to the multiplications. New version attached. It also fixes a bug caused by wrong operator order used on histogram to histogram selectivity estimation for inner join. I've gotten to the inet_semi_join_selec function: [function] This desperately needs comment at the top of the function explaining what it does. Let me try to explain what I think it does: [explanation] I used your explanation on the new version. Now, I think that last step is wrong. Firstly, the Do not bother if histogram weight is smaller than 0.1 rule seems bogus. The his2_weight is the total number of rows represented by the histogram, so surely it can't be less than 1. It can't really be less than the statistics target. Unless maybe if the histogram was collected when the table was large, but it has since shrunk to contain only a few rows, but that seems like a very bizarre corner case. At least it needs more comments explaining what the test is all about, but I think we should just always use the histogram (if it's available). It was an unnecessary check. I put an assert instead of it. Secondly, if we estimate that there is on average 1.0 matching row in the table, it does not follow that the probability that at least one row matches is 1.0. Assuming a gaussian distribution with mean 1.0, the probability that at least one row matches is 0.5. Assuming a gaussian distribution here isn't quite right - I guess a Poisson distribution would be more accurate - but it sure doesn't seem right as it is. The error isn't very big, and perhaps you don't run into that very often, so I'm not sure what the best way to fix that would be. My statistics skills are a bit rusty, but I think the appropriate way would be to apply the Poisson distribution, with the estimated number of matched rows as the mean. The probability of at least one match would be the cumulative distribution function at k=1. It sounds like overkill, if this is case occurs only rarely. But then again, perhaps it's not all that rare. A function of his_weight and his_selec could be a better option than just multiplying them. I am not sure about the function or it worths the trouble. Join selectivity estimation function for equality doesn't even bother to look at the histograms. Others only return constant values. That said, I can't immediately find a test case where that error would matter. I tried this: create table inettbl1 (a inet); insert into inettbl1 select '10.0.0.' || (g % 255) from generate_series(1, 10) g; analyze inettbl1; explain analyze select count(*) from inettbl1 where a = ANY (SELECT a from inettbl1); The estimate for that is pretty accurate, 833 rows estimated vs 1000 actual, with the current patch. I'm afraid if we fixed inet_semi_join_selec the way I suggest, the estimate would be smaller, i.e. more wrong. Is there something else in the estimates that accidentally compensates for this currently? The partial bucket match on inet_his_inclusion_selec() causes low estimates. Which also effects non join estimation but not as much as it effects join estimations. If that works more correctly, semi join estimation can be higher than it should be. network_selfuncs.c:602: /* Partial bucket match. */ left_divider = inet_his_match_divider(left, query, opr_order); right_divider = inet_his_match_divider(right, query, opr_order); if (left_divider = 0 || right_divider = 0) match += 1.0 / pow(2, Max(left_divider, right_divider)); I think this calculation can benefit from a statistical function more than the semi join. Using the different bit count as power of two is the best I could find. It works quite well on most of the cases. diff --git a/src/backend/utils/adt/network_selfuncs.c b/src/backend/utils/adt/network_selfuncs.c index d0d806f..e9f9696 100644 --- a/src/backend/utils/adt/network_selfuncs.c +++ b/src/backend/utils/adt/network_selfuncs.c @@ -3,7 +3,8 @@ * network_selfuncs.c * Functions for selectivity estimation of inet/cidr operators * - * Currently these are just stubs, but we hope to do better soon. + * Estimates are based on null fraction, most common values, and + * histogram of inet/cidr datatypes. * * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California @@ -16,17 +17,864 @@ */ #include postgres.h +#include math.h + +#include access/htup_details.h +#include catalog/pg_collation.h +#include catalog/pg_operator.h +#include
Re: [HACKERS] json (b) and null fields
Andrew, all, * Andrew Dunstan (and...@dunslane.net) wrote: I should have been paying a bit more attention to the recent work on adding an ignore_nulls option to row_to_json(). Here are some belated thought. I apologize to Pavel and Stephen for not having commented earlier. No problem at all and thanks for continuing to think about it! We certainly still have quite a bit of time til 9.5 to get this right. I think this is really a bandaid, and it will fail to catch lots of cases. Several examples: As discussed on IRC- I agree. I tend to think of JSON objects as relatively simple hstore-like structures and so hadn't considered the complex structure case (as I'm guessing Pavel hadn't either). I think a much more comprehensive solution would be preferable. What I have in mind is something like json_strip_null_fields(json) - json and a similar function for jsonb. Right, this makes sense to me. These would operate recursively. There is a downside, in that they would be required to reprocess the json/jsonb. But adding an option like this to all the json generator functions would be seriously ugly, especially since they are mostly aggregate functions or variadic functions. At least in the jsonb case the cost of reprocessing is likely to be fairly low. Yeah, I don't see adding this option to all json generator functions as making a lot of sense but rather just to the select few things which it really makes sense for and then having a function which can be used by users to do the same for results from other operations. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] WITH CHECK and Column-Level Privileges
* Stephen Frost (sfr...@snowman.net) wrote: Looks like there is an issue here with CHECK constraints and NOT NULL constraints, yes. The uniqueness check complains about the key already existing and returns the key, but I don't think that's actually a problem- to get that to happen you have to specify the new key and that's what is returned. Yeah, I take that back. If there is a composite key involved then you can run into the same issue- you update one of the columns to a conflicting value and get back the entire key, including columns you shouldn't be allowed to see. Alright, attached is a patch which addresses this by checking if the user has SELECT rights on the relation first and, if so, the existing error message is returned with the full row as usual, but if not, then the row data is omitted. Also attached is a patch for 9.4 which does the same, but also removes the row reporting (completely) from the WITH CHECK case. It could be argued that it would be helpful to have it there also, perhaps, but I'm not convinced at this point that it's really valuable- and we'd have to think about how this would work with views (permission on the view? or on the table underneath? what if there is more than one?, etc). Thanks, Stephen diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c new file mode 100644 index 17d9765..f9a8bff *** a/src/backend/access/nbtree/nbtinsert.c --- b/src/backend/access/nbtree/nbtinsert.c *** *** 21,26 --- 21,27 #include miscadmin.h #include storage/lmgr.h #include storage/predicate.h + #include utils/acl.h #include utils/inval.h #include utils/tqual.h *** _bt_check_unique(Relation rel, IndexTupl *** 387,400 index_deform_tuple(itup, RelationGetDescr(rel), values, isnull); ! ereport(ERROR, ! (errcode(ERRCODE_UNIQUE_VIOLATION), ! errmsg(duplicate key value violates unique constraint \%s\, ! RelationGetRelationName(rel)), ! errdetail(Key %s already exists., ! BuildIndexValueDescription(rel, ! values, isnull)), ! errtableconstraint(heapRel, RelationGetRelationName(rel; } } --- 388,420 index_deform_tuple(itup, RelationGetDescr(rel), values, isnull); ! /* ! * When reporting a failure, it can be handy to have ! * the key which failed reported. Unfortunately, when ! * using column-level privileges, this could expose ! * a column the user does not have rights for. Instead, ! * only report the key if the user has full table-level ! * SELECT rights on the relation. ! */ ! if (pg_class_aclcheck(RelationGetRelid(rel), ! GetUserId(), ACL_SELECT) == ! ACLCHECK_OK) ! ereport(ERROR, ! (errcode(ERRCODE_UNIQUE_VIOLATION), ! errmsg(duplicate key value violates unique constraint \%s\, ! RelationGetRelationName(rel)), ! errdetail(Key %s already exists., ! BuildIndexValueDescription(rel, ! values, ! isnull)), ! errtableconstraint(heapRel, ! RelationGetRelationName(rel; ! else ! ereport(ERROR, ! (errcode(ERRCODE_UNIQUE_VIOLATION), ! errmsg(duplicate key value violates unique constraint \%s\, ! RelationGetRelationName(rel)), ! errtableconstraint(heapRel, RelationGetRelationName(rel; } } diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c new file mode 100644 index 85d1c63..fe98599 *** a/src/backend/executor/execMain.c --- b/src/backend/executor/execMain.c *** ExecConstraints(ResultRelInfo *resultRel *** 1600,1614 { if (tupdesc-attrs[attrChk - 1]-attnotnull slot_attisnull(slot, attrChk)) ! ereport(ERROR, ! (errcode(ERRCODE_NOT_NULL_VIOLATION), ! errmsg(null value in column \%s\ violates not-null constraint, ! NameStr(tupdesc-attrs[attrChk - 1]-attname)), ! errdetail(Failing row contains %s., ! ExecBuildSlotValueDescription(slot, ! tupdesc, ! 64)), ! errtablecol(rel, attrChk))); } } --- 1600,1631 { if (tupdesc-attrs[attrChk - 1]-attnotnull slot_attisnull(slot, attrChk)) ! { ! /* ! * When reporting failure, it's handy to have the whole row ! * which failed returned, but we can only show it if the user ! * has full SELECT rights on the relation, otherwise we might ! * end up showing fields the user does not have access to, due ! * to column-level privileges. ! */ ! if (pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), ! ACL_SELECT) == ACLCHECK_OK) ! ereport(ERROR, !
Re: [HACKERS] Sloppy thinking about leakproof properties of opclass co-members
Dean Rasheed dean.a.rash...@gmail.com writes: Rather than (or perhaps as well as) marking all these leakproof, perhaps we should teach contain_leaky_functions() to automatically treat any no-arg function as leakproof, so that we allow user-defined functions too. Taking that one step further, perhaps what it should really be looking for is Vars in the argument list of a leaky function, i.e., contain_leaked_vars() rather than contain_leaky_functions(). +1, but that's a totally independent question from what I'm on about at the moment. 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] Last Commitfest patches waiting review
Heikki Linnakangas hlinnakan...@vmware.com writes: [ unreviewed patches ] Grouping Sets There has been a lot of discussion, but no decisions. See http://www.postgresql.org/message-id/87vbodhtvb@news-spur.riddles.org.uk. Would a committer be interested to take responsibility of this? If not, this will just linger indefinitely. I should and will take this, but not in this commitfest: I've been just horribly busy lately with both work and personal issues. If we can punt it to the next fest, I will promise to work on it then. INNER JOIN removals The latest patch is significantly different from what was originally submitted for the commitfest, so I wouldn't feel bad just bumping this to the next one. I'll do that unless someone picks this up soon. Tom: I know you're busy with the more urgent jsonb patch.. Do you think you would find the time to review this anytime soon? Anyone else? Same deal here. Selectivity estimation for inet operators I think there's a bug in the estimation of semi-joins, see http://www.postgresql.org/message-id/5423dc8d.3080...@vmware.com. But I think we could split this patch into two, and commit the non-join selectivity estimator first, as that seems OK. If no-one else steps up to the plate, I can do that.. And I'd like to look this one over too ... Escaping from blocked send() by pg_terminate_backend(). I've had a look, but I'd like to have a second opinion on this. I concur with your opinion that this is scary as heck. We need multiple pairs of eyeballs if we're going to do anything in this area. In the long run though, I think pushing functionality into signal handlers is exactly backwards; we ought to be trying to go the other way. 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] Last Commitfest patches waiting review
On 2014-09-27 10:12:03 -0400, Tom Lane wrote: Escaping from blocked send() by pg_terminate_backend(). I've had a look, but I'd like to have a second opinion on this. I concur with your opinion that this is scary as heck. We need multiple pairs of eyeballs if we're going to do anything in this area. In the long run though, I think pushing functionality into signal handlers is exactly backwards; we ought to be trying to go the other way. I very much agree with that concern. The interactions are just complicated. I'm now refreshing my work to do all waiting in client communication via latches. While far from trivial, I'm pretty sure that's the better course in the long run. I guess I'll post it in the other thread. 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] Replication identifiers, take 3
On 09/26/2014 06:05 PM, Andres Freund wrote: On 2014-09-26 14:57:12 -0400, Robert Haas wrote: Sure, it'll possibly not be trivial to move them elsewhere. On the other hand, the padding bytes have been unused for 8+ years without somebody laying claim on them but me. I don't think it's a good idea to leave them there unused when nobody even has proposed another use for a long while. That'll just end up with them continuing to be unused. And there's actually four more consecutive bytes on 64bit systems that are unused. Should there really be a dire need after that, we can simply bump the record size. WAL volume wise it'd not be too bad to make the record a tiny bit bigger - the header is only a relatively small fraction of the entire content. If we were now increasing the WAL record size anyway for some unrelated reason, would we be willing to increase it by a further 2 bytes for the node identifier? If the answer is 'no' then I don't think we can justify using the 2 padding bytes just because they are there and have been unused for years. But if the answer is yes, we feel this important enough to justfiy a slightly (2 byte) larger WAL record header then we shouldn't use the excuse of maybe needing those 2 bytes for something else. When something else comes along that needs the WAL space we'll have to increase the record size. To say that if some other patch comes along that needs the space we'll redo this feature to use the method Robert describes is unrealistic. If we think that the replication identifier isn't general/important/necessary to justify 2 bytes of WAL header space then we should start out with something that doesn't use the WAL header, Steve I've also wondered about that. Perhaps we simply should have an additional 'name' column indicating the replication solution? Yeah, maybe, but there's still the question of substructure within the non-replication-solution part of the name. Not sure if we can assume that a bipartite identifier, specifically, is right, or whether some solutions will end up with different numbers of components. Ah. I thought you only wanted to suggest a separator between the replication solution and it's internal dat. But you actually want to suggest an internal separator to be used in the solution's namespace? I'm fine with that. I don't think we can suggest much beyond that - different solutions will have fundamentally differing requirements about which information to store. Agreed. So, let's recommend underscores as that separator? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Replication identifiers, take 3
On 09/26/2014 10:21 AM, Andres Freund wrote: On 2014-09-26 09:53:09 -0400, Robert Haas wrote: On Fri, Sep 26, 2014 at 5:05 AM, Andres Freund and...@2ndquadrant.com wrote: Let me try to summarize the information requirements for each of these things. For #1, you need to know, after crash recovery, for each standby, the last commit LSN which the client has confirmed via a feedback message. I'm not sure I understand what you mean here? This is all happening on the *standby*. The standby needs to know, after crash recovery, the latest commit LSN from the primary that it has successfully replayed. Ah, sorry, you're right: so, you need to know, after crash recovery, for each machine you are replicating *from*, the last transaction (in terms of LSN) from that server that you successfully replayed. Precisely. I don't think a solution which logs the change of origin will be simpler. When the origin is in every record, you can filter without keep track of any state. That's different if you can switch the origin per tx. At the very least you need a in memory entry for the origin. But again, that complexity pertains only to logical decoding. Somebody who wants to tweak the WAL format for an UPDATE in the future doesn't need to understand how this works, or care. I agree that that's a worthy goal. But I don't see how this isn't the case with what I propose? This isn't happening on the level of individual rmgrs/ams - there've been two padding bytes after 'xl_rmid' in struct XLogRecord for a long time. There's also the significant advantage that not basing this on the xid allows it to work correctly with records not tied to a transaction. There's not that much of that happening yet, but I've several features in mind: * separately replicate 2PC commits. 2PC commits don't have an xid anymore... With some tooling on top replication 2PC in two phases allow for very cool stuff. Like optionally synchronous multimaster replication. * I have a pending patch that allows to send 'messages' through logical decoding - yielding a really fast and persistent queue. For that it's useful have transactional *and* nontransactional messages. * Sanely replicating CONCURRENTLY stuff gets harder if you tie things to the xid. At what point in the decoding stream should something related to a CONCURRENTLY command show up? Also, for a logical message queue why couldn't you have a xid associated with the message that had nothing else in the transaction? l -- 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] WITH CHECK and Column-Level Privileges
On 27 September 2014 14:33, Stephen Frost sfr...@snowman.net wrote: Yeah, I take that back. If there is a composite key involved then you can run into the same issue- you update one of the columns to a conflicting value and get back the entire key, including columns you shouldn't be allowed to see. Alright, attached is a patch which addresses this by checking if the user has SELECT rights on the relation first and, if so, the existing error message is returned with the full row as usual, but if not, then the row data is omitted. Seems reasonable. Also attached is a patch for 9.4 which does the same, but also removes the row reporting (completely) from the WITH CHECK case. It could be argued that it would be helpful to have it there also, perhaps, but I'm not convinced at this point that it's really valuable- and we'd have to think about how this would work with views (permission on the view? or on the table underneath? what if there is more than one?, etc). Well by that point in the code, the query has been rewritten and the row being reported is for the underlying table, so it's the table's ACLs that should apply. In fact not all the values from the table are even necessarily exported in the view, so its ACLs are not appropriate to the values being reported. I suspect that in many/most real-world cases, the user will only have permissions on the view, not on the underlying table, in which case it won't work anyway. So +1 for just removing it. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] json (b) and null fields
On 09/27/2014 08:00 AM, Stephen Frost wrote: Andrew, all, * Andrew Dunstan (and...@dunslane.net) wrote: I should have been paying a bit more attention to the recent work on adding an ignore_nulls option to row_to_json(). Here are some belated thought. I apologize to Pavel and Stephen for not having commented earlier. No problem at all and thanks for continuing to think about it! We certainly still have quite a bit of time til 9.5 to get this right. I think this is really a bandaid, and it will fail to catch lots of cases. Several examples: As discussed on IRC- I agree. I tend to think of JSON objects as relatively simple hstore-like structures and so hadn't considered the complex structure case (as I'm guessing Pavel hadn't either). I think a much more comprehensive solution would be preferable. What I have in mind is something like json_strip_null_fields(json) - json and a similar function for jsonb. Right, this makes sense to me. These would operate recursively. There is a downside, in that they would be required to reprocess the json/jsonb. But adding an option like this to all the json generator functions would be seriously ugly, especially since they are mostly aggregate functions or variadic functions. At least in the jsonb case the cost of reprocessing is likely to be fairly low. Yeah, I don't see adding this option to all json generator functions as making a lot of sense but rather just to the select few things which it really makes sense for and then having a function which can be used by users to do the same for results from other operations. I guess I'm questioning the wisdom of keeping it for row_to_json given that it doesn't operate recursively anyway (and making it do so would be difficult and ugly). The counter argument for this is that nested composites and arrays of composites are relatively rare in records, so providing a fast non-recursive stripping of nulls for the common case is reasonable. If we're going to keep this, I think we also need to provide it (non-recursive) for json_agg via an optional second argument. This should be a fairly simple change: just steer the result via composite_to_json if it's a record, rather than to datum_to_json. 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] Escaping from blocked send() reprised.
On 2014-09-03 15:09:54 +0300, Heikki Linnakangas wrote: On 09/03/2014 12:23 AM, Andres Freund wrote: On 2014-09-02 17:21:03 -0400, Tom Lane wrote: Heikki Linnakangas hlinnakan...@vmware.com writes: I was going to suggest using WaitLatchOrSocket instead of sleeping in 1 second increment, but I see that WaitLatchOrSocket() doesn't currently support waiting for a socket to become writeable, without also waiting for it to become readable. I wonder how difficult it would be to lift that restriction. My recollection is that there was a reason for that, but I don't recall details any more. http://git.postgresql.org/pg/commitdiff/e42a21b9e6c9b9e6346a34b62628d48ff2fc6ddf In my prototype I've changed the API that errors set both READABLE/WRITABLE. Seems to work Andres, would you mind posting the WIP patch you have? That could be a better foundation for this patch. Sorry, I missed this message and only cought up when reading your CF status mail. I've attached three patches: 0001: Allows WaitLatchOrSocket(WL_WRITABLE) without WL_READABLE. I've tested the poll() and select() implementations on linux and blindly patched up windows. 0002: Put the socket the backend uses to communicate with the client into nonblocking mode as soon as latches are ready and use latches to wait. This probably doesn't work correctly without 0003, but seems easier to review separately. 0003: Don't do sinval catchup and notify processing in signal handlers. It's quite cool that it worked that well so far, but it requires some complicated code and is rather fragile. 0002 allows to move that out of signal handlers and just use a latch there. This seems remarkably simpler: 4 files changed, 69 insertions(+), 229 deletions(-) These aren't ready for commit, especially not 0003, but I think they are quite a good foundation for getting rid of the blocking in send(). I haven't added any interrupt processing after interrupted writes, but marked the relevant places with XXXs. With regard to 0002, I dislike the current need to do interrupt processing both in be-secure.c and be-secure-openssl.c. I guess we could solve that by returning something like EINTR from the ssl routines when they need further reads/writes and do all the processing in one place in be-secure.c. There's also some cleanup in 0002/0003 needed: prepare_for_client_read()/client_read_ended() aren't needed in that form anymore and should probably rather be something like CHECK_FOR_READ_INTERRUPT() or similar. Similarly the EnableCatchupInterrupt()/DisableCatchupInterrupt() in autovacuum.c is pretty ugly. Btw, be-secure.c is really not a good name anymore... What do you think? 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
[HACKERS] HEADS UP: PGCon 2015 is in June
HEADS UP. PGCon 2015 will be in June. That’s a few weeks later than in previous years. — Dan Langille signature.asc Description: Message signed with OpenPGP using GPGMail
Re: [HACKERS] pgcrypto: PGP armor headers
Hi, On 9/25/14, 3:56 PM, I wrote: On 9/25/14 3:50 PM, Heikki Linnakangas wrote: Are you planning to post the main patch rebased on top of this soon? As in the next day or two? Otherwise I'll mark this as Returned with feedback for this commitfest. Yes. With good luck I'll get you a rebased one today, otherwise it'll have to wait until tomorrow. Missed that promise by a day since something unexpected came up yesterday. Attached is v3 of the patch. The changes are: - Rebased on top of the current master - Added a function pgp_armor_header_keys() to list all keys present in an armor - Changed pgp_armor_header() to use a StringInfo instead of an mbuf - Fixed the error is returned problem in the documentation pointed out earlier .marko *** a/contrib/pgcrypto/Makefile --- b/contrib/pgcrypto/Makefile *** *** 26,32 MODULE_big = pgcrypto OBJS = $(SRCS:.c=.o) $(WIN32RES) EXTENSION = pgcrypto ! DATA = pgcrypto--1.1.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql PGFILEDESC = pgcrypto - cryptographic functions REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \ --- 26,32 OBJS = $(SRCS:.c=.o) $(WIN32RES) EXTENSION = pgcrypto ! DATA = pgcrypto--1.2.sql pgcrypto--1.1--1.2.sql pgcrypto--1.0--1.1.sql pgcrypto--unpackaged--1.0.sql PGFILEDESC = pgcrypto - cryptographic functions REGRESS = init md5 sha1 hmac-md5 hmac-sha1 blowfish rijndael \ *** a/contrib/pgcrypto/expected/pgp-armor.out --- b/contrib/pgcrypto/expected/pgp-armor.out *** *** 102,104 em9va2E= --- 102,423 -END PGP MESSAGE- '); ERROR: Corrupt ascii-armor + -- corrupt + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + foo: + + em9va2E= + = + -END PGP MESSAGE- + ', 'foo'); + ERROR: Corrupt ascii-armor + -- empty + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + foo: + + em9va2E= + = + -END PGP MESSAGE- + ', 'foo'); + pgp_armor_header + -- + + (1 row) + + -- simple + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + foo: bar + + em9va2E= + = + -END PGP MESSAGE- + ', 'foo'); + pgp_armor_header + -- + bar + (1 row) + + -- uninteresting keys, part 1 + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + foo: found + bar: ignored + + em9va2E= + = + -END PGP MESSAGE- + ', 'foo'); + pgp_armor_header + -- + found + (1 row) + + -- uninteresting keys, part 2 + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + bar: ignored + foo: found + + em9va2E= + = + -END PGP MESSAGE- + ', 'foo'); + pgp_armor_header + -- + found + (1 row) + + -- uninteresting keys, part 3 + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + bar: ignored + foo: found + bar: ignored + + em9va2E= + = + -END PGP MESSAGE- + ', 'foo'); + pgp_armor_header + -- + found + (1 row) + + -- insane keys, part 1 + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + insane:key : + + em9va2E= + = + -END PGP MESSAGE- + ', 'insane:key '); + pgp_armor_header + -- + + (1 row) + + -- insane keys, part 2 + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + insane:key : text value here + + em9va2E= + = + -END PGP MESSAGE- + ', 'insane:key '); + pgp_armor_header + -- + text value here + (1 row) + + -- long value + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + long: this value is more than 76 characters long, but it should still parse correctly as that''s permitted by RFC 4880 + + em9va2E= + = + -END PGP MESSAGE- + ', 'long'); + pgp_armor_header + - + this value is more than 76 characters long, but it should still parse correctly as that's permitted by RFC 4880 + (1 row) + + -- long value, split up + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + long: this value is more than 76 characters long, but it should still + long: parse correctly as that''s permitted by RFC 4880 + + em9va2E= + = + -END PGP MESSAGE- + ', 'long'); + pgp_armor_header + - + this value is more than 76 characters long, but it should still parse correctly as that's permitted by RFC 4880 + (1 row) + + -- long value, split up, part 2 + select pgp_armor_header(' + -BEGIN PGP MESSAGE- + long: this value is more than + long: 76 characters long, but it should still + long: parse correctly as that''s permitted by RFC 4880 + +
[HACKERS] trivial patch for dynahash logging
under HASH_STATISTICS, the output reporting this HTAB upon destruction is pretty useless. Which HTAB would this one be? It is not necessarily the most recently created one. This makes it output the %p to the actual HTAB, so it can be matched up with the logging of the creation. I'm not sure why it bypasses elog. Is that so it can run during start-up before elog is active? I'd like to make it go through elog so that log_line_prefix are applied, but then it would no longer be a trivial patch. Cheers, Jeff diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c new file mode 100644 index 2b99e4b..89c3a9d *** a/src/backend/utils/hash/dynahash.c --- b/src/backend/utils/hash/dynahash.c *** void *** 733,740 hash_stats(const char *where, HTAB *hashp) { #if HASH_STATISTICS ! fprintf(stderr, %s: this HTAB -- accesses %ld collisions %ld\n, ! where, hashp-hctl-accesses, hashp-hctl-collisions); fprintf(stderr, hash_stats: entries %ld keysize %ld maxp %u segmentcount %ld\n, hashp-hctl-nentries, (long) hashp-hctl-keysize, --- 737,744 hash_stats(const char *where, HTAB *hashp) { #if HASH_STATISTICS ! fprintf(stderr, %s: HTAB %p -- accesses %ld collisions %ld\n, ! where, hashp, hashp-hctl-accesses, hashp-hctl-collisions); fprintf(stderr, hash_stats: entries %ld keysize %ld maxp %u segmentcount %ld\n, hashp-hctl-nentries, (long) hashp-hctl-keysize, -- 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] trivial patch for dynahash logging
Jeff Janes jeff.ja...@gmail.com writes: under HASH_STATISTICS, the output reporting this HTAB upon destruction is pretty useless. Which HTAB would this one be? It is not necessarily the most recently created one. This makes it output the %p to the actual HTAB, so it can be matched up with the logging of the creation. Seems reasonable, although I wonder why neither of them print the tabname string. More generally, I see no calls to hash_stats() anywhere except in hash_destroy(), so wouldn't you need a rather larger patch to make it actually do anything useful? I'm not sure why it bypasses elog. Is that so it can run during start-up before elog is active? I'd like to make it go through elog so that log_line_prefix are applied, but then it would no longer be a trivial patch. A quick dig in our git history shows that it was like that in Postgres95, so the answer is most likely this code predates elog(). I would think it'd be a safe assumption that elog is initialized before any hashtables are created, but certainly you could test that pretty quickly ... 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] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs si...@2ndquadrant.com wrote: I hate the fact that you have written no user facing documentation for this feature. Attached patch adds a commit to the existing patchset. For the convenience of reviewers, I've uploaded and made publicly accessible a html build of the documentation. This page is of most interest: http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-insert.html See also: http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/transaction-iso.html http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/ddl-inherit.html http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-createrule.html http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/trigger-definition.html http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-createtrigger.html http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/index-unique-checks.html http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/sql-createview.html http://postgres-benchmarks.s3-website-us-east-1.amazonaws.com/on-conflict-docs/postgres-fdw.html -- Peter Geoghegan From 005eb2760b356c7383c591bb92294cc9626baabe Mon Sep 17 00:00:00 2001 From: Peter Geoghegan p...@heroku.com Date: Fri, 26 Sep 2014 20:59:04 -0700 Subject: [PATCH 6/6] User-visible documentation for INSERT ... ON CONFLICT {UPDATE | IGNORE} INSERT ... ON CONFLICT {UPDATE | IGNORE} is documented as a new clause of the INSERT command. Some potentially surprising interactions with triggers are noted -- statement level UPDATE triggers will not fire when INSERT ... ON CONFLICT UPDATE is executed, for example. All the existing features that INSERT ... ON CONFLICT {UPDATE | IGNORE} fails to completely play nice with have those limitations noted. (Notes are added to the existing documentation for those other features, although some of these cases will need to be revisited). This includes postgres_fdw, updatable views and table inheritance. In principle it is the responsibility of writable foreign data wrapper authors to provide appropriate support for this new clause (although it's hard to see how the optional WITHIN `unique_index` clause could work there). Finally, a user-level description of the new MVCC violation that INSERT ... ON CONFLICT {UPDATE | IGNORE} sometimes requires has been added to Chapter 13 - Concurrency Control, beside existing commentary on Read Committed mode's special handling of concurrent updates, and the implications for snapshot isolation (i.e. what is internally referred to as the EvalPlanQual() mechanism). The new MVCC violation introduced seems somewhat distinct from the existing one, because in Read Committed mode it is no longer necessary for any row version to be conventionally visible to the command's MVCC snapshot for an UPDATE of the row to occur (or for the row to be locked). --- doc/src/sgml/ddl.sgml| 14 +++ doc/src/sgml/mvcc.sgml | 43 ++-- doc/src/sgml/plpgsql.sgml| 14 +-- doc/src/sgml/postgres-fdw.sgml | 8 ++ doc/src/sgml/ref/create_rule.sgml| 6 +- doc/src/sgml/ref/create_trigger.sgml | 5 +- doc/src/sgml/ref/create_view.sgml| 15 ++- doc/src/sgml/ref/insert.sgml | 203 --- doc/src/sgml/trigger.sgml| 30 +- 9 files changed, 302 insertions(+), 36 deletions(-) diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index c07f5a2..2910890 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -2292,6 +2292,20 @@ VALUES ('Albany', NULL, NULL, 'NY'); but in the meantime considerable care is needed in deciding whether inheritance is useful for your application. /para + para + !-- XXX: This limitation can probably be removed, at least for +the simple case where a unique index constrains an attribute +or attributes that are effectively contained within the +(implied) partitioning key. Clearly doing anything more +advanced than that would require a large overhaul of +partitioning in PostgreSQL. +-- + Since unique indexes do not constrain every child table in an + inheritance hierarchy, inheritance is not supported for + commandINSERT/command statements that contain an literalON + CONFLICT UPDATE/ clause, or an literalON CONFLICT IGNORE/ + clause. + /para /sect2 /sect1 diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index cd55be8..dd05cfe 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -326,14 +326,41 @@ /para para -Because of the above rule, it is possible for an updating command to see an -inconsistent snapshot: it can see the effects of concurrent updating -commands on the same rows it is trying to update, but it -does not
Re: [HACKERS] json (b) and null fields
Andrew Dunstan and...@dunslane.net writes: On 09/27/2014 08:00 AM, Stephen Frost wrote: Yeah, I don't see adding this option to all json generator functions as making a lot of sense but rather just to the select few things which it really makes sense for and then having a function which can be used by users to do the same for results from other operations. I guess I'm questioning the wisdom of keeping it for row_to_json given that it doesn't operate recursively anyway (and making it do so would be difficult and ugly). IMO, adding it to row_to_json was really ugly to start with, independently of whether it's difficult or not. That function had one too many optional arguments already, and in its current form it's nothing but a loaded gun pointed at users' feet. (Quick, which bool argument is which?) If we're going to keep this, I think we also need to provide it (non-recursive) for json_agg via an optional second argument. This should be a fairly simple change: just steer the result via composite_to_json if it's a record, rather than to datum_to_json. Unfortunately, any such thing will fall foul of an established project policy. I quote the opr_sanity regression test that will complain: -- Check that there are not aggregates with the same name and different -- numbers of arguments. While not technically wrong, we have a project policy -- to avoid this because it opens the door for confusion in connection with -- ORDER BY: novices frequently put the ORDER BY in the wrong place. -- See the fate of the single-argument form of string_agg() for history. So my vote is for a separate function and no optional arguments. 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] Last Commitfest patches waiting review
On Sat, Sep 27, 2014 at 1:18 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: Sort support for text with strxfrm() poor man's keys --- Peter: Are you waiting for Robert to review this? Robert, could you review the latest patch, please? Peter: Could you try to get rid of the extra SortSupport object that Robert didn't like? (http://www.postgresql.org/message-id/ca+tgmobde+ydfnhts0gwpt54-er8bpt3vx8rpshd+98ctdo...@mail.gmail.com). I think it would speed up the process if you did that, instead of waiting for Robert to find the time. I am not waiting on Robert to spend the time, FWIW. The question that resolving if we should not have an extra sortsupport object is blocking on is the need to have a consistent sorttuple.datum1 representation for the benefit of having comparetup_heap() know that it's either always abbreviated keys or always pointers to text. My view is that it's not worth going back to fix up datum1 to always be a pointer to text when we abort abbreviation - I think we should just forget about datum1 on the rare occasion that happens (due to the costs involved, as well as the complexity implied). I think that it will be necessary for me to rigorously prove that view, as with the memcmp() == 0 thing. So I'm looking at 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] json (b) and null fields
On 09/27/2014 06:27 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 09/27/2014 08:00 AM, Stephen Frost wrote: Yeah, I don't see adding this option to all json generator functions as making a lot of sense but rather just to the select few things which it really makes sense for and then having a function which can be used by users to do the same for results from other operations. I guess I'm questioning the wisdom of keeping it for row_to_json given that it doesn't operate recursively anyway (and making it do so would be difficult and ugly). IMO, adding it to row_to_json was really ugly to start with, independently of whether it's difficult or not. That function had one too many optional arguments already, and in its current form it's nothing but a loaded gun pointed at users' feet. (Quick, which bool argument is which?) If we're going to keep this, I think we also need to provide it (non-recursive) for json_agg via an optional second argument. This should be a fairly simple change: just steer the result via composite_to_json if it's a record, rather than to datum_to_json. Unfortunately, any such thing will fall foul of an established project policy. I quote the opr_sanity regression test that will complain: -- Check that there are not aggregates with the same name and different -- numbers of arguments. While not technically wrong, we have a project policy -- to avoid this because it opens the door for confusion in connection with -- ORDER BY: novices frequently put the ORDER BY in the wrong place. -- See the fate of the single-argument form of string_agg() for history. So my vote is for a separate function and no optional arguments. You mean like row_to_json_no_nulls() and json_agg_no_nulls()? 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] proposal: rounding up time value less than its unit.
On 9/26/14, 3:22 PM, Tom Lane wrote: We could alternatively try to split up these cases into multiple GUCs, which I guess is what you're imagining as a multi-year battle. No, I was just pointing out that even the cleanest major refactoring possible here is going to result in broken config files complaints for years. And no matter how well that goes, a second rewrite in the next major version, addressing whatever things nobody saw coming in the first redesign, is a very real possibility. The minute the GUC box is opened that far, it will not close up again in a release, or likely even two, and the griping from the field is going to take 3+ years to settle. I have no desire to waste time on partial measures either though. I think I've been clear that's my theme on this--if we're gonna mess with this significantly, let's do it right and in a big way. If there's a really trivial fix to apply, that's fine too. Throw an error when the value is between the special one and the useful minimum, no other changes; that I could see slipping into a single release without much pain. Might even be possible to write as a useful sub-step toward a bigger plan too. Wouldn't bother breaking that out as a goal unless it just happened to fall out of the larger design though. The rest of the rounding and error handling ideas wandering around seem in this uncomfortable middle ground to me. They are neither large enough to feel like a major improvement happened, nor trivial enough to apply with minimal work/risk. And this is not a bug that must be fixed ASAP--it's an unfriendly UI surprise. -- Greg Smith greg.sm...@crunchydatasolutions.com Chief PostgreSQL Evangelist - http://crunchydatasolutions.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] Last Commitfest patches waiting review
On 9/27/14, 4:18 AM, Heikki Linnakangas wrote: add latency limit to pgbench throttling (--rate) --- Rukh: Are you planning to review the latest patch version? Rukh is free to jump onto this ASAP, but if it's still there next week I already had my eye on picking that up and taking it out for a spin. I already updated my pgbench-tools set to incorporate the rate limit for 9.4, and I use that part all the time now. I think I understand Fabien's entire game plan for this now, and I want the remainder of the set to land in 9.5. -- Greg Smith greg.sm...@crunchydatasolutions.com Chief PostgreSQL Evangelist - http://crunchydatasolutions.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 Thu, Sep 25, 2014 at 1:48 PM, Simon Riggs si...@2ndquadrant.com wrote: At this stage, poll the Django and Rails communities for acceptance and early warning of these features. Listen. FYI, I have asked for input from the Django developers here: https://groups.google.com/forum/#!topic/django-developers/hdzkoLYVjBY -- 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] json (b) and null fields
Andrew Dunstan and...@dunslane.net writes: On 09/27/2014 06:27 PM, Tom Lane wrote: So my vote is for a separate function and no optional arguments. You mean like row_to_json_no_nulls() and json_agg_no_nulls()? I thought you were proposing that we should revert the committed patch lock-stock-n-barrel, and instead invent json_strip_null_fields(). That's instead, not in addition to. Even if you weren't saying that exactly, that's where my vote goes. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] json (b) and null fields
All, On Saturday, September 27, 2014, Andrew Dunstan and...@dunslane.net wrote: On 09/27/2014 10:52 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 09/27/2014 06:27 PM, Tom Lane wrote: So my vote is for a separate function and no optional arguments. You mean like row_to_json_no_nulls() and json_agg_no_nulls()? I thought you were proposing that we should revert the committed patch lock-stock-n-barrel, and instead invent json_strip_null_fields(). That's instead, not in addition to. Even if you weren't saying that exactly, that's where my vote goes. I was just exploring alternatives. But I think that's where my vote goes too. I'm fine with that. I'd like the strip-Nulls capability, but seems like it'd be better off as an independent function (or functions) instead. Thanks, Stephen
Re: [HACKERS] Patch to support SEMI and ANTI join removal
On Fri, Sep 26, 2014 at 12:36 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 09/16/2014 01:20 PM, David Rowley wrote: + /* +* We mustn't allow any joins to be removed if there are any pending +* foreign key triggers in the queue. This could happen if we are planning +* a query that has been executed from within a volatile function and the +* query which called this volatile function has made some changes to a +* table referenced by a foreign key. The reason for this is that any +* updates to a table which is referenced by a foreign key constraint will +* only have the referencing tables updated after the command is complete, +* so there is a window of time where records may violate the foreign key +* constraint. +* +* Currently this code is quite naive, as we won't even attempt to remove +* the join if there are *any* pending foreign key triggers, on any +* relation. It may be worthwhile to improve this to check if there's any +* pending triggers for the referencing relation in the join. +*/ + if (!AfterTriggerQueueIsEmpty()) + return false; Hi Heikki, Thanks for having a look at the patch. Hmm. This code runs when the query is planned. There is no guarantee that there won't be after triggers pending when the query is later *executed*. Please correct anything that sounds wrong here, but my understanding is that we'll always plan a query right before we execute it, with the exception of PREPARE statements where PostgreSQL will cache the query plan when the prepare statement is first executed. So I think you may have a point here regarding PREPARE'd statements, but I think that it is isolated to those. I think in all other cases we'll plan right before we execute. So if we happen to be planning an UPDATE statement which has a sub-query that perform some INNER JOINs, I think we're safe to remove INNER JOINs when possible, as the UPDATE statement won't get visibility of its own changes. We can see that here: create table updatetest (id int primary key, value int, value2 int); create or replace function getvalue2(p_id int) returns int as $$select value2 from updatetest where id = p_id$$ language sql volatile; insert into updatetest values(0,0,0); insert into updatetest values(1,10,10); insert into updatetest values(2,20,20); insert into updatetest values(3,30,30); update updatetest set value = COALESCE((select value from updatetest u2 where updatetest.id - 1 = u2.id) + 1,0); update updatetest set value2 = COALESCE(getvalue2(id - 1) + 1,0); select * from updatetest; id | value | value2 +---+ 0 | 0 | 0 1 | 1 | 1 2 |11 | 2 3 |21 | 3 The value column appears to have been set based on the value that was previously in the value column, and has not come from the newly set value. The behaviour is different for the value2 column as the value for this has been fetched from another query, which *does* see the newly updated value stored in the value2 column. My understanding of foreign keys is that any pending foreign key triggers will be executed just before the query completes, so we should only ever encounter pending foreign key triggers during planning when we're planning a query that's being executed from somewhere like a volatile function or trigger function, if the outer query has updated or deleted some records which are referenced by a foreign key. So I think with the check for pending triggers at planning time this is safe at least for queries being planned right before they're executed, but you've caused me to realise that I'll probably need to do some more work on this for when it comes to PREPARE'd queries, as it looks like if we executed a prepared query from inside a volatile function or trigger function that was called from a DELETE or UPDATE statement that caused foreign key triggers to be queued, and we'd happened to have removed some INNER JOINs when we originally planned that prepare statement, then that would be wrong. The only thing that comes to mind to fix that right now is to tag something maybe in PlannerInfo to say if we've removed any INNER JOINs in planning, then when we execute a prepared statement we could void the cached plan we see that some INNER JOINs were removed, but only do this if the foreign key trigger queue has pending triggers. (which will hopefully not be very often). Another thing that comes to mind which may be similar is how we handle something like: PREPARE a AS SELECT * from tbl WHERE name LIKE $1; Where, if $1 is 'foo' or 'foo%' we might want to use an index scan, but if $1 was '%foo' then we'd probably not. I've not yet looked into great detail of what happens here, but from some quick simple tests it seems to replan each time!? But perhaps that'd due to the parameter, where with my other tests the
[HACKERS] Missing newlines in verbose logs of pg_dump, introduced by RLS patch
Hi all, Recent commit 491c029 introducing RLS has broken a bit the verbose logs of pg_dump, one message missing a newline: + if (g_verbose) + write_msg(NULL, reading row-security enabled for table \%s\, + tbinfo-dobj.name); The patch attached corrects that. Regards, -- Michael 20140928_rls_pgdump_fix.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers