Re: [HACKERS] Planner hints in Postgresql
On Monday, March 17, 2014, Atri Sharma atri.j...@gmail.com wrote: On Mon, Mar 17, 2014 at 10:58 PM, Stephen Frost sfr...@snowman.netjavascript:_e(%7B%7D,'cvml','sfr...@snowman.net'); wrote: * Atri Sharma (atri.j...@gmail.comjavascript:_e(%7B%7D,'cvml','atri.j...@gmail.com');) wrote: Isnt using a user given value for selectivity a pretty risky situation as it can horribly screw up the plan selection? Why not allow the user to specify an alternate plan and have the planner Uh, you're worried about the user given us a garbage selectivity, but they're going to get a full-blown plan perfect? I never said that the user plan would be perfect. The entire point of planner hints is based on the assumption that the user knows more about the data than the planner does hence the user's ideas about the plan should be given a preference. Garbage selectivity can screw up the cost estimation of *all* our possible plans and we could end up preferring a sequential scan over an index only scan for e.g. I am trying to think of ways that give some preference to a user plan but do not interfere with the cost estimation of our other potential plans. I'm not opposed to planner hints (or plan mandates), but also not optimistic they will ever get implemented, much less accepted. But if they were, I don't see a use for such fudge factors. By mandating a plan, I am already asserting I know more than the optimizer does. Maybe I am right, maybe I am wrong, but either way I have taken it out of the optimizer's hands and would not welcome it snatching control back. If it is too deranged for me to trust, why would it not become somewhat more deranged and so decide to ignore my hints? The only setting for such a factor I would ever see myself using was the minimum, or the maximum. The feature I would like in such hints, if they are to exist, is to set a version to which they apply. Often a fix is made very quickly after the problem is pointed out, but it could take well over a year for the fix to see production if it is not backpatched and it lands at the wrong part of the release cycle. Cheers, Jeff
Re: [HACKERS] Planner hints in Postgresql
On Monday, March 17, 2014, Tom Lane t...@sss.pgh.pa.us wrote: Claudio Freire klaussfre...@gmail.com javascript:; writes: On Mon, Mar 17, 2014 at 7:01 PM, Jim Nasby j...@nasby.net javascript:; wrote: Even better would be if the planner could estimate how bad a plan will become if we made assumptions that turn out to be wrong. That's precisely what risk estimation was about. Yeah. I would like to see the planner's cost estimates extended to include some sort of uncertainty estimate, whereupon risk-averse people could ask it to prefer low-uncertainty plans over high-uncertainty ones (the plans we typically choose for ORDER BY ... LIMIT queries being great examples of the latter). But it's a long way from wishing that to making it so. Right now it's not even clear (to me anyway) how we'd measure or model such uncertainty. Most of the cases where I've run into horrible estimates, it seemed like the same level of knowledge/reasoning that could allow us to know it was risky, would allow us to just do a better job in the first place. The exception I can think of is in an antijoin between two huge rels. It is like subtracting two large measurements to get a much smaller result. We should know the uncertainty will be large. Cheers, Jeff
Re: [HACKERS] Triggers on foreign tables
Le mardi 18 mars 2014 03:54:19 Kouhei Kaigai a écrit : I hacked on this for awhile, but there remain two matters on which I'm uncertain about the right way forward. (1) To acquire the old tuple for UPDATE/DELETE operations, the patch closely parallels our handling for INSTEAD OF triggers on views. It adds a wholerow resjunk attribute, from which it constructs a HeapTuple before calling a trigger function. This loses the system columns, an irrelevant consideration for views but meaningful for foreign tables. postgres_fdw maintains the ctid system column (t_self), but triggers will always see an invalid t_self for the old tuple. One way to fix this is to pass around the old tuple data as ROW(ctid, oid, xmin, cmin, xmax, cmax, tableoid, wholerow). That's fairly close to sufficient, but it doesn't cover t_ctid. Frankly, I would like to find a principled excuse to not worry about making foreign table system columns accessible from triggers. Supporting system columns dramatically affects the mechanism, and what trigger is likely to care? Unfortunately, that argument seems too weak. Does anyone have a cleaner idea for keeping track of the system column values or a stronger argument for not bothering? Regarding to the first suggestion, I think, it is better not to care about system columns on foreign tables, because it fully depends on driver's implementation whether FDW fetches ctid from its data source, or not. Usually, system columns of foreign table, except for tableoid, are nonsense. Because of implementation reason, postgres_fdw fetches ctid of remote tables on UPDATE / DELETE, it is not a common nature for all FDW drivers. For example, we can assume an implementation that uses primary key of remote table to identify the record to be updated or deleted. In this case, local ctid does not have meaningful value. So, fundamentally, we cannot guarantee FDW driver returns meaningful ctid or other system columns. I agree, I think it is somewhat clunky to have postgres_fdw use a feature that is basically meaningless for other FDWs. Looking at some threads in this list, it confused many people. This is off-topic, but maybe we could devise an API allowing for local system attributes on foreign table. This would allow FDWs to carry attributes that weren't declared as part of the table definition. This could then be used for postgres_fdw ctid, as well as others foreign data wrappers equivalent of an implicit tuple id. (2) When a foreign table has an AFTER ROW trigger, the FDW's ExecForeign{Insert,Update,Delete} callbacks must return a slot covering all columns. Current FDW API documentation says things like this: The data in the returned slot is used only if the INSERT query has a RETURNING clause. Hence, the FDW could choose to optimize away returning some or all columns depending on the contents of the RETURNING clause. Consistent with that, postgres_fdw inspects the returningLists of the ModifyTable node to decide which columns are necessary. This patch has rewriteTargetListIU() add a resjunk wholerow var to the returningList of any query that will fire an AFTER ROW trigger on a foreign table. That avoids the need to change the FDW API contract or any postgres_fdw code. I was pleased about that for a time, but on further review, I'm doubting that the benefit for writable FDWs justifies muddying the definition of returningList; until now, returningList has been free from resjunk TLEs. For example, callers of FetchStatementTargetList() may be unprepared to see an all-resjunk list, instead of NIL, for a data modification statement that returns nothing. If we do keep the patch's approach, I'm inclined to rename returningList. However, I more lean toward adding a separate flag to indicate the need to return a complete tuple regardless of the RETURNING list. The benefits of overloading returningList are all short-term benefits. We know that the FDW API is still converging, so changing it seems eventually-preferable to, and safer than, changing the name or meaning of returningList. Thoughts? On Thu, Mar 06, 2014 at 09:11:19AM +0100, Ronan Dunklau wrote: Le mercredi 5 mars 2014 22:36:44 Noah Misch a écrit : Agreed. More specifically, I see only two scenarios for retrieving tuples from the tuplestore. Scenario one is that we need the next tuple (or pair of tuples, depending on the TriggerEvent). Scenario two is that we need the tuple(s) most recently retrieved. If that's correct, I'm inclined to rearrange afterTriggerInvokeEvents() and AfterTriggerExecute() to remember the tuple or pair of tuples most recently retrieved. They'll then never call tuplestore_advance() just to reposition. Do you see a problem with that? I don't see any problem with that. I don't know how this would be implemented, but
Re: [HACKERS] gaussian distribution pgbench
(2014/03/17 22:37), Tom Lane wrote: KONDO Mitsumasa kondo.mitsum...@lab.ntt.co.jp writes: (2014/03/17 18:02), Heikki Linnakangas wrote: On 03/17/2014 10:40 AM, KONDO Mitsumasa wrote: There is an infinite number of variants of the TPC-B test that we could include in pgbench. If we start adding every one of them, we're quickly going to have hundreds of options to choose the workload. I'd like to keep pgbench simple. These two new test variants, gaussian and exponential, are not that special that they'd deserve to be included in the program itself. Well, I add only two options, and they are major distribution that are seen in real database system than uniform distiribution. I'm afraid, I think you are too worried and it will not be added hundreds of options. And pgbench is still simple. FWIW, I concur with Heikki on this. Adding new versions of \setrandom is useful functionality. Embedding them in the standard test is not, because that just makes it (even) less standard. And pgbench has too darn many switches already. Hmm, I cooled down and see the pgbench option. I can understand his arguments, there are many sitches already and it will become more largear options unless we stop adding new option. However, I think that the man who added the option in the past thought the option will be useful for PostgreSQL performance improvement. But now, they are disturb the new option such like my feature which can create more real system benchmark distribution. I think it is very unfortunate and also tending to stop progress of improvement of PostgreSQL performance, not only pgbench. And if we remove command line option, I think new feature will tend to reject. It is not also good. By the way, if we remove command line option, it is difficult to understand distirbution of gaussian, because threshold parameter is very sensitive and it is also very useful feature. It is difficult and taking labor that analyzing and visualization pgbench_history using SQL. What do you think about this problem? This is not disscussed yet. [mitsu-ko@pg-rex31 pgbench]$ ./pgbench --gaussian=2 ~ access probability of top 20%, 10% and 5% records: 0.32566 0.16608 0.08345 ~ [mitsu-ko@pg-rex31 pgbench]$ ./pgbench --gaussian=4 ~ access probability of top 20%, 10% and 5% records: 0.57633 0.31086 0.15853 ~ [mitsu-ko@pg-rex31 pgbench]$ ./pgbench --gaussian=10 ~ access probability of top 20%, 10% and 5% records: 0.95450 0.68269 0.38292 ~ Regards, -- Mitsumasa KONDO NTT Open Source Software Center -- 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] gaussian distribution pgbench
(2014/03/17 23:29), Robert Haas wrote: On Sat, Mar 15, 2014 at 4:50 AM, Mitsumasa KONDO kondo.mitsum...@gmail.com wrote: There are explanations and computations as comments in the code. If it is about the documentation, I'm not sure that a very precise mathematical definition will help a lot of people, and might rather hinder understanding, so the doc focuses on an intuitive explanation instead. Yeah, I think that we had better to only explain necessary infomation for using this feature. If we add mathematical theory in docs, it will be too difficult for user. And it's waste. Well, if you *don't* include at least *some* mathematical description of what the feature does in the documentation, then users who need to understand it will have to read the source code to figure it out, which is going to be even more difficult. I had fixed this problem. Please see the v12 patch. I think it doesn't includ mathematical description, but user will be able to understand intuitive from the explanation of document. Regards, -- Mitsumasa KONDO NTT Open Source Software Center -- 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] inherit support for foreign tables
Hello, By the way, Can I have a simple script to build an environment to run this on? I built test environment and ran the simple test using postgres_fdw and got parameterized path from v3 patch on the following operation as shown there, and v6 also gives one, but I haven't seen the reparameterization of v6 patch work. # How could I think to have got it work before? Do you have any idea to make postgreReparameterizeForeignPath on foreign (child) tables works effectively? regards, ### on pg1/pg2: create table pu1 (a int not null, b int not null, c int, d text); create unique index i_pu1_ab on pu1 (a, b); create unique index i_pu1_c on pu1 (c); create table cu11 (like pu1 including all) inherits (pu1); create table cu12 (like pu1 including all) inherits (pu1); insert into cu11 (select a / 5, 4 - (a % 5), a, 'cu11' from generate_series(00, 09) a); insert into cu12 (select a / 5, 4 - (a % 5), a, 'cu12' from generate_series(10, 19) a); ### on pg1: create extension postgres_fdw; create server pg2 foreign data wrapper postgres_fdw options (host '/tmp', port '5433', dbname 'postgres'); create user mapping for current_user server pg2 options (user 'horiguti'); create foreign table _cu11 (a int, b int, c int, d text) server pg2 options (table_name 'cu11', use_remote_estimate 'true'); create foreign table _cu12 (a int, b int, c int, d text) server pg2 options (table_name 'cu12', use_remote_estimate 'true'); create table rpu1 (a int, b int, c int, d text); alter foreign table _cu11 inherit rpu1; alter foreign table _cu12 inherit rpu1; analyze rpu1; =# explain analyze select pu1.* from pu1 join rpu1 on (pu1.c = rpu1.c) where pu1.a = 3; QUERY PLAN --- Nested Loop (cost=0.00..2414.57 rows=11 width=19) (actual time=0.710..7.167 rows=5 loops=1) - Append (cost=0.00..30.76 rows=11 width=19) local side.. ommitted - Append (cost=0.00..216.68 rows=3 width=4) (actual time=0.726..1.419 rows=1 loops=5) - Seq Scan on rpu1 (cost=0.00..0.00 rows=1 width=4) (actual time=0.000..0.000 rows=0 loops=5) Filter: (pu1.c = c) - Foreign Scan on _cu11 (cost=100.30..108.34 rows=1 width=4) (actual time=0.602..0.603 rows=1 loops=5) - Foreign Scan on _cu12 (cost=100.30..108.34 rows=1 width=4) (actual time=0.566..0.566 rows=0 loops=5) Planning time: 4.452 ms Total runtime: 7.663 ms (15 rows) -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] gaussian distribution pgbench
And I find new useful point of this feature. Under following results are '--gaussian=20' case and '--gaussian=2' case, and postgresql setting is same. [mitsu-ko@pg-rex31 pgbench]$ ./pgbench -c8 -j4 --gaussian=20 -T30 -P 5 starting vacuum...end. progress: 5.0 s, 4285.8 tps, lat 1.860 ms stddev 0.425 progress: 10.0 s, 4249.2 tps, lat 1.879 ms stddev 0.372 progress: 15.0 s, 4230.3 tps, lat 1.888 ms stddev 0.430 progress: 20.0 s, 4247.3 tps, lat 1.880 ms stddev 0.400 LOG: checkpoints are occurring too frequently (12 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. progress: 25.0 s, 4269.0 tps, lat 1.870 ms stddev 0.427 progress: 30.0 s, 4318.1 tps, lat 1.849 ms stddev 0.415 transaction type: Gaussian distribution TPC-B (sort of) scaling factor: 10 standard deviation threshold: 20.0 access probability of top 20%, 10% and 5% records: 0.4 0.95450 0.68269 query mode: simple number of clients: 8 number of threads: 4 duration: 30 s number of transactions actually processed: 128008 latency average: 1.871 ms latency stddev: 0.412 ms tps = 4266.266374 (including connections establishing) tps = 4267.312022 (excluding connections establishing) [mitsu-ko@pg-rex31 pgbench]$ ./pgbench -c8 -j4 --gaussian=2 -T30 -P 5 starting vacuum...end. LOG: checkpoints are occurring too frequently (13 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (1 second apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. progress: 5.0 s, 3927.9 tps, lat 2.030 ms stddev 0.691 LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (1 second apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. progress: 10.0 s, 4045.8 tps, lat 1.974 ms stddev 0.835 LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (1 second apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. progress: 15.0 s, 4042.5 tps, lat 1.976 ms stddev 0.613 LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. progress: 20.0 s, 4103.9 tps, lat 1.946 ms stddev 0.540 LOG: checkpoints are occurring too frequently (1 second apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. progress: 25.0 s, 4003.1 tps, lat 1.995 ms stddev 0.526 LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (1 second apart) HINT: Consider increasing the configuration parameter checkpoint_segments. LOG: checkpoints are occurring too frequently (2 seconds apart) HINT: Consider increasing the configuration parameter checkpoint_segments. progress: 30.0 s, 4025.5 tps, lat 1.984 ms stddev 0.568 transaction type: Gaussian distribution TPC-B (sort of) scaling factor: 10 standard deviation threshold: 2.0 access probability of top 20%, 10% and 5% records: 0.32566 0.16608 0.08345 query mode: simple number of clients: 8 number of threads: 4 duration: 30 s number of transactions actually processed: 120752 latency average: 1.984 ms latency stddev: 0.638 ms tps = 4024.823433 (including connections establishing) tps = 4025.87 (excluding connections establishing) In '--gaussian=2' benchmark, checkpoint is frequently happen than '--gaussian=20' benchmark. Because former update large range of records so that fullpage write WALs are bigger than later. Former distribution updates large range of records, so that fullpage-write WALs are
Re: [HACKERS] gaussian distribution pgbench
On 03/18/2014 11:57 AM, KONDO Mitsumasa wrote: I think that this feature will be also useful for survey new buffer-replace algorithm and checkpoint strategy, so on. Sure. No doubt about that. If we remove this option, it is really dissapointed.. As long as we get the \setrandom changes in, you can easily do these tests using a custom script. There's nothing wrong with using a custom script, it will be just as useful for exploring buffer replacement algorithms, checkpoints etc. as a built-in option. - 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_dump without explicit table locking
On 18.03.14 00:15, Tom Lane wrote: Jim Nasby j...@nasby.net writes: On 3/17/14, 8:47 AM, Tom Lane wrote: (Note that this is only one of assorted O(N^2) behaviors in older versions of pg_dump; we've gradually stamped them out over time.) On that note, it's recommended that when you are taking a backup to restore into a newer version of Postgres you create the dump using the NEWER version of pg_dump, not the old one. Right. IIRC, the OP said he *did* use a recent pg_dump ... but this particular issue got fixed server-side, so the new pg_dump didn't help against an 8.1 server :-( Yes, I did use 9.3's pg_dump against my 8.1 DB initially. The patch was created against github's master. -Jürgen -- 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] ALTER TABLE lock strength reduction patch is unsafe Reply-To:
On 8 March 2014 11:14, Simon Riggs si...@2ndquadrant.com wrote: On 7 March 2014 09:04, Simon Riggs si...@2ndquadrant.com wrote: The right thing to do here is to not push to the extremes. If we mess too much with the ruleutil stuff it will just be buggy. A more considered analysis in a later release is required for a full and complete approach. As I indicated earlier, an 80/20 solution is better for this release. Slimming down the patch, I've removed changes to lock levels for almost all variants. The only lock levels now reduced are those for VALIDATE, plus setting of relation and attribute level options. VALIDATE is implemented by calling pg_get_constraintdef_mvcc(), a slightly modified variant of pg_get_constraintdef that uses the transaction snapshot. I propose this rather than Noah's solution solely because this will allow any user to request the MVCC data, rather than implement a hack that only works for pg_dump. I will post the patch later today. Implemented in attached patch, v22 The following commands (only) are allowed with ShareUpdateExclusiveLock, patch includes doc changes. ALTER TABLE ... VALIDATE CONSTRAINT constraint_name covered by isolation test, plus verified manually with pg_dump ALTER TABLE ... ALTER COLUMN ... SET STATISTICS ALTER TABLE ... ALTER COLUMN ... SET (...) ALTER TABLE ... ALTER COLUMN ... RESET (...) ALTER TABLE ... CLUSTER ON ... ALTER TABLE ... SET WITHOUT CLUSTER ALTER TABLE ... SET (...) covered by isolation test ALTER TABLE ... RESET (...) ALTER INDEX ... SET (...) ALTER INDEX ... RESET (...) All other ALTER commands take AccessExclusiveLock I commend this patch to you for final review; I would like to commit this in a few days. I'm planning to commit this today at 1500UTC barring objections or negative reviews. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] GSoC question
Hi, PostgreSQL community! My name is Artur Gadelshin, I'm from Saint-Petersburg, Russia. I'm 4th year student in Computer Scince. I haven't enough experience in C/C++, but I really in love with PostgreSQL (but, yes, haven't experience to develop this). My dream is to be computer scientist or data scientist. So, I should read a lot of books about data structures, mathematics, algorithms and so on. Is there any chance to participate in your community and get some help in envolving, may be simple tasks, advices about books which I should read or smth else. If I will be useless in GSoC projects, I glad to participate without GSoC with your help and advices. What I already know: Programming languages: Erlang, Python, basic knowledge of Clojure/Java/Scala/C OS: advanced knowledge of Linux, I'm very interested in learning, how operating systems works and Linux kernel development.DBMS: SQL (MariaDB, MySQL), Riak, CouchDB, PostgreSQL Experience: RESTful web service development: API for cloud storage, VDS service and internal hosting company backend with Python, Erlang, cowboy_rest, MySQL/Riak.
Re: [HACKERS] GSoC question
Artur, * Artur Gadelshin (ar.gadels...@gmail.com) wrote: Is there any chance to participate in your community and get some help in envolving, may be simple tasks, advices about books which I should read or smth else. There are a number of GSoC project ideas here: https://wiki.postgresql.org/wiki/GSoC_2014 If I will be useless in GSoC projects, I glad to participate without GSoC with your help and advices. It's up to you to submit a proposal for GSoC, if you're interested in participating and meet the requirements. We'd certainly welcome new members to the community even if you don't want to participate in GSoC. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] B-tree descend for insertion locking
When inserting into a B-tree index, all the pages are read-locked when descending the tree. When we reach the leaf page, the read-lock is exchanged for a write-lock. There's nothing wrong with that, but why don't we just directly grab a write-lock on the leaf page? When descending, we know the level we're on, and what level the child page is. The only downside I can see is that we would unnecessarily hold a write-lock when a read-lock would suffice, if the page was just split and we have to move right. But that seems like a really bad bet - hitting the page when it was just split is highly unlikely. Grabbing the write-lock directly would obviously save one buffer unlock+lock sequence, for what it's worth, but I think it would also slightly simplify the code. Am I missing something? See attached patch. The new contract of _bt_getroot is a bit weird: it locks the returned page in the requested lock-mode, shared or exclusive, if the root page was also the leaf page. Otherwise it's locked in shared mode regardless off the requested lock mode. But OTOH, the new contract for _bt_search() is more clear now: it actually locks the returned page in the requested mode, where it used to only use the access parameter to decide whether to create a root page if the index was empty. - Heikki diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c index 5f7953f..3cb6404 100644 --- a/src/backend/access/nbtree/nbtinsert.c +++ b/src/backend/access/nbtree/nbtinsert.c @@ -113,24 +113,11 @@ _bt_doinsert(Relation rel, IndexTuple itup, itup_scankey = _bt_mkscankey(rel, itup); top: - /* find the first page containing this key */ + /* find the first page containing this key, and write-lock it */ stack = _bt_search(rel, natts, itup_scankey, false, buf, BT_WRITE); offset = InvalidOffsetNumber; - /* trade in our read lock for a write lock */ - LockBuffer(buf, BUFFER_LOCK_UNLOCK); - LockBuffer(buf, BT_WRITE); - - /* - * If the page was split between the time that we surrendered our read - * lock and acquired our write lock, then this page may no longer be the - * right place for the key we want to insert. In this case, we need to - * move right in the tree. See Lehman and Yao for an excruciatingly - * precise description. - */ - buf = _bt_moveright(rel, buf, natts, itup_scankey, false, BT_WRITE); - /* * If we're not allowing duplicates, make sure the key isn't already in * the index. diff --git a/src/backend/access/nbtree/nbtpage.c b/src/backend/access/nbtree/nbtpage.c index 14e422a..d6a4933 100644 --- a/src/backend/access/nbtree/nbtpage.c +++ b/src/backend/access/nbtree/nbtpage.c @@ -78,12 +78,13 @@ _bt_initmetapage(Page page, BlockNumber rootbknum, uint32 level) * standard class of race conditions exists here; I think I covered * them all in the Hopi Indian rain dance of lock requests below. * - * The access type parameter (BT_READ or BT_WRITE) controls whether - * a new root page will be created or not. If access = BT_READ, - * and no root page exists, we just return InvalidBuffer. For - * BT_WRITE, we try to create the root page if it doesn't exist. - * NOTE that the returned root page will have only a read lock set - * on it even if access = BT_WRITE! + * The access type parameter (BT_READ or BT_WRITE) indicates whether + * we're fetching the root for a search or an insertion. If access = + * BT_READ, and no root page exists, we just return InvalidBuffer. + * For BT_WRITE, we try to create the root page if it doesn't exist. + * If the root page is also a leaf-page, it is locked in the mode + * specified. NOTE that otherwise the returned root page will have + * only a read lock on it, even if access = BT_WRITE! * * The returned page is not necessarily the true root --- it could be * a fast root (a page that is alone in its level due to deletions). @@ -127,7 +128,8 @@ _bt_getroot(Relation rel, int access) Assert(rootblkno != P_NONE); rootlevel = metad-btm_fastlevel; - rootbuf = _bt_getbuf(rel, rootblkno, BT_READ); + rootbuf = _bt_getbuf(rel, rootblkno, + (rootlevel == 0) ? access : BT_READ); rootpage = BufferGetPage(rootbuf); rootopaque = (BTPageOpaque) PageGetSpecialPointer(rootpage); @@ -253,14 +255,6 @@ _bt_getroot(Relation rel, int access) END_CRIT_SECTION(); - /* - * swap root write lock for read lock. There is no danger of anyone - * else accessing the new root page while it's unlocked, since no one - * else knows where it is yet. - */ - LockBuffer(rootbuf, BUFFER_LOCK_UNLOCK); - LockBuffer(rootbuf, BT_READ); - /* okay, metadata is correct, release lock on it */ _bt_relbuf(rel, metabuf); } @@ -285,7 +279,8 @@ _bt_getroot(Relation rel, int access) for (;;) { - rootbuf = _bt_relandgetbuf(rel, rootbuf, rootblkno, BT_READ); + rootbuf = _bt_relandgetbuf(rel, rootbuf, rootblkno, + (rootlevel == 0) ? access : BT_READ); rootpage =
Re: [HACKERS] Triggers on foreign tables
On Mon, Mar 17, 2014 at 11:54 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote: I hacked on this for awhile, but there remain two matters on which I'm uncertain about the right way forward. (1) To acquire the old tuple for UPDATE/DELETE operations, the patch closely parallels our handling for INSTEAD OF triggers on views. It adds a wholerow resjunk attribute, from which it constructs a HeapTuple before calling a trigger function. This loses the system columns, an irrelevant consideration for views but meaningful for foreign tables. postgres_fdw maintains the ctid system column (t_self), but triggers will always see an invalid t_self for the old tuple. One way to fix this is to pass around the old tuple data as ROW(ctid, oid, xmin, cmin, xmax, cmax, tableoid, wholerow). That's fairly close to sufficient, but it doesn't cover t_ctid. Frankly, I would like to find a principled excuse to not worry about making foreign table system columns accessible from triggers. Supporting system columns dramatically affects the mechanism, and what trigger is likely to care? Unfortunately, that argument seems too weak. Does anyone have a cleaner idea for keeping track of the system column values or a stronger argument for not bothering? Regarding to the first suggestion, I think, it is better not to care about system columns on foreign tables, because it fully depends on driver's implementation whether FDW fetches ctid from its data source, or not. Usually, system columns of foreign table, except for tableoid, are nonsense. Because of implementation reason, postgres_fdw fetches ctid of remote tables on UPDATE / DELETE, it is not a common nature for all FDW drivers. For example, we can assume an implementation that uses primary key of remote table to identify the record to be updated or deleted. In this case, local ctid does not have meaningful value. So, fundamentally, we cannot guarantee FDW driver returns meaningful ctid or other system columns. I'm not sure I particularly agree with this reasoning - after all, just because some people might not find a feature useful isn't a reason not to have it. On the other hand, I don't think it's a very useful feature, and I don't feel like we have to have it. Most system columns can't be updated or indexed, and none of them can be dropped or renamed, so it's not like they aren't second-class citizens to some degree already. By way of comparison, the first version of the index-only scan patch gave up when it saw an expression index, instead of making an effort to figure out whether a matching expression was present in the query. Somebody could have looked at that patch and said, oh, well, that's an ugly and unacceptable limitation, and we ought to reject the patch until it's fixed. Well, instead, Tom committed the patch, and we still have that limitation today, and it's still something we really ought to fix some day, but in the meantime we have the feature. Obviously, the line between your patch is only part of a feature, please finish it and try again and your patch implements a nice self-contained feature and there are some more things that we could build on top of it later is to some extent a matter of judgement; but for what it's worth, I can't get too excited about this particular limitation of this particular patch. I just don't think that very many people are going to miss the functionality in question. -- 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] Replication slots and footguns
On Thu, Mar 13, 2014 at 9:07 PM, Josh Berkus j...@agliodbs.com wrote: On 03/13/2014 05:28 PM, Robert Haas wrote: Well we may have kind of hosed ourselves, because the in-memory data structures that represent the data structure have an in_use flag that indicates whether the structure is allocated at all, and then an active flag that indicates whether some backend is using it. I never liked that naming much. Maybe we should go through and let in_use - allocated and active - in_use. Wait, which one of those does pg_drop_replication_slot() care about? Well... the slots that aren't in_use can't be dropped because they don't exist in the first place. The ones that aren't active can't be dropped because somebody else is using them. So both, sorta, I guess? -- 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] on_exit_reset fails to clear DSM-related exit actions
On Mon, Mar 17, 2014 at 1:41 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: After mulling over a few possible approaches, I came up with the attached, which seems short and to the point. Looks reasonable in principle. I didn't run through all the existing PGSharedMemoryDetach calls to see if there are any other places to call dsm_detach_all, but it's an easy fix if there are any. OK, committed. -- 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] plpgsql.warn_shadow
On 14/03/14 13:12, Simon Riggs wrote: On 14 March 2014 11:10, Pavel Stehule pavel.steh...@gmail.com wrote: 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja ma...@joh.to: On 3/14/14 10:56 AM, Simon Riggs wrote: The patch looks fine, apart from some non-guideline code formatting issues. I'm not sure what you're referring to. I thought it looked fine. Having looked at gcc and clang, I have a proposal for naming/API We just have two variables plpgsql.compile_warnings = 'list'default = 'none' +1 plpgsql.compile_errors = 'list'default = 'none' Only possible values in 9.4 are 'shadow', 'all', 'none' what is compile_errors ? We don't allow to ignore any error now. How about plpgsql.additional_warnings = 'list' plpgsql.additional_errors = 'list' Agree that compile_errors dos not make sense, additional_errors and additional_warnings seems better, maybe plpgsql.extra_warnings and plpgsql.extra_errors? -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_dump without explicit table locking
On 18.03.14 02:32, Joe Conway wrote: On 03/17/2014 05:55 PM, Jeff Janes wrote: On Mon, Mar 17, 2014 at 5:48 PM, Craig Ringer cr...@2ndquadrant.com I wonder if doing large batches of LOCK TABLE table1, table2, table3, ... would help, instead of doing individual statements? If I recall correctly, someone did submit a patch to do that. It helped when dumping schema only, but not much when dumping data. Not surprising at all. The huge time is incurred in taking the locks, but if you are trying to use pg_upgrade in link mode to speed your upgrade, you are totally hosed by the time it takes to grab those locks. This patch applied to 9.3 substantially fixes the issue: 8--- commit eeb6f37d89fc60c6449ca12ef9e91491069369cb Author: Heikki Linnakangas heikki.linnakan...@iki.fi Date: Thu Jun 21 15:01:17 2012 +0300 Add a small cache of locks owned by a resource owner in ResourceOwner. 8--- On my 8.4 database, with 500,000 tables there were about 2.5 million locks taken including toast tables and indexes during the schema dump. Without the patch grabbing locks took many, many days with that many objects to lock. With a backported version of the patch, one hour. So if you have a problem due to many tables on an older than 9.3 version of Postgres, this is the direction to head (a custom patch applied to your old version just long enough to get successfully upgraded). In a testing environment I restored my 8.1 DB with 300,000 tables to a 9.3 server (using my patched pg_dump). Then I ran the original 9.3 pg_dump against the 9.3 DB again, and it works reasonably well. So I can confirm the server side improvements in 9.3 do to work for my test case. Still when I finally get around to do this on production I plan to use my patched pg_dump rather than backporting the server fix to 8.1, as I'd rather not touch our already-patched-for-something-else 8.1 server. I can't wait to get my hand on 9.x replication features and other stuff :-) -Jürgen -- 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] HEAD seems to generate larger WAL regarding GIN index
On Mon, Mar 17, 2014 at 10:44 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Fujii Masao escribió: On Sun, Mar 16, 2014 at 7:15 AM, Alexander Korotkov aekorot...@gmail.com wrote: That could be optimized, but I figured we can live with it, thanks to the fastupdate feature. Fastupdate allows amortizing that cost over several insertions. But of course, you explicitly disabled that... Let me know if you want me to write patch addressing this issue. Yeah, I really want you to address this problem! That's definitely useful for every users disabling FASTUPDATE option for some reasons. Users that disable FASTUPDATE, in my experience, do so because their stock work_mem is way too high and GIN searches become too slow due to having to scan too large a list. Yes. Another reason that I've heard from users so far is that the size of GIN index with FASTUPDATE=off is likely to be smaller than that with FASTUPDATE=on. I think it might make sense to invest a modest amount of time in getting FASTUPDATE to be sized completely differently from today -- perhaps base it on a hardcoded factor of BLCKSZ, rather than work_mem. Or, if we really need to make it configurable, then let it have its own parameter. I prefer to have the parameter. When users create multiple GIN indexes for various uses, they might want to use different thresholds of the pending list for each index. So, GIN index parameter might be better than GUC one. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Make it easy to detach completely from shared memory.
On 18 March 2014 11:59, Robert Haas rh...@postgresql.org wrote: Make it easy to detach completely from shared memory. The new function dsm_detach_all() can be used either by postmaster children that don't wish to take any risk of accidentally corrupting shared memory; or by forked children of regular backends with the same need. This patch also updates the postmaster children that already do PGSharedMemoryDetach() to do dsm_detach_all() as well. Per discussion with Tom Lane. I think we need to document exactly why dsm_detach_all() isn't simply part of PGSharedMemoryDetach() ? Having two calls seems like a recipe for error in core and extensions. Perhaps we should consider a parameter for PGSharedMemoryDetach() ? -- 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] plpgsql.warn_shadow
2014-03-18 13:23 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: On 14/03/14 13:12, Simon Riggs wrote: On 14 March 2014 11:10, Pavel Stehule pavel.steh...@gmail.com wrote: 2014-03-14 12:02 GMT+01:00 Marko Tiikkaja ma...@joh.to: On 3/14/14 10:56 AM, Simon Riggs wrote: The patch looks fine, apart from some non-guideline code formatting issues. I'm not sure what you're referring to. I thought it looked fine. Having looked at gcc and clang, I have a proposal for naming/API We just have two variables plpgsql.compile_warnings = 'list'default = 'none' +1 plpgsql.compile_errors = 'list'default = 'none' Only possible values in 9.4 are 'shadow', 'all', 'none' what is compile_errors ? We don't allow to ignore any error now. How about plpgsql.additional_warnings = 'list' plpgsql.additional_errors = 'list' Agree that compile_errors dos not make sense, additional_errors and additional_warnings seems better, maybe plpgsql.extra_warnings and plpgsql.extra_errors? extra* sounds better Pavel -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] plpgsql.warn_shadow
On 18 March 2014 12:23, Petr Jelinek p...@2ndquadrant.com wrote: Agree that compile_errors dos not make sense, additional_errors and additional_warnings seems better, maybe plpgsql.extra_warnings and plpgsql.extra_errors? That sems better to me also. -- 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] [COMMITTERS] pgsql: Make it easy to detach completely from shared memory.
On Tue, Mar 18, 2014 at 8:41 AM, Simon Riggs si...@2ndquadrant.com wrote: On 18 March 2014 11:59, Robert Haas rh...@postgresql.org wrote: Make it easy to detach completely from shared memory. The new function dsm_detach_all() can be used either by postmaster children that don't wish to take any risk of accidentally corrupting shared memory; or by forked children of regular backends with the same need. This patch also updates the postmaster children that already do PGSharedMemoryDetach() to do dsm_detach_all() as well. Per discussion with Tom Lane. I think we need to document exactly why dsm_detach_all() isn't simply part of PGSharedMemoryDetach() ? Having two calls seems like a recipe for error in core and extensions. Perhaps we should consider a parameter for PGSharedMemoryDetach() ? Yeah, maybe. It seems like a possible modularity violation, because the PGSharedMemory... stuff has heretofore not needed to know anything about DSM, and apart from this one function, it still wouldn't. On the other hand, your argument is good, too. -- 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] Is this a bug?
On Mon, Mar 17, 2014 at 10:27 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Mar 18, 2014 at 10:24 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Mar 13, 2014 at 10:22 AM, Robert Haas robertmh...@gmail.com wrote: Well, it's fairly harmless, but it might not be a bad idea to tighten that up. The attached patch tighten that up. Hm... It might be interesting to include it in 9.4 IMO, somewhat grouping with what has been done in a6542a4 for SET and ABORT. Meh. There will always be another thing we could squeeze in; I don't think this is particularly urgent, and it's late to the party. -- 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] GSoC proposal. Index-only scans for GIST
Hello! Here is the text of my proposal which I've applied to GSoC. (and link http://www.google-melange.com/gsoc/proposal/public/google/gsoc2014/lubennikovaav/5629499534213120) Any suggestions and comments are welcome. *Project name* Support for index-only scans for GIST index *Brief review* Currently GiST index don't have index-only scan functionality. Index-only scans are a major performance feature added to Postgres 9.2. They allow certain types of queries to be satisfied just by retrieving data from indexes, and not from tables. This feature for B-trees (implemented since PostgreSQL-9.2) allows doing certain types of queries significantly faster. This is achieved by reduction in the amount of I/O necessary to satisfy queries. I think it will be useful to implement this feature for user defined data types that use GiST index. *Benefits to the PostgreSQL Community* Faster GiST index search would be actual for many PostgreSQL applications (for example some GIS systems). *Quantifiable results* Acceleration of GiST index search. *Project details* 1. The GiST is a balanced tree structure like a B-tree, containing key, pointer pairs. GiST key is a member of a user-defined class, and represents some property that is true of all data items reachable from the pointer associated with the key. The GiST provides a possibility to create custom data types with indexed access methods and extensible set of queries. There are seven methods that an index operator class for GiST must provide, and an eighth that is optional. -consistent -union -compress -decompress -penalty -picksplit -equal -distance (optional) I'm going to create new optional method fetch() in addition to existing. Thus user can create a method of retrieving data from the index without loss. It will be used when performing search queries to speed data retrieving. 2. gistget algorithm (several parts omitted): Check the key gistindex_keytest() - does this index tuple satisfy the scan key(s)? Scan all items on the GiST index page and insert them into the queue (or directly to output areas) plain scan Heap tuple TIDs are returned into so-pageData[] ordered scan Heap tuple TIDs are pushed into individual search queue items If the fetch() is specified by the developer, then using it, algorithm can retrieve the data directly to output areas at this stage, without reference to the heap. 3. Create function gistcanreturn to check whether fetch() is specified by user. Amcanreturn - Function to check whether index supports index-only scans, or zero if none There is the question of support index-only scans for multicolumn indexes. Probably it would require extend the interface to add separate columns checking. To solve this question I need the community's help. 4. Add details for index only scans into gistcostestimate function *Links* 1) Hellerstein J. M., Naughton J. F., Pfeffer A. Generalized search trees for database systems. - September, 1995. 2) http://www.sai.msu.su/~megera/postgres/gist/ 3) PostgreSQL 9.3.3 Documentation: chapters 11. Indexes, 55. GiST Indexes. -- Best regards, Lubennikova Anastasia
Re: [HACKERS] pg_archivecleanup bug
On Tue, Mar 18, 2014 at 11:25:46AM +0530, Amit Kapila wrote: On Thu, Mar 13, 2014 at 11:18 AM, Bruce Momjian br...@momjian.us wrote: I have developed the attached patch which fixes all cases where readdir() wasn't checking for errno, and cleaned up the syntax in other cases to be consistent. 1. One common thing missed wherever handling for errno is added is below check which is present in all existing cases where errno is used (initdb.c, pg_resetxlog.c, ReadDir, ..) #ifdef WIN32 /* * This fix is in mingw cvs (runtime/mingwex/dirent.c rev 1.4), but not in * released version */ if (GetLastError() == ERROR_NO_MORE_FILES) errno = 0; #endif Very good point. I have modified the patch to add this block in all cases where it was missing. I started to wonder about the comment and if the Mingw fix was released. Based on some research, I see this as fixed in mingw-runtime-3.2, released 2003-10-10. That's pretty old. (What I don't know is when that was paired with Msys in a bundled release.) Here is the Mingw fixed code: http://ftp.ntua.gr/mirror/mingw/OldFiles/mingw-runtime-3.2-src.tar.gz { /* Get the next search entry. */ if (_tfindnext (dirp-dd_handle, (dirp-dd_dta))) { /* We are off the end or otherwise error. _findnext sets errno to ENOENT if no more file Undo this. */ DWORD winerr = GetLastError(); if (winerr == ERROR_NO_MORE_FILES) errno = 0; The current code has a better explanation: http://sourceforge.net/p/mingw/mingw-org-wsl/ci/master/tree/src/libcrt/tchar/dirent.c if( dirp-dd_private.dd_stat++ 0 ) { /* Otherwise... * * Get the next search entry. POSIX mandates that this must * return NULL after the last entry has been read, but that it * MUST NOT change errno in this case. MS-Windows _findnext() * DOES change errno (to ENOENT) after the last entry has been * read, so we must be prepared to restore it to its previous * value, when no actual error has occurred. */ int prev_errno = errno; if( DIRENT_UPDATE( dirp-dd_private ) != 0 ) { /* May be an error, or just the case described above... */ if( GetLastError() == ERROR_NO_MORE_FILES ) /* * ...which requires us to reset errno. */ errno = prev_errno; but it is basically doing the same thing. I am wondering if we should back-patch the PG code block where it was missing, and remove it from head in all places on the logic that everyone running 9.4 will have a post-3.1 version of Mingw. Postgres 8.4 was released in 2009 and it is possible some people are still using pre-3.2 Mingw versions with that PG release. 2. ! if (errno || closedir(chkdir) == -1) result = -1; /* some kind of I/O error? */ Is there a special need to check return value of closedir in this function, as all other uses (initdb.c, pg_resetxlog.c, pgfnames.c) of it in similar context doesn't check the same? One thing I think for which this code needs change is to check errno before closedir as is done in initdb.c or pg_resetxlog.c Yes, good point. Patch adjusted to add this. While I am not a fan of backpatching, the fact we are ignoring errors in some critical cases seems the non-cosmetic parts should be backpatched. +1 The larger the patch gets, the more worried I am about backpatching. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_basebackup --slot=SLOTNAME -X stream
Hi, Is there the explicit reason why --slot option that pg_receivexlog already has has not been added into pg_basebackup? If not, I'd like to support that option to eliminate the need to increase wal_keep_segments for pg_basebackup. Thought? Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup --slot=SLOTNAME -X stream
Hi, On 2014-03-18 22:25:05 +0900, Fujii Masao wrote: Is there the explicit reason why --slot option that pg_receivexlog already has has not been added into pg_basebackup? If not, I'd like to support that option to eliminate the need to increase wal_keep_segments for pg_basebackup. Thought? I think to make the option really interesting for pg_basebackup we'd need to make pg_basebackup create the slot, use it for streaming, and then dump it afterwards. At the moment the walsender interface isn't expressive enough to do that safely, without any chance of leaving a slot behind on error. I don't have time to implement the necessary scaffolding for 9.4. I wouldn't mind a raw --slot argument, that requires the user to create/drop the slot outside the calls to pg_basebackup. I just am not sure how interesting it is to the majority of users. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup --slot=SLOTNAME -X stream
On 03/18/2014 03:25 PM, Fujii Masao wrote: Hi, Is there the explicit reason why --slot option that pg_receivexlog already has has not been added into pg_basebackup? If not, I'd like to support that option to eliminate the need to increase wal_keep_segments for pg_basebackup. Thought? That seems rather cumbersome. pg_basebackup is a run once, and then it dies. The slot would have to be created before running pg_basebackup, and destroyed manually. It would make sense to provide a new option that would tell the server to recycle segments until they've been sent to this client, or until the client disconnects. But that would look quite different from pg_receivexlog's --slot option. - 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_basebackup --slot=SLOTNAME -X stream
On Tue, Mar 18, 2014 at 2:32 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 03/18/2014 03:25 PM, Fujii Masao wrote: Hi, Is there the explicit reason why --slot option that pg_receivexlog already has has not been added into pg_basebackup? If not, I'd like to support that option to eliminate the need to increase wal_keep_segments for pg_basebackup. Thought? That seems rather cumbersome. pg_basebackup is a run once, and then it dies. The slot would have to be created before running pg_basebackup, and destroyed manually. It would make sense to provide a new option that would tell the server to recycle segments until they've been sent to this client, or until the client disconnects. But that would look quite different from pg_receivexlog's --slot option. I started working on that at some point long ago, but never got far enough. Right now, it seems the right way to do that is to somehow reimplement it on top of a lightweight slot that gets automatically destroyed when pg_basebackup stops. But as Andres said, there's a little more to it. You'd want the slot to be created in the connection that does BASE_BACKUP, then used by the replication client side, and then destroyed by BASE_BACKUP. But you also want it auto-destroyed if the BASE_BACKUP connection gets terminated for example.. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] pg_archivecleanup bug
Bruce Momjian br...@momjian.us writes: Very good point. I have modified the patch to add this block in all cases where it was missing. I started to wonder about the comment and if the Mingw fix was released. Based on some research, I see this as fixed in mingw-runtime-3.2, released 2003-10-10. That's pretty old. Yeah. I would vote for removing that code in all branches. There is no reason to suppose somebody is going to install 8.4.22 on a machine that they haven't updated mingw on since 2003. Or, if you prefer, just remove it in HEAD --- but going around and *adding* more copies seems like make-work. The fact that we've not heard complaints about the omissions is good evidence that nobody's using the buggy mingw versions anymore. 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] [COMMITTERS] pgsql: Make it easy to detach completely from shared memory.
Robert Haas robertmh...@gmail.com writes: On Tue, Mar 18, 2014 at 8:41 AM, Simon Riggs si...@2ndquadrant.com wrote: Perhaps we should consider a parameter for PGSharedMemoryDetach() ? Yeah, maybe. It seems like a possible modularity violation, because the PGSharedMemory... stuff has heretofore not needed to know anything about DSM, and apart from this one function, it still wouldn't. That was exactly the reason we rejected that design upthread. PGSharedMemoryDetach is specific to the main shmem segment, and in fact has multiple OS-dependent implementations. You could make an argument for inventing some new wrapper function that calls both PGSharedMemoryDetach and dsm_detach_all, but I don't believe that the existing flavors of that function should know about DSM. 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_archivecleanup bug
On Tue, Mar 18, 2014 at 9:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: Bruce Momjian br...@momjian.us writes: Very good point. I have modified the patch to add this block in all cases where it was missing. I started to wonder about the comment and if the Mingw fix was released. Based on some research, I see this as fixed in mingw-runtime-3.2, released 2003-10-10. That's pretty old. Yeah. I would vote for removing that code in all branches. There is no reason to suppose somebody is going to install 8.4.22 on a machine that they haven't updated mingw on since 2003. Or, if you prefer, just remove it in HEAD --- but going around and *adding* more copies seems like make-work. The fact that we've not heard complaints about the omissions is good evidence that nobody's using the buggy mingw versions anymore. I don't think it is. Right now we're not checking errno *at all* in a bunch of these places, so we're sure not going to get complaints about doing it incorrectly in those places. Or do I need more caffeine? -- 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_archivecleanup bug
On Tue, Mar 18, 2014 at 10:03:46AM -0400, Robert Haas wrote: On Tue, Mar 18, 2014 at 9:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: Bruce Momjian br...@momjian.us writes: Very good point. I have modified the patch to add this block in all cases where it was missing. I started to wonder about the comment and if the Mingw fix was released. Based on some research, I see this as fixed in mingw-runtime-3.2, released 2003-10-10. That's pretty old. Yeah. I would vote for removing that code in all branches. There is no reason to suppose somebody is going to install 8.4.22 on a machine that they haven't updated mingw on since 2003. Or, if you prefer, just remove it in HEAD --- but going around and *adding* more copies seems like make-work. The fact that we've not heard complaints about the omissions is good evidence that nobody's using the buggy mingw versions anymore. I don't think it is. Right now we're not checking errno *at all* in a bunch of these places, so we're sure not going to get complaints about doing it incorrectly in those places. Or do I need more caffeine? You are correct. This code is seriously broken and I am susprised we have not gotten more complaints. Good thing readdir/closedir rarely fail. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Make it easy to detach completely from shared memory.
On 18 March 2014 13:51, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Mar 18, 2014 at 8:41 AM, Simon Riggs si...@2ndquadrant.com wrote: Perhaps we should consider a parameter for PGSharedMemoryDetach() ? Yeah, maybe. It seems like a possible modularity violation, because the PGSharedMemory... stuff has heretofore not needed to know anything about DSM, and apart from this one function, it still wouldn't. That was exactly the reason we rejected that design upthread. PGSharedMemoryDetach is specific to the main shmem segment, and in fact has multiple OS-dependent implementations. You could make an argument for inventing some new wrapper function that calls both PGSharedMemoryDetach and dsm_detach_all, but I don't believe that the existing flavors of that function should know about DSM. I'm not bothered which we choose, as long as its well documented to ensure people that use those calls don't detach from just one when they really would wish to detach from both. -- 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_basebackup --slot=SLOTNAME -X stream
On Tue, Mar 18, 2014 at 9:46 AM, Magnus Hagander mag...@hagander.net wrote: I started working on that at some point long ago, but never got far enough. Right now, it seems the right way to do that is to somehow reimplement it on top of a lightweight slot that gets automatically destroyed when pg_basebackup stops. But as Andres said, there's a little more to it. You'd want the slot to be created in the connection that does BASE_BACKUP, then used by the replication client side, and then destroyed by BASE_BACKUP. But you also want it auto-destroyed if the BASE_BACKUP connection gets terminated for example.. The slot machinery already has provision for drop-on-release slots. But I don't think we expose that at the protocol level in any way just yet. CREATE_REPLICATION_SLOT ... TEMPORARY or something might not be that hard, but at this point I think it's too late to invent new things for 9.4. I am hoping for a raft of improvements to the slot stuff for next release, but trying to do that now seems like pushing our luck. -- 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_archivecleanup bug
Bruce Momjian escribió: On Tue, Mar 18, 2014 at 10:03:46AM -0400, Robert Haas wrote: On Tue, Mar 18, 2014 at 9:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: Bruce Momjian br...@momjian.us writes: Very good point. I have modified the patch to add this block in all cases where it was missing. I started to wonder about the comment and if the Mingw fix was released. Based on some research, I see this as fixed in mingw-runtime-3.2, released 2003-10-10. That's pretty old. Yeah. I would vote for removing that code in all branches. There is no reason to suppose somebody is going to install 8.4.22 on a machine that they haven't updated mingw on since 2003. Or, if you prefer, just remove it in HEAD --- but going around and *adding* more copies seems like make-work. The fact that we've not heard complaints about the omissions is good evidence that nobody's using the buggy mingw versions anymore. I don't think it is. Right now we're not checking errno *at all* in a bunch of these places, so we're sure not going to get complaints about doing it incorrectly in those places. Or do I need more caffeine? You are correct. This code is seriously broken and I am susprised we have not gotten more complaints. Good thing readdir/closedir rarely fail. I think we need to keep the check for old mingw runtime in older branches; it seems reasonable to keep updating Postgres when new versions come out but keep mingw the same if it doesn't break. A good criterion here, to me, is: would we make it a runtime error if an old mingw version is detected? If we would, then let's go and remove all those errno checks. Then we force everyone to update to a sane mingw. But if we're not adding such a check, then we might cause subtle trouble just because we think running old mingw is unlikely. On another note, please let's not make the code dissimilar in some branches just because of source code embellishments are not back-ported out of fear. I mean, if we want them in master, let them be in older branches as well. Otherwise we end up with slightly different versions that make back-patching future fixes a lot harder, for no gain. -- Á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] Portability issues in shm_mq
On Mon, Mar 17, 2014 at 11:09 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: OK, I tried this out. The major complication that cropped up was that, if we make the length word always a Size but align the buffer to MAXIMUM_ALIGNOF, then the length word might get split if sizeof(Size) MAXIMUM_ALIGNOF. Hmm ... do we support any platforms where that's actually the case? It's possible I guess but I don't know of any offhand. The reverse case is real, but I'm not sure about this one. Man, I don't have a clue what exists. I certainly wouldn't want to discourage someone from making a 64-bit machine that only requires 32-bit alignment for 8-bit values, but color me clueless as to whether such things are really out there. That doesn't look too bad, but required changing a couple of if statements into while loops, and changing around the structure of a shm_mq_handle a bit. See attached. Would it get noticeably simpler or faster if you omitted support for the sizeof(Size) MAXIMUM_ALIGNOF case? It looks like perhaps not, but if we were paying anything much I'd be tempted to just put in a static assert to the contrary and see if anyone complains. Not really. I installed a fast path into the receive code for the common case where the length word isn't split, which will always be true on platforms where sizeof(Size) = MAXIMUM_ALIGNOF and usually true otherwise. We could ditch the slow path completely by ignoring that case, but it's not all that much code. On the sending side, the logic is pretty trivial, so I definitely don't feel bad about carrying that. The thing I kind of like about this approach is that it makes the code fully independent of the relationship between MAXIMUM_ALIGNOF and sizeof(Size). If the former is smaller, we'll write the length word in chunks if needed; if the latter is smaller, we'll insert useless padding bytes. In the perhaps-common case where they're identical, it all works as before, except for minor space savings on 32-bit platforms. I was a little annoyed by having to write the extra code and thought about objecting to this whole line of attack yet again, but I think it's actually likely for the best. If we start persuading ourselves that certain cases don't need to work, and rely on that throughout the backend, and then such machines crop up and we want to support them, we'll have a deep hole to climb out of. With this approach, there might be bugs, of course, but it's a lot easier to fix a bug that only occurs on a new platform than it is to reconsider the whole design in light of a new platform. BTW, can't we drop the MAXALIGN64 stuff again? It's still used by xlog.c. -- 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] Portability issues in shm_mq
Robert Haas robertmh...@gmail.com writes: On Mon, Mar 17, 2014 at 11:09 PM, Tom Lane t...@sss.pgh.pa.us wrote: Would it get noticeably simpler or faster if you omitted support for the sizeof(Size) MAXIMUM_ALIGNOF case? It looks like perhaps not, but if we were paying anything much I'd be tempted to just put in a static assert to the contrary and see if anyone complains. Not really. I installed a fast path into the receive code for the common case where the length word isn't split, which will always be true on platforms where sizeof(Size) = MAXIMUM_ALIGNOF and usually true otherwise. We could ditch the slow path completely by ignoring that case, but it's not all that much code. On the sending side, the logic is pretty trivial, so I definitely don't feel bad about carrying that. Works for me. The thing I kind of like about this approach is that it makes the code fully independent of the relationship between MAXIMUM_ALIGNOF and sizeof(Size). Yeah. If it's not costing us much to support both cases, let's do so. 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] GSoC proposal. Index-only scans for GIST
On Tue, Mar 18, 2014 at 9:12 AM, Anastasia Lubennikova lubennikov...@gmail.com wrote: Support for index-only scans for GIST index This is a cool idea, if it can be made to work. If the fetch() is specified by the developer, then using it, algorithm can retrieve the data directly to output areas at this stage, without reference to the heap. This seems to be the crux of your proposal, but it seems vague: what exactly do you mean by retrieve the data directly to output areas? What data are you going to retrieve and where are you going to put it? Another question to consider is: which operator classes do you anticipate that this will work for and which ones do you anticipate that it will not work for? Any operator class that lossifies that input data before storing it in the index is presumably doomed, but which ones do that, and which do not? Tom Lane previously proposed extending SP-GiST to support index-only scans. You might find that discussing worth reading, or perhaps consider it as an alternative if GiST doesn't work out: http://www.postgresql.org/message-id/10839.1323885...@sss.pgh.pa.us -- 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] GSoC proposal. Index-only scans for GIST
Robert Haas robertmh...@gmail.com writes: Tom Lane previously proposed extending SP-GiST to support index-only scans. You might find that discussing worth reading, or perhaps consider it as an alternative if GiST doesn't work out: http://www.postgresql.org/message-id/10839.1323885...@sss.pgh.pa.us That wasn't just a proposal, see commits 3695a555136a6d179cac8ae48d5f90171d5b30e9 and 92203624934095163f8b57b5b3d7bbd2645da2c8. But yeah, that might be a useful reference for what is likely to be involved with making GIST do it. 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] Minimum supported version of Python?
On 03/17/2014 10:31 PM, Peter Eisentraut wrote: On Sun, 2014-03-16 at 22:34 -0400, Tom Lane wrote: As for 2.4 vs 2.5, I don't have a lot of faith that we're really supporting anything that's not represented in the buildfarm... There are many other features that the build farm doesn't test and that I don't have a lot of faith in, but I'm not proposing to remove those. I don't control what the build farm tests, I only control my own work. Actually, if you run a buildfarm animal you have considerable control over what it tests. It's a very loosely coupled distributed system. The whole basis on which it was constructed was that people who were interested in certain combinations of hardware and infrastructure would create animals to test those combinations. If that's too much trouble for people, I have often offered to set up an animal for people if they give me ssh access. If that's too much trouble for people I tend to conclude that they aren't actually all that interested. 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] Portability issues in shm_mq
On Tue, Mar 18, 2014 at 10:44 AM, Tom Lane t...@sss.pgh.pa.us wrote: The thing I kind of like about this approach is that it makes the code fully independent of the relationship between MAXIMUM_ALIGNOF and sizeof(Size). Yeah. If it's not costing us much to support both cases, let's do so. OK, committed as posted. -- 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_archivecleanup bug
On 18 March 2014 14:15, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Bruce Momjian escribió: On Tue, Mar 18, 2014 at 10:03:46AM -0400, Robert Haas wrote: On Tue, Mar 18, 2014 at 9:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: Bruce Momjian br...@momjian.us writes: Very good point. I have modified the patch to add this block in all cases where it was missing. I started to wonder about the comment and if the Mingw fix was released. Based on some research, I see this as fixed in mingw-runtime-3.2, released 2003-10-10. That's pretty old. Yeah. I would vote for removing that code in all branches. There is no reason to suppose somebody is going to install 8.4.22 on a machine that they haven't updated mingw on since 2003. Or, if you prefer, just remove it in HEAD --- but going around and *adding* more copies seems like make-work. The fact that we've not heard complaints about the omissions is good evidence that nobody's using the buggy mingw versions anymore. I don't think it is. Right now we're not checking errno *at all* in a bunch of these places, so we're sure not going to get complaints about doing it incorrectly in those places. Or do I need more caffeine? You are correct. This code is seriously broken and I am susprised we have not gotten more complaints. Good thing readdir/closedir rarely fail. back-patching Some commentary on this... Obviously, all errors are mine. If pg_archivecleanup is a problem, then so is pg_standby a problem. Given the above, this means we've run for about 7 years without a reported issue on this. If we are going to make this better by actually having it throw errors in places that didn't throw errors before, are we sure that is going to make people happier? The archive cleanup isn't exactly critical in most cases, so dynamic errors don't matter much. Also, the programs were originally written to work as standalone program as well as an archive_cleanup_command. So we can't use PostgreSQL infrastructure (can we?). That aspect is needed to allow testing the program before it goes live. -- 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] Minimum supported version of Python?
On 3/17/14, 10:55 PM, Tom Lane wrote: It doesn't pass the regression tests. Do you need more of a bug report than that? It does pass the tests for me and others. If you are seeing something different, then we need to see some details, like what platform, what version, what Python version, how installed, what PostgreSQL version, how installed, actual diffs, etc. -- 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_archivecleanup bug
On Tue, Mar 18, 2014 at 11:36 AM, Simon Riggs si...@2ndquadrant.com wrote: Given the above, this means we've run for about 7 years without a reported issue on this. If we are going to make this better by actually having it throw errors in places that didn't throw errors before, are we sure that is going to make people happier? The archive cleanup isn't exactly critical in most cases, so dynamic errors don't matter much. We report errors returned by system calls in many other places. I can't see why this place should be any different. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: show relation and tuple infos of a lock to acquire
On Tue, Mar 18, 2014 at 3:42 AM, Amit Kapila amit.kapil...@gmail.com wrote: The message for exclusive lock on tuple print the database information. It is true that it is possible to have a deadlock or lock chains that involves locks on other databases. In this example the table test is not in the database that just logged the deadlock. STATEMENT: create role test; ERROR: deadlock detected DETAIL: Process 8968 waits for ShareLock on transaction 1067; blocked by process 8973. Process 8973 waits for ShareLock on transaction 1064; blocked by process 8971. Process 8971 waits for ShareLock on transaction 1062; blocked by process 8968. Process 8968: create role test; Process 8973: insert into test values (2); Process 8971: create role test2; -- greg -- 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] Minimum supported version of Python?
On 3/17/14, 10:47 PM, Joshua D. Drake wrote: We shouldn't be supporting anything the community doesn't support. Python 2.3 is dead. We shouldn't actively support it nor suggest that we could or should via the docs. The information that is available to me about this issue is lacking details, but as far as I can tell, we are partially talking about backbranches, which were released at a time when Python 2.3 and 2.4 were not unreasonable targets. We're not going to desupport build dependencies in the middle of a stable release branch unless we have a solid reason. -- 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_archivecleanup bug
On 18 March 2014 15:50, Robert Haas robertmh...@gmail.com wrote: On Tue, Mar 18, 2014 at 11:36 AM, Simon Riggs si...@2ndquadrant.com wrote: Given the above, this means we've run for about 7 years without a reported issue on this. If we are going to make this better by actually having it throw errors in places that didn't throw errors before, are we sure that is going to make people happier? The archive cleanup isn't exactly critical in most cases, so dynamic errors don't matter much. We report errors returned by system calls in many other places. I can't see why this place should be any different. Sure. Just wanted to make sure it's a conscious, explicit choice to do so. -- 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] Minimum supported version of Python?
On 3/18/14, 11:22 AM, Andrew Dunstan wrote: Actually, if you run a buildfarm animal you have considerable control over what it tests. I appreciate that. My problem here isn't time or ideas or coding, but lack of hardware resources. If I had hardware, I could set up tests for every build dependency under the sun. -- 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] Portability issues in shm_mq
All right. On Fri, Mar 14, 2014 at 4:43 PM, Tom Lane t...@sss.pgh.pa.us wrote: Whilst setting up a buildfarm member on an old, now-spare Mac, I was somewhat astonished to discover that contrib/test_shm_mq crashes thus: TRAP: FailedAssertion(!(rb = sizeof(uint64)), File: shm_mq.c, Line: 429) but only in UTF8 locales, not in C locale. You'd have bet your last dollar that that code was locale-independent, right? First, can you retest this with the latest code? Recommendations: 1. Reduce the random() multiplier from 96 to 95. In multibyte encodings other than UTF8, chr() would flat out reject values of 128, so this test case is unportable. 2. Why in the world is the test case testing exactly one message length that happens to be a multiple of 8? Put some randomness into that, instead. 3. Either you need to work a bit harder at forcing alignment, or you need to fix shm_mq_receive to cope with split message length words. 4. The header comment for shm_mq_receive_bytes may once have described its API accurately, but that appears to have been a long long time ago in a galaxy far far away. Please fix. On these recommendations, I believe that #3 and #4 are now dealt with. That leaves #1 and #2. #1 is of course easy, but I think we should do them both together. If we want to inject some randomness into the test, which parameters do we want to randomize and over what ranges? Also, if a buildfarm critter falls over, how will we know what value triggered the failure? It's tempting to instead add one or more tests that we specifically choose to have values we think are likely to exercise platform-specific differences or otherwise find bugs - e.g. just add a second test where the queue size and message length are both odd. And maybe at a test where the queue is smaller than the message size, so that every message wraps (multiple times?). Thoughts? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: show relation and tuple infos of a lock to acquire
On Tue, Mar 18, 2014 at 11:59 AM, Greg Stark st...@mit.edu wrote: On Tue, Mar 18, 2014 at 3:42 AM, Amit Kapila amit.kapil...@gmail.com wrote: The message for exclusive lock on tuple print the database information. It is true that it is possible to have a deadlock or lock chains that involves locks on other databases. In this example the table test is not in the database that just logged the deadlock. STATEMENT: create role test; ERROR: deadlock detected DETAIL: Process 8968 waits for ShareLock on transaction 1067; blocked by process 8973. Process 8973 waits for ShareLock on transaction 1064; blocked by process 8971. Process 8971 waits for ShareLock on transaction 1062; blocked by process 8968. Process 8968: create role test; Process 8973: insert into test values (2); Process 8971: create role test2; This is a good point, but I think it's acceptable to leave out the database name as Tom proposes. The context message applies to what the current backend was doing when the message got printed, and that's always relative to the current database. -- 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] Portability issues in shm_mq
Robert Haas robertmh...@gmail.com writes: First, can you retest this with the latest code? Yeah, on it now. If we want to inject some randomness into the test, which parameters do we want to randomize and over what ranges? I think the message length is the only particularly interesting parameter. It'd be nice if the length varied *within* a test, but that would take rather considerable restructuring, so maybe it's not worth the trouble. Also, if a buildfarm critter falls over, how will we know what value triggered the failure? Maybe we won't, but I think knowing that it does fail on platform X is likely to be enough to find the problem. It's tempting to instead add one or more tests that we specifically choose to have values we think are likely to exercise platform-specific differences or otherwise find bugs - e.g. just add a second test where the queue size and message length are both odd. Meh. I think you're putting a bit too much faith in your ability to predict the locus of bugs that you think aren't there. maybe at a test where the queue is smaller than the message size, so that every message wraps (multiple times?). Does the code support messages larger than the queue size? If so, yes, that case clearly oughta be tested. 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] Patch: show relation and tuple infos of a lock to acquire
On Tue, Mar 18, 2014 at 12:00 AM, Amit Kapila amit.kapil...@gmail.com wrote: Therefore I think the only case worth considering here is when database/relation/TID are all defined. The other cases where there is partial information are dead code; and the case where there is nothing defined (such as the one in SnapBuildFindSnapshot) is already handled by simply not setting up a context at all. Right. So I think we should just keep one version of message. Well, if we're back to one version of the message, and I'm glad we are, can we go back to saying: CONTEXT: while updating tuple (0,2) in relation public.foo of database postgres Instead of: CONTEXT: while operating on tuple (0,2) in relation public.foo of database postgres: updating tuple -- 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_archivecleanup bug
On 13 March 2014 05:48, Bruce Momjian br...@momjian.us wrote: On Mon, Dec 9, 2013 at 11:27:28AM -0500, Robert Haas wrote: On Thu, Dec 5, 2013 at 6:15 PM, Tom Lane t...@sss.pgh.pa.us wrote: But the other usages seem to be in assorted utilities, which will need to do it right for themselves. initdb.c's walkdir() seems to have it right and might be a reasonable model to follow. Or maybe we should invent a frontend-friendly version of ReadDir() rather than duplicating all the error checking code in ten-and-counting places? If there's enough uniformity in all of those places to make that feasible, it certainly seems wise to do it that way. I don't know if that's the case, though - e.g. maybe some callers want to exit and others do not. pg_resetxlog wants to exit; pg_archivecleanup and pg_standby most likely want to print an error and carry on. I have developed the attached patch which fixes all cases where readdir() wasn't checking for errno, and cleaned up the syntax in other cases to be consistent. While I am not a fan of backpatching, the fact we are ignoring errors in some critical cases seems the non-cosmetic parts should be backpatched. pg_resetxlog was not an offender here; its coding was sound. We shouldn't be discussing backpatching a patch that contains changes to coding style. ISTM we should change the code with missing checks to adopt the coding style of pg_resetxlog, not the other way around. I assume you or Kevin have this in hand and you don't want me to apply the patch? (Since it was originally my bug) -- 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] Minimum supported version of Python?
Peter Eisentraut-2 wrote On 3/18/14, 11:22 AM, Andrew Dunstan wrote: Actually, if you run a buildfarm animal you have considerable control over what it tests. I appreciate that. My problem here isn't time or ideas or coding, but lack of hardware resources. If I had hardware, I could set up tests for every build dependency under the sun. I don't imagine there is enough churn in this area that having a constantly testing buildfarm animal is a strong need; but a wiki page dedicated to Python Support in PostgreSQL where we can publicly and officially release testing results and commentary would be an improvement. As it sounds like the only caveat to supporting 2.3 is that we don't technically (or do we - is Decimal mandatory?) support an un-modified core installation but require that at one add-on module be installed. Assuming that clears up all the errors Tom is seeing then saying that plpythonu works with a slightly modified 2.3 on all current releases of PostgreSQL isn't a stretch nor does it commit us to fixing bugs in the unlikely event that any are discovered in two extremely stable environments. -- View this message in context: http://postgresql.1045698.n5.nabble.com/Minimum-supported-version-of-Python-tp5796175p5796615.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] Portability issues in shm_mq
I wrote: Robert Haas robertmh...@gmail.com writes: First, can you retest this with the latest code? Yeah, on it now. Early returns not good: *** /Users/buildfarm/bf-data/HEAD/pgsql.93630/contrib/test_shm_mq/expected/test_shm_mq.out Tue Mar 18 12:00:18 2014 --- /Users/buildfarm/bf-data/HEAD/pgsql.93630/contrib/test_shm_mq/results/test_shm_mq.out Tue Mar 18 12:17:04 2014 *** *** 5,18 -- internal sanity tests fail. -- SELECT test_shm_mq(32768, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,400)), 1, 1); ! test_shm_mq ! - ! ! (1 row) ! SELECT test_shm_mq_pipelined(16384, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,27)), 200, 3); ! test_shm_mq_pipelined ! --- ! ! (1 row) ! --- 5,12 -- internal sanity tests fail. -- SELECT test_shm_mq(32768, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,400)), 1, 1); ! ERROR: message corrupted ! DETAIL: The original message was 400 bytes but the final message is 7492059346764176 bytes. SELECT test_shm_mq_pipelined(16384, (select string_agg(chr(32+(random()*96)::int), '') from generate_series(1,27)), 200, 3); ! ERROR: message corrupted ! DETAIL: The original message was 27 bytes but the final message is 7492059347033776 bytes. This is C locale on a 32-bit machine, so you'll likely be seeing the same complaint in already-online buildfarm members. Note that size_t is definitely not int64 on this machine, so it looks to me like your int64-ectomy was incomplete. Those message lengths should be impossible no matter what on this hardware. 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] Patch: show relation and tuple infos of a lock to acquire
Robert Haas escribió: On Tue, Mar 18, 2014 at 12:00 AM, Amit Kapila amit.kapil...@gmail.com wrote: Therefore I think the only case worth considering here is when database/relation/TID are all defined. The other cases where there is partial information are dead code; and the case where there is nothing defined (such as the one in SnapBuildFindSnapshot) is already handled by simply not setting up a context at all. Right. So I think we should just keep one version of message. Well, if we're back to one version of the message, and I'm glad we are, can we go back to saying: CONTEXT: while updating tuple (0,2) in relation public.foo of database postgres That isn't easy. The callers would need to pass the whole message in order for it to be translatable; and generating a format string in one module and having the arguments be stapled on top in a separate module doesn't seem a very good idea to me. Currently we have this: /* wait for regular transaction to end */ XactLockTableWait(xwait, relation, tp.t_data-t_ctid, /* translator: string is XactLockTableWait operation name */ deleting tuple); And if we go with what you propose, we would have this: /* wait for regular transaction to end */ XactLockTableWait(xwait, relation, tp.t_data-t_ctid, while updating tuple (%u,%u) in relation \%s\) which doesn't seem an improvement to me. Now another idea would be to pass a code or something; so callers would do XactLockTableWait(xwait, relation, TID, XLTW_OPER_UPDATE); and the callback would select the appropriate message. Not really excited about that, though. In the current version of the patch, attached, I've reduced the callback to this: if (ItemPointerIsValid(info-ctid) RelationIsValid(info-rel)) { errcontext(while operating on tuple (%u,%u) in relation \%s\: %s, ItemPointerGetBlockNumber(info-ctid), ItemPointerGetOffsetNumber(info-ctid), RelationGetRelationName(info-rel), _(info-opr_name)); } Note that: 1. the database name is gone, as discussed 2. the schema name is gone too, because it requires a syscache lookup Now we can go back to having a schema name, but I think we would have to add an IsTransactionState() check first or something like that. Or save the schema name when setting up the callback, but this doesn't sound so good (particularly considering that lock waits might end without actually waiting at all, so this'd be wasted effort.) If you have a better idea, I'm all ears. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services *** a/src/backend/access/heap/heapam.c --- b/src/backend/access/heap/heapam.c *** *** 105,115 static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask, uint16 *new_infomask2); static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask); ! static void MultiXactIdWait(MultiXactId multi, MultiXactStatus status, ! int *remaining, uint16 infomask); ! static bool ConditionalMultiXactIdWait(MultiXactId multi, ! MultiXactStatus status, int *remaining, ! uint16 infomask); static XLogRecPtr log_heap_new_cid(Relation relation, HeapTuple tup); static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_modified, bool *copy); --- 105,116 uint16 *new_infomask2); static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask); ! static void MultiXactIdWait(MultiXactId multi, MultiXactStatus status, uint16 infomask, ! Relation rel, ItemPointer ctid, const char *opr, ! int *remaining); ! static bool ConditionalMultiXactIdWait(MultiXactId multi, MultiXactStatus status, ! uint16 infomask, Relation rel, ItemPointer ctid, ! const char *opr, int *remaining); static XLogRecPtr log_heap_new_cid(Relation relation, HeapTuple tup); static HeapTuple ExtractReplicaIdentity(Relation rel, HeapTuple tup, bool key_modified, bool *copy); *** *** 2714,2721 l1: if (infomask HEAP_XMAX_IS_MULTI) { /* wait for multixact */ ! MultiXactIdWait((MultiXactId) xwait, MultiXactStatusUpdate, ! NULL, infomask); LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); /* --- 2715,2724 if (infomask HEAP_XMAX_IS_MULTI) { /* wait for multixact */ ! MultiXactIdWait((MultiXactId) xwait, MultiXactStatusUpdate, infomask, ! /* translator: string is XactLockTableWait operation name */ ! relation, tp.t_data-t_ctid, deleting tuple, ! NULL);
Re: [HACKERS] Patch: show relation and tuple infos of a lock to acquire
Robert Haas robertmh...@gmail.com writes: Well, if we're back to one version of the message, and I'm glad we are, can we go back to saying: CONTEXT: while updating tuple (0,2) in relation public.foo of database postgres If I end up being the one who commits this, it's going to say while updating tuple (0,2) in table foo Not more, and not less. It is not project style to include schema names (much less database names) in error messages where they're not central to the meaning. One reason why not is that schema and database names are generally not available without an extra lookup step, which you don't really want to do in an error-reporting code path. Every extra action you take increases the risk of a cascading failure, so that the user will get something unhelpful like out of memory rather than the oh-so-extra-helpful message you wanted to print. The added utility of the extra information, for most cases, is insufficient to justify that risk. Even without that argument, it's still not project style; why should this message be randomly different from many hundreds of others? If you want to start a push to include schema names anywhere a table name is given, that should be debated separately and then done in a reasonably uniform fashion. 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] Patch: show relation and tuple infos of a lock to acquire
Tom Lane escribió: Robert Haas robertmh...@gmail.com writes: Well, if we're back to one version of the message, and I'm glad we are, can we go back to saying: CONTEXT: while updating tuple (0,2) in relation public.foo of database postgres If I end up being the one who commits this, it's going to say while updating tuple (0,2) in table foo Not more, and not less. Please see my reply to Robert. My proposal (in form of a patch) is while operating on tuple (0,2) in table foo: updating tuple Would this work for you? -- Á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] Patch: show relation and tuple infos of a lock to acquire
Alvaro Herrera alvhe...@2ndquadrant.com writes: Please see my reply to Robert. My proposal (in form of a patch) is while operating on tuple (0,2) in table foo: updating tuple Would this work for you? It's pretty lousy from a readability standpoint, even in English; I shudder to think what it might come out as after translation. I think the enum idea you floated is probably worth doing. It's certainly no more complex than passing a string, no? 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] Planner hints in Postgresql
On Mon, Mar 17, 2014 at 8:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: Claudio Freire klaussfre...@gmail.com writes: On Mon, Mar 17, 2014 at 7:01 PM, Jim Nasby j...@nasby.net wrote: Even better would be if the planner could estimate how bad a plan will become if we made assumptions that turn out to be wrong. That's precisely what risk estimation was about. Yeah. I would like to see the planner's cost estimates extended to include some sort of uncertainty estimate, whereupon risk-averse people could ask it to prefer low-uncertainty plans over high-uncertainty ones (the plans we typically choose for ORDER BY ... LIMIT queries being great examples of the latter). But it's a long way from wishing that to making it so. Right now it's not even clear (to me anyway) how we'd measure or model such uncertainty. Well, currently, selectivity estimates based on MCV should be pretty low-uncertainty, whereas certainty of other estimates could be modeled as a random variable if ANALYZE gathered a few statistical moments (for variables that are prone to that kind of statistical analysis). That alone could improve things considerably, and statistical info could be propagated along expressions to make it possible to model uncertainty in complex expressions as well.
[HACKERS] json/jsonb/hstore operator precedence
Fwiw I'm finding myself repeatedly caught up by the operator precedence rules when experimenting with jsonb: stark=***# select segment-'id' as id from flight_segments where segment-'marketing_airline_code' segment-'operating_airline_code' ; ERROR: 42883: operator does not exist: text jsonb LINE 2: ...segments where segment-'marketing_airline_code' segment... ^ HINT: No operator matches the given name and argument type(s). You might need to add explicit type casts. LOCATION: op_error, parse_oper.c:722 Time: 0.407 ms stark=***# select segment-'id' as id from flight_segments where (segment-'marketing_airline_code') (segment-'operating_airline_code') ; id - 45866185 95575359 I don't think this is related to the jsonb patch -- json and hstore have the same behaviour so jsonb is obviously going to follow suit. The only option right now would be to use a higher precedence operator like % or ^ for all of these data types which I'm not for. I suspect it's a pipe dream to think we might be able to override the '.' and changing the precedence of - and - would be fraught... I think the best we can do is to highlight it in the docs. Incidentally it's a good thing there wasn't an implicit cast text-jsonb. In this case it would have resulted in just a confusing error of jsonb-boolean not existing. -- greg -- 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] GSoC proposal. Index-only scans for GIST
Alexander, Can you comment on the below proposal? I'd like your opinion on how difficult it will be. On 03/18/2014 06:12 AM, Anastasia Lubennikova wrote: Hello! Here is the text of my proposal which I've applied to GSoC. (and link http://www.google-melange.com/gsoc/proposal/public/google/gsoc2014/lubennikovaav/5629499534213120) Any suggestions and comments are welcome. *Project name* Support for index-only scans for GIST index *Brief review* Currently GiST index don't have index-only scan functionality. Index-only scans are a major performance feature added to Postgres 9.2. They allow certain types of queries to be satisfied just by retrieving data from indexes, and not from tables. This feature for B-trees (implemented since PostgreSQL-9.2) allows doing certain types of queries significantly faster. This is achieved by reduction in the amount of I/O necessary to satisfy queries. I think it will be useful to implement this feature for user defined data types that use GiST index. *Benefits to the PostgreSQL Community* Faster GiST index search would be actual for many PostgreSQL applications (for example some GIS systems). *Quantifiable results* Acceleration of GiST index search. *Project details* 1. The GiST is a balanced tree structure like a B-tree, containing key, pointer pairs. GiST key is a member of a user-defined class, and represents some property that is true of all data items reachable from the pointer associated with the key. The GiST provides a possibility to create custom data types with indexed access methods and extensible set of queries. There are seven methods that an index operator class for GiST must provide, and an eighth that is optional. -consistent -union -compress -decompress -penalty -picksplit -equal -distance (optional) I'm going to create new optional method fetch() in addition to existing. Thus user can create a method of retrieving data from the index without loss. It will be used when performing search queries to speed data retrieving. 2. gistget algorithm (several parts omitted): Check the key gistindex_keytest() - does this index tuple satisfy the scan key(s)? Scan all items on the GiST index page and insert them into the queue (or directly to output areas) plain scan Heap tuple TIDs are returned into so-pageData[] ordered scan Heap tuple TIDs are pushed into individual search queue items If the fetch() is specified by the developer, then using it, algorithm can retrieve the data directly to output areas at this stage, without reference to the heap. 3. Create function gistcanreturn to check whether fetch() is specified by user. Amcanreturn - Function to check whether index supports index-only scans, or zero if none There is the question of support index-only scans for multicolumn indexes. Probably it would require extend the interface to add separate columns checking. To solve this question I need the community's help. 4. Add details for index only scans into gistcostestimate function *Links* 1) Hellerstein J. M., Naughton J. F., Pfeffer A. Generalized search trees for database systems. - September, 1995. 2) http://www.sai.msu.su/~megera/postgres/gist/ 3) PostgreSQL 9.3.3 Documentation: chapters 11. Indexes, 55. GiST Indexes. -- 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] Portability issues in shm_mq
I wrote: Early returns not good: Also, these compiler messages are probably relevant: ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o test.o test.c test.c: In function 'test_shm_mq': test.c:89:3: warning: passing argument 2 of 'shm_mq_receive' from incompatible pointer type [enabled by default] In file included from test_shm_mq.h:18:0, from test.c:19: ../../src/include/storage/shm_mq.h:61:22: note: expected 'Size *' but argument is of type 'uint64 *' test.c: In function 'test_shm_mq_pipelined': test.c:198:4: warning: passing argument 2 of 'shm_mq_receive' from incompatible pointer type [enabled by default] In file included from test_shm_mq.h:18:0, from test.c:19: ../../src/include/storage/shm_mq.h:61:22: note: expected 'Size *' but argument is of type 'uint64 *' ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o setup.o setup.c ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o worker.o worker.c worker.c: In function 'copy_messages': worker.c:193:3: warning: passing argument 2 of 'shm_mq_receive' from incompatible pointer type [enabled by default] In file included from worker.c:25:0: ../../src/include/storage/shm_mq.h:61:22: note: expected 'Size *' but argument is of type 'uint64 *' I'm thinking maybe you just forgot to update the contrib module. 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] Portability issues in shm_mq
On Tue, Mar 18, 2014 at 1:16 PM, Tom Lane t...@sss.pgh.pa.us wrote: I wrote: Early returns not good: Also, these compiler messages are probably relevant: ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o test.o test.c test.c: In function 'test_shm_mq': test.c:89:3: warning: passing argument 2 of 'shm_mq_receive' from incompatible pointer type [enabled by default] In file included from test_shm_mq.h:18:0, from test.c:19: ../../src/include/storage/shm_mq.h:61:22: note: expected 'Size *' but argument is of type 'uint64 *' test.c: In function 'test_shm_mq_pipelined': test.c:198:4: warning: passing argument 2 of 'shm_mq_receive' from incompatible pointer type [enabled by default] In file included from test_shm_mq.h:18:0, from test.c:19: ../../src/include/storage/shm_mq.h:61:22: note: expected 'Size *' but argument is of type 'uint64 *' ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o setup.o setup.c ccache gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -I/usr/include/et -c -o worker.o worker.c worker.c: In function 'copy_messages': worker.c:193:3: warning: passing argument 2 of 'shm_mq_receive' from incompatible pointer type [enabled by default] In file included from worker.c:25:0: ../../src/include/storage/shm_mq.h:61:22: note: expected 'Size *' but argument is of type 'uint64 *' I'm thinking maybe you just forgot to update the contrib module. Well, I definitely forgot that. I'll count myself lucky if that's the only problem. -- 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] Portability issues in shm_mq
On 2014-03-18 13:31:47 -0400, Robert Haas wrote: Well, I definitely forgot that. I'll count myself lucky if that's the only problem. One minor thing missing, patch attached. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/contrib/test_shm_mq/test.c b/contrib/test_shm_mq/test.c index dba5e69..5ff1e9a 100644 --- a/contrib/test_shm_mq/test.c +++ b/contrib/test_shm_mq/test.c @@ -254,12 +254,12 @@ verify_message(Size origlen, char *origdata, Size newlen, char *newdata) if (origlen != newlen) ereport(ERROR, (errmsg(message corrupted), - errdetail(The original message was UINT64_FORMAT bytes but the final message is UINT64_FORMAT bytes., + errdetail(The original message was %zu bytes but the final message is %zu bytes., origlen, newlen))); for (i = 0; i origlen; ++i) if (origdata[i] != newdata[i]) ereport(ERROR, (errmsg(message corrupted), - errdetail(The new and original messages differ at byte UINT64_FORMAT of UINT64_FORMAT ., i, origlen))); + errdetail(The new and original messages differ at byte %zu of %zu., i, origlen))); } -- 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_archivecleanup bug
On Tue, Mar 18, 2014 at 04:17:53PM +, Simon Riggs wrote: While I am not a fan of backpatching, the fact we are ignoring errors in some critical cases seems the non-cosmetic parts should be backpatched. pg_resetxlog was not an offender here; its coding was sound. We shouldn't be discussing backpatching a patch that contains changes to coding style. I was going to remove the coding style changes to pg_resetxlog from the backpatched portion. ISTM we should change the code with missing checks to adopt the coding style of pg_resetxlog, not the other way around. I assume you or Kevin have this in hand and you don't want me to apply the patch? (Since it was originally my bug) I know the email subject says pg_archivecleanup but the problem is all over our code. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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_archivecleanup bug
On 18 March 2014 18:01, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 18, 2014 at 04:17:53PM +, Simon Riggs wrote: While I am not a fan of backpatching, the fact we are ignoring errors in some critical cases seems the non-cosmetic parts should be backpatched. pg_resetxlog was not an offender here; its coding was sound. We shouldn't be discussing backpatching a patch that contains changes to coding style. I was going to remove the coding style changes to pg_resetxlog from the backpatched portion. Why make style changes at all? A patch with no style changes would mean backpatch and HEAD versions would be the same. ISTM we should change the code with missing checks to adopt the coding style of pg_resetxlog, not the other way around. I assume you or Kevin have this in hand and you don't want me to apply the patch? (Since it was originally my bug) I know the email subject says pg_archivecleanup but the problem is all over our code. Yes, understood. -- 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] Risk Estimation WAS: Planner hints in Postgresql
On Mon, Mar 17, 2014 at 8:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah. I would like to see the planner's cost estimates extended to include some sort of uncertainty estimate, whereupon risk-averse people could ask it to prefer low-uncertainty plans over high-uncertainty ones (the plans we typically choose for ORDER BY ... LIMIT queries being great examples of the latter). But it's a long way from wishing that to making it so. Right now it's not even clear (to me anyway) how we'd measure or model such uncertainty. This is not a model, but here's some starting thoughts: A high risk plan has two components: a) our statistical data is out-of-date or inadequate b) the potential execution time if our estimates of selectivity are wrong is high c) the cost ratio of certain operations is wrong. Factor (a) can be modeled two ways: 1. If last_analyze is a long time ago, we have increased the risk. (Ideally, we'd have some idea of the change rate on the table vs. the last analyze time; right now we don't have those stats) 2. Certain patterns, such as multi-column selectivity and GIN/GiST selectivity are known to have poor estimates, and be higher risk. Certainly selectivity functions which have been programmed with a flat coefficient (like default 0.05 selectivity for gist_ops) could also return a risk factor which is fairly high. Factor (b) can be modeled simply by estimating the cost of a plan where all row estimates are changed by 10X, or even better by a calculation on the risk factor calculated in (a). This would then give us the failure cost of the bad plan. Note that we need to estimate in both directions, both for higher estimates and lower ones; abort early plans fail because the rows returned are lower than expected, for example. (b) estimation would be expensive if we did every combination of the entire plan with wrong estimates, so I'm wondering if it would be adequate to just estimate the node selectivity being off on a per-node basis. (c) we can't realistically estimate for at all (i.e. if we knew the cost factor was wrong, we'd fix it) so I suggest ignoring it for risk estimation. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_archivecleanup bug
On Tue, Mar 18, 2014 at 06:11:30PM +, Simon Riggs wrote: On 18 March 2014 18:01, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 18, 2014 at 04:17:53PM +, Simon Riggs wrote: While I am not a fan of backpatching, the fact we are ignoring errors in some critical cases seems the non-cosmetic parts should be backpatched. pg_resetxlog was not an offender here; its coding was sound. We shouldn't be discussing backpatching a patch that contains changes to coding style. I was going to remove the coding style changes to pg_resetxlog from the backpatched portion. Why make style changes at all? A patch with no style changes would mean backpatch and HEAD versions would be the same. The old style had errno set in two places in the loop, while the new style has it set in only one place. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] Portability issues in shm_mq
and...@anarazel.de (Andres Freund) writes: On 2014-03-18 13:31:47 -0400, Robert Haas wrote: Well, I definitely forgot that. I'll count myself lucky if that's the only problem. One minor thing missing, patch attached. setup_dynamic_shared_memory needed some more hacking too. Committed. 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] Risk Estimation WAS: Planner hints in Postgresql
On Tue, Mar 18, 2014 at 11:43 PM, Josh Berkus j...@agliodbs.com wrote: On Mon, Mar 17, 2014 at 8:47 PM, Tom Lane t...@sss.pgh.pa.us wrote: Yeah. I would like to see the planner's cost estimates extended to include some sort of uncertainty estimate, whereupon risk-averse people could ask it to prefer low-uncertainty plans over high-uncertainty ones (the plans we typically choose for ORDER BY ... LIMIT queries being great examples of the latter). But it's a long way from wishing that to making it so. Right now it's not even clear (to me anyway) how we'd measure or model such uncertainty. I have been thinking of some ways to have a risk estimate of each selectivity that our planner gives. I think a way to do it is as follows: One of the factors that leads to bad estimates is that the histogram of the values of a column maintained by the planner gets old by time and the data in the column changes. So, the histogram is no longer a quite accurate view of the data and it leads to bad selectivity. One thing we can try to do is to add a factor of error that we feel the selectivity given can have. This allows us to factor in the probability that the data changed and the estimate of the difference of the current histogram and the histogram of the actual data currently present in the column in the table. We can use Central Limit Theorem ( http://en.wikipedia.org/wiki/Central_limit_theorem). Essentially, what the theorem says is that given a distribution that has finite variance and finite mean, we can take random independent samples from the data and calculate the standard deviation and the mean of the sample. If we have large enough number of samples and if we plot the mean and SD, they would follow a normal distribution. What is interesting is that this can allow us to predict the SD of a given dataset from the curve and the SD should be directly proportional to the deviation it has from the given planner histogram. I am no mathematician hence its hard for me to explain. I think this link [1] will be more helpful. So, we can have a probability value for the random variable and that shall model the confidence we have in our estimate. I may be wrong in some parts but I hope I have been able to convey the general idea. If this idea develops, I shall be happy to work on this but my hands are full in ROLLUPS right now, so for my part it shall take time. I just want to float the idea and get a general feel about the idea right now. Please let me know your comments and feedback on this. Regards, Atri [1]: http://www.theriac.org/DeskReference/viewDocument.php?id=177 -- Regards, Atri *l'apprenant*
Re: [HACKERS] Risk Estimation WAS: Planner hints in Postgresql
Atri Sharma atri.j...@gmail.com writes: One of the factors that leads to bad estimates is that the histogram of the values of a column maintained by the planner gets old by time and the data in the column changes. So, the histogram is no longer a quite accurate view of the data and it leads to bad selectivity. TBH, this is so far down the list of problems that it'll be a long time before we need to worry about it. It's certainly not the number one priority for any project to model risk in the planner. The thing that I think is probably the number one problem is estimates that depend on an assumption of uniform distribution of sought-after rows among those encountered by a scan. This is usually where bad plans for LIMIT queries are coming from. We could certainly add some sort of fudge factor to those costs, but I'd like to have a more-or-less principled framework for doing so. 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] Wiki Page Draft for upcoming release
On 03/17/2014 05:49 PM, Josh Berkus wrote: All, https://wiki.postgresql.org/wiki/20140320UpdateIssues I'm sure my explanation of the data corruption issue is not correct, so please fix it. Thanks! Anyone? We're going public with this in 36 hours, so I'd love for it to actually be correct. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_archivecleanup bug
Bruce Momjian escribió: On Tue, Mar 18, 2014 at 06:11:30PM +, Simon Riggs wrote: On 18 March 2014 18:01, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 18, 2014 at 04:17:53PM +, Simon Riggs wrote: While I am not a fan of backpatching, the fact we are ignoring errors in some critical cases seems the non-cosmetic parts should be backpatched. pg_resetxlog was not an offender here; its coding was sound. We shouldn't be discussing backpatching a patch that contains changes to coding style. I was going to remove the coding style changes to pg_resetxlog from the backpatched portion. Why make style changes at all? A patch with no style changes would mean backpatch and HEAD versions would be the same. The old style had errno set in two places in the loop, while the new style has it set in only one place. I think it makes more sense to have all callsites look the same in all supported branches. -- Á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_archivecleanup bug
On 18 March 2014 18:18, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 18, 2014 at 06:11:30PM +, Simon Riggs wrote: On 18 March 2014 18:01, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 18, 2014 at 04:17:53PM +, Simon Riggs wrote: While I am not a fan of backpatching, the fact we are ignoring errors in some critical cases seems the non-cosmetic parts should be backpatched. pg_resetxlog was not an offender here; its coding was sound. We shouldn't be discussing backpatching a patch that contains changes to coding style. I was going to remove the coding style changes to pg_resetxlog from the backpatched portion. Why make style changes at all? A patch with no style changes would mean backpatch and HEAD versions would be the same. The old style had errno set in two places in the loop, while the new style has it set in only one place. Seems better to leave the previously-good coding in place. ISTM to be clearer to use simple C. You're doing this anyway for the backpatch, so I'm not causing you any more work. Better one patch than two. -- 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_archivecleanup bug
Simon Riggs escribió: On 18 March 2014 18:18, Bruce Momjian br...@momjian.us wrote: On Tue, Mar 18, 2014 at 06:11:30PM +, Simon Riggs wrote: Why make style changes at all? A patch with no style changes would mean backpatch and HEAD versions would be the same. The old style had errno set in two places in the loop, while the new style has it set in only one place. Seems better to leave the previously-good coding in place. ISTM to be clearer to use simple C. If you're saying we should use that style in all readdir loops, with the errno=0 before the loop and at the bottom of it, I don't disagree. Let's just make sure they're all safe though (i.e. watch out for continue for instance). That said, I don't find comma expression to be particularly not simple. -- Á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] plpgsql.warn_shadow
On 18/03/14 13:43, Pavel Stehule wrote: 2014-03-18 13:23 GMT+01:00 Petr Jelinek p...@2ndquadrant.com mailto:p...@2ndquadrant.com Agree that compile_errors dos not make sense, additional_errors and additional_warnings seems better, maybe plpgsql.extra_warnings and plpgsql.extra_errors? extra* sounds better Ok, so I took the liberty of rewriting the patch so that it uses plpgsql.extra_warnings and plpgsql.extra_errors configuration variables with possible values none, all and shadow (none being the default). Updated doc and regression tests to reflect the code changes, everything builds and tests pass just fine. I did one small change (that I think was agreed anyway) from Marko's original patch in that warnings are only emitted during function creation, no runtime warnings and no warnings for inline (DO) plpgsql code either as I really don't think these optional warnings/errors during runtime are a good idea. Note that the patch does not really handle the list of values as list, basically all and shadow are translated to true and proper handling of this is left to whoever will want to implement additional checks. I think this approach is fine as I don't see the need to build frameworks here and it was same in the original patch. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index bddd458..56ee2b3 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -4711,6 +4711,51 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ /variablelist /sect2 + sect2 id=plpgsql-extra-checks + titleAdditional compile-time checks/title + + para +To aid the user in finding instances of simple but common problems before +they cause harm, applicationPL/PgSQL/ provides a number of additional +replaceablechecks/. When enabled, depending on the configuration, they +can be used to emit a literalWARNING/ or an literalERROR/ +during the compilation of a function. + /para + + para + These additional checks are enabled through the configuration variables + varnameplpgsql.extra_warnings/ for warnings and + varnameplpgsql.extra_errors/ for errors. Both can be set either to + a comma-separated list of checks, literalnone/ or literalall/. + The default is literalnone/. Currently the list of available checks + includes only one: + variablelist + varlistentry +termvarnameshadow/varname/term +listitem + para + Checks if a declaration shadows a previously defined variable. For + example (with varnameplpgsql.extra_warnings/ set to + varnameshadow/varname): +programlisting +CREATE FUNCTION foo(f1 int) RETURNS int AS $$ +DECLARE +f1 int; +BEGIN +RETURN f1; +END +$$ LANGUAGE plpgsql; +WARNING: variable f1 shadows a previously defined variable +LINE 3: f1 int; +^ +CREATE FUNCTION +/programlisting + /para +/listitem + /varlistentry + /variablelist + /para + /sect2 /sect1 !-- Porting from Oracle PL/SQL -- diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index 5afc2e5..8732efc 100644 --- a/src/pl/plpgsql/src/pl_comp.c +++ b/src/pl/plpgsql/src/pl_comp.c @@ -352,6 +352,9 @@ do_compile(FunctionCallInfo fcinfo, function-out_param_varno = -1; /* set up for no OUT param */ function-resolve_option = plpgsql_variable_conflict; function-print_strict_params = plpgsql_print_strict_params; + /* only promote extra warnings and errors at CREATE FUNCTION time */ + function-extra_warnings = plpgsql_extra_warnings forValidator; + function-extra_errors = plpgsql_extra_errors forValidator; if (is_dml_trigger) function-fn_is_trigger = PLPGSQL_DML_TRIGGER; @@ -849,6 +852,9 @@ plpgsql_compile_inline(char *proc_source) function-out_param_varno = -1; /* set up for no OUT param */ function-resolve_option = plpgsql_variable_conflict; function-print_strict_params = plpgsql_print_strict_params; + /* don't do extra validation for inline code as we don't want to add spam at runtime */ + function-extra_warnings = false; + function-extra_errors = false; plpgsql_ns_init(); plpgsql_ns_push(func_name); diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index c0cb585..98f7ddd 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -727,6 +727,20 @@ decl_varname : T_WORD $1.ident, NULL, NULL, NULL) != NULL) yyerror(duplicate declaration); + + if (plpgsql_curr_compile-extra_warnings || plpgsql_curr_compile-extra_errors) + { + PLpgSQL_nsitem *nsi; + nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false, + $1.ident, NULL, NULL, NULL); + if (nsi != NULL) +ereport(plpgsql_curr_compile-extra_errors ? ERROR : WARNING, + (errcode(ERRCODE_DUPLICATE_ALIAS), + errmsg(variable \%s\
Re: [HACKERS] Portability issues in shm_mq
After the last round of changes, I can confirm that my original test with UTF8 locale works, and my HPPA box is happy too. We could still stand to improve the regression test though. 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] Failure while inserting parent tuple to B-tree is not fun
On 02/06/2014 06:42 AM, Peter Geoghegan wrote: I'm not sure about this: *** _bt_findinsertloc(Relation rel, *** 675,680 --- 701,707 static void _bt_insertonpg(Relation rel, Buffer buf, + Buffer cbuf, BTStack stack, IndexTuple itup, OffsetNumber newitemoff, Wouldn't lcbuf be a better name for the new argument? After all, in relevant contexts 'buf' is the parent of both of the pages post-split, but there are two children in play here. You say: + *When inserting to a non-leaf page, 'cbuf' is the left-sibling of the + *page we're inserting the downlink for. This function will clear the + *INCOMPLETE_SPLIT flag on it, and release the buffer. I suggest also noting in comments at this point that cbuf/the left-sibling is the old half from the split. Even having vented about cbuf's name, I can kind of see why you didn't call it lcbuf: we already have an BTPageOpaque lpageop variable for the special 'buf' metadata within the _bt_insertonpg() function; so there might be an ambiguity between the possibly-soon-to-be-left page (the target page) and the *child* left page that needs to have its flag cleared. Although I still think you should change the name of lpageop (further details follow). Consider this: /* release buffers */ + if (!P_ISLEAF(lpageop)) + _bt_relbuf(rel, cbuf); (Rebased, so this looks a bit different to your original version IIRC). This is checking that the _bt_insertonpg() target page (whose special data lpageop points to) is not a leaf page, and releasing the *child* left page if it isn't. This is pretty confusing. So I suggest naming lpageop tpageop ('t' for target, a terminology already used in the comments above the function). I don't know, the buf/page/lpageop variable names are used in many functions in nbtinsert.c. Perhaps they should all be renamed, but I think it's fine as it is, and not this patch's responsibility anyway. (The lpageop name makes sense when the page has to be split, and the page becomes the left page. Otherwise, admittedly, not so much) Also, I suggest you change this existing code comment: * On entry, we must have the right buffer in which to do the * insertion, and the buffer must be pinned and write-locked. On return, * we will have dropped both the pin and the lock on the buffer. Change right to correct here. Minor point, but right is a loaded word. Fixed. But why are you doing new things while checking P_ISLEAF(lpageop) in _bt_insertonpg() to begin with? Would you not be better off doing things when there is a child buffer passed (e.g. if (BufferIsValid(cbuf))...), and only then asserting !P_ISLEAF(lpageop)? That seems to me to more naturally establish the context of _bt_insertonpg() is called to insert downlink after split. Although maybe that conflates things within XLOG stuff. Hmm. Changed that way for the places where we deal with the incomplete-split flag. I committed the patch now. Thanks for the review! Before committing, I spotted a bug in the way the new page-deletion code interacts with this: there was a check that the page that's about to be deleted was not marked with the INCOMPLETE_SPLIT flag, it was possible that the page was the right half of an incomplete split, and so didn't have a downlink. Vacuuming that failed with an failed to re-find parent key error. I fixed that by checking that the left sibling is also not marked with the flag. It would be fairly easy to delete a page that is the right half of an incomplete split. Such a page doesn't have a downlink pointing to it, so just skip removing it, and instead clear the INCOMPLETE_SPLIT flag in the left sibling. But deleting incompletely split pages isn't important from a disk-space point of view, they should be extremely rare, so I decided to just punt. - 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_archivecleanup bug
On 18 March 2014 18:55, Alvaro Herrera alvhe...@2ndquadrant.com wrote: That said, I don't find comma expression to be particularly not simple. Maybe, but we've not used it before anywhere in Postgres, so I don't see a reason to start now. Especially since C is not the native language of many people these days and people just won't understand it. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.warn_shadow
2014-03-18 19:56 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: On 18/03/14 13:43, Pavel Stehule wrote: 2014-03-18 13:23 GMT+01:00 Petr Jelinek p...@2ndquadrant.com Agree that compile_errors dos not make sense, additional_errors and additional_warnings seems better, maybe plpgsql.extra_warnings and plpgsql.extra_errors? extra* sounds better Ok, so I took the liberty of rewriting the patch so that it uses plpgsql.extra_warnings and plpgsql.extra_errors configuration variables with possible values none, all and shadow (none being the default). Updated doc and regression tests to reflect the code changes, everything builds and tests pass just fine. I don't think so only shadow is good name for some check. Maybe shadow-variables-check ?? I did one small change (that I think was agreed anyway) from Marko's original patch in that warnings are only emitted during function creation, no runtime warnings and no warnings for inline (DO) plpgsql code either as I really don't think these optional warnings/errors during runtime are a good idea. Note that the patch does not really handle the list of values as list, basically all and shadow are translated to true and proper handling of this is left to whoever will want to implement additional checks. I think this approach is fine as I don't see the need to build frameworks here and it was same in the original patch. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Wiki Page Draft for upcoming release
On Tue, Mar 18, 2014 at 6:41 PM, Josh Berkus j...@agliodbs.com wrote: Anyone? We're going public with this in 36 hours, so I'd love for it to actually be correct. I'll do a first pass now. -- greg -- 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] GSoC proposal. Index-only scans for GIST
On Tue, Mar 18, 2014 at 5:12 PM, Anastasia Lubennikova lubennikov...@gmail.com wrote: 2. gistget algorithm (several parts omitted): Check the key gistindex_keytest() - does this index tuple satisfy the scan key(s)? Scan all items on the GiST index page and insert them into the queue (or directly to output areas) plain scan Heap tuple TIDs are returned into so-pageData[] ordered scan Heap tuple TIDs are pushed into individual search queue items If the fetch() is specified by the developer, then using it, algorithm can retrieve the data directly to output areas at this stage, without reference to the heap. I think there are following changes to be made to GiST code: 1) When next consistent IndexTuple is found extract original Datums using fetch method. 2) Put those original Datums to queue. 3) When returning next ItemPointer from queue put original Datums to IndexScanDesc (into xs_itup). 3. Create function gistcanreturn to check whether fetch() is specified by user. Amcanreturn - Function to check whether index supports index-only scans, or zero if none There is the question of support index-only scans for multicolumn indexes. Probably it would require extend the interface to add separate columns checking. To solve this question I need the community's help. 4. Add details for index only scans into gistcostestimate function Also, another part of work to be mentioned is to implement fetch function for all suitable opclasses. With best regards, Alexander Korotkov.
Re: [HACKERS] Portability issues in shm_mq
On Tue, Mar 18, 2014 at 12:15 PM, Tom Lane t...@sss.pgh.pa.us wrote: It's tempting to instead add one or more tests that we specifically choose to have values we think are likely to exercise platform-specific differences or otherwise find bugs - e.g. just add a second test where the queue size and message length are both odd. Meh. I think you're putting a bit too much faith in your ability to predict the locus of bugs that you think aren't there. Well, I'm open to suggestions. maybe at a test where the queue is smaller than the message size, so that every message wraps (multiple times?). Does the code support messages larger than the queue size? If so, yes, that case clearly oughta be tested. Yep. You should be able to send and receive any message that fits within MaxAllocSize on even the smallest possible queue. Performance may suck, but if that's an issue for you then don't use such a blasted small queue. The intended use of this is to stream (potentially long) error messages or (potentially long and numerous) tuples between cooperating backends without having to preallocate space for all the data you want to send (which won't be any more feasible in a DSM than it would be in the main segment). Actually, you should be able to send or receive arbitrarily large messages if you change the MemoryContextAlloc call in shm_mq.c to MemoryContextAllocHuge, but I can't see any compelling reason to do that. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSoC proposal. Index-only scans for GIST
Josh, Anastasia has already consulted to me in person. It is not big proposal. But for newbie who is not familiar with PostgreSQL code base and especially GiST it seems fair enough. With best regards, Alexander Korotkov. On Tue, Mar 18, 2014 at 9:16 PM, Josh Berkus j...@agliodbs.com wrote: Alexander, Can you comment on the below proposal? I'd like your opinion on how difficult it will be. On 03/18/2014 06:12 AM, Anastasia Lubennikova wrote: Hello! Here is the text of my proposal which I've applied to GSoC. (and link http://www.google-melange.com/gsoc/proposal/public/google/gsoc2014/lubennikovaav/5629499534213120 ) Any suggestions and comments are welcome. *Project name* Support for index-only scans for GIST index *Brief review* Currently GiST index don't have index-only scan functionality. Index-only scans are a major performance feature added to Postgres 9.2. They allow certain types of queries to be satisfied just by retrieving data from indexes, and not from tables. This feature for B-trees (implemented since PostgreSQL-9.2) allows doing certain types of queries significantly faster. This is achieved by reduction in the amount of I/O necessary to satisfy queries. I think it will be useful to implement this feature for user defined data types that use GiST index. *Benefits to the PostgreSQL Community* Faster GiST index search would be actual for many PostgreSQL applications (for example some GIS systems). *Quantifiable results* Acceleration of GiST index search. *Project details* 1. The GiST is a balanced tree structure like a B-tree, containing key, pointer pairs. GiST key is a member of a user-defined class, and represents some property that is true of all data items reachable from the pointer associated with the key. The GiST provides a possibility to create custom data types with indexed access methods and extensible set of queries. There are seven methods that an index operator class for GiST must provide, and an eighth that is optional. -consistent -union -compress -decompress -penalty -picksplit -equal -distance (optional) I'm going to create new optional method fetch() in addition to existing. Thus user can create a method of retrieving data from the index without loss. It will be used when performing search queries to speed data retrieving. 2. gistget algorithm (several parts omitted): Check the key gistindex_keytest() - does this index tuple satisfy the scan key(s)? Scan all items on the GiST index page and insert them into the queue (or directly to output areas) plain scan Heap tuple TIDs are returned into so-pageData[] ordered scan Heap tuple TIDs are pushed into individual search queue items If the fetch() is specified by the developer, then using it, algorithm can retrieve the data directly to output areas at this stage, without reference to the heap. 3. Create function gistcanreturn to check whether fetch() is specified by user. Amcanreturn - Function to check whether index supports index-only scans, or zero if none There is the question of support index-only scans for multicolumn indexes. Probably it would require extend the interface to add separate columns checking. To solve this question I need the community's help. 4. Add details for index only scans into gistcostestimate function *Links* 1) Hellerstein J. M., Naughton J. F., Pfeffer A. Generalized search trees for database systems. - September, 1995. 2) http://www.sai.msu.su/~megera/postgres/gist/ 3) PostgreSQL 9.3.3 Documentation: chapters 11. Indexes, 55. GiST Indexes. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Re: [HACKERS] GSoC proposal. Index-only scans for GIST
On 03/18/2014 12:11 PM, Alexander Korotkov wrote: Josh, Anastasia has already consulted to me in person. It is not big proposal. But for newbie who is not familiar with PostgreSQL code base and especially GiST it seems fair enough. Thanks, that's what I wanted to know. -- 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] B-tree descend for insertion locking
On Tue, Mar 18, 2014 at 4:12 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: See attached patch. The new contract of _bt_getroot is a bit weird: it locks the returned page in the requested lock-mode, shared or exclusive, if the root page was also the leaf page. Otherwise it's locked in shared mode regardless off the requested lock mode. But OTOH, the new contract for _bt_search() is more clear now: it actually locks the returned page in the requested mode, where it used to only use the access parameter to decide whether to create a root page if the index was empty. I had considered this myself, and am under the impression that the only reason things work this way is because it makes the interface of _bt_getroot() clearer. I think what you propose is logical, and you should pursue it, but fwiw I'm not convinced that you've really simplified _bt_search(). However, you have kind of made _bt_search() into something that succinctly represents what is useful about Lehman and Yao as compared to earlier concurrent locking techniques, and that's a good thing. I would probably have remarked on that in the comments if I were you. I certainly would not have left out some reference to LY. You've removed an existing one where the lock escalation occurs, emphasizing that it's safe, and I'd like to see you at least compensate for 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] pg_archivecleanup bug
On 03/18/2014 09:04 PM, Simon Riggs wrote: On 18 March 2014 18:55, Alvaro Herrera alvhe...@2ndquadrant.com wrote: That said, I don't find comma expression to be particularly not simple. Maybe, but we've not used it before anywhere in Postgres, so I don't see a reason to start now. Especially since C is not the native language of many people these days and people just won't understand it. Agreed. The psqlODBC code is littered with comma expressions, and the first time I saw it, it took me a really long time to figure out what if (foo = malloc(...), foo) { } meant. And I consider myself quite experienced with C. - 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] plpgsql.warn_shadow
On 18 March 2014 19:04, Pavel Stehule pavel.steh...@gmail.com wrote: I don't think so only shadow is good name for some check. Maybe shadow-variables-check shadowed-variables would be a better name. -- 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] plpgsql.warn_shadow
2014-03-18 20:14 GMT+01:00 Simon Riggs si...@2ndquadrant.com: On 18 March 2014 19:04, Pavel Stehule pavel.steh...@gmail.com wrote: I don't think so only shadow is good name for some check. Maybe shadow-variables-check shadowed-variables would be a better name. +1 -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] plpgsql.warn_shadow
On 18/03/14 20:15, Pavel Stehule wrote: 2014-03-18 20:14 GMT+01:00 Simon Riggs si...@2ndquadrant.com mailto:si...@2ndquadrant.com: On 18 March 2014 19:04, Pavel Stehule pavel.steh...@gmail.com mailto:pavel.steh...@gmail.com wrote: I don't think so only shadow is good name for some check. Maybe shadow-variables-check shadowed-variables would be a better name. +1 Attached V4 uses shadowed-variables instead of shadow. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index bddd458..d6709fd 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -4711,6 +4711,51 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$ /variablelist /sect2 + sect2 id=plpgsql-extra-checks + titleAdditional compile-time checks/title + + para +To aid the user in finding instances of simple but common problems before +they cause harm, applicationPL/PgSQL/ provides a number of additional +replaceablechecks/. When enabled, depending on the configuration, they +can be used to emit either a literalWARNING/ or an literalERROR/ +during the compilation of a function. + /para + + para + These additional checks are enabled through the configuration variables + varnameplpgsql.extra_warnings/ for warnings and + varnameplpgsql.extra_errors/ for errors. Both can be set either to + a comma-separated list of checks, literalnone/ or literalall/. + The default is literalnone/. Currently the list of available checks + includes only one: + variablelist + varlistentry +termvarnameshadowed-variables/varname/term +listitem + para + Checks if a declaration shadows a previously defined variable. For + example (with varnameplpgsql.extra_warnings/ set to + varnameshadowed-variables/varname): +programlisting +CREATE FUNCTION foo(f1 int) RETURNS int AS $$ +DECLARE +f1 int; +BEGIN +RETURN f1; +END +$$ LANGUAGE plpgsql; +WARNING: variable f1 shadows a previously defined variable +LINE 3: f1 int; +^ +CREATE FUNCTION +/programlisting + /para +/listitem + /varlistentry + /variablelist + /para + /sect2 /sect1 !-- Porting from Oracle PL/SQL -- diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c index 5afc2e5..8732efc 100644 --- a/src/pl/plpgsql/src/pl_comp.c +++ b/src/pl/plpgsql/src/pl_comp.c @@ -352,6 +352,9 @@ do_compile(FunctionCallInfo fcinfo, function-out_param_varno = -1; /* set up for no OUT param */ function-resolve_option = plpgsql_variable_conflict; function-print_strict_params = plpgsql_print_strict_params; + /* only promote extra warnings and errors at CREATE FUNCTION time */ + function-extra_warnings = plpgsql_extra_warnings forValidator; + function-extra_errors = plpgsql_extra_errors forValidator; if (is_dml_trigger) function-fn_is_trigger = PLPGSQL_DML_TRIGGER; @@ -849,6 +852,9 @@ plpgsql_compile_inline(char *proc_source) function-out_param_varno = -1; /* set up for no OUT param */ function-resolve_option = plpgsql_variable_conflict; function-print_strict_params = plpgsql_print_strict_params; + /* don't do extra validation for inline code as we don't want to add spam at runtime */ + function-extra_warnings = false; + function-extra_errors = false; plpgsql_ns_init(); plpgsql_ns_push(func_name); diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index c0cb585..98f7ddd 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -727,6 +727,20 @@ decl_varname : T_WORD $1.ident, NULL, NULL, NULL) != NULL) yyerror(duplicate declaration); + + if (plpgsql_curr_compile-extra_warnings || plpgsql_curr_compile-extra_errors) + { + PLpgSQL_nsitem *nsi; + nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false, + $1.ident, NULL, NULL, NULL); + if (nsi != NULL) +ereport(plpgsql_curr_compile-extra_errors ? ERROR : WARNING, + (errcode(ERRCODE_DUPLICATE_ALIAS), + errmsg(variable \%s\ shadows a previously defined variable, +$1.ident), + parser_errposition(@1))); + } + } | unreserved_keyword { @@ -740,6 +754,20 @@ decl_varname : T_WORD $1, NULL, NULL, NULL) != NULL) yyerror(duplicate declaration); + + if (plpgsql_curr_compile-extra_warnings || plpgsql_curr_compile-extra_errors) + { + PLpgSQL_nsitem *nsi; + nsi = plpgsql_ns_lookup(plpgsql_ns_top(), false, + $1, NULL, NULL, NULL); + if (nsi != NULL) +ereport(plpgsql_curr_compile-extra_errors ? ERROR : WARNING, + (errcode(ERRCODE_DUPLICATE_ALIAS), + errmsg(variable \%s\ shadows a previously defined variable, +$1), + parser_errposition(@1))); + } + }
Re: [HACKERS] Wiki Page Draft for upcoming release
On Tue, Mar 18, 2014 at 7:05 PM, Greg Stark st...@mit.edu wrote: I'll do a first pass now. I fixed the Causes section. I haven't touched the other sections which are pretty reasonable. It would be nice to have more suggestions for what people should do other than dump/restore. It would be nice to be able to tell people that if they vacuum, then reindex and check all their foreign key constraints then they should be ok. I think that's almost true except I'm not sure how to tell that they've waited long enough before vacuuming and I'm not 100% sure it'll fix the problem. (Also they could have lost a row which has since had all all it's referencing rows deleted. The database won't be able to help them find that.) -- greg -- 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] Wiki Page Draft for upcoming release
On 2014-03-18 19:28:53 +, Greg Stark wrote: It would be nice to be able to tell people that if they vacuum, then reindex and check all their foreign key constraints then they should be ok. I don't think so: http://archives.postgresql.org/message-id/20140317233919.GS16438%40awork2.anarazel.de I still think a rewriting noop ALTER TABLE ... ALTER COLUMN col TYPE old_type USING (col); is the only real thing to do. 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] plpgsql.warn_shadow
Hello Tomorrow I'll do a review fast check para +To aid the user in finding instances of simple but common problems before +they cause harm, applicationPL/PgSQL/ provides a number of additional +replaceablechecks/. When enabled, depending on the configuration, they +can be used to emit either a literalWARNING/ or an literalERROR/ +during the compilation of a function. + /para provides a number of additional There will be only one check in 9.4, so this sentence is confusing and should be reformulated. Regards Pavel 2014-03-18 20:29 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: On 18/03/14 20:15, Pavel Stehule wrote: 2014-03-18 20:14 GMT+01:00 Simon Riggs si...@2ndquadrant.com: On 18 March 2014 19:04, Pavel Stehule pavel.steh...@gmail.com wrote: I don't think so only shadow is good name for some check. Maybe shadow-variables-check shadowed-variables would be a better name. +1 Attached V4 uses shadowed-variables instead of shadow. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] plpgsql.warn_shadow
On 3/18/14, 7:56 PM, Petr Jelinek wrote: Ok, so I took the liberty of rewriting the patch so that it uses plpgsql.extra_warnings and plpgsql.extra_errors configuration variables with possible values none, all and shadow (none being the default). Updated doc and regression tests to reflect the code changes, everything builds and tests pass just fine. Cool, thanks! I did one small change (that I think was agreed anyway) from Marko's original patch in that warnings are only emitted during function creation, no runtime warnings and no warnings for inline (DO) plpgsql code either as I really don't think these optional warnings/errors during runtime are a good idea. Not super excited, but I can live with that. Note that the patch does not really handle the list of values as list, basically all and shadow are translated to true and proper handling of this is left to whoever will want to implement additional checks. I think this approach is fine as I don't see the need to build frameworks here and it was same in the original patch. Yeah, I don't think rushing all that logic into 9.4 would be such a good idea. Especially since it's not necessary at all. Regards, Marko Tiikkaja -- 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: show relation and tuple infos of a lock to acquire
On Tue, Mar 18, 2014 at 12:53 PM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Please see my reply to Robert. My proposal (in form of a patch) is while operating on tuple (0,2) in table foo: updating tuple Would this work for you? It's pretty lousy from a readability standpoint, even in English; I shudder to think what it might come out as after translation. I think the enum idea you floated is probably worth doing. It's certainly no more complex than passing a string, no? +1. We've done similar things in tablecmds.c. -- 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] plpgsql.warn_shadow
On 18/03/14 20:36, Pavel Stehule wrote: para +To aid the user in finding instances of simple but common problems before +they cause harm, applicationPL/PgSQL/ provides a number of additional +replaceablechecks/. When enabled, depending on the configuration, they +can be used to emit either a literalWARNING/ or an literalERROR/ +during the compilation of a function. + /para provides a number of additional There will be only one check in 9.4, so this sentence is confusing and should be reformulated. Thanks, yeah I left that sentence unchanged from original patch, maybe I should remove the word number there as it implies that there are a lot of them, but I don't really want to change everything to singular when the input is specified as list. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] plpgsql.warn_shadow
2014-03-18 20:44 GMT+01:00 Petr Jelinek p...@2ndquadrant.com: On 18/03/14 20:36, Pavel Stehule wrote: para +To aid the user in finding instances of simple but common problems before +they cause harm, applicationPL/PgSQL/ provides a number of additional +replaceablechecks/. When enabled, depending on the configuration, they +can be used to emit either a literalWARNING/ or an literalERROR/ +during the compilation of a function. + /para provides a number of additional There will be only one check in 9.4, so this sentence is confusing and should be reformulated. Thanks, yeah I left that sentence unchanged from original patch, maybe I should remove the word number there as it implies that there are a lot of them, but I don't really want to change everything to singular when the input is specified as list. What about add sentence: in this moment only shadowed-variables is available? Pavel -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services