[HACKERS] Review: query result history in psql
Hi Maciej, I've been reviewing your patch for the ongoing commitfest. First let me say that I think it's a great idea and provides some very useful functionality. However, there are a few minor problems. There were a few english/grammatical mistakes that I went ahead and fixed. Additionally, I think some of the string manipulation might be placed outside of the main ans.c file. I don't know if there's a better place for 'EscapeForCopy' and 'GetEscapedLen'. Not really a big deal, just an organizational idea. I also changed 'EscapeForCopy' to 'EscapeAndCopy'. I think that better describes the functionality. 'EscapeForCopy' kind of implies that another function is needed to copy the string. What does 'ans' stand for? I am not sure how it relates to the concept of a query history. It didn't stop my understanding of the code, but I don't know if a user will immediately know the meaning. Probably the biggest problem is that the query history list is missing a maximum size variable. I think this could be valuable for preventing users from shooting themselves in the foot. If the user is running large queries, they might accidentally store too much data. This probably somewhat of an edge-case but I believe it is worth considering. We could provide a sensible default limit (10 queries?) and also allow the user to change it. Finally, is it worth resetting the query history every time a user reconnects to the database? I can see how this might interrupt a user's workflow. If the user suddenly disconnects (network connection interrupted, etc) then they would lose their history. I think this is definitely up for debate. It would add more management overhead (psql options etc) and might just be unnecessary. However, with a sane limit to the size of the query history, I don't know if there would be too many drawbacks from a storage perspective. Those issues aside - I think it's a great feature! I can add the grammatical fixes I made whenever the final patch is ready. Or earlier, whatever works for you. Also, this is my first time reviewing a patch, so please let me know if I can improve on anything. Thanks! Ian Link
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
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 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 : > >> 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
No worries! :) On Fri, Jun 28, 2013 at 12:20 AM, Pavel Stehule wrote: > Hello > > It's look like my bug - wrong compilation > > I am sorry > > Pavel > > 2013/6/28 ian link : > > 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 > > > 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 : > >> >> 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
Not sure about all of your suggestions. Let me see if I can clarify what you're looking for. > * simply decision if content should be stored in history or not, Do you mean that the user should use a flag to place the result of a query into the history? like: --ans SELECT * FROM cities... Not sure if that's what you mean, but it seems kind of unnecesary. They can just hit the \ans flag beforehand. * simply remove last entry (table) of history That could be useful. What do you think Maciej? * queries should be joined to content, only name is not enough Don't know what you mean. Could you try re-wording that? Ian On Fri, Jun 28, 2013 at 8:49 AM, Pavel Stehule wrote: > 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 : > > 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] Support for RANGE ... PRECEDING windows in OVER
I found some time and I think I am up to speed now. I finally figured out how to add new operator strategies and made a little test operator for myself. It seems pretty clear that assuming '+' and '-' are addition and subtraction is a bad idea. I don't think it would be too tricky to add support for new operator strategies. Andrew Gierth suggested calling these new strategies "offset -" and "offset +", which I think describes it pretty well. I assigned the operator itself to be "@+" and "@-" but that can obviously be changed. If this sounds like a good path to you guys, I will go ahead and implement the operators for the appropriate types. Please let me know if I am misunderstanding something - I am still figuring stuff out :) Aside from the opclass stuff, there were some other important issues mentioned with the original RANGE support. I think I will address those after the opclass stuff is done. Thanks! Ian On Sat, Jun 22, 2013 at 4:38 PM, ian link wrote: > Thanks Craig! That definitely does help. I probably still have some > questions but I think I will read through the rest of the code before > asking. Thanks again! > > Ian > > > Craig Ringer > > Friday, June 21, 2013 8:41 PM > > > > > On 06/22/2013 03:30 AM, ian link wrote: > >> > >> Forgive my ignorance, but I don't entirely understand the problem. What > >> does '+' and '-' refer to exactly? > > > > Consider "RANGE 4.5 PRECEDING'. > > > > You need to be able to test whether, for the current row 'b', any given > > row 'a' is within the range (b - 4.5) < a <= b . Not 100% sure about the > > < vs <= boundaries, but that's irrelevant for the example. > > > > To test that, you have to be able to do two things: you have to be able > > to test whether one value is greater than another, and you have to be > > able to add or subtract a constant from one of the values. > > > > Right now, the b-tree access method provides information on the ordering > > operators < <= = > >= <> , which provides half the answer. But these > > don't give any concept of *distance* - you can test ordinality but not > > cardinality. > > > > To implement the "different by 4.5" part, you have to be able to add 4.5 > > to one value or subtract it from the other. > > > > The obvious way to do that is to look up the function that implements > > the '+' or '-' operator, and do: > > > > ((OPERATOR(+))(a, 4.5)) > b AND (a <= b) > > > > or > > > > ((OPERATOR(-))(b, 4.5)) < a AND (a <= b); > > > > The problem outlined by Tom in prior discussion about this is that > > PostgreSQL tries really hard not to assume that particular operator > > names mean particular things. Rather than "knowing" that "+" is always > > "an operator that adds two values together; is transitive, symmetric and > > reflexive", PostgreSQL requires that you define an *operator class* that > > names the operator that has those properties. > > > > Or at least, it does for less-than, less-than-or-equals, equals, > > greater-than-or-equals, greater-than, and not-equals as part of the > > b-tree operator class, which *usually* defines these operators as < <= = > >> > >> = > <>, but you could use any operator names you wanted if you really > > > > liked. > > > > Right now (as far as I know) there's no operator class that lets you > > identify operators for addition and subtraction in a similar way. So > > it's necessary to either add such an operator class (in which case > > support has to be added for it for every type), extend the existing > > b-tree operator class to provide the info, or blindly assume that "+" > > and "-" are always addition and subtraction. > > > > For an example of why such assumptions are a bad idea, consider matrix > > multiplication. Normally, "a * b" = "b * a", but this isn't true for > > multiplication of matrices. Similarly, if someone defined a "+" operator > > as an alias for string concatenation (||), we'd be totally wrong to > > assume we could use that for doing range-offset windowing. > > > > So. Yeah. Operator classes required, unless we're going to change the > > rules and make certain operator names "special" in PostgreSQL, so that > > if you implement them they *must* have certain properties. This seems > > like a pretty poor
Re: [HACKERS] Support for RANGE ... PRECEDING windows in OVER
Definitely not this week. Hopefully for next commit fest. On Sun, Jun 30, 2013 at 9:56 PM, Josh Berkus wrote: > On 06/30/2013 08:54 PM, ian link wrote: > > I found some time and I think I am up to speed now. I finally figured out > > how to add new operator strategies and made a little test operator for > > myself. > > > > It seems pretty clear that assuming '+' and '-' are addition and > > subtraction is a bad idea. I don't think it would be too tricky to add > > support for new operator strategies. Andrew Gierth suggested calling > these > > new strategies "offset -" and "offset +", which I think describes it > pretty > > well. I assigned the operator itself to be "@+" and "@-" but that can > > obviously be changed. If this sounds like a good path to you guys, I will > > go ahead and implement the operators for the appropriate types. Please > let > > me know if I am misunderstanding something - I am still figuring stuff > out > > :) > > > > Aside from the opclass stuff, there were some other important issues > > mentioned with the original RANGE support. I think I will address those > > after the opclass stuff is done. > > Are these things you plan to get done this week, or for next CommitFest? > > -- > Josh Berkus > PostgreSQL Experts Inc. > http://pgexperts.com >
Re: [HACKERS] Review: query result history in psql
> > but maybe some interactive mode should be usefull - so after > execution, and showing result, will be prompt if result should be > saved or not. I like the idea, in addition to the ordinary mode. Personally, I would use the ordinary mode, but I can see how 'interactive' would be useful. yes, the names :ans01, :ans02, ... miss semantics - How I can join this name (and content) with some SQL query? That makes sense. I think having part of / the whole query string would be very helpful. Great suggestion! Maciej, would you be able/have time to implement these? Or do you need any help getting them done? On Sun, Jun 30, 2013 at 11:35 PM, Pavel Stehule wrote: > Hello > > 2013/7/1 ian link : > > Not sure about all of your suggestions. Let me see if I can clarify what > > you're looking for. > > > >> > >> * simply decision if content should be stored in history or not, > > > > Do you mean that the user should use a flag to place the result of a > query > > into the history? > > like: > > --ans SELECT * FROM cities... > > Not sure if that's what you mean, but it seems kind of unnecesary. They > can > > just hit the \ans flag beforehand. > > switching off on is not user friendly > > but maybe some interactive mode should be usefull - so after > execution, and showing result, will be prompt if result should be > saved or not. > > some like: > > \ans interactive > > SELECT * FROM pg_proc; > > result > > should be saved last result [y, n]? > > y > result is saved in :ans22 > > > > > > > > >> * simply remove last entry (table) of history > > > > That could be useful. What do you think Maciej? > > yes, lot of queries is just +/- experiment and you don't would store result > > > > >> * queries should be joined to content, only name is not enough > > > > Don't know what you mean. Could you try re-wording that? > > > > yes, the names :ans01, :ans02, ... miss semantics - How I can join > this name (and content) with some SQL query? > > I needs to reverese search in SQL of stored caches, and I need a > information > > ans01 SELECT * FROM pg_proc > ans02 SELECT * FROM ans02 WHERE ... > ans03 ... > > Regards > > Pavel > > > Ian > > > > > > > > On Fri, Jun 28, 2013 at 8:49 AM, Pavel Stehule > > wrote: > >> > >> 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 : > >> > 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] Review: query result history in psql
Maciej - I can see your resistance to some kind of interactive mode. It would definitely be more code and create a less simple user interface. I would be perfectly happy if we left that part as it is now. However, I think it would be important to have a way of displaying the query history. Yes, you can search through your console backwards. You can still search through your old queries, but this would be a good alternative. What if the user reconnects after working for a while? Their old bash history might be gone. This would leave them with a big query history and no point of reference. Personally, I would find this feature very worthwhile. The query history wouldn't be crippled without it, but it would be a lot less flexible. Pavel Stehule Monday, July 01, 2013 4:05 AM a idea is good, but I don't think, it can be useful with currentimplementation. How I can identify, what is correct answer for myquery? Have I remember twenty numbers and twenty queries?RegardsPavel Maciej Gajewski Monday, July 01, 2013 4:01 AM When I tested this feature, I had 30 caches per 5 minutes, and only a few from these queries had a sense. Switch between off and on is not user friendly. I believe so there can be other solution than mine, but a possibility to friendly clean unwanted caches is necessary. If you know that you'll need the result of a query beforehand, you can always use SELECT ... INTO ... . No client-side features required. This feature is intended for people running plenty of ad-hoc queries, when every result could potentially be useful. Pavel Stehule Monday, July 01, 2013 1:31 AM 2013/7/1 Maciej Gajewski : I'm not really bought into some of the ideas. but maybe some interactive mode should be usefull - so after execution, and showing result, will be prompt if result should be saved or not. I like the idea, in addition to the ordinary mode. Personally, I would use the ordinary mode, but I can see how 'interactive' would be useful. This would require a complex change to the client code. And the result would eventually become annoying: an interactive question after each and every query. Currently, when turned on, every result is stored and simple notification is printed. . When I tested this feature, I had 30 caches per 5 minutes, and only a few from these queries had a sense. Switch between off and on is not user friendly. I believe so there can be other solution than mine, but a possibility to friendly clean unwanted caches is necessary. yes, the names :ans01, :ans02, ... miss semantics - How I can join this name (and content) with some SQL query? That makes sense. I think having part of / the whole query string would be very helpful. Great suggestion! The naming is obscure and non-informative, I agree. If you have a nice idea how to make it better, I'd love to discuss it. But please remember that it has one huge advantage: simplicity. The client is a classical command-line tool, and as such it delegates some of the functionality to external programs, like pager or terminal. Personally, I don't see a strong price for all users without friendly interface. Regards Pavel I'm pretty sure that your terminal emulator has a 'find' function that would allow you to quickly locate the variable and associated query in the scrollback. M Maciej Gajewski Monday, July 01, 2013 1:23 AM I'm not really bought into some of the ideas. but maybe some interactive mode should be usefull - so after execution, and showing result, will be prompt if result should besaved or not. I like the idea, in addition to the ordinary mode. Personally, I would use the ordinary mode, but I can see how 'interactive' would be useful. This would require a complex change to the client code. And the result would eventually become annoying: an interactive question after each and every query. Currently, when turned on, every result is stored and simple notification is printed. yes, the names :ans01, :ans02, ... miss semantics - How I can join this name (and content) with some SQL query?That makes sense. I think having part of / the whole query string would be very helpful. Great suggestion! The naming is obscure and non-informative, I agree. If you have a nice idea how to make it better, I'd love to discuss it. But please remember that it has one huge advantage: simplicity. The client is a classical command-line tool, and as such it delegates some of the functionality to external programs, like pager or terminal. I'm pretty sure that your terminal emulator has a 'find' function that would allow you to quickly locate the variable and associated query in the scrollback. M ian link Monday, July 01, 2013 12:19 AM but maybe some interactive mode should be usefull - so after execution
Re: [HACKERS] Review: query result history in psql
> > I haven't been able to find any real documentation on this feature, > other than the additions to the psql help. You're right, I missed that in my review. I agree that it needs some more documentation. What is "ans"? We covered that earlier in the email thread, but it's the current name for the query history. I think we are going to change it to something a little more descriptive. I was thinking "qh" for short or "query-history". I'm not sure if I'll be able to implement this feature any time soon, > as I'm very busy at the moment and going for a business trip in few > days. I can try implementing the feature, I might have some time. On Tue, Jul 2, 2013 at 5:36 AM, Peter Eisentraut wrote: > On 7/2/13 5:53 AM, Maciej Gajewski wrote: > > In the meantime, I've applied your suggestions and moved the > > sting-manipulating functions to stringutils. Also fixed a tiny bug. > > Patch attached. > > I haven't been able to find any real documentation on this feature, > other than the additions to the psql help. Could someone write some > mock documentation at least, so we know what the proposed interfaces and > intended ways of use are? > > What is "ans"? > >
Re: [HACKERS] Support for RANGE ... PRECEDING windows in OVER
I'm fine with moving the operators over to functions. I just don't want to implement anything that is against best practice. If we are OK with that direction, I'll go ahead and start on the new patch. Ian On Mon, Jul 1, 2013 at 9:03 PM, Tom Lane wrote: > Craig Ringer writes: > > On 07/02/2013 02:39 AM, Robert Haas wrote: > >> I'm actually > >> not clear that it would be all that bad to assume fixed operator > >> names, as we apparently do in a few places despite the existence of > >> operator classes. But if that is bad, then I don't know how using @+ > >> and @- instead helps anything. > > > Personally I'm not clear why it's bad to reserve certain fundamental > > operators like '+' and '-', requiring that they have particular > semantics. > > It is bad. It's against project policy, not least because we have > assorted *existing* datatypes for which "obvious" operator names like > "=" do not have all the properties you might expect. > > If you need a more concrete example of why that sort of thinking is > bad, you might consider the difference between < and ~<~ for type text. > If we hard-wired knowledge about operator behavior to operator names, > it would be impossible for the system to understand that both of those > operators represent sorting-related behaviors. > > Or to be even more concrete: if we allow RANGE to suppose that there's > only one possible definition of "+" for a datatype, we're effectively > supposing that there's only one possible sort ordering for that type. > Which is already a wrong assumption, and has been since Postgres was > still at Berkeley. If you go this way, you won't be able to support > both WINDOW ... ORDER BY foo USING < RANGE ... and WINDOW ... ORDER BY > foo USING ~<~ RANGE ... because you won't know which addition operator > to apply. > > (And yeah, I'm aware that the SQL standard only expects RANGE to support > sort keys that are of numeric, datetime, or interval type. I would hope > that we have higher expectations than that. Even if we don't, it's not > exactly hard to credit that people might have multiple ideas about how > to sort interval values.) > > There are indeed still some places where we rely on operator names to > mean something, but we need to get away from that idea not add more. > Ideally, any property the system understands about an operator or > function should be explicitly declared through opclass membership or > some similar representation. We've made substantial progress in that > direction in the last fifteen years. I don't want to reverse that > progress in the name of minor expediencies, especially not ones that > fail to support flexibility that has been in the system for a couple > or three decades already. > > regards, tom lane >
Re: [HACKERS] Review: query result history in psql
> > I'm kinda not all that convinced that this feature does anything > that's actually useful. If you want to save your query results, you > can just stick "CREATE TEMP TABLE ans AS" in front of your query. > What does that not give you that this gives you? Convenience. It auto-increments with each new query, giving you a different temporary table for each query. Seems pretty helpful to me. On Tue, Jul 2, 2013 at 6:16 PM, Robert Haas wrote: > On Tue, Jul 2, 2013 at 8:16 PM, ian link wrote: > > We covered that earlier in the email thread, but it's the current name > for > > the query history. I think we are going to change it to something a > little > > more descriptive. I was thinking "qh" for short or "query-history". > > > >> I'm not sure if I'll be able to implement this feature any time soon, > >> as I'm very busy at the moment and going for a business trip in few > >> days. > > > > I can try implementing the feature, I might have some time. > > I'm kinda not all that convinced that this feature does anything > that's actually useful. If you want to save your query results, you > can just stick "CREATE TEMP TABLE ans AS" in front of your query. > What does that not give you that this gives you? > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
Re: [HACKERS] Patch for fast gin cache performance improvement
Hi Etsuro, Sorry for the delay but I have been very busy with work. I have been away from postgres for a while, so I will need a little time to review the code and make sure I give you an informed response. I'll get back to you as soon as I am able. Thanks for understanding. Ian Link Etsuro Fujita Friday, September 27, 2013 2:24 AM I wrote: I had a look over this patch. I think this patch is interesting and very useful. Here are my review points: 8. I think there are no issues in this patch. However, I have one question: how this patch works in the case where gin_fast_limit/fast_cache_size = 0? In this case, in my understanding, this patch inserts new entries into the pending list temporarily and immediately moves them to the main GIN data structure using ginInsertCleanup(). Am I right? If so, that is obviously inefficient. Sorry, There are incorrect expressions. I mean gin_fast_limit > 0 and fast_cache_size = 0. Although I asked this question, I've reconsidered about these parameters, and it seems that these parameters not only make code rather complex but are a little confusing to users. So I'd like to propose to introduce only one parameter: fast_cache_size. While users that give weight to update performance for the fast update technique set this parameter to a large value, users that give weight not only to update performance but to search performance set this parameter to a small value. What do you think about this? Thanks, Best regards, Etsuro Fujita Etsuro Fujita Thursday, September 26, 2013 6:02 AM Hi Ian, This patch contains a performance improvement for the fast gin cache. As you may know, the performance of the fast gin cache decreases with its size. Currently, the size of the fast gin cache is tied to work_mem. The size of work_mem can often be quite high. The large size of work_mem is inappropriate for the fast gin cache size. Therefore, we created a separate cache size called gin_fast_limit. This global variable controls the size of the fast gin cache, independently of work_mem. Currently, the default gin_fast_limit is set to 128kB. However, that value could need tweaking. 64kB may work better, but it's hard to say with only my single machine to test on. On my machine, this patch results in a nice speed up. Our test queries improve from about 0.9 ms to 0.030 ms. Please feel free to use the test case yourself: it should be attached. I can look into additional test cases (tsvectors) if anyone is interested. In addition to the global limit, we have provided a per-index limit: fast_cache_size. This per-index limit begins at -1, which means that it is disabled. If the user does not specify a per-index limit, the index will simply use the global limit. I had a look over this patch. I think this patch is interesting and very useful. Here are my review points: 1. Patch applies cleanly. 2. make, make install and make check is good. 3. I did performance evaluation using your test queries with 64kB and 128kB of gin_fast_limit (or fast_cache_size), and saw that both values achieved the performance gains over gin_fast_limit = '256MB'. 64kB worked better than 128kB. 64kB improved from 1.057 ms to 0.075 ms. Great! 4. In my understanding, the small value of gin_fast_limit/fast_cache_size leads to the increase in GIN search performance, which, however, leads to the decrease in GIN update performance. Am I right? If so, I think the tradeoff should be noted in the documentation. 5. The following documents in Chapter 57. GIN Indexes need to be updated: * 57.3.1. GIN Fast Update Technique * 57.4. GIN Tips and Tricks 6. I would like to see the results for the additional test cases (tsvectors). 7. The commented-out elog() code should be removed. 8. I think there are no issues in this patch. However, I have one question: how this patch works in the case where gin_fast_limit/fast_cache_size = 0? In this case, in my understanding, this patch inserts new entries into the pending list temporarily and immediately moves them to the main GIN data structure using ginInsertCleanup(). Am I right? If so, that is obviously inefficient. Sorry for the delay. Best regards, Etsuro Fujita Ian Link Monday, June 17, 2013 9:42 PM This patch contains a performance improvement for the fast gin cache. As you may know, the performance of the fast gin cache decreases with its size. Currently, the size of the fast gin cache is tied to work_mem. The size of work_mem can often be quite high. The large size of work_mem is inappropriate for the fast gin cache size. Therefore, we created a separate cache size called gin_fast_limit. This global variable controls the size of the fast gin cache, independently of work_mem. Currently, the default gin_fast_limit is set to 128kB. However, that value could need tweaking. 64kB may work better, but it's hard to say with only my single ma
Re: [HACKERS] Patch for fast gin cache performance improvement
No worries! I'll just wait until it's up again. Thanks Ian Kevin Grittner Tuesday, June 18, 2013 9:15 AM Oops -- we seem to have a problem with new community logins at themoment, which will hopefully be straightened out soon. You mightwant to wait a few days if you don't already have a login.--Kevin GrittnerEnterpriseDB: http://www.enterprisedb.comThe Enterprise PostgreSQL Company Kevin Grittner Tuesday, June 18, 2013 9:09 AM Ian Link wrote: This patch contains a performance improvement for the fast gin cache. Our test queries improve from about 0.9 ms to 0.030 ms. Impressive! Thanks for reading and considering this patch! Congratulations on your first PostgreSQL patch! To get it scheduled for review, please add it to this page: https://commitfest.postgresql.org/action/commitfest_view/open You will need to get a community login (if you don't already have one), but that is a quick and painless process. Choose an appropriate topic (like "Performance") and reference the message ID of the email to which you attached the patch. Don't worry about the fields for reviewers, committer, or date closed. Sorry for the administrative overhead, but without it things can fall through the cracks. You can find an overview of the review process with links to more detail here: http://wiki.postgresql.org/wiki/CommitFest Thanks for contributing! Ian Link Monday, June 17, 2013 9:42 PM This patch contains a performance improvement for the fast gin cache. As you may know, the performance of the fast gin cache decreases with its size. Currently, the size of the fast gin cache is tied to work_mem. The size of work_mem can often be quite high. The large size of work_mem is inappropriate for the fast gin cache size. Therefore, we created a separate cache size called gin_fast_limit. This global variable controls the size of the fast gin cache, independently of work_mem. Currently, the default gin_fast_limit is set to 128kB. However, that value could need tweaking. 64kB may work better, but it's hard to say with only my single machine to test on.On my machine, this patch results in a nice speed up. Our test queries improve from about 0.9 ms to 0.030 ms. Please feel free to use the test case yourself: it should be attached. I can look into additional test cases (tsvectors) if anyone is interested. In addition to the global limit, we have provided a per-index limit: fast_cache_size. This per-index limit begins at -1, which means that it is disabled. If the user does not specify a per-index limit, the index will simply use the global limit. I would like to thank Andrew Gierth for all his help on this patch. As this is my first patch he was extremely helpful. The idea for this performance improvement was entirely his. I just did the implementation. Thanks for reading and considering this patch! Ian Link
Re: [HACKERS] Support for RANGE ... PRECEDING windows in OVER
I am currently looking into this feature. However, as I am quite new to Postgres, I think it might take me a while to get up to speed. Anyways, I would also appreciate another round of discussion on the future of the windowing functions. Ian Link Craig Ringer Thursday, June 20, 2013 7:24 PM Hi allSince 8.4, PostgreSQL has had extremely useful window function support -but support for "RANGE PRECEDING / FOLLOWING" windows was dropped latein 8.4's development in order to get the rest of the feature in, perhttp://archives.postgresql.org/pgsql-hackers/2010-02/msg00540.php.It looks like there was discussion of requiring a new opclass to bedeclared for types or otherwise extending opclasses to provide theinformation required for RANGE ... PRECEDING / FOLLOWING (http://www.postgresql.org/message-id/20100211201444.ga28...@svana.org ). I can't find any sign that it went anywhere beyond some broaddiscussion:http://www.postgresql.org/message-id/13993.1265920...@sss.pgh.pa.us atthe time.I've missed this feature more than once, and am curious about whetherany more recent changes may have made it cleaner to tackle this, orwhether consensus can be formed on adding the new entries to btree'sopclass to avoid the undesirable explicit lookups of the '+' and '-'oprators.Some question seems to remain open about how ranges overtimestamps/intervals should work, but this wasn't elaborated on.There's been interest in this, eg:http://pgsql.hackers.free-usenet.eu/[HACKERS]-range-intervals-in-window-function-frames_T66085695_S1http://grokbase.com/t/postgresql/pgsql-general/105a89gm2n/postgresql-9-0-support-for-range-value-preceding-window-functions
Re: [HACKERS] Support for RANGE ... PRECEDING windows in OVER
Thanks! The discussions have been useful, although I am currently just reviewing the code. I think a good starting point will be to refactor/imrpove the WinGetFuncArgInPartition and WinGetFuncArgInFrame functions. Tom Lane wrote this about them before comitting the patch: I'm not terribly happy with the changes you made in WinGetFuncArgInPartitionand WinGetFuncArgInFrame to force the window function mark to not gopast frame start in some modes. Not only is that pretty ugly, but Ithink it can mask bugs in window functions: it's an error for a windowfunction to fetch a row before what it has set its mark to be, but insome cases that wouldn't be detected because of this change. I thinkit would be better to revert those changes and find another method ofprotecting fetches needed to determine the frame head. One idea isto create a separate read pointer that tracks the frame head wheneveractual fetches of the frame head might be needed by update_frameheadpos.I committed it without changing that, but I think this should berevisited before trying to add the RANGE value PRECEDING/FOLLOWINGoptions, because those will substantially expand the number of caseswhere that hack affects the behavior. I am honestly not 100% certain why these functions have issues, but this seems a good place to start investigating. Ian Link Craig Ringer Thursday, June 20, 2013 7:37 PM Good to know, and welcome.I hope the links to the archived discussions on the matter were usefulto you. Craig Ringer Thursday, June 20, 2013 7:24 PM Hi allSince 8.4, PostgreSQL has had extremely useful window function support -but support for "RANGE PRECEDING / FOLLOWING" windows was dropped latein 8.4's development in order to get the rest of the feature in, perhttp://archives.postgresql.org/pgsql-hackers/2010-02/msg00540.php.It looks like there was discussion of requiring a new opclass to bedeclared for types or otherwise extending opclasses to provide theinformation required for RANGE ... PRECEDING / FOLLOWING (http://www.postgresql.org/message-id/20100211201444.ga28...@svana.org ). I can't find any sign that it went anywhere beyond some broaddiscussion:http://www.postgresql.org/message-id/13993.1265920...@sss.pgh.pa.us atthe time.I've missed this feature more than once, and am curious about whetherany more recent changes may have made it cleaner to tackle this, orwhether consensus can be formed on adding the new entries to btree'sopclass to avoid the undesirable explicit lookups of the '+' and '-'oprators.Some question seems to remain open about how ranges overtimestamps/intervals should work, but this wasn't elaborated on.There's been interest in this, eg:http://pgsql.hackers.free-usenet.eu/[HACKERS]-range-intervals-in-window-function-frames_T66085695_S1http://grokbase.com/t/postgresql/pgsql-general/105a89gm2n/postgresql-9-0-support-for-range-value-preceding-window-functions
Re: [HACKERS] Support for RANGE ... PRECEDING windows in OVER
Forgive my ignorance, but I don't entirely understand the problem. What does '+' and '-' refer to exactly? Thanks! On Fri, Jun 21, 2013 at 4:35 AM, Hitoshi Harada wrote: > > > > On Fri, Jun 21, 2013 at 3:20 AM, Craig Ringer wrote: > >> On 06/21/2013 05:32 PM, Hitoshi Harada wrote: >> >> > I also later found that we are missing not only notion of '+' or '-', >> > but also notion of 'zero value' in our catalog. Per spec, RANGE BETWEEN >> > needs to detect ERROR if the offset value is negative, but it is not >> > always easy if you think about interval, numeric types as opposed to >> > int64 used in ROWS BETWEEN. >> >> Zero can be tested for with `val = (@ val)` ie `val = abs(val)`. That >> should make sense for any type in which the concept of zero makes sense. >> >> >> Yeah, I mean, it needs to know if offset is negative or not by testing > with zero. So we need "zero value" or "is_negative function" for each type. > > Thanks, > -- > Hitoshi Harada >
Re: [HACKERS] Support for RANGE ... PRECEDING windows in OVER
Thanks Craig! That definitely does help. I probably still have some questions but I think I will read through the rest of the code before asking. Thanks again! Ian > Craig Ringer > Friday, June 21, 2013 8:41 PM > > On 06/22/2013 03:30 AM, ian link wrote: >> >> Forgive my ignorance, but I don't entirely understand the problem. What >> does '+' and '-' refer to exactly? > > Consider "RANGE 4.5 PRECEDING'. > > You need to be able to test whether, for the current row 'b', any given > row 'a' is within the range (b - 4.5) < a <= b . Not 100% sure about the > < vs <= boundaries, but that's irrelevant for the example. > > To test that, you have to be able to do two things: you have to be able > to test whether one value is greater than another, and you have to be > able to add or subtract a constant from one of the values. > > Right now, the b-tree access method provides information on the ordering > operators < <= = > >= <> , which provides half the answer. But these > don't give any concept of *distance* - you can test ordinality but not > cardinality. > > To implement the "different by 4.5" part, you have to be able to add 4.5 > to one value or subtract it from the other. > > The obvious way to do that is to look up the function that implements > the '+' or '-' operator, and do: > > ((OPERATOR(+))(a, 4.5)) > b AND (a <= b) > > or > > ((OPERATOR(-))(b, 4.5)) < a AND (a <= b); > > The problem outlined by Tom in prior discussion about this is that > PostgreSQL tries really hard not to assume that particular operator > names mean particular things. Rather than "knowing" that "+" is always > "an operator that adds two values together; is transitive, symmetric and > reflexive", PostgreSQL requires that you define an *operator class* that > names the operator that has those properties. > > Or at least, it does for less-than, less-than-or-equals, equals, > greater-than-or-equals, greater-than, and not-equals as part of the > b-tree operator class, which *usually* defines these operators as < <= = >> >> = > <>, but you could use any operator names you wanted if you really > > liked. > > Right now (as far as I know) there's no operator class that lets you > identify operators for addition and subtraction in a similar way. So > it's necessary to either add such an operator class (in which case > support has to be added for it for every type), extend the existing > b-tree operator class to provide the info, or blindly assume that "+" > and "-" are always addition and subtraction. > > For an example of why such assumptions are a bad idea, consider matrix > multiplication. Normally, "a * b" = "b * a", but this isn't true for > multiplication of matrices. Similarly, if someone defined a "+" operator > as an alias for string concatenation (||), we'd be totally wrong to > assume we could use that for doing range-offset windowing. > > So. Yeah. Operator classes required, unless we're going to change the > rules and make certain operator names "special" in PostgreSQL, so that > if you implement them they *must* have certain properties. This seems > like a pretty poor reason to add such a big change. > > I hope this explanation (a) is actually correct and (b) is helpful. > > ian link > Friday, June 21, 2013 12:30 PM > Forgive my ignorance, but I don't entirely understand the problem. What does '+' and '-' refer to exactly? > Thanks! > > > > Hitoshi Harada > Friday, June 21, 2013 4:35 AM > > > On 06/22/2013 03:30 AM, ian link wrote: > Forgive my ignorance, but I don't entirely understand the problem. What > does '+' and '-' refer to exactly? Consider "RANGE 4.5 PRECEDING'. You need to be able to test whether, for the current row 'b', any given row 'a' is within the range (b - 4.5) < a <= b . Not 100% sure about the < vs <= boundaries, but that's irrelevant for the example. To test that, you have to be able to do two things: you have to be able to test whether one value is greater than another, and you have to be able to add or subtract a constant from one of the values. Right now, the b-tree access method provides information on the ordering operators < <= = > >= <> , which provides half the answer. But these don't give any concept of *distance* - you can test ordinality but not cardinality. To implement the "different by 4.5" part, you have to be able to add 4.5 to one
Re: [HACKERS] Patch for fast gin cache performance improvement
Looks like my community login is still not working. No rush, just wanted to let you know. Thanks! Ian On Tue, Jun 18, 2013 at 11:41 AM, Ian Link wrote: > > No worries! I'll just wait until it's up again. > Thanks > Ian > > Kevin Grittner > Tuesday, June 18, 2013 9:15 AM > > Oops -- we seem to have a problem with new community logins at the > moment, which will hopefully be straightened out soon. You might > want to wait a few days if you don't already have a login. > > -- > Kevin Grittner > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > Kevin Grittner > Tuesday, June 18, 2013 9:09 AM > > Ian Link wrote: > > > This patch contains a performance improvement for the fast gin > cache. > > Our test queries improve from about 0.9 ms to 0.030 ms. > > Impressive! > > > Thanks for reading and considering this patch! > > > Congratulations on your first PostgreSQL patch! To get it > scheduled for review, please add it to this page: > https://commitfest.postgresql.org/action/commitfest_view/open > > > You will need to get a community login (if you don't already have > one), but that is a quick and painless process. Choose an > appropriate topic (like "Performance") and reference the message ID > of the email to which you attached the patch. Don't worry about > the fields for reviewers, committer, or date closed. > > Sorry for the administrative overhead, but without it things can > fall through the cracks. You can find an overview of the review > process with links to more detail here: > > http://wiki.postgresql.org/wiki/CommitFest > > Thanks for contributing! > > > Ian Link > Monday, June 17, 2013 9:42 PM > * > > This patch contains a performance improvement for the fast gin cache. As > you may know, the performance of the fast gin cache decreases with its > size. Currently, the size of the fast gin cache is tied to work_mem. The > size of work_mem can often be quite high. The large size of work_mem is > inappropriate for the fast gin cache size. Therefore, we created a separate > cache size called gin_fast_limit. This global variable controls the size of > the fast gin cache, independently of work_mem. Currently, the default > gin_fast_limit is set to 128kB. However, that value could need tweaking. > 64kB may work better, but it's hard to say with only my single machine to > test on. > > On my machine, this patch results in a nice speed up. Our test queries > improve from about 0.9 ms to 0.030 ms. Please feel free to use the test > case yourself: it should be attached. I can look into additional test cases > (tsvectors) if anyone is interested. > > In addition to the global limit, we have provided a per-index limit: > fast_cache_size. This per-index limit begins at -1, which means that it is > disabled. If the user does not specify a per-index limit, the index will > simply use the global limit. > > I would like to thank Andrew Gierth for all his help on this patch. As > this is my first patch he was extremely helpful. The idea for this > performance improvement was entirely his. I just did the implementation. > Thanks for reading and considering this patch!* > > > Ian Link > > <><>
Re: [HACKERS] Patch for fast gin cache performance improvement
I just tried it and my account works now. Thanks! Ian Stefan Kaltenbrunner Sunday, June 23, 2013 2:05 AM have you tried to log in once to the main website per:http://www.postgresql.org/message-id/CABUevEyt9tQfcF7T2Uzcr8WeF9M=s8qSACuCmN5L2Et26=r...@mail.gmail.com?Stefan ian link Saturday, June 22, 2013 7:03 PM Looks like my community login is still not working. No rush, just wanted to let you know. Thanks!Ian Ian Link Tuesday, June 18, 2013 11:41 AM No worries! I'll just wait until it's up again. Thanks Ian Kevin Grittner Tuesday, June 18, 2013 9:15 AM Oops -- we seem to have a problem with new community logins at themoment, which will hopefully be straightened out soon. You mightwant to wait a few days if you don't already have a login.--Kevin GrittnerEnterpriseDB: http://www.enterprisedb.comThe Enterprise PostgreSQL Company Kevin Grittner Tuesday, June 18, 2013 9:09 AM Ian Link wrote: This patch contains a performance improvement for the fast gin cache. Our test queries improve from about 0.9 ms to 0.030 ms. Impressive! Thanks for reading and considering this patch! Congratulations on your first PostgreSQL patch! To get it scheduled for review, please add it to this page: https://commitfest.postgresql.org/action/commitfest_view/open You will need to get a community login (if you don't already have one), but that is a quick and painless process. Choose an appropriate topic (like "Performance") and reference the message ID of the email to which you attached the patch. Don't worry about the fields for reviewers, committer, or date closed. Sorry for the administrative overhead, but without it things can fall through the cracks. You can find an overview of the review process with links to more detail here: http://wiki.postgresql.org/wiki/CommitFest Thanks for contributing!
Re: [HACKERS] Patch for fast gin cache performance improvement
Although I asked this question, I've reconsidered about these parameters, and it seems that these parameters not only make code rather complex but are a little confusing to users. So I'd like to propose to introduce only one parameter: fast_cache_size. While users that give weight to update performance for the fast update technique set this parameter to a large value, users that give weight not only to update performance but to search performance set this parameter to a small value. What do you think about this? I think it makes sense to maintain this separation. If the user doesn't need a per-index setting, they don't have to use the parameter. Since the parameter is off by default, they don't even need to worry about it. There might as well be one parameter for users that don't need fine-grained control. We can document this and I don't think it will be confusing to the user. 4. In my understanding, the small value of gin_fast_limit/fast_cache_size leads to the increase in GIN search performance, which, however, leads to the decrease in GIN update performance. Am I right? If so, I think the tradeoff should be noted in the documentation. I believe this is correct. 5. The following documents in Chapter 57. GIN Indexes need to be updated: * 57.3.1. GIN Fast Update Technique * 57.4. GIN Tips and Tricks Sure, I can add something. 6. I would like to see the results for the additional test cases (tsvectors). I don't really have any good test cases for this available, and have very limited time for postgres at the moment. Feel free to create a test case, but I don't believe I can at the moment. Sorry! 7. The commented-out elog() code should be removed. Sorry about that, I shouldn't have submitted the patch with those still there. I should have a new patch soonish, hopefully. Thanks for your feedback! Ian Ian Link wrote: 8. I think there are no issues in this patch. However, I have one question: how this patch works in the case where gin_fast_limit/fast_cache_size = 0? In this case, in my understanding, this patch inserts new entries into the pending list temporarily and immediately moves them to the main GIN data structure using ginInsertCleanup(). Am I right? If so, that is obviously inefficient. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for fast gin cache performance improvement
I think it is desirable that this patch should be resubmitted for the next CommitFest for further review and testing mentioned above. So I'd like to mark this patch as Returned with Feedback. Is it OK? Sounds like a good idea. Thanks for the review! Ian Link Etsuro Fujita Thursday, October 10, 2013 1:01 AM Ian Link wrote: Although I asked this question, I've reconsidered about these parameters, and it seems that these parameters not only make code rather complex but are a little confusing to users. So I'd like to propose to introduce only one parameter: fast_cache_size. While users that give weight to update performance for the fast update technique set this parameter to a large value, users that give weight not only to update performance but to search performance set this parameter to a small value. What do you think about this? I think it makes sense to maintain this separation. If the user doesn't need a per-index setting, they don't have to use the parameter. Since the parameter is off by default, they don't even need to worry about it. There might as well be one parameter for users that don't need fine-grained control. We can document this and I don't think it will be confusing to the user. OK, though I'd like to hear the opinion of others. 4. In my understanding, the small value of gin_fast_limit/fast_cache_size leads to the increase in GIN search performance, which, however, leads to the decrease in GIN update performance. Am I right? If so, I think the tradeoff should be noted in the documentation. I believe this is correct. 5. The following documents in Chapter 57. GIN Indexes need to be updated: * 57.3.1. GIN Fast Update Technique * 57.4. GIN Tips and Tricks Sure, I can add something. 6. I would like to see the results for the additional test cases (tsvectors). I don't really have any good test cases for this available, and have very limited time for postgres at the moment. Feel free to create a test case, but I don't believe I can at the moment. Sorry! Unfortunately, I don't have much time to do such a thing, though I think we should do that. (In addition, we should do another performance test to make clear an influence of fast update performance from introducing these parameters, which would be required to determine the default values of these parameters.) 7. The commented-out elog() code should be removed. Sorry about that, I shouldn't have submitted the patch with those still there. I should have a new patch soonish, hopefully. Thanks for your feedback! I think it is desirable that this patch should be resubmitted for the next CommitFest for further review and testing mentioned above. So I'd like to mark this patch as Returned with Feedback. Is it OK? Thanks, Best regards, Etsuro Fujita Ian Link Monday, September 30, 2013 3:09 PM Hi Etsuro, Sorry for the delay but I have been very busy with work. I have been away from postgres for a while, so I will need a little time to review the code and make sure I give you an informed response. I'll get back to you as soon as I am able. Thanks for understanding. Ian Link Etsuro Fujita Friday, September 27, 2013 2:24 AM I wrote: I had a look over this patch. I think this patch is interesting and very useful. Here are my review points: 8. I think there are no issues in this patch. However, I have one question: how this patch works in the case where gin_fast_limit/fast_cache_size = 0? In this case, in my understanding, this patch inserts new entries into the pending list temporarily and immediately moves them to the main GIN data structure using ginInsertCleanup(). Am I right? If so, that is obviously inefficient. Sorry, There are incorrect expressions. I mean gin_fast_limit > 0 and fast_cache_size = 0. Although I asked this question, I've reconsidered about these parameters, and it seems that these parameters not only make code rather complex but are a little confusing to users. So I'd like to propose to introduce only one parameter: fast_cache_size. While users that give weight to update performance for the fast update technique set this parameter to a large value, users that give weight not only to update performance but to search performance set this parameter to a small value. What do you think about this? Thanks, Best regards, Etsuro Fujita Etsuro Fujita Thursday, September 26, 2013 6:02 AM Hi Ian, This patch contains a performance improvement for the fast gin cache. As you may know, the performance of the fast gin cache decreases with its size. Currently, the size of the fast gin cache is tied to work_mem. The size of work_mem can often be quite high. The large size of work_mem is inappropriate for the fast gin cache size. Therefore, we created a separate cache size called gin_fast_limit. This global variable co