[HACKERS] wrong state of patch in CF
Patch (https://commitfest.postgresql.org/action/patch_view?id=1161) is already committed by Commit http://git.postgresql.org/pg/commitdiff/b23160889c963dfe23d8cf1f9be64fb3c535a2d6 It should be marked as Committed in CF. With Regards, Amit Kapila. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On 23 June 2013 03:16, Stephen Frost sfr...@snowman.net wrote: Still doesn't really address the issue of dups though. Checking for duplicates in all cases would be wasteful, since often we are joining to the PK of a smaller table. If duplicates are possible at all for a join, then it would make sense to build the hash table more carefully to remove dupes. I think we should treat that as a separate issue. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pgbench --throttle (submission 7 - with lag measurement)
So my argumented conclusion is that the issue is somewhere within PQfinish(), possibly in interaction with pgbench doings, but is *NOT* related in any way to the throttling patch, as it is preexisting it. Gregs stumbled upon it because he looked at latencies. An additional thought: The latency measures *elapsed* time. As a small laptop is running 30 clients and their server processes at a significant load, there are a lot of context switching going on, so maybe it just happens that the pgbench process is switched off and then on as PQfinish() is running, so the point would really be that the host is loaded and that's it. I'm not sure of the likelyhood of such an event. It is possible that would be more frequent after timer_exceeded because the system is closing postgres processes, and would depend on what the kernel process scheduler does. So the explanation would be: your system is loaded, and it shows in subtle ways here and there when you do detailed measures. That is life. Basically this is a summary my (long) experience with performance experiments on computers. What are you really measuring? What is really happening? When a system is loaded, there are many things which interact one with the other and induce particular effects on performance measures. So usually what is measured is not what one is expecting. Greg thought that he was measuring transaction latencies, but it was really client contention in a thread. I thought that I was measuring PQfinish() time, but maybe it is the probability of a context switch. -- Fabien. -- Sent 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 for fast gin cache performance improvement
On 06/23/2013 04:03 AM, ian link wrote: Looks like my community login is still not working. No rush, just wanted to let you know. Thanks! have you tried to log in once to the main website per: http://www.postgresql.org/message-id/CABUevEyt9tQfcF7T2Uzcr8WeF9M=s8qSACuCmN5L2Et26=r...@mail.gmail.com ? Stefan -- Sent 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 for fast gin cache performance improvement
I just tried it and my account works now. Thanks! Ian Stefan Kaltenbrunner Sunday, June 23, 2013 2:05 AM have you tried to log in once to the main website per:http://www.postgresql.org/message-id/CABUevEyt9tQfcF7T2Uzcr8WeF9M=s8qSACuCmN5L2Et26=r...@mail.gmail.com?Stefan ian link Saturday, June 22, 2013 7:03 PM Looks like my community login is still not working. No rush, just wanted to let you know. Thanks!Ian Ian Link Tuesday, June 18, 2013 11:41 AM No worries! I'll just wait until it's up again. Thanks Ian Kevin Grittner Tuesday, June 18, 2013 9:15 AM Oops -- we seem to have a problem with new community logins at themoment, which will hopefully be straightened out soon. You mightwant to wait a few days if you don't already have a login.--Kevin GrittnerEnterpriseDB: http://www.enterprisedb.comThe Enterprise PostgreSQL Company Kevin Grittner Tuesday, June 18, 2013 9:09 AM Ian Link i...@ilink.io wrote: This patch contains a performance improvement for the fast gin cache. Our test queries improve from about 0.9 ms to 0.030 ms. Impressive! Thanks for reading and considering this patch! Congratulations on your first PostgreSQL patch! To get it scheduled for review, please add it to this page: https://commitfest.postgresql.org/action/commitfest_view/open You will need to get a community login (if you don't already have one), but that is a quick and painless process. Choose an appropriate topic (like "Performance") and reference the message ID of the email to which you attached the patch. Don't worry about the fields for reviewers, committer, or date closed. Sorry for the administrative overhead, but without it things can fall through the cracks. You can find an overview of the review process with links to more detail here: http://wiki.postgresql.org/wiki/CommitFest Thanks for contributing!
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On 23 June 2013 03:16, Stephen Frost sfr...@snowman.net wrote: Will think on it more. Some other thoughts related to this... * Why are we building a special kind of hash table? Why don't we just use the hash table code that we in every other place in the backend. If that code is so bad why do we use it everywhere else? That is extensible, so we could try just using that. (Has anyone actually tried?) * We're not thinking about cache locality and set correspondence either. If the join is expected to hardly ever match, then we should be using a bitmap as a bloom filter rather than assuming that a very large hash table is easily accessible. * The skew hash table will be hit frequently and would show good L2 cache usage. I think I'll try adding the skew table always to see if that improves the speed of the hash join. -- 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] INTERVAL overflow detection is terribly broken
Hi, after studying ITERVAL and having a long chat with RhoidumToad and StuckMojo on #postgresql, I am presenting you 3 bugs regarding INTERVAL. As far as I understand, the Interval struct (binary internal representation) consists of: int32 months int32 days int64 microseconds 1. OUTPUT ERRORS: Since 2^63 microseconds equals 2,562,047,788 2^31 hours, the overflow in pg_tm when displaying the value causes overflow. The value of Interval struct is actually correct, error happens only on displaying it. SELECT INTERVAL '2147483647 hours' + INTERVAL '5 hours' -2147483644:00:00 Even wireder: SELECT INTERVAL '2147483647 hours' + '1 hour' --2147483648:00:00 notice the double minus? Don't ask how I came across this two bugs. 2. OPERATION ERRORS: When summing two intervals, the user is not notified when overflow occurs: SELECT INT '2147483647' + INT '1' ERROR: integer out of range SELECT INTERVAL '2147483647 days' + INTERVAL '1 day' -2147483648 days This should be analogous. 3. PARSER / INPUT ERRORS: This is perhaps the hardest one to explain, since this is an architectural flaw. You are checking the overflows when parsing string - pg_tm struct. However, at this point, the parser doesn't know, that weeks and days are going to get combined, or years are going to get converted to months, for example. Unawarness of underlying Interval struct causes two types of suberrors: a) False positive SELECT INTERVAL '2147483648 microseconds' ERROR: interval field value out of range: 2147483648 microseconds This is not right. Microseconds are internally stored as 64 bit signed integer. The catch is: this amount of microseconds is representable in Interval data structure, this shouldn't be an error. b) False negative SELECT INTERVAL '10 years' -73741824 years We don't catch errors like this, because parser only checks for overflow in pg_tm. If overflow laters happens in Interval, we don't seem to care. 4. POSSIBLE SOLUTIONS: a) Move the overflow checking just after the conversion of pg_tm - Interval is made. This way, you can accurately predict if the result is really not store-able. b) Because of 1), you have to declare tm_hour as int64, if you want to use that for the output. But, why not use Interval struct for printing directly, without intermediate pg_tm? 5. Offtopic: Correct the documentation, INTERVAL data size is 16 bytes, not 12. Rok Kralj -- eMail: rok.kr...@gmail.com
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On Sunday, June 23, 2013, Simon Riggs wrote: On 23 June 2013 03:16, Stephen Frost sfr...@snowman.net javascript:; wrote: Still doesn't really address the issue of dups though. Checking for duplicates in all cases would be wasteful, since often we are joining to the PK of a smaller table. Well, that's what ndistinct is there to help us figure out. If we don't trust that though... If duplicates are possible at all for a join, then it would make sense to build the hash table more carefully to remove dupes. I think we should treat that as a separate issue. We can't simply remove the dups... We have to return all the matching dups in the join. I did write a patch which created a 2-level list structure where the first level was uniques and the 2nd was dups, but it was extremely expensive to create the hash table then and scanning it wasn't much cheaper. Thanks, Stephen
Re: [HACKERS] A better way than tweaking NTUP_PER_BUCKET
On Sunday, June 23, 2013, Simon Riggs wrote: On 23 June 2013 03:16, Stephen Frost sfr...@snowman.net javascript:; wrote: Will think on it more. Some other thoughts related to this... * Why are we building a special kind of hash table? Why don't we just use the hash table code that we in every other place in the backend. If that code is so bad why do we use it everywhere else? That is extensible, so we could try just using that. (Has anyone actually tried?) I've not looked at the hash table in the rest of the backend. * We're not thinking about cache locality and set correspondence either. If the join is expected to hardly ever match, then we should be using a bitmap as a bloom filter rather than assuming that a very large hash table is easily accessible. That's what I was suggesting earlier, though I don't think it's technically a bloom filter- doesn't that require multiple hash functions?I don't think we want to require every data type to provide multiple hash functions. * The skew hash table will be hit frequently and would show good L2 cache usage. I think I'll try adding the skew table always to see if that improves the speed of the hash join. The skew tables is just for common values though... To be honest, I have some doubts about that structure really being a terribly good approach for anything which is completely in memory. Thanks, Stephen
Re: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
On 21 June 2013 10:02, Dean Rasheed dean.a.rash...@gmail.com wrote: On 21 June 2013 06:16, David Fetter da...@fetter.org wrote: On Fri, Jun 21, 2013 at 12:10:25AM -0400, Alvaro Herrera wrote: David Fetter escribió: On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote: In my testing of sub-queries in the FILTER clause (an extension to the spec), I was able to produce the following error: Per the spec, B) A filter clause shall not contain a query expression, a window function, or an outer reference. If this is not allowed, I think there should be a clearer error message. What Dean showed is an internal (not user-facing) error message. Please find attached a patch which allows subqueries in the FILTER clause and adds regression testing for same. Nice! I should have pointed out that it was already working for some sub-queries, just not all. It's good to see that it was basically just a one-line fix, because I think it would have been a shame to not support sub-queries. I hope to take another look at it over the weekend. I'm still not happy that this patch is making FILTER a new reserved keyword, because I think it is a common enough English word (and an obscure enough SQL keyword) that people may well have used it for table names or aliases, and so their code will break. There is another thread discussing a similar issue with lead/lag ignore nulls: http://www.postgresql.org/message-id/CAOdE5WQnfR737OkxNXuLWnwpL7=OUssC-63ijP2=2rnrtw8...@mail.gmail.com Playing around with the idea proposed there, it seems that it is possible to make FILTER (and also OVER) unreserved keywords, by splitting out the production of aggregate/window functions from normal functions in gram.y. Aggregate/window functions are not allowed in index_elem or func_table constructs, but they may appear in c_expr's. That resolves the shift/reduce conflicts that would otherwise occur from subsequent alias clauses, allowing FILTER and OVER to be unreserved. There is a drawback --- certain error messages become less specific, for example: SELECT * FROM rank() OVER(ORDER BY random()); is now a syntax error, rather than the more useful error saying that window functions aren't allowed in the FROM clause. That doesn't seem like such a common case though. What do you think? Regards, Dean make-filter-unreserved.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review [was Re: [HACKERS] MD5 aggregate]
On 21 June 2013 21:04, David Fetter da...@fetter.org wrote: On Fri, Jun 21, 2013 at 10:48:35AM -0700, David Fetter wrote: On Mon, Jun 17, 2013 at 11:34:52AM +0100, Dean Rasheed wrote: On 15 June 2013 10:22, Dean Rasheed dean.a.rash...@gmail.com wrote: There seem to be 2 separate directions that this could go, which really meet different requirements: 1). Produce an unordered sum for SQL to compare 2 tables regardless of the order in which they are scanned. A possible approach to this might be something like an aggregate md5_total(text/bytea) returns text that returns the sum of the md5 values of each input value, treating each md5 value as an unsigned 128-bit integer, and then producing the hexadecimal representation of the final sum. This should out-perform a solution based on numeric addition, and in typical cases, the result wouldn't be much longer than a regular md5 sum, and so would be easy to eyeball for differences. I've been playing around with the idea of an aggregate that computes the sum of the md5 hashes of each of its inputs, which I've called md5_total() for now, although I'm not particularly wedded to that name. Comparing it with md5_agg() on a 100M row table (see attached test script) produces interesting results: SELECT md5_agg(foo.*::text) FROM (SELECT * FROM foo ORDER BY id) foo; 50bc42127fb9b028c9708248f835ed8f Time: 92960.021 ms SELECT md5_total(foo.*::text) FROM foo; 02faea7fafee4d253fc94cfae031afc43c03479c Time: 96190.343 ms Unlike md5_agg(), it is no longer a true MD5 sum (for one thing, its result is longer) but it seems like it would be very useful for quickly comparing data in SQL, since its value is not dependent on the row-order making it easier to use and better performing if there is no usable index for ordering. Note, however, that if there is an index that can be used for ordering, the performance is not necessarily better than md5_agg(), as this example shows. There is a small additional overhead per row for initialising the MD5 sums, and adding the results to the total, but I think the biggest factor is that md5_total() is processing more data. The reason is that MD5 works on 64-byte blocks, so the total amount of data going through the core MD5 algorithm is each row's size is rounded up to a multiple of 64. In this simple case it ends up processing around 1.5 times as much data: SELECT sum(length(foo.*::text)) AS md5_agg, sum(((length(foo.*::text)+63)/64)*64) AS md5_total FROM foo; md5_agg | md5_total +- 8103815438 | 12799909248 although of course that overhead won't be as large on wider tables, and even in this case the overall performance is still on a par with md5_agg(). ISTM that both aggregates are potentially useful in different situations. I would probably typically use md5_total() because of its simplicity/order-independence and consistent performance, but md5_agg() might also be useful when comparing with external data. Regards, Dean Performance review (skills needed: ability to time performance) Does the patch slow down simple tests? Yes. For an MD5 checksum of some random data, here are results from master: shackle@postgres:5493=# WITH t1 AS (SELECT string_agg(chr(floor(255*random()+1)::int),'') AS a FROM generate_series(1,1)), postgres-# t2 AS (SELECT a FROM t1 CROSS JOIN generate_series(1,1)) postgres-# select md5(a::text) FROM t2; shackle@postgres:5493=# \timing Timing is on. shackle@postgres:5493=# \g Time: 955.393 ms shackle@postgres:5493=# \g Time: 950.909 ms shackle@postgres:5493=# \g Time: 947.955 ms shackle@postgres:5493=# \g Time: 946.193 ms shackle@postgres:5493=# \g Time: 950.591 ms shackle@postgres:5493=# \g Time: 952.346 ms shackle@postgres:5493=# \g Time: 948.623 ms shackle@postgres:5493=# \g Time: 939.819 ms and here from master + the patch: WITH t1 AS (SELECT string_agg(chr(floor(255*random()+1)::int),'') AS a FROM generate_series(1,1)), t2 AS (SELECT a FROM t1 CROSS JOIN generate_series(1,1)) select md5(a::text) FROM t2; Time: 1129.178 ms shackle@postgres:5494=# \g Time: 1134.013 ms shackle@postgres:5494=# \g Time: 1124.387 ms shackle@postgres:5494=# \g Time: 1119.733 ms shackle@postgres:5494=# \g Time: 1104.906 ms shackle@postgres:5494=# \g Time: 1137.055 ms shackle@postgres:5494=# \g Time: 1128.977 ms
Re: [HACKERS] wrong state of patch in CF
Amit kapila amit.kap...@huawei.com wrote: Patch (https://commitfest.postgresql.org/action/patch_view?id=1161) is already committed by Commit http://git.postgresql.org/pg/commitdiff/b23160889c963dfe23d8cf1f9be64fb3c535a2d6 It should be marked as Committed in CF. Done. Thanks! -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: The git tree is at: git://git.postgresql.org/git/users/andresfreund/postgres.git branch xlog-decoding-rebasing-cf4 http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/xlog-decoding-rebasing-cf4 On 2013-06-15 00:48:17 +0200, Andres Freund wrote: Overview of the attached patches: 0001: indirect toast tuples; required but submitted independently 0002: functions for testing; not required, 0003: (tablespace, filenode) syscache; required 0004: RelationMapFilenodeToOid: required, simple 0005: pg_relation_by_filenode() function; not required but useful 0006: Introduce InvalidCommandId: required, simple 0007: Adjust Satisfies* interface: required, mechanical, 0008: Allow walsender to attach to a database: required, needs review 0009: New GetOldestXmin() parameter; required, pretty boring 0010: Log xl_running_xact regularly in the bgwriter: required 0011: make fsync_fname() public; required, needs to be in a different file 0012: Relcache support for an Relation's primary key: required 0013: Actual changeset extraction; required 0014: Output plugin demo; not required (except for testing) but useful 0015: Add pg_receivellog program: not required but useful 0016: Add test_logical_decoding extension; not required, but contains the tests for the feature. Uses 0014 0017: Snapshot building docs; not required Version v5-01 attached Confirmed that all 17 patch files now apply cleanly, and that `make check-world` builds cleanly after each patch in turn. Reviewing and testing the final result now. If that all looks good, will submit a separate review of each patch. Simon, do you want to do the final review and commit after I do each piece? -- Kevin Grittner 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: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
Dean Rasheed dean.a.rash...@gmail.com wrote: I'm still not happy that this patch is making FILTER a new reserved keyword, because I think it is a common enough English word (and an obscure enough SQL keyword) that people may well have used it for table names or aliases, and so their code will break. Well, if it is a useful capability with a standard syntax, I think we should go with the standard syntax even if it might break application code that uses, as unquoted identifiers, words reserved by the SQL standard. Of course, non-reserved is better than reserved if we can manage it. Playing around with the idea proposed there, it seems that it is possible to make FILTER (and also OVER) unreserved keywords, by splitting out the production of aggregate/window functions from normal functions in gram.y. Aggregate/window functions are not allowed in index_elem or func_table constructs, but they may appear in c_expr's. That resolves the shift/reduce conflicts that would otherwise occur from subsequent alias clauses, allowing FILTER and OVER to be unreserved. There is a drawback --- certain error messages become less specific, for example: SELECT * FROM rank() OVER(ORDER BY random()); is now a syntax error, rather than the more useful error saying that window functions aren't allowed in the FROM clause. That doesn't seem like such a common case though. What do you think? I think it is OK if that gets a syntax error. If that's the worst case I like this approach. This also helps keep down the size of the generated parse tables, doesn't it? -- Kevin Grittner 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] A better way than tweaking NTUP_PER_BUCKET
On 23.06.2013 01:48, Simon Riggs wrote: On 22 June 2013 21:40, Stephen Frostsfr...@snowman.net wrote: I'm actually not a huge fan of this as it's certainly not cheap to do. If it can be shown to be better than an improved heuristic then perhaps it would work but I'm not convinced. We need two heuristics, it would seem: * an initial heuristic to overestimate the number of buckets when we have sufficient memory to do so * a heuristic to determine whether it is cheaper to rebuild a dense hash table into a better one. Although I like Heikki's rebuild approach we can't do this every x2 overstretch. Given large underestimates exist we'll end up rehashing 5-12 times, which seems bad. It's not very expensive. The hash values of all tuples have already been calculated, so rebuilding just means moving the tuples to the right bins. Better to let the hash table build and then re-hash once, it we can see it will be useful. That sounds even less expensive, though. - 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] changeset generation v5-01 - Patches git tree
Kevin Grittner kgri...@ymail.com wrote: Confirmed that all 17 patch files now apply cleanly, and that `make check-world` builds cleanly after each patch in turn. Just to be paranoid, I did one last build with all 17 patch files applied to 7dfd5cd21c0091e467b16b31a10e20bbedd0a836 using this line: make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python make -j4 world and it died with this: 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 -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_receivexlog.o pg_receivexlog.c -MMD -MP -MF .deps/pg_receivexlog.Po 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 -I. -I. -I../../../src/interfaces/libpq -I../../../src/bin/pg_dump -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o mainloop.o mainloop.c -MMD -MP -MF .deps/mainloop.Po 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 pg_receivellog.o receivelog.o streamutil.o -L../../../src/port -lpgport -L../../../src/common -lpgcommon -L../../../src/interfaces/libpq -lpq -L../../../src/port -L../../../src/common -L/usr/lib -Wl,--as-needed -Wl,-rpath,'/home/kgrittn/pg/master/Debug/lib',--enable-new-dtags -lpgport -lpgcommon -lxslt -lxml2 -lssl -lcrypto -lz -lreadline -lcrypt -ldl -lm -o pg_receivellog gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 make[3]: Leaving directory `/home/kgrittn/pg/master/src/bin/pg_basebackup' make[2]: *** [all-pg_basebackup-recurse] Error 2 make[2]: *** Waiting for unfinished jobs It works with this patch-on-patch: diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile index a41b73c..18d02f3 100644 --- a/src/bin/pg_basebackup/Makefile +++ b/src/bin/pg_basebackup/Makefile @@ -42,6 +42,7 @@ installdirs: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o It appears to be an omission from file 0015. -- Kevin Grittner 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] changeset generation v5-01 - Patches git tree
Kevin Grittner kgri...@ymail.com wrote: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o Just that part. -- Kevin Grittner 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] BUG #7493: Postmaster messages unreadable in a Windows console
On Wed, Feb 20, 2013 at 04:00:00PM +0400, Alexander Law wrote: 15.02.2013 02:59, Noah Misch wrote: With your proposed change, the problem will resurface in an actual SQL_ASCII database. At the problem's root is write_console()'s assumption that messages are in the database encoding. pg_bind_textdomain_codeset() tries to make that so, but it only works for encodings with a pg_enc2gettext_tbl entry. That excludes SQL_ASCII, MULE_INTERNAL, and others. write_console() needs to behave differently in such cases. Thank you for the notice. So it seems that DatabaseEncoding variable alone can't present a database encoding (for communication with a client) and current process messages encoding (for logging messages) at once. There should be another variable, something like CurrentProcessEncoding, that will be set to OS encoding at start and can be changed to encoding of a connected database (if bind_textdomain_codeset succeeded). I'd call it MessageEncoding unless it corresponds with similar rigor to a broader concept. Please look at the next version of the patch. I studied this patch. I like the APIs it adds, but the implementation details required attention. Your patch assumes the message encoding will be a backend encoding, but this need not be so; locale Japanese (Japan) uses CP932 aka SJIS. I've also added the non-backend encodings to pg_enc2gettext_tbl; I'd welcome sanity checks from folks who know those encodings well. write_console() still garbled the payload in all cases where it used write() to a console instead of WriteConsoleW(). You can observe this by configuring Windows for Russian, running initdb with default locale settings, and running select 1/0 in template1. See comments for the technical details; I fixed this by removing the logic to preemptively skip WriteConsoleW() for encoding-related reasons. (Over in write_eventlog(), I am suspicious of the benefits we get from avoiding ReportEventW() where possible. I have not removed that, though.) --- a/src/backend/main/main.c +++ b/src/backend/main/main.c @@ -100,6 +100,8 @@ main(int argc, char *argv[]) set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN(postgres)); + SetMessageEncoding(GetPlatformEncoding()); SetMessageEncoding() and GetPlatformEncoding() can both elog()/ereport(), but this is too early in the life of a PostgreSQL process for that. The goal of this line of code is to capture gettext's default encoding, which is platform-specific. On non-Windows platforms, it's the encoding implied by LC_CTYPE. On Windows, it's the Windows ANSI code page. GetPlatformEncoding() always gives the encoding implied by LC_CTYPE, which is therefore not correct on Windows. You can observe broken output by setting your locale (in control panel Region and Language - Formats - Format) to something unrelated to your Windows ANSI code page (Region and Language - Administrative - Change system locale...). Let's make PostgreSQL independent of gettext's Windows-specific default by *always* calling bind_textdomain_codeset() on Windows. In the postmaster and other non-database-attached processes, as well as in SQL_ASCII databases, we would pass the encoding implied by LC_CTYPE. Besides removing a discrepancy in PostgreSQL behavior between Windows and non-Windows platforms, this has the great practical advantage of reducing PostgreSQL's dependence on the deprecated Windows ANSI code page, which can only be changed on a system-wide basis, by an administrator, after a reboot. For message creation purposes, the encoding implied by LC_CTYPE on Windows is somewhat arbitrary. Microsoft has deprecated them all in favor of UTF16, and some locales (e.g. Hindi (India)) have no legacy code page. I can see adding a postmaster_encoding GUC to be used in place of consulting LC_CTYPE. I haven't done that now; I just mention it for future reference. On a !ENABLE_NLS build, messages are ASCII with database-encoding strings sometimes interpolated. Therefore, such builds should keep the message encoding equal to the database encoding. The MessageEncoding was not maintained accurately on non-Windows systems. For example, connecting to an lc_ctype=LATIN1, encoding=LATIN1 database does not require a bind_textdomain_codeset() call on such systems, so we did not update the message encoding. This was academic for the time being, because MessageEncoding is only consulted on Windows. The attached revision fixes all above points. Would you look it over? The area was painfully light on comments, so I added some. I renamed pgwin32_toUTF16(), which ceases to be a good name now that it converts from message encoding, not database encoding. GetPlatformEncoding() became unused, so I removed it. (If we have cause reintroduce the exact same concept later, GetTTYEncoding() would name it more accurately.) What should we do for the back branches, if anything? Fixes for garbled display on consoles and event logs are fair to
Re: [HACKERS] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com wrote: Pushed and attached. The contrib/test_logical_decoding/sql/ddl.sql script is generating unexpected results. For both table_with_pkey and table_with_unique_not_null, updates of the primary key column are showing: old-pkey: id[int4]:0 ... instead of the expected value of 2 or -2. See attached. -- Kevin Grittner EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company regression.diffs Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: backend hangs at immediate shutdown (Re: Back-branch update releases coming in a couple weeks)
On Sun, Jun 23, 2013 at 01:55:19PM +0900, MauMau wrote: From: Robert Haas robertmh...@gmail.com On Fri, Jun 21, 2013 at 10:02 PM, MauMau maumau...@gmail.com wrote: I'm comfortable with 5 seconds. We are talking about the interval between sending SIGQUIT to the children and then sending SIGKILL to them. In most situations, the backends should terminate immediately. However, as I said a few months ago, ereport() call in quickdie() can deadlock indefinitely. This is a PostgreSQL's bug. So let's fix that bug. Then we don't need this. [quoted part filtered to undo caps lock] There are two ways to fix that bug. You are suggesting 1 of the following two methods, aren't you? I suspect Robert meant to prevent the deadlock by limiting quickdie() to calling only async-signal-safe functions. Implementing that looked grotty when we last discussed it, though, and it would not help against a library or trusted PL function suspending SIGQUIT. 1. (Robert-san's idea) Upon receipt of immediate shutdown request, postmaster sends SIGKILL to its children. 2. (Tom-san's idea) Upon receipt of immediate shutdown request, postmaster first sends SIGQUIT to its children, wait a while for them to terminate, then sends SIGKILL to them. At first I proposed 1. Then Tom san suggested 2 so that the server is as friendly to the clients as now by notifying them of the server shutdown. I was convinced by that idea and chose 2. Basically, I think both ideas are right. They can solve the original problem. Agreed. However, re-considering the meaning of immediate shutdown, I feel 1 is a bit better, because trying to do something in quickdie() or somewhere does not match the idea of immediate. We may not have to be friendly to Perhaps so, but more important than the definition of the word immediate is what an immediate shutdown has long meant in PostgreSQL. All this time it has made some effort to send a message to the client. Note also that the patch under discussion affects both immediate shutdowns and the automatic reset in response to a backend crash. I think the latter is the more-important use case, and the message is nice to have there. the clients at the immediate shutdown. The code gets much simpler. In addition, it may be better that we similarly send SIGKILL in backend crash (FatalError) case, eliminate the use of SIGQUIT and remove quickdie() and other SIGQUIT handlers. My take is that the client message has some value, and we shouldn't just discard it to simplify the code slightly. Finishing the shutdown quickly has value, of course. The relative importance of those things should guide the choice of a timeout under method #2, but I don't see a rigorous way to draw the line. I feel five seconds is, if anything, too high. What about using deadlock_timeout? It constitutes a rough barometer on the system's tolerance for failure detection latency, and we already overload it by having it guide log_lock_waits. The default of 1s makes sense to me for this purpose, too. We can always add a separate immediate_shutdown_timeout if there's demand, but I don't expect such demand. (If we did add such a GUC, setting it to zero could be the way to request method 1. If some folks strongly desire method 1, that's probably the way to offer it.) Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.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] changeset generation v5-01 - Patches git tree
On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: Kevin Grittner kgri...@ymail.com wrote: Confirmed that all 17 patch files now apply cleanly, and that `make check-world` builds cleanly after each patch in turn. Just to be paranoid, I did one last build with all 17 patch files applied to 7dfd5cd21c0091e467b16b31a10e20bbedd0a836 using this line: make maintainer-clean ; ./configure --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python make -j4 world and it died with this: 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 -I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o pg_receivexlog.o pg_receivexlog.c -MMD -MP -MF .deps/pg_receivexlog.Po 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 -I. -I. -I../../../src/interfaces/libpq -I../../../src/bin/pg_dump -I../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o mainloop.o mainloop.c -MMD -MP -MF .deps/mainloop.Po 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 pg_receivellog.o receivelog.o streamutil.o -L../../../src/port -lpgport -L../../../src/common -lpgcommon -L../../../src/interfaces/libpq -lpq -L../../../src/port -L../../../src/common -L/usr/lib -Wl,--as-needed -Wl,-rpath,'/home/kgrittn/pg/master/Debug/lib',--enable-new-dtags -lpgport -lpgcommon -lxslt -lxml2 -lssl -lcrypto -lz -lreadline -lcrypt -ldl -lm -o pg_receivellog gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 make[3]: Leaving directory `/home/kgrittn/pg/master/src/bin/pg_basebackup' make[2]: *** [all-pg_basebackup-recurse] Error 2 make[2]: *** Waiting for unfinished jobs I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. It works with this patch-on-patch: diff --git a/src/bin/pg_basebackup/Makefile b/src/bin/pg_basebackup/Makefile index a41b73c..18d02f3 100644 --- a/src/bin/pg_basebackup/Makefile +++ b/src/bin/pg_basebackup/Makefile @@ -42,6 +42,7 @@ installdirs: uninstall: rm -f '$(DESTDIR)$(bindir)/pg_basebackup$(X)' rm -f '$(DESTDIR)$(bindir)/pg_receivexlog$(X)' + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' clean distclean maintainer-clean: - rm -f pg_basebackup$(X) pg_receivexlog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o + rm -f pg_basebackup$(X) pg_receivexlog$(X) pg_receivellog$(X) $(OBJS) pg_basebackup.o pg_receivexlog.o pg_receivellog.o It appears to be an omission from file 0015. Yes, both are missing. + rm -f '$(DESTDIR)$(bindir)/pg_receivellog$(X)' Oops. That part is not needed. Hm. Why not? I don't think either hunk has anything to do with that buildfailure though - can you reproduce the error without? Thanks, 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] updated emacs configuration
On 06/13/2013 09:27 PM, Peter Eisentraut wrote: I think the suggested emacs configuration snippets in src/tools/editors/emacs.samples no longer represent current best practices. I have come up with some newer things that I'd like to propose for review. First, I propose adding a .dir-locals.el file to the top-level directory with basic emacs settings. These get applied automatically. This especially covers the particular tab and indentation settings that PostgreSQL uses. With this, casual developers will not need to modify any of their emacs settings. (In the attachment, .dir-locals.el is called _dir-locals.el so that it doesn't get lost. To clarify, it goes into the same directory that contains configure.in.) With that, emacs.samples can be shrunk significantly. The only real reason to keep is that that c-offsets-alist and (more dubiously) sgml-basic-offset cannot be set from .dir-locals.el because they are not safe. I have also removed many of the redundant examples and settled on a hook-based solution. I think together this setup would be significantly simpler and more practical. The idea is a very good one in principle, but my short experiment with the provided .dir-locals.el didn't give me BSD style brace indentation. It works if we can do those unsafe things, but then we surely don't want to have a user prompted to accept the settings each time. If .dir-locals.el won't do what we need on its own, it seems to me hardly worth having. 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] updated emacs configuration
On Thu, Jun 13, 2013 at 09:27:07PM -0400, Peter Eisentraut wrote: I think the suggested emacs configuration snippets in src/tools/editors/emacs.samples no longer represent current best practices. I have come up with some newer things that I'd like to propose for review. ((c-mode . ((c-basic-offset . 4) (fill-column . 79) I don't know whether you'd consider it to fall within the scope of this update, but 78 is the fill-column setting that matches pgindent. (sgml-mode . ((fill-column . 79) (indent-tabs-mode . nil SGML fill-column practice has varied widely. I estimate 72 is more the norm, though I personally favor longer lines like we use in source code comments. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pgbench --throttle (submission 7 - with lag measurement)
An additional thought: Yet another thought, hopefully final on this subject. I think that the probability of a context switch is higher when calling PQfinish than in other parts of pgbench because it contains system calls (e.g. closing the network connection) where the kernel is likely to stop this process and activate another one. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] updated emacs configuration
On 06/23/2013 03:43 PM, Andrew Dunstan wrote: The idea is a very good one in principle, but my short experiment with the provided .dir-locals.el didn't give me BSD style brace indentation. It works if we can do those unsafe things, but then we surely don't want to have a user prompted to accept the settings each time. If .dir-locals.el won't do what we need on its own, it seems to me hardly worth having. However, it does work if you add this at the beginning of the c-mode list: (c-file-style . bsd) 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] updated emacs configuration
Andrew Dunstan and...@dunslane.net writes: The idea is a very good one in principle, but my short experiment with the provided .dir-locals.el didn't give me BSD style brace indentation. It works if we can do those unsafe things, but then we surely don't want to have a user prompted to accept the settings each time. If .dir-locals.el won't do what we need on its own, it seems to me hardly worth having. I'm un-thrilled with this as well, though for a slightly different reason: we have a policy that the PG sources should be tool agnostic, and in fact removed file-local emacs settings awhile back because of that. Even though I use emacs, I would much rather keep such settings in my personal library. (TBH, I keep enable-local-variables turned off, and would thus get no benefit from the proposed file anyhow.) Another thing I'm not too happy about is that the proposed patch summarily discards the information we had about how to work with older emacs versions. I'm not sure it will actually break anybody's setup, but that would depend on whether .dir-locals.el was added before or after the last round of rewriting of c-mode. 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] changeset generation v5-01 - Patches git tree
On 2013-06-23 10:32:05 -0700, Kevin Grittner wrote: Andres Freund and...@2ndquadrant.com wrote: Pushed and attached. The contrib/test_logical_decoding/sql/ddl.sql script is generating unexpected results. For both table_with_pkey and table_with_unique_not_null, updates of the primary key column are showing: old-pkey: id[int4]:0 ... instead of the expected value of 2 or -2. See attached. Hm. Any chance this was an incomplete rebuild? I seem to remember having seen that once because some header dependency wasn't recognized correctly after applying some patch. Otherwise, could you give me: * the version you aplied the patch on * os/compiler Because I can't reproduce it, despite some playing around... 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com writes: On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. What versions of gmake are you guys using? It wouldn't be the first time we've tripped over bugs in parallel make. See for instance http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1fc698cf14d17a3a8ad018cf9ec100198a339447 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] changeset generation v5-01 - Patches git tree
On 2013-06-23 16:48:41 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-06-23 08:27:32 -0700, Kevin Grittner wrote: gcc: error: pg_receivellog.o: No such file or directory make[3]: *** [pg_receivellog] Error 1 I have seen that once as well. It's really rather strange since pg_receivellog.o is a clear prerequisite for pg_receivellog. I couldn't reproduce it reliably though, even after doing some dozen rebuilds or so. What versions of gmake are you guys using? It wouldn't be the first time we've tripped over bugs in parallel make. See for instance http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1fc698cf14d17a3a8ad018cf9ec100198a339447 3.81 here. That was supposed to be the safe one, right? At least to the bugs seen/fixed recently. Kevin, any chance you still have more log than in the upthread mail available? 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] in-catalog Extension Scripts and Control parameters (templates?)
On Wed, Apr 3, 2013 at 8:29 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Hi, Dimitri Fontaine dimi...@2ndquadrant.fr writes: Documentation doesn't build, multiple errors. In addition to the reference pages, there should be a section in the main docs about these templates. I'm now working on that, setting up the documentation tool set. Fixed in the attached version 6 of the patch. just tried to build this one, but it doesn't apply cleanly anymore... specially the ColId_or_Sconst contruct in gram.y -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación Phone: +593 4 5107566 Cell: +593 987171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] in-catalog Extension Scripts and Control parameters (templates?)
Jaime Casanova ja...@2ndquadrant.com writes: just tried to build this one, but it doesn't apply cleanly anymore... specially the ColId_or_Sconst contruct in gram.y Will rebase tomorrow, thanks for the notice! -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
On Sun, Jun 23, 2013 at 07:44:26AM -0700, Kevin Grittner wrote: Dean Rasheed dean.a.rash...@gmail.com wrote: I'm still not happy that this patch is making FILTER a new reserved keyword, because I think it is a common enough English word (and an obscure enough SQL keyword) that people may well have used it for table names or aliases, and so their code will break. Well, if it is a useful capability with a standard syntax, I think we should go with the standard syntax even if it might break application code that uses, as unquoted identifiers, words reserved by the SQL standard. Of course, non-reserved is better than reserved if we can manage it. I'm not entirely sure that I agree with this. The SQL standard doesn't go adding keywords willy-nilly, or at least hasn't over a fairly long stretch of time. I can get precise numbers on this if needed. So far, only Tom and Greg have weighed in, Greg's response being here: http://www.postgresql.org/message-id/CAM-w4HOd3N_ozMygs==lm5+hu8yqkkayutgjinp6z2hazdr...@mail.gmail.com Playing around with the idea proposed there, it seems that it is possible to make FILTER (and also OVER) unreserved keywords, by splitting out the production of aggregate/window functions from normal functions in gram.y. Aggregate/window functions are not allowed in index_elem or func_table constructs, but they may appear in c_expr's. That resolves the shift/reduce conflicts that would otherwise occur from subsequent alias clauses, allowing FILTER and OVER to be unreserved. There is a drawback --- certain error messages become less specific, for example: SELECT * FROM rank() OVER(ORDER BY random()); is now a syntax error, rather than the more useful error saying that window functions aren't allowed in the FROM clause. That doesn't seem like such a common case though. What do you think? I think it is OK if that gets a syntax error. If that's the worst case I like this approach. I think reducing the usefulness of error messages is something we need to think extremely hard about before we do. Is there maybe a way to keep the error messages even if by some magic we manage to unreserve the words? This also helps keep down the size of the generated parse tables, doesn't it? Could well. I suspect we may need to rethink the whole way we do grammar at some point, but that's for a later discussion when I (or someone else) has something choate to say about it. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On Wed, Jun 19, 2013 at 9:50 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Wed, Jun 19, 2013 at 12:36 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jun 18, 2013 at 10:53 AM, Michael Paquier michael.paqu...@gmail.com wrote: An updated patch for the toast part is attached. On Tue, Jun 18, 2013 at 3:26 AM, Fujii Masao masao.fu...@gmail.com wrote: Here are the review comments of the removal_of_reltoastidxid patch. I've not completed the review yet, but I'd like to post the current comments before going to bed ;) *** a/src/backend/catalog/system_views.sql -pg_stat_get_blocks_fetched(X.oid) - -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_read, -pg_stat_get_blocks_hit(X.oid) AS tidx_blks_hit +pg_stat_get_blocks_fetched(X.indrelid) - +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_read, +pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_hit ISTM that X.indrelid indicates the TOAST table not the TOAST index. Shouldn't we use X.indexrelid instead of X.indrelid? Indeed good catch! We need in this case the statistics on the index and here I used the table OID. Btw, I also noticed that as multiple indexes may be involved for a given toast relation, it makes sense to actually calculate tidx_blks_read and tidx_blks_hit as the sum of all stats of the indexes. Yep. You seem to need to change X.indexrelid to X.indrelid in GROUP clause. Otherwise, you may get two rows of the same table from pg_statio_all_tables. I changed it a little bit in a different way in my latest patch by adding a sum on all the indexes when getting tidx_blks stats. doc/src/sgml/diskusage.sgml There will be one index on the acronymTOAST/ table, if present. + table (see xref linkend=storage-toast). There will be one valid index + on the acronymTOAST/ table, if present. There also might be indexes When I used gdb and tracked the code path of concurrent reindex patch, I found it's possible that more than one *valid* toast indexes appear. Those multiple valid toast indexes are viewable, for example, from pg_indexes. I'm not sure whether this is the bug of concurrent reindex patch. But if it's not, you seem to need to change the above description again. Not sure about that. The latest code is made such as only one valid index is present on the toast relation at the same time. *** a/src/bin/pg_dump/pg_dump.c + SELECT c.reltoastrelid, t.indexrelid FROM pg_catalog.pg_class c LEFT JOIN - pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) - WHERE c.oid = '%u'::pg_catalog.oid;, + pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) + WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid + LIMIT 1, Is there the case where TOAST table has more than one *valid* indexes? I just rechecked the patch and is answer is no. The concurrent index is set as valid inside the same transaction as swap. So only the backend performing the swap will be able to see two valid toast indexes at the same time. According to my quick gdb testing, this seems not to be true Well, I have to disagree. I am not able to reproduce it. Which version did you use? Here is what I get with the latest version of REINDEX CONCURRENTLY patch... I checked with the following process: Sorry. This is my mistake. 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] Support for REINDEX CONCURRENTLY
On Sun, Jun 23, 2013 at 3:34 PM, Michael Paquier michael.paqu...@gmail.com wrote: OK. Please find an updated patch for the toast part. On Sat, Jun 22, 2013 at 10:48 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-06-22 22:45:26 +0900, Michael Paquier wrote: On Sat, Jun 22, 2013 at 10:34 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-06-22 12:50:52 +0900, Michael Paquier wrote: By looking at the comments of RelationGetIndexList:relcache.c, actually the method of the patch is correct because in the event of a shared cache invalidation, rd_indexvalid is set to 0 when the index list is reset, so the index list would get recomputed even in the case of shared mem invalidation. The problem I see is something else. Consider code like the following: RelationFetchIndexListIfInvalid(toastrel); foreach(lc, toastrel-rd_indexlist) toastidxs[i++] = index_open(lfirst_oid(lc), RowExclusiveLock); index_open calls relation_open calls LockRelationOid which does: if (res != LOCKACQUIRE_ALREADY_HELD) AcceptInvalidationMessages(); So, what might happen is that you open the first index, which accepts an invalidation message which in turn might delete the indexlist. Which means we would likely read invalid memory if there are two indexes. And I imagine that you have the same problem even with RelationGetIndexList, not only RelationGetIndexListIfInvalid, because this would appear as long as you try to open more than 1 index with an index list. No. RelationGetIndexList() returns a copy of the list for exactly that reason. The danger is not to see an outdated list - we should be protected by locks against that - but looking at uninitialized or reused memory. OK, so I removed RelationGetIndexListIfInvalid (such things could be an optimization for another patch) and replaced it by calls to RelationGetIndexList to get a copy of rd_indexlist in a local list variable, list free'd when it is not necessary anymore. It looks that there is nothing left for this patch, no? Compile error ;) gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -g -I../../../src/include -c -o index.o index.c index.c: In function 'index_constraint_create': index.c:1257: error: too many arguments to function 'index_update_stats' index.c: At top level: index.c:1785: error: conflicting types for 'index_update_stats' index.c:106: error: previous declaration of 'index_update_stats' was here index.c: In function 'index_update_stats': index.c:1881: error: 'FormData_pg_class' has no member named 'reltoastidxid' index.c:1883: error: 'FormData_pg_class' has no member named 'reltoastidxid' make[3]: *** [index.o] Error 1 make[2]: *** [catalog-recursive] Error 2 make[1]: *** [install-backend-recurse] Error 2 make: *** [install-src-recurse] Error 2 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: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
David Fetter da...@fetter.org writes: On Sun, Jun 23, 2013 at 07:44:26AM -0700, Kevin Grittner wrote: I think it is OK if that gets a syntax error. If that's the worst case I like this approach. I think reducing the usefulness of error messages is something we need to think extremely hard about before we do. Is there maybe a way to keep the error messages even if by some magic we manage to unreserve the words? Of the alternatives discussed so far, I don't really like anything better than adding another special case to base_yylex(). Robert opined in the other thread about RESPECT NULLS that the potential failure cases with that approach are harder to reason about, which is true, but that doesn't mean that we should accept failures we know of because there might be failures we don't know of. One thing that struck me while thinking about this is that it seems like we've been going about it wrong in base_yylex() in any case. For example, because we fold WITH followed by TIME into a special token WITH_TIME, we will fail on a statement beginning WITH time AS ... even though time is a legal ColId. But suppose that instead of merging the two tokens into one, we just changed the first token into something special; that is, base_yylex would return a special token WITH_FOLLOWED_BY_TIME and then TIME. We could then fix the above problem by allowing either WITH or WITH_FOLLOWED_BY_TIME as the leading keyword of a statement; and similarly for the few other places where WITH can be followed by an arbitrary identifier. Going on the same principle, we could probably let FILTER be an unreserved keyword while FILTER_FOLLOWED_BY_PAREN could be a type_func_name_keyword. (I've not tried this though.) This idea doesn't help much for OVER because one of the alternatives for over_clause is OVER ColId, and I doubt we want to have base_yylex know all the alternatives for ColId. I also had no great success with the NULLS FIRST/LAST case: AFAICT the substitute token for NULLS still has to be fully reserved, meaning that something like select nulls last still doesn't work without quoting. We could maybe fix that with enough denormalization of the index_elem productions, but it'd be ugly. Could well. I suspect we may need to rethink the whole way we do grammar at some point, but that's for a later discussion when I (or someone else) has something choate to say about it. It'd sure be interesting to know what the SQL committee's target parsing algorithm is. I find it hard to believe they're uninformed enough to not know that these random syntaxes they keep inventing are hard to deal with in LALR(1). Or maybe they really don't give a damn about breaking applications every time they invent a new reserved word? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Naming of ORDINALITY column (was: Re: Review: UNNEST (and other functions) WITH ORDINALITY)
OK, let's try to cover all the bases here in one go. First, the spec definition of WITH ORDINALITY simply says that the column name in the result is not equivalent to any other identifier in the same table primary (including the correlation name). It is clear that the intention of the spec is that any non-positional reference to this column (which is defined as being positionally last) requires an alias at some point, whether directly attached to the table primary or at an outer level. Second, all the documentation I've looked at for other databases that implement this feature (such as DB2, Teradata, etc.) takes it for granted that the user must always supply an alias, even though the syntax does not actually require one. None of the ones I've seen suggest that the ordinality column has a useful or consistent name if no alias is supplied. So, while clearly there's nothing stopping us from going beyond the spec and using a column name that people can refer to without needing an alias, it would be a significant divergence from common practice in other dbs. (iirc, it was my suggestion to David to use ?column? in the first place for this reason.) So as I see it the options are: 1. Stick with ?column? as a warning flag that you're not supposed to be using this without aliasing it to something. 2. Use some other fixed name like ordinality simply to allow people to do things like select ... from unnest(x) with ordinality; without having to bother to provide an alias, simply as a convenience, without regard for consistency with others. (This will result in a duplicate name if x is of a composite type containing a column called ordinality, so the caller will have to provide an alias in that specific case or get an ambiguous reference error. Similarly if using some other SRF which defines its own return column names.) 3. Generate an actually unique name (probably pointless) 4. Something else I haven't thought of. My vote remains with option 1 here; I don't think users should be encouraged to assume that the ordinality column will have a known name. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Naming of ORDINALITY column
On 06/23/2013 08:00 PM, Andrew Gierth wrote: OK, let's try to cover all the bases here in one go. 1. Stick with ?column? as a warning flag that you're not supposed to be using this without aliasing it to something. How do I actually supply an alias which covers both columns? What does that look like, syntactically? -- 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] ALTER TABLE ... ALTER CONSTRAINT
At 2013-06-08 21:45:24 +0100, si...@2ndquadrant.com wrote: ALTER TABLE foo ALTER CONSTRAINT fktable_fk_fkey DEFERRED INITIALLY IMMEDIATE; I read the patch (looks good), applied it on HEAD (fine), ran make check (fine), and played with it in a test database. It looks great, and from previous responses it's something a lot of people have wished for. I'm marking this ready for committer. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [9.4 CF 1] Commitfest Week One
Current Stats: Needs Review: 61, Waiting on Author: 19, Ready for Committer: 11, Committed: 11, Returned with Feedback: 3, Rejected: 1 34 Patches are awaiting review, and do not have a reviewer assigned. In the last week, 3 patches have moved to Ready for Committer and 3 patches have been committed. 10 patches have been reviewed and are Waiting on Author, probably preparatory to moving to Returned with Feedback. At the current review/commit rate, this commitfest will not conclude until almost the end of July. Time to pick up the pace, people! For the first time, we have removed reviewers who did not post a review in 5 days. In the upcoming week, we will be moving patches which are Waiting on Author to Returned with Feedback without further discussion, if they have seen no activity for 5 or more days. This means that you can expect Review assignments on patches to be reasonably current. Thank you so SO much to Mike Blackwell for help with this! 26 people who submitted patches have not claimed any patch for review. More of this in an email of shame later. Among the patches needing review, several will need a lot of attention from committers, and should maybe be reviewed as soon as possible. Those include: * Extension Templates * minmax indexes * Auto-tuning checkpoint_segments And then there's the assortment of WAL/Hint Bit patches, which will no doubt have some toxic interactions: preserving forensic information, performance improvement by reducing WAL, XLogInsert scaling, preserving forensic information when we freeze, remove PD_ALL_VISIBLE. These shoud be addressed early so that we can work out the conflicts between them. So, some progress, but more is needed. --Josh Berkus CommitFest Manager -- 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
[HACKERS] [9.4 CF 1] The Commitfest Slacker List
Folks, For 9.2, we adopted it as policy that anyone submitting a patch to a commitfest is expected to review at least one patch submitted by someone else. And that failure to do so would affect the attention your patches received in the future. For that reason, I'm publishing the list below of people who submitted patches and have not yet claimed any patch in the commitfest to review. For those of you who are contributing patches for your company, please let your boss know that reviewing is part of contributing, and that if you don't do the one you may not be able to do the other. The two lists below, idle submitters and committers, constitutes 26 people. This *outnumbers* the list of contributors who are busy reviewing patches -- some of them four or more patches. If each of these people took just *one* patch to review, it would almost entirely clear the list of patches which do not have a reviewer. If these folks continue not to do reviews, this commitfest will drag on for at least 2 weeks past its closure date. Andrew Geirth Nick White Peter Eisentrout Alexander Korotkov Etsuro Fujita Hari Babu Jameison Martin Jon Nelson Oleg Bartunov Chris Farmiloe Samrat Revagade Alexander Lakhin Mark Kirkwood Liming Hu Maciej Gajewski Josh Kuperschmidt Mark Wong Gurjeet Singh Robins Tharakan Tatsuo Ishii Karl O Pinc Additionally, the following committers are not listed as reviewers on any patch. Note that I have no way to search which ones might be *committers* on a patch, so these folks are not necessarily slackers (although with Bruce, we know for sure): Bruce Momjian (momjian) Michael Meskes (meskes) Teodor Sigaev (teodor) Andrew Dunstan (adunstan) Magnus Hagander (mha) Itagaki Takahiro (itagaki) -- 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] [PERFORM] In progress INSERT wrecks plans on table
(Cc: to pgsql-performance dropped, pgsql-hackers added.) At 2013-05-06 09:14:01 +0100, si...@2ndquadrant.com wrote: New version of patch attached which fixes a few bugs. I read the patch, but only skimmed the earlier discussion about it. In isolation, I can say that the patch applies cleanly and looks sensible for what it does (i.e., cache pgprocno to speed up repeated calls to TransactionIdIsInProgress(somexid)). In that sense, it's ready for committer, but I don't know if there's a better/more complete/etc. way to address the original problem. -- Abhijit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
