Re: [HACKERS] Review: query result history in psql
It's better to post a review as a reply to the message which contains the patch. Sorry about that, I did not have the email in my inbox and couldn't figure out how to use the old message ID to send a reply. Here is the thread: http://www.postgresql.org/message-id/flat/caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com#caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com The 'EscapeForCopy' was meant to mean 'Escape string in a format require by the COPY TEXT format', so 'copy' in the name refers to the escaping format, not the action performed by the function. I see, that makes sense now. Keep it as you see fit, it's not a big deal in my opinion. Some mathematical toolkits, like Matlab or Mathematica, automatically set a variable called 'ans' (short for answer) containing the result of the last operation. I was trying to emulate exactly this behaviour. I've actually been using Matlab lately, which must be why the name made sense to me intuitively. I don't know if this is the best name, however. It kind of assumes that our users use Matlab/Octave/Mathematica. Maybe 'qhist' or 'hist' or something? The history is not erased. The history is always stored in the client's memory. Ah, I did not pick up on that. Thank you for explaining it! That's actually a very neat way of doing it. Sorry I did not realize that at first. I was considering such a behaviour. But since the feature is turned off by default, I decided that whoever is using it, is aware of cost. Instead of truncating the history automatically (which could lead to a nasty surprise), I decided to equip the user with \ansclean , a command erasing the history. I believe that it is better to let the user decide when history should be erased, instead of doing it automatically. I think you are correct. However, if we turn on the feature by default (at some point in the future) the discussion should probably be re-visited. This is my first submitted patch, so I can't really comment on the process. But if you could add the author's email to CC, the message would be much easier to spot. I replied after two days only because I missed the message in the flood of other pgsql-hacker messages. I think I need to scan the list more carefully... My fault, I definitely should have CC'd you. As for the patch, I made a new version of the latest one you provided in the original thread. Let me know if anything breaks, but it compiles fine on my box. Thanks for the feedback! Ian query-history-review1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: query result history in psql
Hello after patching I god segfault Program terminated with signal 11, Segmentation fault. #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 98 for (p = prompt_string; Missing separate debuginfos, use: debuginfo-install glibc-2.13-2.i686 ncurses-libs-5.7-9.20100703.fc14.i686 readline-6.1-2.fc14.i386 (gdb) bt #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 #1 0x0805786a in MainLoop (source=0xc45440) at mainloop.c:134 #2 0x0805a68d in main (argc=2, argv=0xbfcf2894) at startup.c:336 Regards Pavel Stehule 2013/6/28 ian link i...@ilink.io: It's better to post a review as a reply to the message which contains the patch. Sorry about that, I did not have the email in my inbox and couldn't figure out how to use the old message ID to send a reply. Here is the thread: http://www.postgresql.org/message-id/flat/caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com#caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com The 'EscapeForCopy' was meant to mean 'Escape string in a format require by the COPY TEXT format', so 'copy' in the name refers to the escaping format, not the action performed by the function. I see, that makes sense now. Keep it as you see fit, it's not a big deal in my opinion. Some mathematical toolkits, like Matlab or Mathematica, automatically set a variable called 'ans' (short for answer) containing the result of the last operation. I was trying to emulate exactly this behaviour. I've actually been using Matlab lately, which must be why the name made sense to me intuitively. I don't know if this is the best name, however. It kind of assumes that our users use Matlab/Octave/Mathematica. Maybe 'qhist' or 'hist' or something? The history is not erased. The history is always stored in the client's memory. Ah, I did not pick up on that. Thank you for explaining it! That's actually a very neat way of doing it. Sorry I did not realize that at first. I was considering such a behaviour. But since the feature is turned off by default, I decided that whoever is using it, is aware of cost. Instead of truncating the history automatically (which could lead to a nasty surprise), I decided to equip the user with \ansclean , a command erasing the history. I believe that it is better to let the user decide when history should be erased, instead of doing it automatically. I think you are correct. However, if we turn on the feature by default (at some point in the future) the discussion should probably be re-visited. This is my first submitted patch, so I can't really comment on the process. But if you could add the author's email to CC, the message would be much easier to spot. I replied after two days only because I missed the message in the flood of other pgsql-hacker messages. I think I need to scan the list more carefully... My fault, I definitely should have CC'd you. As for the patch, I made a new version of the latest one you provided in the original thread. Let me know if anything breaks, but it compiles fine on my box. Thanks for the feedback! Ian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: query result history in psql
I just applied the patch to a clean branch from the latest master. I couldn't get a segfault from using the new feature. Could you provide a little more info to reproduce the segfault? Thanks On Thu, Jun 27, 2013 at 11:28 PM, Pavel Stehule pavel.steh...@gmail.comwrote: Hello after patching I god segfault Program terminated with signal 11, Segmentation fault. #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 98 for (p = prompt_string; Missing separate debuginfos, use: debuginfo-install glibc-2.13-2.i686 ncurses-libs-5.7-9.20100703.fc14.i686 readline-6.1-2.fc14.i386 (gdb) bt #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 #1 0x0805786a in MainLoop (source=0xc45440) at mainloop.c:134 #2 0x0805a68d in main (argc=2, argv=0xbfcf2894) at startup.c:336 Regards Pavel Stehule 2013/6/28 ian link i...@ilink.io: It's better to post a review as a reply to the message which contains the patch. Sorry about that, I did not have the email in my inbox and couldn't figure out how to use the old message ID to send a reply. Here is the thread: http://www.postgresql.org/message-id/flat/caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com#caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com The 'EscapeForCopy' was meant to mean 'Escape string in a format require by the COPY TEXT format', so 'copy' in the name refers to the escaping format, not the action performed by the function. I see, that makes sense now. Keep it as you see fit, it's not a big deal in my opinion. Some mathematical toolkits, like Matlab or Mathematica, automatically set a variable called 'ans' (short for answer) containing the result of the last operation. I was trying to emulate exactly this behaviour. I've actually been using Matlab lately, which must be why the name made sense to me intuitively. I don't know if this is the best name, however. It kind of assumes that our users use Matlab/Octave/Mathematica. Maybe 'qhist' or 'hist' or something? The history is not erased. The history is always stored in the client's memory. Ah, I did not pick up on that. Thank you for explaining it! That's actually a very neat way of doing it. Sorry I did not realize that at first. I was considering such a behaviour. But since the feature is turned off by default, I decided that whoever is using it, is aware of cost. Instead of truncating the history automatically (which could lead to a nasty surprise), I decided to equip the user with \ansclean , a command erasing the history. I believe that it is better to let the user decide when history should be erased, instead of doing it automatically. I think you are correct. However, if we turn on the feature by default (at some point in the future) the discussion should probably be re-visited. This is my first submitted patch, so I can't really comment on the process. But if you could add the author's email to CC, the message would be much easier to spot. I replied after two days only because I missed the message in the flood of other pgsql-hacker messages. I think I need to scan the list more carefully... My fault, I definitely should have CC'd you. As for the patch, I made a new version of the latest one you provided in the original thread. Let me know if anything breaks, but it compiles fine on my box. Thanks for the feedback! Ian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: query result history in psql
Hello It's look like my bug - wrong compilation I am sorry Pavel 2013/6/28 ian link i...@ilink.io: I just applied the patch to a clean branch from the latest master. I couldn't get a segfault from using the new feature. Could you provide a little more info to reproduce the segfault? Thanks On Thu, Jun 27, 2013 at 11:28 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello after patching I god segfault Program terminated with signal 11, Segmentation fault. #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 98 for (p = prompt_string; Missing separate debuginfos, use: debuginfo-install glibc-2.13-2.i686 ncurses-libs-5.7-9.20100703.fc14.i686 readline-6.1-2.fc14.i386 (gdb) bt #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 #1 0x0805786a in MainLoop (source=0xc45440) at mainloop.c:134 #2 0x0805a68d in main (argc=2, argv=0xbfcf2894) at startup.c:336 Regards Pavel Stehule 2013/6/28 ian link i...@ilink.io: It's better to post a review as a reply to the message which contains the patch. Sorry about that, I did not have the email in my inbox and couldn't figure out how to use the old message ID to send a reply. Here is the thread: http://www.postgresql.org/message-id/flat/caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com#caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com The 'EscapeForCopy' was meant to mean 'Escape string in a format require by the COPY TEXT format', so 'copy' in the name refers to the escaping format, not the action performed by the function. I see, that makes sense now. Keep it as you see fit, it's not a big deal in my opinion. Some mathematical toolkits, like Matlab or Mathematica, automatically set a variable called 'ans' (short for answer) containing the result of the last operation. I was trying to emulate exactly this behaviour. I've actually been using Matlab lately, which must be why the name made sense to me intuitively. I don't know if this is the best name, however. It kind of assumes that our users use Matlab/Octave/Mathematica. Maybe 'qhist' or 'hist' or something? The history is not erased. The history is always stored in the client's memory. Ah, I did not pick up on that. Thank you for explaining it! That's actually a very neat way of doing it. Sorry I did not realize that at first. I was considering such a behaviour. But since the feature is turned off by default, I decided that whoever is using it, is aware of cost. Instead of truncating the history automatically (which could lead to a nasty surprise), I decided to equip the user with \ansclean , a command erasing the history. I believe that it is better to let the user decide when history should be erased, instead of doing it automatically. I think you are correct. However, if we turn on the feature by default (at some point in the future) the discussion should probably be re-visited. This is my first submitted patch, so I can't really comment on the process. But if you could add the author's email to CC, the message would be much easier to spot. I replied after two days only because I missed the message in the flood of other pgsql-hacker messages. I think I need to scan the list more carefully... My fault, I definitely should have CC'd you. As for the patch, I made a new version of the latest one you provided in the original thread. Let me know if anything breaks, but it compiles fine on my box. Thanks for the feedback! Ian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: query result history in psql
No worries! :) On Fri, Jun 28, 2013 at 12:20 AM, Pavel Stehule pavel.steh...@gmail.comwrote: Hello It's look like my bug - wrong compilation I am sorry Pavel 2013/6/28 ian link i...@ilink.io: I just applied the patch to a clean branch from the latest master. I couldn't get a segfault from using the new feature. Could you provide a little more info to reproduce the segfault? Thanks On Thu, Jun 27, 2013 at 11:28 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello after patching I god segfault Program terminated with signal 11, Segmentation fault. #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 98 for (p = prompt_string; Missing separate debuginfos, use: debuginfo-install glibc-2.13-2.i686 ncurses-libs-5.7-9.20100703.fc14.i686 readline-6.1-2.fc14.i386 (gdb) bt #0 0x0805aab4 in get_prompt (status=PROMPT_READY) at prompt.c:98 #1 0x0805786a in MainLoop (source=0xc45440) at mainloop.c:134 #2 0x0805a68d in main (argc=2, argv=0xbfcf2894) at startup.c:336 Regards Pavel Stehule 2013/6/28 ian link i...@ilink.io: It's better to post a review as a reply to the message which contains the patch. Sorry about that, I did not have the email in my inbox and couldn't figure out how to use the old message ID to send a reply. Here is the thread: http://www.postgresql.org/message-id/flat/caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com#caecsyxjri++t3pevdyzawh2ygx7kg9zrhx8kawtp1fxv3h0...@mail.gmail.com The 'EscapeForCopy' was meant to mean 'Escape string in a format require by the COPY TEXT format', so 'copy' in the name refers to the escaping format, not the action performed by the function. I see, that makes sense now. Keep it as you see fit, it's not a big deal in my opinion. Some mathematical toolkits, like Matlab or Mathematica, automatically set a variable called 'ans' (short for answer) containing the result of the last operation. I was trying to emulate exactly this behaviour. I've actually been using Matlab lately, which must be why the name made sense to me intuitively. I don't know if this is the best name, however. It kind of assumes that our users use Matlab/Octave/Mathematica. Maybe 'qhist' or 'hist' or something? The history is not erased. The history is always stored in the client's memory. Ah, I did not pick up on that. Thank you for explaining it! That's actually a very neat way of doing it. Sorry I did not realize that at first. I was considering such a behaviour. But since the feature is turned off by default, I decided that whoever is using it, is aware of cost. Instead of truncating the history automatically (which could lead to a nasty surprise), I decided to equip the user with \ansclean , a command erasing the history. I believe that it is better to let the user decide when history should be erased, instead of doing it automatically. I think you are correct. However, if we turn on the feature by default (at some point in the future) the discussion should probably be re-visited. This is my first submitted patch, so I can't really comment on the process. But if you could add the author's email to CC, the message would be much easier to spot. I replied after two days only because I missed the message in the flood of other pgsql-hacker messages. I think I need to scan the list more carefully... My fault, I definitely should have CC'd you. As for the patch, I made a new version of the latest one you provided in the original thread. Let me know if anything breaks, but it compiles fine on my box. Thanks for the feedback! Ian -- Sent 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-27 18:18:50 -0400, Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: I'm looking at the combined patches 0003-0005, which are essentially all about adding a function to obtain relation OID from (tablespace, filenode). It takes care to look through the relation mapper, and uses a new syscache underneath for performance. One question about this patch, originally, was about the usage of that relfilenode syscache. It is questionable because it would be the only syscache to apply on top of a non-unique index. ... which, I assume, is on top of a pg_class index that doesn't exist today. Exactly what is the argument that says performance of this function is sufficiently critical to justify adding both the maintenance overhead of a new pg_class index, *and* a broken-by-design syscache? Ok, so this requires some context. When we do the changeset extraction we build a mvcc snapshot that for every heap wal record is consistent with one made at the time the record has been inserted. Then, when we've built that snapshot, we can use it to turn heap wal records into the representation the user wants: For that we first need to know which table a change comes from, since otherwise we obviously cannot interpret the HeapTuple that's essentially contained in the wal record without it. Since we have a correct mvcc snapshot we can query pg_class for (tablespace, relfilenode) to get back the relation. When we know the relation, the user (i.e. the output pluggin) can use normal backend code to transform the HeapTuple into the target representation, e.g. SQL, since we can build a TupleDesc. Since the syscaches are synchronized with the built snapshot normal output functions can be used. What that means is that for every heap record in the target database in the WAL we need to query pg_class to turn the relfilenode into a pg_class.oid. So, we can easily replace syscache.c with some custom caching code, but I don't think it's realistic to get rid of that index. Otherwise we need to cache the entire pg_class in memory which doesn't sound enticing. 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] checking variadic any argument in parser - should be array
Hi Pavel, On Fri, Jun 28, 2013 at 2:53 AM, Pavel Stehule pavel.steh...@gmail.comwrote: Hello 2013/6/27 Jeevan Chalke jeevan.cha...@enterprisedb.com: Hi Pavel, I had a look over your new patch and it looks good to me. My review comments on patch: 1. It cleanly applies with patch -p1 command. 2. make/make install/make check were smooth. 3. My own testing didn't find any issue. 4. I had a code walk-through and I am little bit worried or confused on following changes: if (PG_ARGISNULL(argidx)) return NULL; ! /* ! * Non-null argument had better be an array. The parser doesn't ! * enforce this for VARIADIC ANY functions (maybe it should?), so that ! * check uses ereport not just elog. ! */ ! arr_typid = get_fn_expr_argtype(fcinfo-flinfo, argidx); ! if (!OidIsValid(arr_typid)) ! elog(ERROR, could not determine data type of concat() input); ! ! if (!OidIsValid(get_element_type(arr_typid))) ! ereport(ERROR, ! (errcode(ERRCODE_DATATYPE_MISMATCH), ! errmsg(VARIADIC argument must be an array))); - /* OK, safe to fetch the array value */ arr = PG_GETARG_ARRAYTYPE_P(argidx); /* --- 3820,3828 if (PG_ARGISNULL(argidx)) return NULL; ! /* Non-null argument had better be an array */ ! Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo-flinfo, argidx; arr = PG_GETARG_ARRAYTYPE_P(argidx); We have moved checking of array type in parser (ParseFuncOrColumn()) which basically verifies that argument type is indeed an array. Which exactly same as that of second if statement in earlier code, which you replaced by an Assert. However, what about first if statement ? Is it NO more required now? What if get_fn_expr_argtype() returns InvalidOid, don't you think we need to throw an error saying could not determine data type of concat() input? yes, If I understand well to question, a main differences is in stage of checking. When I do a check in parser stage, then I can expect so actual_arg_types array holds a valid values. That's fine. Well, I tried hard to trigger that code, but didn't able to get any test which fails with that error in earlier version and with your version. And thus I believe it is a dead code, which you removed ? Is it so ? It is removed in this version :), and it is not a bug, so there is not reason for patching previous versions. Probably there should be a Assert instead of error. This code is relatively mature - so I don't expect a issue from SQL level now. On second hand, this functions can be called via DirectFunctionCall API from custom C server side routines, and there a developer can does a bug simply if doesn't fill necessary structs well. So, there can be Asserts still. Moreover, if in any case get_fn_expr_argtype() returns an InvalidOid, we will hit an Assert rather than an error, is this what you expect ? in this moment yes, small change can helps with searching of source of possible issues. so instead on line Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo-flinfo, argidx; use two lines Assert(OidIsValid(get_fn_expr_argtype(fcinfo-flinfo, argidx))); Assert(OidIsValid(get_element_type(get_fn_expr_argtype(fcinfo-flinfo, argidx; what you think? Well, I am still not fully understand or convinced about first Assert, error will be good enough like what we have now. Anyway, converting it over two lines eases the debugging efforts. But please take output of get_fn_expr_argtype(fcinfo-flinfo, argidx) into separate variable so that we will avoid calling same function twice. I think some short comment for these Asserts will be good. At-least for second one as it is already done by parser and non-arrays are not at expected at this point. 5. This patch has user visibility, i.e. now we are throwing an error when user only says VARIADIC NULL like: select concat(variadic NULL) is NULL; Previously it was working but now we are throwing an error. Well we are now more stricter than earlier with using VARIADIC + ANY, so I have no issue as such. But I guess we need to document this user visibility change. I don't know exactly where though. I searched for VARIADIC and all related documentation says it needs an array, so nothing harmful as such, so you can ignore this review comment but I thought it worth mentioning it. yes, it is point for possible issues in RELEASE NOTES, I am thinking ??? Well, writer of release notes should be aware of this. And I hope he will be. So no issue. Thanks Regards Pavel Thanks On Thu, Jun 27, 2013 at 12:35 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello remastered version Regards
Re: [HACKERS] Support for REINDEX CONCURRENTLY
On 2013-06-28 16:30:16 +0900, Michael Paquier wrote: When I ran VACUUM FULL, I got the following error. ERROR: attempt to apply a mapping to unmapped relation 16404 STATEMENT: vacuum full; This can be reproduced when doing a vacuum full on pg_proc, pg_shdescription or pg_db_role_setting for example, or relations that have no relfilenode (mapped catalogs), and a toast relation. I still have no idea what is happening here but I am looking at it. As this patch removes reltoastidxid, could that removal have effect on the relation mapping of mapped catalogs? Does someone have an idea? I'd guess you broke swap_toast_by_content case in cluster.c? We cannot change the oid of a mapped relation (including indexes) since pg_class in other databases wouldn't get the news. 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] Support for REINDEX CONCURRENTLY
On Fri, Jun 28, 2013 at 4:52 PM, Andres Freund and...@2ndquadrant.com wrote: On 2013-06-28 16:30:16 +0900, Michael Paquier wrote: When I ran VACUUM FULL, I got the following error. ERROR: attempt to apply a mapping to unmapped relation 16404 STATEMENT: vacuum full; This can be reproduced when doing a vacuum full on pg_proc, pg_shdescription or pg_db_role_setting for example, or relations that have no relfilenode (mapped catalogs), and a toast relation. I still have no idea what is happening here but I am looking at it. As this patch removes reltoastidxid, could that removal have effect on the relation mapping of mapped catalogs? Does someone have an idea? I'd guess you broke swap_toast_by_content case in cluster.c? We cannot change the oid of a mapped relation (including indexes) since pg_class in other databases wouldn't get the news. Yeah, I thought that something was broken in swap_relation_files, but after comparing the code path taken by my code and master, and the different function calls I can't find any difference. I'm assuming that there is something wrong in tuptoaster.c in the fact of opening toast index relations in order to get the Oids to be swapped... But so far nothing I am just not sure... -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: query result history in psql
Thanks for checking the patch! So what's left to fix? * Moving the escaping-related functions to separate module, * applying your corrections. Did I missed anything? I'll submit corrected patch after the weekend. M
Re: [HACKERS] FILTER for aggregates [was Re: Department of Redundancy Department: makeNode(FuncCall) division]
On 27 June 2013 15:05, Tom Lane t...@sss.pgh.pa.us wrote: Andrew Gierth and...@tao11.riddles.org.uk writes: Tom Lane said: Agreed, separating out the function-call-with-trailing-declaration syntaxes so they aren't considered in FROM and index_elem seems like the best compromise. If we do that for window function OVER clauses as well, can we make OVER less reserved? Yes. At least, I tried it with both OVER and FILTER unreserved and there were no grammar conflicts (and I didn't have to do anything fancy to avoid them), and it passed regression with the exception of the changed error message for window functions in the from-clause. So is this the final decision on how to proceed? It seems good to me, and I can work with David to get it done. Yeah, please submit a separate patch that just refactors the existing grammar as above; that'll simplify reviewing. In that case, I'll re-review the latest FILTER patch over the weekend on the understanding that the reserved/unreserved keyword issue will be resolved in separate patch. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Documentation/help for materialized and recursive views
On Fri, Jun 28, 2013 at 5:54 AM, Kevin Grittner kgri...@ymail.com wrote: Robert Haas robertmh...@gmail.com wrote: Magnus Hagander mag...@hagander.net wrote: The functionality of materialized views will (over time) totally swamp that of normal views, so mixing all the corresponding documentation with the documentation for normal views probably doesn’t make things easier for people that are only interested in normal views. That's a better point I think. That said, it would be very useful if it actually showed up in \h CREATE VIEW in psql - I wonder if we should just add the syntax to that page, and then link said future information on a separate page somehow? IMHO, it's better to keep them separate; they are very different beasts. +1 Although I wonder whether we shouldn't cross-link those pages They are already crosslinked under see also. But that doesn't really help the guy doing \h CREATE VIEW in psql, which was the case where it was brought to my attention. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Spin Lock sleep resolution
On 27.06.2013 17:30, Robert Haas wrote: On Wed, Jun 26, 2013 at 5:49 PM, Jeff Janesjeff.ja...@gmail.com wrote: If we think the patch has a risk of introducing subtle errors, then it probably can't be justified based on the small performance gains you saw. But if we think it has little risk, then I think it is justified simply based on cleaner code, and less confusion for people who are tracing a problematic process. If we want it to do a random escalation, it should probably look like a random escalation to the interested observer. I think it has little risk. If it turns out to be worse for performance, we can always revert it, but I expect it'll be better or a wash, and the results so far seem to bear that out. Another interesting question is whether we should fool with the actual values for minimum and maximum delays, but that's a separate and much harder question, so I think we should just do this for now and call it good. My thoughts exactly. I wanted to see if David Gould would like to do some testing with it, but there's realy no need to hold off committing for that, I'm not expecting any surprises there. Committed. Jeff, in the patch you changed the datatype of cur_delay variable from int to long. AFAICS that makes no difference, so I kept it as int. Let me know if there was a reason for that change. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Group Commits Vs WAL Writes
There is a spot on the disk to which the current WAL is destined to go. That spot on the disk is not going to be under the write-head for (say) another 6 milliseconds. Without commit_delay, I try to commit my record, but find that someone else is already on the lock (and on the fsync as well). I have to wait for 6 milliseconds before that person gets their commit done and releases the lock, then I can start mine, and have to wait another 8 milliseconds (7500 rpm disk) for the spot to come around again, for a total of 14 milliseconds of latency. With commit_delay, I get my record in under the nose of the person who is already doing the delay, and they wake up and flush it for me in time to make the 6 millisecond cutoff. Total 6 milliseconds latency for me. Right. The example makes it very clear. Thanks for such a detailed explanation. One thing I tried a while ago (before the recent group-commit changes were made) was to record in shared memory when the last fsync finished, and then the next time someone needed to fsync, they would sleep until just before the write spot was predicted to be under the write head again (previous_finish + rotation_time - safety_margin, where rotation_time - safety_margin were represented by a single guc). It worked pretty well on the system in which I wrote it, but seemed too brittle to be a general solution. Can we look at researching a general formula for the above? It looks a bit touchy, but could be made to work. Another option is to add a probabilistic factor in the formula. Idk, it just seems to be a hunch I have. Regards, Atri -- Regards, Atri l'apprenant -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Group Commits Vs WAL Writes
Yep. To take a degenerate case, suppose that you had many small WAL records, say 64 bytes each, so more than 100 per 8K block. If you flush those one by one, you're going to rewrite that block 100 times. If you flush them all at once, you write that block once. But even when the range is more than the minimum write size (8K for WAL), there are still wins. Writing 16K or 24K or 32K submitted as a single request can likely be done in a single revolution of the disk head. But if you write 8K and wait until it's done, and then write another 8K and wait until that's done, the second request may not arrive until after the disk head has passed the position where the second block needs to go. Now you have to wait for the drive to spin back around to the right position. The details of course vary with the hardware in use, but there are very few I/O operations where batching smaller requests into larger chunks doesn't help to some degree. Of course, the optimal transfer size does vary considerably based on the type of I/O and the specific hardware in use. This makes a lot of sense. I was always under the impression that batching small requests into larger requests bears the overhead of I/O latency. But, it seems to be the other way round, which I have now understood. Thanks a ton, Regards, Atri -- Regards, Atri l'apprenant -- Sent 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 Fri, Jun 28, 2013 at 3:32 AM, Andres Freund and...@2ndquadrant.com wrote: What that means is that for every heap record in the target database in the WAL we need to query pg_class to turn the relfilenode into a pg_class.oid. So, we can easily replace syscache.c with some custom caching code, but I don't think it's realistic to get rid of that index. Otherwise we need to cache the entire pg_class in memory which doesn't sound enticing. The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. -- 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] changeset generation v5-01 - Patches git tree
On 2013-06-28 08:41:46 -0400, Robert Haas wrote: On Fri, Jun 28, 2013 at 3:32 AM, Andres Freund and...@2ndquadrant.com wrote: What that means is that for every heap record in the target database in the WAL we need to query pg_class to turn the relfilenode into a pg_class.oid. So, we can easily replace syscache.c with some custom caching code, but I don't think it's realistic to get rid of that index. Otherwise we need to cache the entire pg_class in memory which doesn't sound enticing. The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. I don't think index maintenance itself comes close to the biggest cost for DDL we have atm. It also increases the modifications needed to imporantant heap_* functions which doesn't make me happy. How do others see this tradeoff? 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] Improvement of checkpoint IO scheduler for stable transaction responses
(2013/06/28 0:08), Robert Haas wrote: On Tue, Jun 25, 2013 at 4:28 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: I'm pretty sure Greg Smith tried it the fixed-sleep thing before and it didn't work that well. I have also tried it and the resulting behavior was unimpressive. It makes checkpoints take a long time to complete even when there's very little data to flush out to the OS, which is annoying; and when things actually do get ugly, the sleeps aren't long enough to matter. See the timings Kondo-san posted downthread: 100ms delays aren't going let the system recover in any useful way when the fsync can take 13 s for one file. On a system that's badly weighed down by I/O, the fsync times are often *extremely* long - 13 s is far from the worst you can see. You have to give the system a meaningful time to recover from that, allowing other processes to make meaningful progress before you hit it again, or system performance just goes down the tubes. Greg's test, IIRC, used 3 s sleeps rather than your proposal of 100 ms, but it still wasn't enough. Yes. In write phase, checkpointer writes numerous 8KB dirty pages in each SyncOneBuffer(), therefore it can be well for tiny(100ms) sleep time. But in fsync phase, checkpointer writes scores of relation files in each fsync(), therefore it can not be well for tiny sleep. It shoud need longer sleep time for recovery IO performance. If we know its best sleep time, we had better use previous fsync time. And if we want to prevent fast long fsync time, we had better change relation file size which is 1GB in default max size to smaller. Go back to the subject. Here is our patches test results. Fsync + write patch was not good result in past result, so I retry benchmark in same condition. It seems to get good perfomance than past result. * Performance result in DBT-2 (WH340) | TPS 90%tileAverage Maximum ---+--- original_0.7 | 3474.62 18.348328 5.73936.977713 original_1.0 | 3469.03 18.637865 5.84241.754421 fsync | 3525.03 13.872711 5.38228.062947 write | 3465.96 19.653667 5.80440.664066 fsync + write | 3586.85 14.459486 4.96027.266958 Heikki's patch | 3504.3 19.731743 5.76138.33814 * HTML result in DBT-2 http://pgstatsinfo.projects.pgfoundry.org/RESULT/ In attached text, I also describe in each checkpoint time. fsync patch was seemed to have longer time than not fsync patch. However, checkpoint schedule is on time in checkpoint_timeout and allowable time. I think that it is most important things in fsync phase that fast finished checkpoint is not but definitely and assurance write pages in end of checkpoint. So my fsync patch is not wrong working any more. My write patch seems to have lot of riddle, so I try to investigate objective result and theory of effect. Best regards, -- Mitsumasa KONDO NTT Open Source Software Center * Performance result | TPS 90%tileAverage Maximum ---+--- original_0.7 | 3474.62 18.348328 5.73936.977713 original_1.0 | 3469.03 18.637865 5.84241.754421 fsync | 3525.03 13.872711 5.38228.062947 write | 3465.96 19.653667 5.80440.664066 fsync + write | 3586.85 14.459486 4.96027.266958 Heikki's patch | 3504.3 19.731743 5.76138.33814 * Checkpoint duration # original_0.7 instid | start| flags | num_buffers | xlog_added | xlog_removed | xlog_recycled | write_duration | sync_duration | total_duration ++---+-++--+---++---+ 14 | 2013-06-19 15:15:24.658+09 | xlog | 281 | 0 | 0 | 0 | 29.038 | 2.69 | 31.798 14 | 2013-06-19 15:17:13.212+09 | xlog | 177 | 0 | 0 | 300 | 17.9 | 0.886 | 18.818 14 | 2013-06-19 15:18:45.525+09 | xlog | 306 | 0 | 0 | 300 | 30.72 | 4.011 | 35.11 14 | 2013-06-19 15:20:26.951+09 | xlog | 215 | 0 | 0 | 300 | 21.952 | 2.148 | 24.197 14 | 2013-06-19 15:21:56.425+09 | xlog | 182 | 0 | 0 | 300 | 18.527 | 6.323 | 25.069 14 | 2013-06-19 15:27:26.074+09 | xlog | 15770 | 0 | 0 | 300 |335.431 |80.269 |420.405 14 | 2013-06-19 15:42:26.272+09 | time | 82306 | 0 | 0 | 300 | 209.34 | 119.159 |333.762 14 | 2013-06-19 15:57:26.025+09 | time | 88965 | 0 | 0 | 247 |211.095 |
Re: [HACKERS] changeset generation v5-01 - Patches git tree
On 2013-06-27 21:52:03 -0700, Kevin Grittner wrote: Tried that, too, and problem persists. The log shows the last commit on your branch as 022c2da1873de2fbc93ae524819932719ca41bdb. I get the same failure, with primary key or unique index column showing as 0 in results. I have run enough iterations of the test suite locally now that I am confident it's not just happenstance that I don't see this :/. I am going to clone your environment as closely as I can to see where the issue might be as well as going over those codepaths... 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] Move unused buffers to freelist
On Fri, Jun 28, 2013 at 12:52 AM, Amit Kapila amit.kap...@huawei.com wrote: Currently it wakes up based on bgwriterdelay config parameter which is by default 200ms, so you means we should think of waking up bgwriter based on allocations and number of elements left in freelist? I think that's what Andres and I are proposing, yes. As per my understanding Summarization of points raised by you and Andres which this patch should address to have a bigger win: 1. Bgwriter needs to be improved so that it can help in reducing usage count and finding next victim buffer (run the clock sweep and add buffers to the free list). Check. 2. SetLatch for bgwriter (wakeup bgwriter) when elements in freelist are less. Check. The way to do this is to keep a variable in shared memory in the same cache line as the spinlock protecting the freelist, and update it when you update the free list. 3. Split the workdone globallock (Buffreelist) in StrategyGetBuffer (a spinlock for the freelist, and an lwlock for the clock sweep). Check. 4. Separate processes for writing dirty buffers and moving buffers to freelist I think this part might be best pushed to a separate patch, although I agree we probably need it. 5. Bgwriter needs to be more aggressive, logic based on which it calculates how many buffers it needs to process needs to be improved. This is basically overlapping with points already made. I suspect we could just get rid of bgwriter_delay, bgwriter_lru_maxpages, and bgwriter_lru_multiplier altogether. The background writer would just have a high and a low watermark. When the number of buffers on the freelist drops below the low watermark, the allocating backend sets the latch and bgwriter wakes up and begins adding buffers to the freelist. When the number of buffers on the free list reaches the high watermark, the background writer goes back to sleep. Some experimentation might be needed to figure out what values are appropriate for those watermarks. In theory this could be a configuration knob, but I suspect it's better to just make the system tune it right automatically. 6. There can be contention around buffer mapping locks, but we can focus on it later 7. cacheline bouncing around the buffer header spinlocks, is there anything we can do to reduce this? I think these are points that we should leave for the future. -- 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] Move unused buffers to freelist
On Fri, Jun 28, 2013 at 8:50 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Jun 28, 2013 at 12:52 AM, Amit Kapila amit.kap...@huawei.com wrote: Currently it wakes up based on bgwriterdelay config parameter which is by default 200ms, so you means we should think of waking up bgwriter based on allocations and number of elements left in freelist? I think that's what Andres and I are proposing, yes. Incidentally, I'm going to mark this patch Returned with Feedback in the CF application. I think this line of inquiry has potential, but clearly there's a lot more work to do here before we commit anything, and I don't think that's going to happen in the next few weeks. But let's keep discussing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)
On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote: 2013/6/28 Noah Misch n...@leadboat.com: On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote: cleaned patch is in attachment Of the five options you're adding to GET STACKED DIAGNOSTICS, four of them appear in the SQL standard. DATATYPE_NAME does not; I think we should call it PG_DATATYPE_NAME in line with our other extensions in this area. yes, but It should be fixed in 9.3 enhanced fields too - it should be consistent with PostgreSQL fields What else, specifically, should be renamed? (Alternately, would you prepare a new version of the patch incorporating the proper name changes?) 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] extensible external toast tuple support
On Thu, Jun 27, 2013 at 12:56 PM, Andres Freund and...@2ndquadrant.com wrote: Please find attached the next version of the extensible toast support. There basically are two changes: * handle indirect toast tuples properly in heap_tuple_fetch_attr and related places * minor comment adjustments It looks to me like you need to pass true, rather than false, as the first argument to TrapMacro: +#define VARTAG_SIZE(tag) \ + ((tag) == VARTAG_INDIRECT ? sizeof(varatt_indirect) : \ +(tag) == VARTAG_ONDISK ? sizeof(varatt_external) : \ +TrapMacro(false, unknown vartag)) Still looking at the rest of this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)
Hello 2013/6/28 Noah Misch n...@leadboat.com: On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote: 2013/6/28 Noah Misch n...@leadboat.com: On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote: cleaned patch is in attachment Of the five options you're adding to GET STACKED DIAGNOSTICS, four of them appear in the SQL standard. DATATYPE_NAME does not; I think we should call it PG_DATATYPE_NAME in line with our other extensions in this area. yes, but It should be fixed in 9.3 enhanced fields too - it should be consistent with PostgreSQL fields What else, specifically, should be renamed? (Alternately, would you prepare a new version of the patch incorporating the proper name changes?) I looked to source code, and identifiers in our source code are consistent, so my comment hasn't sense. Yes, I agree, so only identifier used in GET DIAGNOSTICS statement should be renamed. So, only DATATYPE_NAME should be renamed to PG_DATATYPE_NAME. Regards Pavel 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] extensible external toast tuple support
On Fri, Jun 28, 2013 at 9:23 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 27, 2013 at 12:56 PM, Andres Freund and...@2ndquadrant.com wrote: Please find attached the next version of the extensible toast support. There basically are two changes: * handle indirect toast tuples properly in heap_tuple_fetch_attr and related places * minor comment adjustments It looks to me like you need to pass true, rather than false, as the first argument to TrapMacro: +#define VARTAG_SIZE(tag) \ + ((tag) == VARTAG_INDIRECT ? sizeof(varatt_indirect) : \ +(tag) == VARTAG_ONDISK ? sizeof(varatt_external) : \ +TrapMacro(false, unknown vartag)) Still looking at the rest of this. Why does toast_insert_or_update() need to go through all the rigamarole in toast_datum_differs()? I would have thought that it could simply treat any external-indirect value as needing to be detoasted and retoasted, since the destination is the disk anyhow. Do you see external-indirect values getting used for anything other than logical replication? Is there code to do so anywhere? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division
On Tue, Jun 25, 2013 at 02:54:55PM +0530, Jeevan Chalke wrote:
On Tue, Jun 25, 2013 at 11:11 AM, Jeevan Chalke
jeevan.cha...@enterprisedb.com wrote:
Hi David,
I hope this is the latest patch to review, right ?
I am going to review it.
I have gone through the discussion on this thread and I agree with Stephen
Frost that it don't add much improvements as such but definitely it is
going to be easy for contributors in this area as they don't need to go all
over to add any extra parameter they need to add. This patch simplifies it
well enough.
Will post my review soon.
Assuming *makeFuncArgs_002.diff* is the latest patch, here are my review
points.
About this patch and feature:
===
This patch tries to reduce redundancy when we need FuncCall expression. With
this patch it will become easier to add new parameter if needed. We just
need
to put it's default value at centralized location (I mean in this new
function)
so that all other places need not require and changes. And this new
parameter
is handled by the new feature who demanded it keep other untouched.
Review comments on patch:
===
1. Can't apply with git apply command but able to do it with patch -p1.
So no
issues
2. Adds one whitespace error, hopefully it will get corrected once it goes
through pgindent.
3. There is absolutely NO user visibility and thus I guess we don't need any
test case. Also no documentation are needed.
4. Also make check went smooth and thus assumes that there is no issue as
such.
Even I couldn't find any issue with my testing other than regression suite.
5. I had a code walk-through over patch and it looks good to me. However,
following line change seems unrelated (Line 186 in your patch)
! $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, ~~, $1,
(Node *) n, @2);
!
Changes required from author:
===
It will be good if you remove unrelated changes from the patch and possibly
all
white-space errors.
Thanks
Thanks for the review!
Please find attached the latest patch.
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
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index c487db9..245aef2 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -508,3 +508,28 @@ makeDefElemExtended(char *nameSpace, char *name, Node *arg,
return res;
}
+
+/*
+ * makeFuncCall -
+ *
+ * Initialize a FuncCall struct with the information every caller must
+ * supply. Any non-default parameters have to be handled by the
+ * caller.
+ *
+ */
+
+FuncCall *
+makeFuncCall(List *name, List *args, int location)
+{
+ FuncCall *n = makeNode(FuncCall);
+ n-funcname = name;
+ n-args = args;
+ n-location = location;
+ n-agg_order = NIL;
+ n-agg_star = FALSE;
+ n-agg_distinct = FALSE;
+ n-func_variadic = FALSE;
+ n-over = NULL;
+ return n;
+}
+
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 5094226..24a585e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10487,16 +10487,9 @@ a_expr:c_expr
{ $$ = $1; }
}
| a_expr AT TIME ZONE a_expr%prec AT
{
- FuncCall *n = makeNode(FuncCall);
- n-funcname =
SystemFuncName(timezone);
- n-args = list_make2($5, $1);
- n-agg_order = NIL;
- n-agg_star = FALSE;
- n-agg_distinct = FALSE;
- n-func_variadic = FALSE;
- n-over = NULL;
- n-location = @2;
- $$ = (Node *) n;
+ $$ = (Node *)
makeFuncCall(SystemFuncName(timezone),
+
list_make2($5, $1),
+
@2);
}
/*
* These operators must be called out explicitly in order to
make use
@@ -10548,113 +10541,65 @@ a_expr: c_expr
{ $$ = $1; }
{ $$ = (Node *) makeSimpleA_Expr(AEXPR_OP,
~~, $1, $3, @2); }
Re: [HACKERS] changeset generation v5-01 - Patches git tree
On 6/28/13 8:46 AM, Andres Freund wrote: I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. I don't think index maintenance itself comes close to the biggest cost for DDL we have atm. That makes sense to me in principle. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division
David Fetter da...@fetter.org writes: Please find attached the latest patch. I remain of the opinion that this is simply a bad idea. It is unlike our habits for constructing other types of nodes, and makes it harder not easier to find all the places that need to be updated when adding another field to FuncCall. 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] Department of Redundancy Department: makeNode(FuncCall) division
On Fri, Jun 28, 2013 at 10:31:16AM -0400, Tom Lane wrote: David Fetter da...@fetter.org writes: Please find attached the latest patch. I remain of the opinion that this is simply a bad idea. It is unlike our habits for constructing other types of nodes, and makes it harder not easier to find all the places that need to be updated when adding another field to FuncCall. With utmost respect, this is just not true. There's exactly one place that needs updating after adding another field to FuncCall in the general case where the default value of the field doesn't affect most setters of FuncCall, i.e. where the default default is the right thing for current setters. In specific cases where such a field might need to be set to something other than its default value, finding calls to makeFuncCall is just as easy, and with some tools like cscope, even easier. 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] changeset generation v5-01 - Patches git tree
Andres Freund and...@2ndquadrant.com writes: On 2013-06-28 08:41:46 -0400, Robert Haas wrote: The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. This argument is nonsense, since it conveniently ignores the added WAL entries created as a result of additional pg_class index manipulations. Robert's idea sounds fairly reasonable to me; another 4 bytes per insert/update/delete WAL entry isn't that big a deal, and it would probably ease many debugging tasks as well as what you want to do. So I'd vote for including the rel OID all the time, not conditionally. The real performance argument against the patch as you have it is that it saddles every PG installation with extra overhead for pg_class updates whether or not that installation ever has or ever will make use of changeset generation --- unlike including rel OIDs in WAL entries, which might be merely difficult to handle conditionally, it's flat-out impossible to turn such an index on or off. Moreover, even if one is using changeset generation, the overhead is being imposed at the wrong place, ie the master not the slave doing changeset extraction. But that's not the only problem, nor even the worst one IMO. I said before that a syscache with a non-unique key is broken by design, and I stand by that estimate. Even assuming that this usage doesn't create bugs in the code as it stands, it might well foreclose future changes or optimizations that we'd like to make in the catcache code. If you don't want to change WAL contents, what I think you should do is create a new cache mechanism (perhaps by extending the relmapper) that caches relfilenode to OID lookups and acts entirely inside the changeset-generating slave. Hacking up the catcache instead of doing that is an expedient kluge that will come back to bite us. 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] Documentation/help for materialized and recursive views
Magnus Hagander escribió: They are already crosslinked under see also. But that doesn't really help the guy doing \h CREATE VIEW in psql, which was the case where it was brought to my attention. Maybe \h should somehow display the see also section? -- Á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] Documentation/help for materialized and recursive views
On Fri, Jun 28, 2013 at 4:49 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Magnus Hagander escribió: They are already crosslinked under see also. But that doesn't really help the guy doing \h CREATE VIEW in psql, which was the case where it was brought to my attention. Maybe \h should somehow display the see also section? I've been toying with the idea getting \h to show an actual http:// link to the reference page on the website, since most terminals lets you deal with URLs easily lately. I haven't actually looked into how feasible that would be though, but it would be interesting to check out. (With a toggle to turn it on/off of course) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] extensible external toast tuple support
On 2013-06-28 09:49:53 -0400, Robert Haas wrote: On Fri, Jun 28, 2013 at 9:23 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Jun 27, 2013 at 12:56 PM, Andres Freund and...@2ndquadrant.com wrote: Please find attached the next version of the extensible toast support. There basically are two changes: * handle indirect toast tuples properly in heap_tuple_fetch_attr and related places * minor comment adjustments It looks to me like you need to pass true, rather than false, as the first argument to TrapMacro: +#define VARTAG_SIZE(tag) \ + ((tag) == VARTAG_INDIRECT ? sizeof(varatt_indirect) : \ +(tag) == VARTAG_ONDISK ? sizeof(varatt_external) : \ +TrapMacro(false, unknown vartag)) Heh. I was obviously trying to be too cute ;) Good idea thanks for committing it separately. Do you see external-indirect values getting used for anything other than logical replication? Is there code to do so anywhere? Yes and not really. I think we can reuse it to avoid repeated detoastings, even in relatively normal queries. What I am absolutely not sure yet is how to automatically decide when we want to keep the tuple in memory because it will be beneficial, and when not because of the obvious memory overhead. And how to keep track of the memory context used to allocate the referenced data. There are some usecases where I think it might be relatively easy to decide its a win. E.g tuples passed to triggers if there are multiple ones of them. I've played a bit with using that functionality in C functions, but nothing publishable, more to make sure things work. Why does toast_insert_or_update() need to go through all the rigamarole in toast_datum_differs()? I would have thought that it could simply treat any external-indirect value as needing to be detoasted and retoasted, since the destination is the disk anyhow. We could do that, yes. But I think it might be better not to: If we simplify the tuples used in a query to not reference ondisk tuples anymore and we then UPDATE using that new version I would rather not retoast all the unchanged columns. I can e.g. very well imagine that we decide to resolve toasted Datums to indirect Datums during an UPDATE if there are multiple BEFORE UPDATE triggers to avoid detoasting in each and every one of them. Such a tuple will then passed to heap_update... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Department of Redundancy Department: makeNode(FuncCall) division
On Fri, Jun 28, 2013 at 10:31:16AM -0400, Tom Lane wrote: David Fetter da...@fetter.org writes: Please find attached the latest patch. I remain of the opinion that this is simply a bad idea. It is unlike our habits for constructing other types of nodes, and makes it harder not easier to find all the places that need to be updated when adding another field to FuncCall. We have precedents in makeRangeVar() and makeDefElem(). For me, this change would make it slightly easier to visit affected code sites after a change. I could cscope for callers of makeFuncCall() instead of doing git grep 'makeNode(FuncCall)'. The advantage could go either way depending on your tooling, though. By having each call site only mention the seldom-used fields for which it does something special, the distinctive aspects of the call site stand out better. That's a nice advantage. -- 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-28 10:49:26 -0400, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2013-06-28 08:41:46 -0400, Robert Haas wrote: The alternative I previously proposed was to make the WAL records carry the relation OID. There are a few problems with that: one is that it's a waste of space when logical replication is turned off, and it might not be easy to only do it when logical replication is on. Also, even when logic replication is turned on, things that make WAL bigger aren't wonderful. On the other hand, it does avoid the overhead of another index on pg_class. I personally favor making catalog modifications a bit more more expensive instead of increasing the WAL volume during routine operations. This argument is nonsense, since it conveniently ignores the added WAL entries created as a result of additional pg_class index manipulations. Huh? Sure, pg_class manipulations get more expensive. But in most clusters pg_class modifications are by far the minority compared to the rest of the updates performed. Robert's idea sounds fairly reasonable to me; another 4 bytes per insert/update/delete WAL entry isn't that big a deal, and it would probably ease many debugging tasks as well as what you want to do. So I'd vote for including the rel OID all the time, not conditionally. Ok, I can sure live with that. I don't think it's a problem to make it conditionally if we want to. Making it unconditional would sure make WAL debugging in general more pleasant though. The real performance argument against the patch as you have it is that it saddles every PG installation with extra overhead for pg_class updates whether or not that installation ever has or ever will make use of changeset generation --- unlike including rel OIDs in WAL entries, which might be merely difficult to handle conditionally, it's flat-out impossible to turn such an index on or off. Moreover, even if one is using changeset generation, the overhead is being imposed at the wrong place, ie the master not the slave doing changeset extraction. There's no required slaves for doing changeset extraction anymore. Various people opposed that pretty violently, so it's now all happening on the master. Which IMHO turned out to be the right decision. We can do it on Hot Standby nodes, but its absolutely not required. But that's not the only problem, nor even the worst one IMO. I said before that a syscache with a non-unique key is broken by design, and I stand by that estimate. Even assuming that this usage doesn't create bugs in the code as it stands, it might well foreclose future changes or optimizations that we'd like to make in the catcache code. Since the only duplicate key that possibly can occur in that cache is InvalidOid, I wondered whether we could define a 'filter' that prohibits those ending up in the cache? Then the cache would be unique. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)
On Fri, Jun 28, 2013 at 03:31:00PM +0200, Pavel Stehule wrote: Hello 2013/6/28 Noah Misch n...@leadboat.com: On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote: 2013/6/28 Noah Misch n...@leadboat.com: On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote: cleaned patch is in attachment Of the five options you're adding to GET STACKED DIAGNOSTICS, four of them appear in the SQL standard. DATATYPE_NAME does not; I think we should call it PG_DATATYPE_NAME in line with our other extensions in this area. yes, but It should be fixed in 9.3 enhanced fields too - it should be consistent with PostgreSQL fields What else, specifically, should be renamed? (Alternately, would you prepare a new version of the patch incorporating the proper name changes?) I looked to source code, and identifiers in our source code are consistent, so my comment hasn't sense. Yes, I agree, so only identifier used in GET DIAGNOSTICS statement should be renamed. So, only DATATYPE_NAME should be renamed to PG_DATATYPE_NAME. Okay. I failed to note the first time through that while the patch uses the same option names for RAISE and GET STACKED DIAGNOSTICS, the existing option lists for those commands differ: --RAISE option----GET STACKED DIAGNOSTICS option-- ERRCODE RETURNED_SQLSTATE MESSAGE MESSAGE_TEXT DETAIL PG_EXCEPTION_DETAIL HINTPG_EXCEPTION_HINT CONTEXT PG_EXCEPTION_CONTEXT To be consistent with that pattern, I think we would use COLUMN, CONSTRAINT, TABLE, TYPE and SCHEMA as the new RAISE options. -- 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 Fri, Jun 28, 2013 at 10:49 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert's idea sounds fairly reasonable to me; another 4 bytes per insert/update/delete WAL entry isn't that big a deal, ... How big a deal is it? This is a serious question, because I don't know. Let's suppose that the average size of an XLOG_HEAP_INSERT record is 100 bytes. Then if we add 4 bytes, isn't that a 4% overhead? And doesn't that seem significant? I'm just talking out of my rear end here because I don't know what the real numbers are, but it's far from obvious to me that there's any free lunch here. That having been said, just because indexing relfilenode or adding relfilenodes to WAL records is expensive doesn't mean we shouldn't do it. But I think we need to know the price tag before we can judge whether to make the purchase. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: enable new error fields in plpgsql (9.4)
2013/6/28 Noah Misch n...@leadboat.com: On Fri, Jun 28, 2013 at 03:31:00PM +0200, Pavel Stehule wrote: Hello 2013/6/28 Noah Misch n...@leadboat.com: On Fri, Jun 28, 2013 at 07:49:46AM +0200, Pavel Stehule wrote: 2013/6/28 Noah Misch n...@leadboat.com: On Tue, Jun 25, 2013 at 03:56:27PM +0200, Pavel Stehule wrote: cleaned patch is in attachment Of the five options you're adding to GET STACKED DIAGNOSTICS, four of them appear in the SQL standard. DATATYPE_NAME does not; I think we should call it PG_DATATYPE_NAME in line with our other extensions in this area. yes, but It should be fixed in 9.3 enhanced fields too - it should be consistent with PostgreSQL fields What else, specifically, should be renamed? (Alternately, would you prepare a new version of the patch incorporating the proper name changes?) I looked to source code, and identifiers in our source code are consistent, so my comment hasn't sense. Yes, I agree, so only identifier used in GET DIAGNOSTICS statement should be renamed. So, only DATATYPE_NAME should be renamed to PG_DATATYPE_NAME. Okay. I failed to note the first time through that while the patch uses the same option names for RAISE and GET STACKED DIAGNOSTICS, the existing option lists for those commands differ: --RAISE option----GET STACKED DIAGNOSTICS option-- ERRCODE RETURNED_SQLSTATE MESSAGE MESSAGE_TEXT DETAIL PG_EXCEPTION_DETAIL HINTPG_EXCEPTION_HINT CONTEXT PG_EXCEPTION_CONTEXT To be consistent with that pattern, I think we would use COLUMN, CONSTRAINT, TABLE, TYPE and SCHEMA as the new RAISE options. I understand to your motivation, but I am not sure. Minimally word TYPE is too general. I have not strong opinion in this area. maybe DATATYPE ?? p.s. you cannot to specify CONTEXT in RAISE statement Regards Pavel -- 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] extensible external toast tuple support
On Fri, Jun 28, 2013 at 10:53 AM, Andres Freund and...@2ndquadrant.com wrote: Why does toast_insert_or_update() need to go through all the rigamarole in toast_datum_differs()? I would have thought that it could simply treat any external-indirect value as needing to be detoasted and retoasted, since the destination is the disk anyhow. We could do that, yes. But I think it might be better not to: If we simplify the tuples used in a query to not reference ondisk tuples anymore and we then UPDATE using that new version I would rather not retoast all the unchanged columns. I can e.g. very well imagine that we decide to resolve toasted Datums to indirect Datums during an UPDATE if there are multiple BEFORE UPDATE triggers to avoid detoasting in each and every one of them. Such a tuple will then passed to heap_update... I must be missing something. At that point, yes, you'd like to avoid re-toasting unnecessarily, but ISTM you've already bought the farm. Unless I'm misunderstanding the code as written, you'd just end up writing the indirect pointer back out to disk in that scenario. That's not gonna work... -- 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] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
On Thu, Jun 27, 2013 at 8:52 PM, Nicholas White n.j.wh...@gmail.com wrote: The result of the current patch using lead Actually, I think I agree with you (and, FWIW, so does Oracle: http://docs.oracle.com/cd/E11882_01/server.112/e25554/analysis.htm#autoId18). I've refactored the window function's implementation so that (e.g.) lead(5) means the 5th non-null value away in front of the current row (the previous implementation was the last non-null value returned if the 5th rows in front was null). These semantics are slower, as the require the function to scan through the tuples discarding non-null ones. I've made the implementation use a bitmap in the partition context to cache whether or not a given tuple produces a null. This seems correct (it passes the regression tests) but as it stores row offsets (which are int64s) I was careful not to use bitmap methods that use ints to refer to set members. I've added more explanation in the code's comments. Thanks - The documentation you've added reads kind of funny to me: + [respect nulls]|[ignore nulls] Wouldn't we normally write that as [ [ RESPECT | IGNORE ] NULLS ] ? I've committed the changes from Troels to avoid the grammar conflicts, and I also took the opportunity to make OVER unreserved, as allowed by his refactoring and per related discussion on other threads. This patch will need to be rebased over those changes (sorry), but hopefully it'll help the review of this patch focus in on the issues that are specific to RESPECT/IGNORE NULLS. -- 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] Department of Redundancy Department: makeNode(FuncCall) division
On Fri, Jun 28, 2013 at 10:31 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Fetter da...@fetter.org writes: Please find attached the latest patch. I remain of the opinion that this is simply a bad idea. It is unlike our habits for constructing other types of nodes, and makes it harder not easier to find all the places that need to be updated when adding another field to FuncCall. I think it's a nice code cleanup. I don't understand your objection. -- 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] changeset generation v5-01 - Patches git tree
Robert Haas escribió:
On Fri, Jun 28, 2013 at 10:49 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Robert's idea sounds fairly reasonable to me; another 4 bytes per
insert/update/delete WAL entry isn't that big a deal, ...
How big a deal is it? This is a serious question, because I don't
know. Let's suppose that the average size of an XLOG_HEAP_INSERT
record is 100 bytes. Then if we add 4 bytes, isn't that a 4%
overhead? And doesn't that seem significant?
An INSERT wal record is:
typedef struct xl_heap_insert
{
xl_heaptid target; /* inserted tuple id */
boolall_visible_cleared;/* PD_ALL_VISIBLE was cleared */
/* xl_heap_header TUPLE DATA FOLLOWS AT END OF STRUCT */
} xl_heap_insert;
typedef struct xl_heap_header
{
uint16 t_infomask2;
uint16 t_infomask;
uint8 t_hoff;
} xl_heap_header;
So the fixed part is just 7 bytes + 5 bytes; tuple data follows that.
So adding four more bytes could indeed be significant (but by how much,
depends on the size of the tuple data). Adding a new pg_class index
would be larger in the sense that there are more WAL records, and
there's the extra vacuuming traffic; but on the other hand that would
only happen when tables are created. It seems safe to assume that in
normal use cases the ratio of tuple insertion vs. table creation is
large.
The only idea that springs to mind is to have the new pg_class index be
created conditionally, i.e. only when logical replication is going to be
used.
--
Á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] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
Robert Haas escribió:
On Thu, Jun 27, 2013 at 8:52 PM, Nicholas White n.j.wh...@gmail.com wrote:
The documentation you've added reads kind of funny to me:
+ [respect nulls]|[ignore nulls]
Wouldn't we normally write that as [ [ RESPECT | IGNORE ] NULLS ] ?
I think it should be
[ { RESPECT | IGNORE } NULLS ]
One of the leading keywords must be present.
--
Á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] Review: query result history in psql
Hello I am not sure, this interface is really user friendly there is not possible searching in history, and not every query push to history some interesting content. It require: * simply decision if content should be stored in history or not, * simply remove last entry (table) of history * queries should be joined to content, only name is not enough Regards Pavel 2013/6/28 Maciej Gajewski maciej.gajews...@gmail.com: Thanks for checking the patch! So what's left to fix? * Moving the escaping-related functions to separate module, * applying your corrections. Did I missed anything? I'll submit corrected patch after the weekend. M -- Sent 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
Alvaro Herrera escribió:
An INSERT wal record is:
typedef struct xl_heap_insert
{
xl_heaptid target; /* inserted tuple id */
boolall_visible_cleared;/* PD_ALL_VISIBLE was cleared */
/* xl_heap_header TUPLE DATA FOLLOWS AT END OF STRUCT */
} xl_heap_insert;
Oops. xl_heaptid is not 6 bytes, but instead:
typedef struct xl_heaptid
{
RelFileNode node;
ItemPointerData tid;
} xl_heaptid;
typedef struct RelFileNode
{
Oid spcNode;
Oid dbNode;
Oid relNode;
} RelFileNode; /* 12 bytes */
typedef struct ItemPointerData
{
BlockIdData ip_blkid;
OffsetNumber ip_posid;
}; /* 6 bytes */
typedef struct BlockIdData
{
uint16 bi_hi;
uint16 bi_lo;
} BlockIdData; /* 4 bytes */
typedef uint16 OffsetNumber;
There's purposely no alignment padding anywhere, so xl_heaptid totals 22 bytes.
Therefore,
So the fixed part is just 22 bytes + 5 bytes; tuple data follows that.
So adding four more bytes could indeed be significant (but by how much,
depends on the size of the tuple data).
4 extra bytes on top of 27 is 14% of added overhead (considering only
the xlog header.)
--
Á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] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
On Fri, Jun 28, 2013 at 11:41 AM, Alvaro Herrera
alvhe...@2ndquadrant.com wrote:
Robert Haas escribió:
On Thu, Jun 27, 2013 at 8:52 PM, Nicholas White n.j.wh...@gmail.com wrote:
The documentation you've added reads kind of funny to me:
+ [respect nulls]|[ignore nulls]
Wouldn't we normally write that as [ [ RESPECT | IGNORE ] NULLS ] ?
I think it should be
[ { RESPECT | IGNORE } NULLS ]
One of the leading keywords must be present.
Oh, yeah. What he said.
--
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] changeset generation v5-01 - Patches git tree
Robert Haas robertmh...@gmail.com writes: I'm just talking out of my rear end here because I don't know what the real numbers are, but it's far from obvious to me that there's any free lunch here. That having been said, just because indexing relfilenode or adding relfilenodes to WAL records is expensive doesn't mean we shouldn't do it. But I think we need to know the price tag before we can judge whether to make the purchase. Certainly, any of these solutions are going to cost us somewhere --- either up-front cost or more expensive (and less reliable?) changeset extraction, take your choice. I will note that somehow tablespaces got put in despite having to add 4 bytes to every WAL record for that feature, which was probably of less use than logical changeset extraction will be. But to tell the truth, I'm mostly exercised about the non-unique syscache. I think that's simply a *bad* idea. 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] PostgreSQL 9.3 latest dev snapshot
On 06/27/2013 12:22 PM, Magnus Hagander wrote: On Tue, Jun 25, 2013 at 3:31 PM, Michael Paquier michael.paqu...@gmail.com wrote: On 2013/06/25, at 22:23, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jun 25, 2013 at 6:33 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Jun 25, 2013 at 5:33 PM, Misa Simic misa.si...@gmail.com wrote: Hi, Where we can find latest snapshot for 9.3 version? We have taken latest snapshot from http://ftp.postgresql.org/pub/snapshot/dev/ But it seems it is for 9.4 version... 9.3 has moved to branch REL9_3_STABLE a couple of days ago. Yes. We can find the snapshot from REL9_3_STABLE git branch. http://git.postgresql.org/gitweb/?p=postgresql.git;a=shortlog;h=refs/heads/REL9_3_STABLE Indeed, I completely forgot that you can download snapshots from postgresql.org's git. Simply use that instead of the FTP server now as long as 9.3 snapshots are not generated there. In case somebody is still looking, snapshots are properly building for 9.3 now. Those snapshots aren't identical to a download from git, as they've gone through a make dist-prep or whatever it's called. But they're pretty close. there is more to that - those snapshots also will also only get published if the source passed a full buildfarm run as a basic form of validation. However, if oyu're looking for a snapshot, please use the one on the ftpsite. Generating those snapshots on the git server is slow and expensive... definitly 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] proposal: enable new error fields in plpgsql (9.4)
On Fri, Jun 28, 2013 at 05:21:29PM +0200, Pavel Stehule wrote: 2013/6/28 Noah Misch n...@leadboat.com: Okay. I failed to note the first time through that while the patch uses the same option names for RAISE and GET STACKED DIAGNOSTICS, the existing option lists for those commands differ: --RAISE option----GET STACKED DIAGNOSTICS option-- ERRCODE RETURNED_SQLSTATE MESSAGE MESSAGE_TEXT DETAIL PG_EXCEPTION_DETAIL HINTPG_EXCEPTION_HINT CONTEXT PG_EXCEPTION_CONTEXT To be consistent with that pattern, I think we would use COLUMN, CONSTRAINT, TABLE, TYPE and SCHEMA as the new RAISE options. I understand to your motivation, but I am not sure. Minimally word TYPE is too general. I have not strong opinion in this area. maybe DATATYPE ?? I'm not positive either. DATATYPE rather than TYPE makes sense. p.s. you cannot to specify CONTEXT in RAISE statement Oops; right. -- 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] ALTER TABLE ... ALTER CONSTRAINT
On 24 June 2013 22:17, Simon Riggs si...@2ndquadrant.com wrote: On 24 June 2013 21:42, Jeff Janes jeff.ja...@gmail.com wrote: On Sun, Jun 23, 2013 at 8:58 PM, Abhijit Menon-Sen a...@2ndquadrant.comwrote: 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. After the commit, I'm now getting the compiler warning: tablecmds.c: In function 'ATPrepCmd': tablecmds.c:2953: warning: 'pass' may be used uninitialized in this function case AT_AlterConstraint (line 3130) is the only case branch that does not set pass. The investigation is into why my current compiler didn't report that. Thanks though. Looks like that really is a deficiency in my tool chain on OSX, rather than some bug/user error. Even at the very latest, very shiny version. Latest versions of gcc trap the error, so I'll have to investigate an alternative. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] Move unused buffers to freelist
On 6/28/13 8:50 AM, Robert Haas wrote: On Fri, Jun 28, 2013 at 12:52 AM, Amit Kapila amit.kap...@huawei.com wrote: 4. Separate processes for writing dirty buffers and moving buffers to freelist I think this part might be best pushed to a separate patch, although I agree we probably need it. This might be necessary eventually, but it's going to make thing more complicated. And I don't think it's a blocker for creating something useful. The two most common workloads are: 1) Lots of low usage count data, typically data that is updated sparsely across a larger database. These are helped by a process that writes dirty buffers in the background. These benefit from the current background writer. Kevin's system he was just mentioning again is the best example of this type that there's public data on. 2) Lots of high usage count data, because there are large hotspots in things like index blocks. Most writes happen at checkpoint time, because the background writer won't touch them. Because there are only a small number of re-usable pages, the clock sweep goes around very fast looking for them. This is the type of workload that should benefit from putting buffers into the free list. pgbench provides a simple example of this type, which is why Amit's tests using it have been useful. If you had a process that tried to handle both background writes and freelist management, I suspect one path would be hot and the other almost idle in each type of system. I don't expect that splitting those into two separate process would buy a lot of value, that can easily be pushed to a later patch. The background writer would just have a high and a low watermark. When the number of buffers on the freelist drops below the low watermark, the allocating backend sets the latch and bgwriter wakes up and begins adding buffers to the freelist. When the number of buffers on the free list reaches the high watermark, the background writer goes back to sleep. This will work fine for all of the common workloads. The main challenge is keeping the buffer allocation counting from turning into a hotspot. Busy systems now can easily hit 100K buffer allocations/second. I'm not too worried about it because those allocations are making the free list lock a hotspot right now. One of the consistently controversial parts of the current background writer is how it tries to loop over the buffer cache every 2 minutes, regardless of activity level. The idea there was that on bursty workloads, buffers would be cleaned during idle periods with that mechanism. Part of why that's in there is to deal with the relatively long pause between background writer runs. This refactoring idea will make that hard to keep around. I think this is OK though. Switching to a latch based design should eliminate the bgwriter_delay, which means you won't have this worst case of a 200ms stall while heavy activity is incoming. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.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 Fri, Jun 28, 2013 at 11:56 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: I'm just talking out of my rear end here because I don't know what the real numbers are, but it's far from obvious to me that there's any free lunch here. That having been said, just because indexing relfilenode or adding relfilenodes to WAL records is expensive doesn't mean we shouldn't do it. But I think we need to know the price tag before we can judge whether to make the purchase. Certainly, any of these solutions are going to cost us somewhere --- either up-front cost or more expensive (and less reliable?) changeset extraction, take your choice. I will note that somehow tablespaces got put in despite having to add 4 bytes to every WAL record for that feature, which was probably of less use than logical changeset extraction will be. Right. I actually think we booted that one. The database ID is a constant for most people. The tablespace ID is not technically redundant, but in 99.99% of cases you could figure it out from the database ID + relation ID. The relation ID is where 99% of the entropy is, but it probably only has 8-16 bits of entropy in most real-world use cases. If we were doing this over we might want to think about storing a proxy for the relfilenode rather than the relfilenode itself, but there's not much good crying over it now. But to tell the truth, I'm mostly exercised about the non-unique syscache. I think that's simply a *bad* idea. +1. I don't think the extra index on pg_class is going to hurt that much, even if we create it always, as long as we use a purpose-built caching mechanism for it rather than forcing it through catcache. The people who are going to suffer are the ones who create and drop a lot of temporary tables, but even there I'm not sure how visible the overhead will be on real-world workloads, and maybe the solution is to work towards not having permanent catalog entries for temporary tables in the first place. In any case, hurting people who use temporary tables heavily seems better than adding overhead to every insert/update/delete operation, which will hit all users who are not read-only. On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. -- 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] Move unused buffers to freelist
On Fri, Jun 28, 2013 at 12:10 PM, Greg Smith g...@2ndquadrant.com wrote: This refactoring idea will make that hard to keep around. I think this is OK though. Switching to a latch based design should eliminate the bgwriter_delay, which means you won't have this worst case of a 200ms stall while heavy activity is incoming. I'm a strong proponent of that 2 minute cycle, so I'd vote for finding a way to keep it around. But I don't think that (or 200 ms wakeups) should be the primary thing driving the background writer, either. -- 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] changeset generation v5-01 - Patches git tree
Robert Haas robertmh...@gmail.com writes: On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. That feeling has been nagging at me too. I can't demonstrate that there's a problem when an ALTER TABLE is in process of rewriting a table into a new relfilenode number, but I don't have a warm fuzzy feeling about the reliability of reverse lookups for this. At the very least it's going to require some hard-to-verify restriction about how we can't start doing changeset reconstruction in the middle of a transaction that's doing DDL. 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] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
This patch will need to be rebased over those changes See attached - lead-lag-ignore-nulls.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE ... ALTER CONSTRAINT
Simon Riggs escribió: Looks like that really is a deficiency in my tool chain on OSX, rather than some bug/user error. Even at the very latest, very shiny version. Latest versions of gcc trap the error, so I'll have to investigate an alternative. Funnily enough, on Debian Wheezy with gcc 4.7.2 I don't get the warning, and Andres with gcc 4.7.3 (from Debian unstable) does see it. (Of course, the 4.8 version shipped with unstable also shows it.) Clang similarly requires pretty new versions to show the warning. -- Á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] PostgreSQL 9.3 latest dev snapshot
Magnus Hagander escribió: However, if oyu're looking for a snapshot, please use the one on the ftpsite. Generating those snapshots on the git server is slow and expensive... Maybe we should redirect those gitweb snapshot URLs to the FTP site? -- Á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] changeset generation v5-01 - Patches git tree
On 28 June 2013 17:10, Robert Haas robertmh...@gmail.com wrote: But to tell the truth, I'm mostly exercised about the non-unique syscache. I think that's simply a *bad* idea. +1. I don't think the extra index on pg_class is going to hurt that much, even if we create it always, as long as we use a purpose-built caching mechanism for it rather than forcing it through catcache. Hmm, does seem like that would be better. The people who are going to suffer are the ones who create and drop a lot of temporary tables, but even there I'm not sure how visible the overhead will be on real-world workloads, and maybe the solution is to work towards not having permanent catalog entries for temporary tables in the first place. In any case, hurting people who use temporary tables heavily seems better than adding overhead to every insert/update/delete operation, which will hit all users who are not read-only. Thinks... If we added a trigger that fired a NOTIFY for any new rows in pg_class that relate to non-temporary relations that would optimise away any overhead for temporary tables or when no changeset extraction was in progress. The changeset extraction could build a private hash table to perform the lookup and then LISTEN on a specific channel for changes. That might work better than an index-plus-syscache. On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. I don't really like the idea of requiring the relid on the WAL record. WAL is big enough already and we want people to turn this on, not avoid it. This is just an index lookup. We do them all the time without any fear of reliability issues. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] [RFC] Minmax indexes
On 6/17/13 3:38 PM, Josh Berkus wrote: Why? Why can't we just update the affected pages in the index? The page range has to be scanned in order to find out the min/max values for the indexed columns on the range; and then, with these data, update the index. Seems like you could incrementally update the range, at least for inserts. If you insert a row which doesn't decrease the min or increase the max, you can ignore it, and if it does increase/decrease, you can change the min/max. No? For updates, things are more complicated. If the row you're updating was the min/max, in theory you should update it to adjust that, but you can't verify that it was the ONLY min/max row without doing a full scan. My suggestion would be to add a dirty flag which would indicate that that block could use a rescan next VACUUM, and otherwise ignore changing the min/max. After all, the only defect to having min to low or max too high for a block would be scanning too many blocks. Which you'd do anyway with it marked invalid. If we add a dirty flag it would probably be wise to allow for more than one value so we can do a clock-sweep. That would allow for detecting a range that is getting dirtied repeatedly and not bother to try and re-summarize it until later. Something else I don't think was mentioned... re-summarization should be somehow tied to access activity: if a query will need to seqscan a segment that needs to be summarized, we should take that opportunity to summarize at the same time while pages are in cache. Maybe that can be done in the backend itself; maybe we'd want a separate process. -- Jim C. Nasby, Data Architect j...@nasby.net 512.569.9461 (cell) http://jim.nasby.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] Minmax indexes
On Fri, Jun 28, 2013 at 2:18 PM, Jim Nasby j...@nasby.net wrote: On 6/17/13 3:38 PM, Josh Berkus wrote: Why? Why can't we just update the affected pages in the index? The page range has to be scanned in order to find out the min/max values for the indexed columns on the range; and then, with these data, update the index. Seems like you could incrementally update the range, at least for inserts. If you insert a row which doesn't decrease the min or increase the max, you can ignore it, and if it does increase/decrease, you can change the min/max. No? For updates, things are more complicated. If the row you're updating was the min/max, in theory you should update it to adjust that, but you can't verify that it was the ONLY min/max row without doing a full scan. My suggestion would be to add a dirty flag which would indicate that that block could use a rescan next VACUUM, and otherwise ignore changing the min/max. After all, the only defect to having min to low or max too high for a block would be scanning too many blocks. Which you'd do anyway with it marked invalid. If we add a dirty flag it would probably be wise to allow for more than one value so we can do a clock-sweep. That would allow for detecting a range that is getting dirtied repeatedly and not bother to try and re-summarize it until later. Something else I don't think was mentioned... re-summarization should be somehow tied to access activity: if a query will need to seqscan a segment that needs to be summarized, we should take that opportunity to summarize at the same time while pages are in cache. Maybe that can be done in the backend itself; maybe we'd want a separate process. This smells a lot like hint bits and all the trouble they bring. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
On Fri, Jun 28, 2013 at 12:29 PM, Nicholas White n.j.wh...@gmail.com wrote: This patch will need to be rebased over those changes See attached - Thanks. But I see a few issues... + [respect nulls]|[ignore nulls] You fixed one of these but missed the other. replaceable class=parameterdefault/replaceable are evaluated with respect to the current row. If omitted, replaceable class=parameteroffset/replaceable defaults to 1 and - replaceable class=parameterdefault/replaceable to null + literalIGNORE NULLS/ is specified then the function will be evaluated + as if the rows containing nulls didn't exist. /entry /row This looks like you dropped a line during cut-and-paste. + null_values = (Bitmapset *) WinGetPartitionLocalMemory( + winobj, + BITMAPSET_SIZE(words_needed)); + Assert(null_values); This certainly seems ugly - isn't there a way to accomplish this without having to violate the Bitmapset abstraction? +* A slight edge case. Consider: +* +* A | lag(A, 1) +* 1 | +* 2 | 1 +* | ? pgindent will reformat this into oblivion. Use - markers around the comment block as done elsewhere in the code where this is an issue, or don't use ASCII art. I haven't really reviewed the windowing-related code in depth; I thought Jeff might jump back in for that part of it. Jeff, is that something you're planning to do? -- 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] Department of Redundancy Department: makeNode(FuncCall) division
On 6/28/13 11:30 AM, Robert Haas wrote: On Fri, Jun 28, 2013 at 10:31 AM, Tom Lane t...@sss.pgh.pa.us wrote: David Fetter da...@fetter.org writes: Please find attached the latest patch. I remain of the opinion that this is simply a bad idea. It is unlike our habits for constructing other types of nodes, and makes it harder not easier to find all the places that need to be updated when adding another field to FuncCall. I think it's a nice code cleanup. I don't understand your objection. Yeah, I was reading the patch thinking, yes, finally someone cleans that up. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Documentation/help for materialized and recursive views
On 6/28/13 10:49 AM, Alvaro Herrera wrote: Magnus Hagander escribió: They are already crosslinked under see also. But that doesn't really help the guy doing \h CREATE VIEW in psql, which was the case where it was brought to my attention. Maybe \h should somehow display the see also section? You can run \! man from within psql, which should give you everything you need. It's admittedly a bit obscure, but it's there. Maybe it ought to be added to the initial help output. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [RFC] Minmax indexes
On 6/28/13 12:26 PM, Claudio Freire wrote: On Fri, Jun 28, 2013 at 2:18 PM, Jim Nasby j...@nasby.net wrote: On 6/17/13 3:38 PM, Josh Berkus wrote: Why? Why can't we just update the affected pages in the index? The page range has to be scanned in order to find out the min/max values for the indexed columns on the range; and then, with these data, update the index. Seems like you could incrementally update the range, at least for inserts. If you insert a row which doesn't decrease the min or increase the max, you can ignore it, and if it does increase/decrease, you can change the min/max. No? For updates, things are more complicated. If the row you're updating was the min/max, in theory you should update it to adjust that, but you can't verify that it was the ONLY min/max row without doing a full scan. My suggestion would be to add a dirty flag which would indicate that that block could use a rescan next VACUUM, and otherwise ignore changing the min/max. After all, the only defect to having min to low or max too high for a block would be scanning too many blocks. Which you'd do anyway with it marked invalid. If we add a dirty flag it would probably be wise to allow for more than one value so we can do a clock-sweep. That would allow for detecting a range that is getting dirtied repeatedly and not bother to try and re-summarize it until later. Something else I don't think was mentioned... re-summarization should be somehow tied to access activity: if a query will need to seqscan a segment that needs to be summarized, we should take that opportunity to summarize at the same time while pages are in cache. Maybe that can be done in the backend itself; maybe we'd want a separate process. This smells a lot like hint bits and all the trouble they bring. Possibly; though if that's the case just having a second process do the work would probably eliminate most of that problem. -- Jim C. Nasby, Data Architect j...@nasby.net 512.569.9461 (cell) http://jim.nasby.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Documentation/help for materialized and recursive views
Peter Eisentraut escribió: On 6/28/13 10:49 AM, Alvaro Herrera wrote: Magnus Hagander escribió: They are already crosslinked under see also. But that doesn't really help the guy doing \h CREATE VIEW in psql, which was the case where it was brought to my attention. Maybe \h should somehow display the see also section? You can run \! man from within psql, which should give you everything you need. It's admittedly a bit obscure, but it's there. Maybe it ought to be added to the initial help output. That's a neat trick, but obviously it won't work in Windows. Maybe we ought to have \man .. -- Á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] [RFC] Minmax indexes
On Fri, Jun 28, 2013 at 2:18 PM, Jim Nasby j...@nasby.net wrote: If we add a dirty flag it would probably be wise to allow for more than one value so we can do a clock-sweep. That would allow for detecting a range that is getting dirtied repeatedly and not bother to try and re-summarize it until later. Given that I'm talking about doing the resummarization at VACUUM time, I don't think that's justified. That only makes sense if you're doing the below ... Something else I don't think was mentioned... re-summarization should be somehow tied to access activity: if a query will need to seqscan a segment that needs to be summarized, we should take that opportunity to summarize at the same time while pages are in cache. Maybe that can be done in the backend itself; maybe we'd want a separate process. Writing at SELECT time is something our users hate, with significant justification. This suggestion is possibly a useful optimization, but I'd like to see the simplest possible version of minmax indexes go into 9.4, so that we can see how they work in practice before we start adding complicated performance optimizations involving extra write overhead and background workers. We're more likely to engineer an expensive de-optimization that way. I wouldn't be unhappy to have the very basic minmax indexes -- the one where we just flag pages as invalid if they get updated -- go into 9.4. That would still work for large, WORM tables, which are a significant and pervasive use case. If Alvaro gets to it, I think the updating the range, rescan at VACUUM time approach isn't much more complex, but if he doesn't I'd still vote to accept the feature. -- 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] Documentation/help for materialized and recursive views
On Fri, Jun 28, 2013 at 01:34:17PM -0400, Peter Eisentraut wrote: On 6/28/13 10:49 AM, Alvaro Herrera wrote: Magnus Hagander escribió: They are already crosslinked under see also. But that doesn't really help the guy doing \h CREATE VIEW in psql, which was the case where it was brought to my attention. Maybe \h should somehow display the see also section? You can run \! man from within psql, And if you're on Windows, you're Sadly Out of Luck with that. Is there an equivalent we could #ifdef in for that platform? 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] fallocate / posix_fallocate for new WAL file creation (etc...)
On 06/20/2013 07:19 PM, Jeff Davis wrote: On Sun, 2013-06-16 at 23:53 -0500, Jon Nelson wrote: Please find attached v5 of the patch, with the above correction in place. The only change from the v4 patch is wrapping the if (wal_use_fallocate) block in an #ifdef USE_POSIX_FALLOCATE. Thanks! Thank you. Greg, are you still working on those tests? Since Greg seems to be busy, what needs to be done to test this? -- 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.3 CF 1] 2 Weeks In The problem with Performance Patches
Folks, We are starting the 3rd week of the commitfest tommorrow, and here's the update on status. In the last week, we are at the following status levels: - 40 patches still Need Review, including 11 which have no reviewer assigned. - 23 patches are Waiting on Author, most of them because they are still under discussion on this list. - 17 patches are marked Ready for Committer, including 10 of the 11 regression tests* - 17 patches have been Committed - 9 patches have been Returned with Feedback - 1 patch has been Rejected (* the tests actually need a test of the cumulative total time to run, which I'll be working on today) At current commit/return rates, this commifest will finish around July 28th. What seems prepared to drag out this commitfest possibly beyond the end of July are the several performance optimization patches, which include: - Remove PD_ALL_VISIBLE** - Improvement of checkpoint IO scheduler for stable transaction responses - Use posix_fallocate to build (new) WAL files** - Performance Improvement by reducing WAL for Update Operation** - GIN improvements (4 patches)** - XLogInsert scaling Items marked ** are waiting on review. The problem with these patches is that (a) people feel reluctant to review them, since not too many hackers feel qualified to review performance, and (b) the review will need to include performance testing, which will make it take longer and likely bottleneck on qualified testers. So, two things: (1) if you are qualified to review any of the above, please get started *now*, and leave the non-performance patches for later; (2) ideas on how we can speed up/parallelize performance testing efforts are extremely welcome. -- 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] [9.3 CF 1] 2 Weeks In The problem with Performance Patches
On 2013-06-28 12:16:09 -0700, Josh Berkus wrote: - Remove PD_ALL_VISIBLE** I think this is primarily delayed because people don't agree it's a good idea. - Use posix_fallocate to build (new) WAL files** I don't think this needs much further testing. We should just remove the enable/disable knob and go ahead. - XLogInsert scaling I don't see to many problems here so far. For a complex feature it's imo already in a pretty good shape. Some more eyes on the code would be good, given it touching lots of code that's central for crash safety, but if not, I don't think it will block things. 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] [9.3 CF 1] 2 Weeks In The problem with Performance Patches
On Fri, Jun 28, 2013 at 4:16 PM, Josh Berkus j...@agliodbs.com wrote: (2) ideas on how we can speed up/parallelize performance testing efforts are extremely welcome. An official perf-test script in GIT, even if it only tests general pg-bench-like performance, that can take two builds and compare them, like unladen-swallow's perf.py[0][1], would enable regular folk perform the testing on various hardware configurations and contribute their results more easily. Results would be in standardized format, which would help on the interpretation of those numbers, and patches would also be able to add patch-specific tests to the script's battery. I had a bash script that ran a few tests I used when evaluating the readahead patch. It's very very green, and very very obvious, so I doubt it'd be useful, but if noone else has one, I could dig through my backups. The test handled the odd stuff about clearing the OS cache between test runs, and stuff like that, which is required for meaningful results. I think this is where an official perf test script can do wonders: accumulate and share knowledge on testing methodology. [0] http://code.google.com/p/unladen-swallow/wiki/Benchmarks [1] http://code.google.com/p/unladen-swallow/source/browse/tests/perf.py -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GIN improvements part2: fast scan
On Tue, Jun 25, 2013 at 2:20 AM, Alexander Korotkov aekorot...@gmail.comwrote: 4. If we do go with a new function, I'd like to just call it consistent (or consistent2 or something, to keep it separate form the old consistent function), and pass it a tri-state input for each search term. It might not be any different for the full-text search implementation, or any of the other ones for that matter, but I think it would be a more understandable API. Understandable API makes sense. But for now, I can't see even potentional usage of third state (exact false). Typo here. I meant exact true. Also, with preConsistent interface as is in some cases we can use old consistent method as both consistent and preConsistent when it implements monotonous boolean function. For example, it's consistent function for opclasses of arrays. Now, I got the point of three state consistent: we can keep only one consistent in opclasses that support new interface. exact true and exact false values will be passed in the case of current patch consistent; exact false and unknown will be passed in the case of current patch preConsistent. That's reasonable. -- With best regards, Alexander Korotkov.
Re: [HACKERS] Add more regression tests for ASYNC
On 05/13/2013 05:37 PM, Robins Tharakan wrote: Hi, Patch to add more regression tests to ASYNC (/src/backend/commands/async). Take code-coverage from 39% to 75%. This isn't applying for me anymore due to changes in the parallel_schedule file. Where did you mean this to execute in the sequence of tests? --- src/test/regress/parallel_schedule +++ src/test/regress/parallel_schedule @@ -88,7 +88,7 @@ # -- # Another group of parallel tests # -- -test: alter_generic misc psql +test: alter_generic misc psql async # rules cannot run concurrently with any test that creates a view test: rules -- 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] changeset generation v5-01 - Patches git tree
On 2013-06-28 12:26:52 -0400, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On the other hand, I can't entirely shake the feeling that adding the information into WAL would be more reliable. That feeling has been nagging at me too. I can't demonstrate that there's a problem when an ALTER TABLE is in process of rewriting a table into a new relfilenode number, but I don't have a warm fuzzy feeling about the reliability of reverse lookups for this. I am pretty sure the mapping thing works, but it indeed requires some complexity. And it's harder to debug because when you want to understand what's going on the relfilenodes involved aren't in the catalog anymore. At the very least it's going to require some hard-to-verify restriction about how we can't start doing changeset reconstruction in the middle of a transaction that's doing DDL. Currently changeset extraction needs to wait (and does so) till it found a point where it has seen the start of all in-progress transactions. All transaction that *commit* after the last partiall observed in-progress transaction finished can be decoded. To make that point visible for external tools to synchronize - e.g. pg_dump - it exports the snapshot of exactly the moment when that last in-progress transaction committed. So, from what I gather there's a slight leaning towards *not* storing the relation's oid in the WAL. Which means the concerns about the uniqueness issues with the syscaches need to be addressed. So far I know of three solutions: 1) develop a custom caching/mapping module 2) Make sure InvalidOid's (the only possible duplicate) can't end up the syscache by adding a hook that prevents that on the catcache level 3) Make sure that there can't be any duplicates by storing the oid of the relation in a mapped relations relfilenode Opinions? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add more regression tests for ASYNC
On 06/28/2013 12:42 PM, Josh Berkus wrote: On 05/13/2013 05:37 PM, Robins Tharakan wrote: Hi, Patch to add more regression tests to ASYNC (/src/backend/commands/async). Take code-coverage from 39% to 75%. This isn't applying for me anymore due to changes in the parallel_schedule file. Where did you mean this to execute in the sequence of tests? Never mind, dirty checkout on my part. Back to testing. -- 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] [PATCH] Fix conversion for Decimal arguments in plpython functions
On 06/27/2013 05:04 AM, Szymon Guz wrote:
On 27 June 2013 05:21, Steve Singer st...@ssinger.info
mailto:st...@ssinger.info wrote:
On 06/26/2013 04:47 PM, Szymon Guz wrote:
Hi Steve,
thanks for the changes.
You're idea about common code for decimal and cdecimal is good,
however not good enough. I like the idea of common code for decimal
and cdecimal. But we need class name, not the value.
I've changed the code from str(x) to x.__class__.__name__ so the
function prints class name (which is Decimal for both packages), not
the value. We need to have the class name check. The value is returned
by the function and is a couple of lines lower in the file.
patch is attached.
I think the value is more important than the name, I want to the tests
to make sure that the conversion is actually converting properly. With
your method of getting the class name without the module we can have both.
The attached patch should print the value and the class name but not the
module name.
Steve
thanks,
Szymon
diff --git a/doc/src/sgml/plpython.sgml b/doc/src/sgml/plpython.sgml
new file mode 100644
index aaf758d..da27874
*** a/doc/src/sgml/plpython.sgml
--- b/doc/src/sgml/plpython.sgml
*** $$ LANGUAGE plpythonu;
*** 308,321
/para
/listitem
listitem
para
!PostgreSQL typereal/type, typedouble/type,
!and typenumeric/type are converted to
!Python typefloat/type. Note that for
!the typenumeric/type this loses information and can lead to
!incorrect results. This might be fixed in a future
!release.
/para
/listitem
--- 308,326
/para
/listitem
+ listitem
+ para
+PostgreSQL typereal/type and typedouble/type are converted to
+Python typefloat/type.
+ /para
+ /listitem
+
listitem
para
!PostgreSQL typenumeric/type is converted to
!Python typeDecimal/type. This type is imported from
! literalcdecimal/literal package if it is available. If cdecimal
! cannot be used, then literaldecimal.Decimal/literal will be used.
/para
/listitem
diff --git a/src/pl/plpython/expected/plpython_types.out b/src/pl/plpython/expected/plpython_types.out
new file mode 100644
index 4641345..e602336
*** a/src/pl/plpython/expected/plpython_types.out
--- b/src/pl/plpython/expected/plpython_types.out
*** CONTEXT: PL/Python function test_type_
*** 213,248
(1 row)
CREATE FUNCTION test_type_conversion_numeric(x numeric) RETURNS numeric AS $$
! plpy.info(x, type(x))
return x
$$ LANGUAGE plpythonu;
! /* The current implementation converts numeric to float. */
SELECT * FROM test_type_conversion_numeric(100);
! INFO: (100.0, type 'float')
CONTEXT: PL/Python function test_type_conversion_numeric
test_type_conversion_numeric
--
! 100.0
(1 row)
SELECT * FROM test_type_conversion_numeric(-100);
! INFO: (-100.0, type 'float')
CONTEXT: PL/Python function test_type_conversion_numeric
test_type_conversion_numeric
--
!-100.0
(1 row)
SELECT * FROM test_type_conversion_numeric(50.5);
! INFO: (50.5, type 'float')
CONTEXT: PL/Python function test_type_conversion_numeric
test_type_conversion_numeric
--
50.5
(1 row)
SELECT * FROM test_type_conversion_numeric(null);
! INFO: (None, type 'NoneType')
CONTEXT: PL/Python function test_type_conversion_numeric
test_type_conversion_numeric
--
--- 213,264
(1 row)
CREATE FUNCTION test_type_conversion_numeric(x numeric) RETURNS numeric AS $$
! plpy.info(x,x.__class__.__name__)
return x
$$ LANGUAGE plpythonu;
! /* The current implementation converts numeric to Decimal. */
SELECT * FROM test_type_conversion_numeric(100);
! INFO: (Decimal('100'), 'Decimal')
CONTEXT: PL/Python function test_type_conversion_numeric
test_type_conversion_numeric
--
! 100
(1 row)
SELECT * FROM test_type_conversion_numeric(-100);
! INFO: (Decimal('-100'), 'Decimal')
CONTEXT: PL/Python function test_type_conversion_numeric
test_type_conversion_numeric
--
! -100
(1 row)
SELECT * FROM test_type_conversion_numeric(50.5);
! INFO: (Decimal('50.5'), 'Decimal')
CONTEXT: PL/Python function test_type_conversion_numeric
test_type_conversion_numeric
--
50.5
(1 row)
+ SELECT * FROM test_type_conversion_numeric(1234567890.0987654321);
+ INFO: (Decimal('1234567890.0987654321'), 'Decimal')
+ CONTEXT: PL/Python function test_type_conversion_numeric
+ test_type_conversion_numeric
+
Re: FILTER for aggregates [was Re: [HACKERS] Department of Redundancy Department: makeNode(FuncCall) division]
On 21 June 2013 06:16, David Fetter da...@fetter.org wrote: Please find attached a patch which allows subqueries in the FILTER clause and adds regression testing for same. This needs re-basing/merging following Robert's recent commit to make OVER unreserved. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Fix conversion for Decimal arguments in plpython functions
On 28 June 2013 22:14, Steve Singer st...@ssinger.info wrote: I think the value is more important than the name, I want to the tests to make sure that the conversion is actually converting properly. With your method of getting the class name without the module we can have both. The attached patch should print the value and the class name but not the module name. Steve Hi Steve, I agree, we can check both. This is quite a nice patch now, I've reviewed it, all tests pass, works as expected. I think it is ready for committing. thanks, Szymon
Re: [HACKERS] [PATCH] Fix conversion for Decimal arguments in plpython functions
On Fri, Jun 28, 2013 at 5:14 PM, Steve Singer st...@ssinger.info wrote: On 06/27/2013 05:04 AM, Szymon Guz wrote: On 27 June 2013 05:21, Steve Singer st...@ssinger.info mailto:st...@ssinger.info wrote: On 06/26/2013 04:47 PM, Szymon Guz wrote: Hi Steve, thanks for the changes. You're idea about common code for decimal and cdecimal is good, however not good enough. I like the idea of common code for decimal and cdecimal. But we need class name, not the value. I've changed the code from str(x) to x.__class__.__name__ so the function prints class name (which is Decimal for both packages), not the value. We need to have the class name check. The value is returned by the function and is a couple of lines lower in the file. patch is attached. I think the value is more important than the name, I want to the tests to make sure that the conversion is actually converting properly. With your method of getting the class name without the module we can have both. The attached patch should print the value and the class name but not the module name. Why not forego checking of the type, and instead check the interface? plpy.info(x.as_tuple()) Should do. d = decimal.Decimal((0,(3,1,4),-2)) d.as_tuple() DecimalTuple(sign=0, digits=(3, 1, 4), exponent=-2) d.as_tuple() == (0,(3,1,4),-2) True d = decimal.Decimal(3.14) d.as_tuple() DecimalTuple(sign=0, digits=(3, 1, 4), exponent=-2) d.as_tuple() == (0,(3,1,4),-2) True -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PostgreSQL 9.3 latest dev snapshot
On 06/28/2013 06:51 PM, Alvaro Herrera wrote: Magnus Hagander escribió: However, if oyu're looking for a snapshot, please use the one on the ftpsite. Generating those snapshots on the git server is slow and expensive... Maybe we should redirect those gitweb snapshot URLs to the FTP site? maybe - but I can actually see the (rare) usecase of being able to create a snapshot on a per-commit base, so redirecting to something that is more of a basic verified snapshot tarball once a day seems wrong to me, despite the fact that I think that using those is a better idea in general. 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] Add some regression tests for SEQUENCE
On 05/07/2013 03:40 PM, Robins Tharakan wrote: Hi, Have provided an updated patch as per Fabien's recent response on Commitfest site. Any and all feedback is appreciated. The updated patch is giving a FAILURE for me: parallel group (19 tests): limit temp plancache conversion rowtypes prepare without_oid copy2 xml returning rangefuncs polymorphism with domain truncate largeobject sequence alter_table plpgsql plancache... ok limit... ok plpgsql ... ok copy2... ok temp ... ok domain ... ok rangefuncs ... ok prepare ... ok without_oid ... ok conversion ... ok truncate ... ok alter_table ... ok sequence ... FAILED Thoughts? -- 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] Add more regression tests for dbcommands
Hi Andres, Just an aside, your point about CONNECTION LIMIT was something that just didn't come to my mind and is probably a good way to test ALTER DATABASE with CONNECTION LIMIT. Its just that that actually wasn't what I was testing there. That 'CONNECTION LIMIT' test was coupled with CREATE DATABASE because I wanted to test that 'branch' in the CREATE DATABASE function just to ensure that there was a regression test that tests CONNECTION LIMIT specifically with CREATE DATABASE. That's all. A check to confirm whether connection limit restrictions actually got enforced was something I missed, but well, its out of the window for now anyway. -- Robins Tharakan On 26 June 2013 06:34, Andres Freund and...@2ndquadrant.com wrote: Hi, I am generally a bit unsure whether the regression tests you propose aren't a bit too verbose. Does any of the committers have an opinion about this? My feeling is that they are ok if they aren't slowing down things much. On 2013-06-26 01:55:53 -0500, Robins Tharakan wrote: The CREATE DATABASE test itself was checking whether the 'CONNECTION LIMIT' was working. Removed that as well. You should be able to test that by setting the connection limit to 1 and then try to connect using \c. The old connection is only dropped after the new one has been successfully performed. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
[HACKERS] New regression test time
Hackers, Per discussion on these tests, I ran make check against 9.4 head, applied all of the regression tests other than DISCARD. Time for 3 make check runs without new tests: 65.9s Time for 3 make check runs with new tests: 71.7s So that's an increase of about 10% in test runtime (or 2 seconds per run on my laptop), in order to greatly improve regression test coverage. I'd say that splitting the tests is not warranted, and that we should go ahead with these tests on their testing merits, not based on any extra check time they might add. -- 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] Request for Patch Feedback: Lag Lead Window Functions Can Ignore Nulls
I've fixed the problems you mentioned (see attached) - sorry, I was a bit careless with the docs. + null_values = (Bitmapset *) WinGetPartitionLocalMemory( + winobj, + BITMAPSET_SIZE(words_needed)); + Assert(null_values); This certainly seems ugly - isn't there a way to accomplish this without having to violate the Bitmapset abstraction? Indeed, it's ugly. I've revised it slightly to: null_values = (Bitmapset *) WinGetPartitionLocalMemory( winobj, BITMAPSET_SIZE(words_needed)); null_values-nwords = (int) words_needed; ...which gives a proper bitmap. It's hard to break this into a factory method like bms_make_singleton as I'm getting the (zero'ed) partition local memory from one call, then forcing a correct bitmap's structure on it. Maybe bitmapset.h needs an bms_initialise(void *, int num_words) factory method? You'd still have to use the BITMAPSET_SIZE macro to get the correct amount of memory for the void*. Maybe the factory method could take a function pointer that would allocate memory of the given size (e.g. Bitmapset* initialize(void* (allocator)(Size_t), int num_words) ) - this means I could still use the partition's local memory. I don't think the solution would be tidier if I re-instated the leadlag_context struct with a single Bitmapset member. Other Bitmapset usage seems to just call bms_make_singleton then bms_add_member over and over again - which afaict will call palloc every BITS_PER_BITMAPWORD calls, which is not really what I want. Thanks - Nick lead-lag-ignore-nulls.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add some regression tests for SEQUENCE
Seems like thats because of a recent (15th May 2013) patch (b14206862278347a379f2bb72d92d16fb9dcea45) that changed the error message that is printed. Its just one line of difference actually. Let me know if this is otherwise good to go. I'll checkout the latest revision and submit this patch again. -- Robins Tharakan On 28 June 2013 15:53, Josh Berkus j...@agliodbs.com wrote: On 05/07/2013 03:40 PM, Robins Tharakan wrote: Hi, Have provided an updated patch as per Fabien's recent response on Commitfest site. Any and all feedback is appreciated. The updated patch is giving a FAILURE for me: parallel group (19 tests): limit temp plancache conversion rowtypes prepare without_oid copy2 xml returning rangefuncs polymorphism with domain truncate largeobject sequence alter_table plpgsql plancache... ok limit... ok plpgsql ... ok copy2... ok temp ... ok domain ... ok rangefuncs ... ok prepare ... ok without_oid ... ok conversion ... ok truncate ... ok alter_table ... ok sequence ... FAILED Thoughts? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Re: [HACKERS] Add some regression tests for SEQUENCE
On 06/28/2013 02:15 PM, Robins Tharakan wrote: Seems like thats because of a recent (15th May 2013) patch (b14206862278347a379f2bb72d92d16fb9dcea45) that changed the error message that is printed. Its just one line of difference actually. Let me know if this is otherwise good to go. I'll checkout the latest revision and submit this patch again. I was only checking test timing, per my earlier email. I haven't looked at the tests themselves at all. -- 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] Add some regression tests for SEQUENCE
On 06/28/2013 02:15 PM, Robins Tharakan wrote: Seems like thats because of a recent (15th May 2013) patch (b14206862278347a379f2bb72d92d16fb9dcea45) that changed the error message that is printed. Its just one line of difference actually. Let me know if this is otherwise good to go. I'll checkout the latest revision and submit this patch again. I was only checking test timing, per my earlier email. I haven't looked at the tests themselves at all. -- 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] New regression test time
On 2013-06-28 14:01:23 -0700, Josh Berkus wrote: Per discussion on these tests, I ran make check against 9.4 head, applied all of the regression tests other than DISCARD. Time for 3 make check runs without new tests: 65.9s Time for 3 make check runs with new tests: 71.7s So that's an increase of about 10% in test runtime (or 2 seconds per run on my laptop), in order to greatly improve regression test coverage. How did you evaluate that coverage increased greatly? I am not generally against these tests but I'd be surprised if the overall test coverage improved noticeably by this. Which makes 10% runtime overhead pretty hefty if the goal is to actually achieve a high coverage. 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] New regression test time
How did you evaluate that coverage increased greatly? I am not generally against these tests but I'd be surprised if the overall test coverage improved noticeably by this. Which makes 10% runtime overhead pretty hefty if the goal is to actually achieve a high coverage. I was relying on Robins' numbers of coverage: Patch to add more regression tests to ASYNC (/src/backend/commands/async). Take code-coverage from 39% to 75%. Please find attached a patch to take code-coverage of ALTER OPERATOR FAMILY.. ADD / DROP (src/backend/commands/opclasscmds.c) from 50% to 87%. Please find attached a patch to take code-coverage of LOCK TABLE ( src/backend/commands/lockcmds.c) from 57% to 84%. ... etc. If we someday add so many tests that make check takes over a minute on a modern laptop, then maybe it'll be worth talking about splitting the test suite into regular and extended. However, it would require 15 more patch sets the size of Robins' to get there, so I don't see that it's worth the trouble any time soon. -- 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: backend hangs at immediate shutdown (Re: [HACKERS] Back-branch update releases coming in a couple weeks)
MauMau escribió: Hi, I did this. Please find attached the revised patch. I modified HandleChildCrash(). I tested the immediate shutdown, and the child cleanup succeeded. Thanks, committed. There are two matters pending here: 1. do we want postmaster to exit immediately after sending the SIGKILL, or hang around until all children are gone? 2. how far do we want to backpatch this, if at all? -- Á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] [GENERAL] pg_upgrade -u
On Tue, May 28, 2013 at 09:08:03PM -0700, Joshua D. Drake wrote: On 05/28/2013 07:55 PM, Bruce Momjian wrote: Perhaps just documenting the behavior is all that is needed, but -U is everywhere and I think that's a good thing. [ moved to hacker ] Wow, I never realized other tools used -U for user, instead of -u. Should I change pg_upgrade to use -U for 9.4? I can keep supporting -u as an undocumented option. Yes, -U makes the most sense as that is what is used with the other tools. I think you should just drop -u, this isn't something people are doing everyday (like psql). The backward compatibility argument is pretty slim. Done for 9.4. I also simplified some of the option values in --help and the docs. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New regression test time
On 2013-06-28 14:46:10 -0700, Josh Berkus wrote: How did you evaluate that coverage increased greatly? I am not generally against these tests but I'd be surprised if the overall test coverage improved noticeably by this. Which makes 10% runtime overhead pretty hefty if the goal is to actually achieve a high coverage. I was relying on Robins' numbers of coverage: Those improvements rather likely end up being an improvement a good bit less than one percent for the whole binary. If we someday add so many tests that make check takes over a minute on a modern laptop, then maybe it'll be worth talking about splitting the test suite into regular and extended. However, it would require 15 more patch sets the size of Robins' to get there, so I don't see that it's worth the trouble any time soon. Was it actually an assert enabled build that you tested? We currently can run make check with stuff like CLOBBER_CACHE_ALWAYS which finds bugs pretty regularly. If we achieve a high coverage we quite possibly can't anymore, at least not regularly. So I actually think having two modes makes sense. Then we could also link stuff like isolationtester automatically into the longer running mode... 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] [GENERAL] pg_upgrade -u
On Wed, May 29, 2013 at 09:44:00AM -0400, Peter Eisentraut wrote: On 5/28/13 10:55 PM, Bruce Momjian wrote: Wow, I never realized other tools used -U for user, instead of -u. Should I change pg_upgrade to use -U for 9.4? I can keep supporting -u as an undocumented option. It seems to me that that option shouldn't be necessary anyway. pg_upgrade should somehow be able to find out by itself what the superuser of the old cluster was. Uh, any idea how to do that? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: backend hangs at immediate shutdown (Re: [HACKERS] Back-branch update releases coming in a couple weeks)
On Fri, Jun 28, 2013 at 6:00 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: MauMau escribió: Hi, I did this. Please find attached the revised patch. I modified HandleChildCrash(). I tested the immediate shutdown, and the child cleanup succeeded. Thanks, committed. There are two matters pending here: 1. do we want postmaster to exit immediately after sending the SIGKILL, or hang around until all children are gone? 2. how far do we want to backpatch this, if at all? Considering that neither Tom nor I was convinced that this was a good idea AT ALL, I'm surprised you committed it in the first place. I'd be more inclined to revert it than back-patch it, but at least if we only change it in HEAD we have some chance of finding out whether or not it is in fact a bad idea before it's too late to change our mind. -- 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] psql and pset without any arguments
Hi,
I was looking at psql 8.3 documention about \pset options and saw that
there was the following note :
Note: It is an error to call \pset without any arguments. In the
future this case might show the current status of all printing options.
I looked backward and forward to find that this note is present in all
versions since 7.1 up to 9.3, maybe it is time to add this little feature.
I've attached a patch to add the usage of the \pset command without any
arguments to displays current status of all printing options instead of
the error message. Here is a sample output:
(postgres@[local]:5494) [postgres] \pset
Output format is aligned.
Border style is 2.
Expanded display is used automatically.
Null display is NULL.
Field separator is |.
Tuples only is off.
Title is unset.
Table attributes unset.
Line style is unicode.
Pager is used for long output.
Record separator is newline.
(postgres@[local]:5494) [postgres]
To avoid redundant code I've added a new method printPsetInfo() so that
do_pset() and exec_command() will used the same output message, they are
all in src/bin/psql/command.c. For example:
(postgres@[local]:5494) [postgres] \pset null 'NULL'
Null display is NULL.
(postgres@[local]:5494) [postgres]
The patch print all variables information from struct printTableOpt when
\pset is given without any arguments and also update documentation.
Let me know if there's any additional work to do on this basic patch or
something that I've omitted.
Best regards,
--
Gilles Darold
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 574db5c..a3bf555 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2272,13 +2272,9 @@ lo_import 152801
/para
/tip
-note
-para
-It is an error to call command\pset/command without any
-arguments. In the future this case might show the current status
-of all printing options.
+paracommand\pset/ without any arguments displays current status
+ of all printing options.
/para
-/note
/listitem
/varlistentry
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 351e684..daf7ac7 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -68,6 +68,7 @@ static int strip_lineno_from_funcdesc(char *func);
static void minimal_error_message(PGresult *res);
static void printSSLInfo(void);
+static bool printPsetInfo(const char *param, struct printQueryOpt *popt);
#ifdef WIN32
static void checkWin32Codepage(void);
@@ -1045,8 +1046,16 @@ exec_command(const char *cmd,
if (!opt0)
{
- psql_error(\\%s: missing required argument\n, cmd);
- success = false;
+ size_t i;
+ /* list all variables */
+ static const char *const my_list[] = {format, border,
+expanded, null, fieldsep, tuples_only, title,
+tableattr, linestyle, pager, recordsep, NULL};
+ for (i = 0; my_list[i] != NULL; i++) {
+printPsetInfo(my_list[i], pset.popt);
+ }
+
+ success = true;
}
else
success = do_pset(opt0, opt1, pset.popt, pset.quiet);
@@ -2275,8 +2284,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
return false;
}
- if (!quiet)
- printf(_(Output format is %s.\n), _align2string(popt-topt.format));
}
/* set table line style */
@@ -2295,10 +2302,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
psql_error(\\pset: allowed line styles are ascii, old-ascii, unicode\n);
return false;
}
-
- if (!quiet)
- printf(_(Line style is %s.\n),
- get_line_style(popt-topt)-name);
}
/* set border style/width */
@@ -2306,9 +2309,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
{
if (value)
popt-topt.border = atoi(value);
-
- if (!quiet)
- printf(_(Border style is %d.\n), popt-topt.border);
}
/* set expanded/vertical mode */
@@ -2320,15 +2320,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
popt-topt.expanded = ParseVariableBool(value);
else
popt-topt.expanded = !popt-topt.expanded;
- if (!quiet)
- {
- if (popt-topt.expanded == 1)
-printf(_(Expanded display is on.\n));
- else if (popt-topt.expanded == 2)
-printf(_(Expanded display is used automatically.\n));
- else
-printf(_(Expanded display is off.\n));
- }
}
/* locale-aware numeric output */
@@ -2338,13 +2329,6 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
popt-topt.numericLocale = ParseVariableBool(value);
else
popt-topt.numericLocale = !popt-topt.numericLocale;
- if (!quiet)
- {
- if (popt-topt.numericLocale)
-puts(_(Showing locale-adjusted numeric output.));
- else
-
Re: [HACKERS] [GENERAL] pg_upgrade -u
On Thu, May 30, 2013 at 08:42:21AM -0400, Ray Stell wrote:
On May 29, 2013, at 11:07 AM, Bruce Momjian wrote:
On Wed, May 29, 2013 at 08:59:42AM -0400, Ray Stell wrote:
[ moved to hacker ] The question is whether hard-wiring these
helps more than it hurts, and which ones should be hard-wired.
I seems to me that superuser is exactly that special case and that if
an alternate superuser is hardwired in the src cluster then -u/-U and
that specific value will be required on both sides of pg_upgrade, no
variability is needed and perhaps not possible. You're point is well
taken for port.
You have convinced me. The attached, applied patch for PG 9.4 passes
the command-line-supplied username into the created analyze script.
I have also attached a sample output analyze script.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/contrib/pg_upgrade/check.c b/contrib/pg_upgrade/check.c
new file mode 100644
index 1f67e60..0376fcb
*** a/contrib/pg_upgrade/check.c
--- b/contrib/pg_upgrade/check.c
*** void
*** 459,464
--- 459,471
create_script_for_cluster_analyze(char **analyze_script_file_name)
{
FILE *script = NULL;
+ char *user_specification = ;
+
+ if (os_info.user_specified)
+ {
+ user_specification = pg_malloc(strlen(os_info.user) + 7);
+ sprintf(user_specification, -U \%s\ , os_info.user);
+ }
*analyze_script_file_name = pg_malloc(MAXPGPATH);
*** create_script_for_cluster_analyze(char *
*** 501,507
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %sthis script and run:%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, echo %s\%s/vacuumdb\ --all %s%s\n, ECHO_QUOTE, new_cluster.bindir,
/* Did we copy the free space files? */
(GET_MAJOR_VERSION(old_cluster.major_version) = 804) ?
--analyze-only : --analyze, ECHO_QUOTE);
--- 508,515
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %sthis script and run:%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, echo %s\%s/vacuumdb\ %s--all %s%s\n, ECHO_QUOTE,
! new_cluster.bindir, user_specification,
/* Did we copy the free space files? */
(GET_MAJOR_VERSION(old_cluster.major_version) = 804) ?
--analyze-only : --analyze, ECHO_QUOTE);
*** create_script_for_cluster_analyze(char *
*** 522,528
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %s--%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, \%s/vacuumdb\ --all --analyze-only\n, new_cluster.bindir);
fprintf(script, echo%s\n, ECHO_BLANK);
fprintf(script, echo %sThe server is now available with minimal optimizer statistics.%s\n,
ECHO_QUOTE, ECHO_QUOTE);
--- 530,537
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %s--%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, \%s/vacuumdb\ %s--all --analyze-only\n,
! new_cluster.bindir, user_specification);
fprintf(script, echo%s\n, ECHO_BLANK);
fprintf(script, echo %sThe server is now available with minimal optimizer statistics.%s\n,
ECHO_QUOTE, ECHO_QUOTE);
*** create_script_for_cluster_analyze(char *
*** 543,549
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %s---%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, \%s/vacuumdb\ --all --analyze-only\n, new_cluster.bindir);
fprintf(script, echo%s\n\n, ECHO_BLANK);
#ifndef WIN32
--- 552,559
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %s---%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, \%s/vacuumdb\ %s--all --analyze-only\n,
! new_cluster.bindir, user_specification);
fprintf(script, echo%s\n\n, ECHO_BLANK);
#ifndef WIN32
*** create_script_for_cluster_analyze(char *
*** 556,562
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %s-%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, \%s/vacuumdb\ --all %s\n, new_cluster.bindir,
/* Did we copy the free space files? */
(GET_MAJOR_VERSION(old_cluster.major_version) = 804) ?
--analyze-only : --analyze);
--- 566,573
ECHO_QUOTE, ECHO_QUOTE);
fprintf(script, echo %s-%s\n,
ECHO_QUOTE, ECHO_QUOTE);
! fprintf(script, \%s/vacuumdb\ %s--all %s\n, new_cluster.bindir,
! user_specification,
/* Did we copy the free space files? */
(GET_MAJOR_VERSION(old_cluster.major_version) = 804) ?
--analyze-only : --analyze);
*** create_script_for_cluster_analyze(char *
*** 573,578
--- 584,592
*analyze_script_file_name, getErrorText(errno));
#endif
+ if (os_info.user_specified)
+
Re: [HACKERS] [NOVICE] index refuses to build
On Sun, Aug 26, 2012 at 09:47:01AM -0400, Bruce Momjian wrote: On Thu, Dec 29, 2011 at 10:40:19PM -0500, Tom Lane wrote: Merlin Moncure mmonc...@gmail.com writes: On Thu, Dec 29, 2011 at 5:10 PM, Jean-Yves F. Barbier 12u...@gmail.com wrote: CREATE INDEX tst1m_name_lu_ix ON tst1m(unaccent(name)); ERROR: functions in index expression must be marked IMMUTABLE your problem is the unaccent function. it's defined stable because the rules function it depends on can change after the index is built -- that would effectively introduce index corruption. it's possible to bypass that restriction, but are you sure that's what you want to do? Hmm ... it's clear why unaccent(text) is only stable, because it depends on the current search_path to find the unaccent dictionary. But I wonder whether it was an oversight that unaccent(regdictionary, text) is stable and not immutable. We don't normally mark functions as stable just because you could in principle change their behavior by altering some outside-the-database configuration files. Should we change the function signature for unaccent(regdictionary, text)? Did we decide not to do this? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor inheritance/check bug: Inconsistent behavior
On Sat, Jan 26, 2013 at 11:31:49AM +0530, Amit Kapila wrote: On Friday, January 25, 2013 8:36 PM Bruce Momjian wrote: On Fri, Sep 14, 2012 at 02:04:51PM +, Amit kapila wrote: On Thu, 6 Sep 2012 14:50:05 -0400 Robert Hass wrote: On Tue, Aug 28, 2012 at 6:40 AM, Amit Kapila amit(dot)kapila(at)huawei(dot) com wrote: AFAICT during Update also, it doesn't contain useful. The only chance it would have contain something useful is when it goes for EvalPlanQual and then again comes to check for constraints. However these attributes get filled in heap_update much later. So now should the fix be that it returns an error for system column reference except for OID case? +1. 1. I think in this scenario the error for system column except for tableOID should be thrown at Create/Alter time. 2. After setting OID in ExecInsert/ExecUpdate may be setting of same inside heap functions can be removed. But for now I have kept them as it is. Please find the Patch for bug-fix. If this is okay, I shall send you the test cases for same. Did we ever make any progress on this? I have done the initial analysis and prepared a patch, don't know if anything more I can do until someone can give any suggestions to further proceed on this bug. So, I guess we never figured this out. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COPY and Volatile default expressions
On 24 June 2013 10:21, Kohei KaiGai kai...@kaigai.gr.jp wrote: Hi Simon, I checked this patch. One thing I could comment on is, do you think it is a good idea to have oid of exception function list on contain_volatile_functions_walker()? The walker function is static thus here is no impact for other caller, and its context argument is unused. My proposition is to enhance 2nd argument of contain_volatile_functions_walker() to deliver list of exceptional functions, then contain_volatile_functions_not_nextval() calls contain_volatile_functions_walker() with list_make1_oid(F_NEXTVAL_OID) to handle nextval() as exception. Otherwise, all we need to do is put NIL as 2nd argument. It kills code duplication and reduces future maintenance burden. How about your opinion? That approach is more flexible than the one in the patch, I agree. Ultimately, I see this as a choice between a special purpose piece of code (as originally supplied) and a much more generic facility for labelling functions as to whether they contain SQL or not, per the SQL standard as Jaime suggests. There's not much mileage in something in between. So I'm mid way through updating the patch to implement the generic facility. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] [GENERAL] pg_upgrade -u
On 6/28/13 6:06 PM, Bruce Momjian wrote: On Wed, May 29, 2013 at 09:44:00AM -0400, Peter Eisentraut wrote: On 5/28/13 10:55 PM, Bruce Momjian wrote: Wow, I never realized other tools used -U for user, instead of -u. Should I change pg_upgrade to use -U for 9.4? I can keep supporting -u as an undocumented option. It seems to me that that option shouldn't be necessary anyway. pg_upgrade should somehow be able to find out by itself what the superuser of the old cluster was. Uh, any idea how to do that? select rolname from pg_authid where oid = 10; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
