Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On 05/25/2009 07:58 PM, Andres Freund wrote: On 05/25/2009 07:53 PM, Andres Freund wrote: On 05/25/2009 07:31 PM, Tom Lane wrote: David Fetterda...@fetter.org writes: On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? If we were to put it into a repository config file, that would more or less have the effect of enforcing a project style for diffs, no? Yes and no. You can define that a subset (or all) files use a specific diff driver in the repository - unfortunately the definition of that driver has to be done locally. Defining it currently involves installing a wrapper like the one on http://wiki.postgresql.org/wiki/Talk:Working_with_Git and doing Ugh, hit the wrong key: and executing `git config --global diff.context.command git-external-diff` The content of the former page is now merged into the main page about git http://wiki.postgresql.org/wiki/Working_with_Git and the notes on the Talk: page are deleted. Andres -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On Monday 25 May 2009 20:58:59 Andres Freund wrote: and executing `git config --global diff.context.command git-external-diff` We already knew that you could do it with a wrapper. But that isn't the answer we were looking for, because it will basically mean that 98% of casual contributors will get it wrong, and it will probably not work very well on Windows. The goal is to get git-diff to do it itself. -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Hi, On 05/26/2009 01:39 PM, Peter Eisentraut wrote: On Monday 25 May 2009 20:58:59 Andres Freund wrote: and executing `git config --global diff.context.command git-external-diff` We already knew that you could do it with a wrapper. But that isn't the answer we were looking for, because it will basically mean that 98% of casual contributors will get it wrong, and it will probably not work very well on Windows. It works on windows, linux, solaris (thats what I could get my hands on without bothering). I tested it - it works on any non ancient version of git. (Ancient in the sense, that git at that time didnt work properly on win anyway). And providing a 5-line wrapper download-ready surely makes it easier than figuring it out how to write one out of some git manpages. Also it allows at least those who prefer context diffs to use them easily when using git - that are the ones which seem to prefer using them most. The goal is to get git-diff to do it itself. I do not disagree. Andres -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
I'll repeat my suggestion that everyone poo-pooed: we can have the mail list filters recognize patches, run filterdiff on them with our prefered options, and attach the result as an additional attachment (or link to some web directory). I think it would be simple to do and would be happy to give it a go if I can get the necessary access. It doesn't solve *all* the problems since the committee still needs a unified diff if he wants to take advantage of git's merge abilities. I think this is actually all a red herring since it's pretty easy for the reviewer to run filterdiff anyways. But having things be automatic is still always easier than not. -- Greg On 26 May 2009, at 13:54, Andres Freund and...@anarazel.de wrote: Hi, On 05/26/2009 01:39 PM, Peter Eisentraut wrote: On Monday 25 May 2009 20:58:59 Andres Freund wrote: and executing `git config --global diff.context.command git-external-diff` We already knew that you could do it with a wrapper. But that isn't the answer we were looking for, because it will basically mean that 98% of casual contributors will get it wrong, and it will probably not work very well on Windows. It works on windows, linux, solaris (thats what I could get my hands on without bothering). I tested it - it works on any non ancient version of git. (Ancient in the sense, that git at that time didnt work properly on win anyway). And providing a 5-line wrapper download-ready surely makes it easier than figuring it out how to write one out of some git manpages. Also it allows at least those who prefer context diffs to use them easily when using git - that are the ones which seem to prefer using them most. The goal is to get git-diff to do it itself. I do not disagree. Andres -- 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] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Tom Lane píše v po 25. 05. 2009 v 13:07 -0400: Zdenek Kotala zdenek.kot...@sun.com writes: Tom Lane píše v ne 24. 05. 2009 v 18:46 -0400: In any case, the barriers to implementing 8.3-style hash indexes in 8.4 are pretty huge: you'd need to duplicate not only the hash AM code, but also all the hash functions, and therefore all of the hash pg_amop and pg_amproc entries. I'm not sure if I need duplicate functions. Generally yes but It seems to me that hash index does not changed functions behavior and they could be shared at this moment. No, the behavior of the hash functions themselves changed during 8.4. Twice, even: hmm, I'm missed it. :( So as far as I can see, you need completely separate copies of both hash_any() and the SQL-level functions that call it. I'm not really seeing that the proposed refactoring makes this any easier. You might as well just copy-and-paste all that old code into a separate set of files, and not worry about what is in access/hash.h. Yeah, in this case everything have to be duplicated which is not big deal in comparison to do same amount of work for GIN. Then I can start with GIN. The advantage of refactoring is then only nicer code. thanks Zdenek -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Greg Stark greg.st...@enterprisedb.com writes: I'll repeat my suggestion that everyone poo-pooed: we can have the mail list filters recognize patches, run filterdiff on them with our prefered options, and attach the result as an additional attachment (or link to some web directory). The argument that was made at the developer meeting is that the preferred way of working will be to apply the submitted patch in one's local git repository, and then do any needed editorialization as a second patch on top of it. So the critical need as I see it is to be able to see a -c version of a patch-in-progress (ie, diff current working state versus some previous committed state). Readability of the patch as-submitted is useful for quick eyeball checks, but I think all serious reviewing is going to be done on local copies. I think this is actually all a red herring since it's pretty easy for the reviewer to run filterdiff anyways. I don't trust filterdiff one bit :-( regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On Tue, May 26, 2009 at 10:09 AM, Tom Lane t...@sss.pgh.pa.us wrote: Greg Stark greg.st...@enterprisedb.com writes: I'll repeat my suggestion that everyone poo-pooed: we can have the mail list filters recognize patches, run filterdiff on them with our prefered options, and attach the result as an additional attachment (or link to some web directory). The argument that was made at the developer meeting is that the preferred way of working will be to apply the submitted patch in one's local git repository, and then do any needed editorialization as a second patch on top of it. So the critical need as I see it is to be able to see a -c version of a patch-in-progress (ie, diff current working state versus some previous committed state). Readability of the patch as-submitted is useful for quick eyeball checks, but I think all serious reviewing is going to be done on local copies. I think this is actually all a red herring since it's pretty easy for the reviewer to run filterdiff anyways. I don't trust filterdiff one bit :-( For any particular reason, or just natural skepticism? I believe there have been some wild-eyed claims tossed around in this space previously that unified diffs don't provide all the same information as context diffs, which is flatly false. AIUI, the reason for the name unified diff is that it combines, or unifies, the before and after versions of the code into a single chunk. The nice thing about this is that when you have a bunch of small changes in a file, you don't end up with all of the surrounding lines repeated in both the before and after sections. If you change four consecutive lines and run a unified diff, you end up with 4 +s, 4 -s, and 6 lines of context (3 before and 3 after), for a total of 14 lines. If you run a context diff, you end up with 4 !s and 6 lines of context in the before section and the same in the after section, for a total of 20 lines, 6 of which are duplicated. This means that in many cases you can see what's changed without having to page up and down in the diff. The not-so-nice thing about unified diffs is that when there is a huge hunk of code that's changed, there are probably by chance a few identical lines buried in there, like }, so the + and - lines end up mixed together in a way that wouldn't happen in a context diff (which would turn the whole thing into two big ! sections). It's no problem for a machine to understand this, but it's hard to read for a human being. I haven't personally verified the filterdiff code, but the transformation is pretty mechanical so I'm not sure why we should believe that it hasn't been implemented correctly without some evidence along those lines. I don't think there's any way to make anyone 100% happy here. I personally prefer unified diffs, so when I'm reviewing a complex patch formatted as a context diff I typically apply it and then run a unified diff using git. When I'm submitting a patch I use a unified diff to check my work and then convert it to a context diff for submission. On the other hand, I assume that, if you were presented with a complex unified diff, would just apply it and then run a context-diff to review it. Since, as you say, serious reviewing will be done on local copies anyway, I really don't see the point of worrying too much about how they're submitted to the mailing list. Let's just tell everyone to keep using context diffs as the have been doing, and if anyone doesn't then let's THROW THEIR PATCH ON THE DUST-HEAP OF HISTORY AND HAUL THEM OUT TO BE DRAWN AND QUARTERED... er, um, I mean, ask them not to do it that way the next time. If there's an issue here that's worth getting worked up about, I'm not seeing it. ...Robert -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Robert Haas robertmh...@gmail.com writes: On Tue, May 26, 2009 at 10:09 AM, Tom Lane t...@sss.pgh.pa.us wrote: I don't trust filterdiff one bit :-( For any particular reason, or just natural skepticism? IIRC it was demonstrated to be broken the last time it was proposed as a solution to our problems. Maybe it's been fixed since then, but I don't have any confidence in it, since evidently it's not been stress tested very hard. I believe there have been some wild-eyed claims tossed around in this space previously that unified diffs don't provide all the same information as context diffs, which is flatly false. No, the gripe has always been just that they're less readable for nontrivial changes. The not-so-nice thing about unified diffs is that when there is a huge hunk of code that's changed, there are probably by chance a few identical lines buried in there, like }, so the + and - lines end up mixed together in a way that wouldn't happen in a context diff (which would turn the whole thing into two big ! sections). It's no problem for a machine to understand this, but it's hard to read for a human being. Exactly. Even without identical lines, I find that the old and new code gets intermixed in easily-confusing ways. -u is very readable for isolated single-line changes, but for anything larger, not so much. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Tom Lane escribió: Robert Haas robertmh...@gmail.com writes: On Tue, May 26, 2009 at 10:09 AM, Tom Lane t...@sss.pgh.pa.us wrote: I don't trust filterdiff one bit :-( For any particular reason, or just natural skepticism? IIRC it was demonstrated to be broken the last time it was proposed as a solution to our problems. Maybe it's been fixed since then, but I don't have any confidence in it, since evidently it's not been stress tested very hard. I think you're probably confusing it with interdiff. I've had the latter fail several times (and I haven't really used it all that much), but I've never seem filterdiff make a mistake even though I use it frequently. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Alvaro Herrera alvhe...@commandprompt.com writes: Tom Lane escribió: IIRC it was demonstrated to be broken the last time it was proposed as a solution to our problems. Maybe it's been fixed since then, but I don't have any confidence in it, since evidently it's not been stress tested very hard. I think you're probably confusing it with interdiff. No, because I never heard of interdiff before. Checking the archives, the discussion I was remembering was definitely about filterdiff, but the rap on it was undocumented (so maybe demonstrated is too harsh): http://archives.postgresql.org/pgsql-hackers/2007-10/msg01243.php regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Uhm the rap you quoted was ambiguous but I read it as referring to the ability I described if viewing the difference between two patches -- which I didn't name but is in fact interdiff. -- Greg On 26 May 2009, at 19:58, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@commandprompt.com writes: Tom Lane escribió: IIRC it was demonstrated to be broken the last time it was proposed as a solution to our problems. Maybe it's been fixed since then, but I don't have any confidence in it, since evidently it's not been stress tested very hard. I think you're probably confusing it with interdiff. No, because I never heard of interdiff before. Checking the archives, the discussion I was remembering was definitely about filterdiff, but the rap on it was undocumented (so maybe demonstrated is too harsh): http://archives.postgresql.org/pgsql-hackers/2007-10/msg01243.php regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Greg Stark greg.st...@enterprisedb.com writes: On 26 May 2009, at 19:58, Tom Lane t...@sss.pgh.pa.us wrote: http://archives.postgresql.org/pgsql-hackers/2007-10/msg01243.php Uhm the rap you quoted was ambiguous but I read it as referring to the ability I described if viewing the difference between two patches -- which I didn't name but is in fact interdiff. [ squint... ] Hmm, maybe you're right. I see how it could be read that way, anyway. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Hi, Tom Lane t...@sss.pgh.pa.us writes: The rearrangement might be marginally nicer from a code beautification point of view --- right now we're a bit inconsistent about whether datatype-specific hash functions live in hashfunc.c or in the datatype's utils/adt/ file. But I'm not sure that removing hashfunc.c altogether is an appropriate solution to that, not least because of the loss of CVS history for the functions. I'd be inclined to leave the core hash_any() code where it is, if not all of these functions altogether. I guess someone has to talk about it: git will follow the code even when the file hosting it changes. It's not all magic though: http://kerneltrap.org/node/11765 And when using git, the whole 'keep code movement separate from changes' has an even more fundamental reason: git can track code movement (again, whether moving a whole file or just a function between files), and doing a 'git blame -C' will actually follow code movement between files. It does that by similarity analysis, but it does mean that if you both move the code *and* change it at the same time, git cannot see that 'oh, that function came originally from that other file', and now you get worse annotations about where code actually originated. Having better tools maybe could help maintain the high quality standards that are established code wise, too. Regards, -- dim -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Dimitri Fontaine dfonta...@hi-media.com writes: Tom Lane t...@sss.pgh.pa.us writes: The rearrangement might be marginally nicer from a code beautification point of view --- right now we're a bit inconsistent about whether datatype-specific hash functions live in hashfunc.c or in the datatype's utils/adt/ file. But I'm not sure that removing hashfunc.c altogether is an appropriate solution to that, not least because of the loss of CVS history for the functions. I'd be inclined to leave the core hash_any() code where it is, if not all of these functions altogether. I guess someone has to talk about it: git will follow the code even when the file hosting it changes. That might possibly be relevant a year from now; it is 100% irrelevant to a change being proposed for 8.4. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On Mon, May 25, 2009 at 09:59:14AM -0400, Tom Lane wrote: Dimitri Fontaine dfonta...@hi-media.com writes: Tom Lane t...@sss.pgh.pa.us writes: The rearrangement might be marginally nicer from a code beautification point of view --- right now we're a bit inconsistent about whether datatype-specific hash functions live in hashfunc.c or in the datatype's utils/adt/ file. But I'm not sure that removing hashfunc.c altogether is an appropriate solution to that, not least because of the loss of CVS history for the functions. I'd be inclined to leave the core hash_any() code where it is, if not all of these functions altogether. I guess someone has to talk about it: git will follow the code even when the file hosting it changes. That might possibly be relevant a year from now; it is 100% irrelevant to a change being proposed for 8.4. It's pretty relevant as far as the schedule goes. I'm not alone thinking that the appropriate place to make this change, given buildfarm support, is at the transition to 8.5. CVS is dead. Long live git! :) 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 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] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Tom Lane píše v ne 24. 05. 2009 v 18:46 -0400: Kenneth Marshall k...@rice.edu writes: On Sat, May 23, 2009 at 02:52:49PM -0400, Zdenek Kotala wrote: Attached patch cleanups hash index headers to allow compile hasham for 8.3 version. It helps to improve pg_migrator with capability to migrate database with hash index without reindexing. How does that work with the updated hash functions without a reindex? I looked at this patch and I don't see how it helps pg_migrator at all. It's just pushing some code and function declarations around. The main important thing is move hash_any/hash_uint32 function from hash am. It should reduce amount of duplicated code necessary for old hash index implementation. Rest is only rearrangement and cleanup. The rearrangement might be marginally nicer from a code beautification point of view --- right now we're a bit inconsistent about whether datatype-specific hash functions live in hashfunc.c or in the datatype's utils/adt/ file. I personally prefer to keep it in type definition. AM should be independent on types which are installed and data type code should be self contained. But I'm not sure that removing hashfunc.c altogether is an appropriatera solution to that, not least because of the loss of CVS history for the functions. I'd be inclined to leave the core hash_any() code where it is, if not all of these functions altogether. Until we will have better version control system, hashfunc.c can stay here, but what I need is hashfunc.h. Minimalistic version of this patch is to create hashfuct.h and modified related #include in C and header files. What does seem useful is to refactor the headers so that datatype hash functions don't need to include all of the AM's implementation details. But this patch seems to do both more and less than that --- I don't think it's gotten rid of all external #includes of access/hash.h, and in any case moving the function code is not necessary to that goal. Agree, I will prepare minimalistic version with hashfunc.h only. In any case, the barriers to implementing 8.3-style hash indexes in 8.4 are pretty huge: you'd need to duplicate not only the hash AM code, but also all the hash functions, and therefore all of the hash pg_amop and pg_amproc entries. I'm not sure if I need duplicate functions. Generally yes but It seems to me that hash index does not changed functions behavior and they could be shared at this moment. Given the close-to-zero usefulness of hash indexes in production installations, I don't think it's worth the trouble. It would be much more helpful to look into supporting 8.3-compatible GIN indexes. Agree, I wanted to quickly verify function naming collision problem and HASH index seems to me better candidate for this basic test. GIN has priority. Zdenek -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
David, * David Fetter (da...@fetter.org) wrote: It's pretty relevant as far as the schedule goes. I'm not alone thinking that the appropriate place to make this change, given buildfarm support, is at the transition to 8.5. CVS is dead. Long live git! :) I'm all for moving to git, but not until at least the core folks are more familiar with it and have been using it. I don't believe that experience will be there by the time we open for 8.5 and a forced march when we have numerous big things hopefully hitting on the first commitfest seems like a bad idea. I would encourage core, committers and contributors to start becoming familiar with git on the expectation that we'll be making that move when we open for 8.6/9.0. Ideally, there could be an official decision made about when it's going to happen followed by an announcment when 8.4 is released. Thoughts? Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On Mon, May 25, 2009 at 11:45:33AM -0400, Stephen Frost wrote: David, * David Fetter (da...@fetter.org) wrote: It's pretty relevant as far as the schedule goes. I'm not alone thinking that the appropriate place to make this change, given buildfarm support, is at the transition to 8.5. CVS is dead. Long live git! :) I'm all for moving to git, but not until at least the core folks are more familiar with it and have been using it. Which ones aren't familiar and haven't been using it for at least the past year? I count two. I don't believe that experience will be there by the time we open for 8.5 and a forced march when we have numerous big things hopefully hitting on the first commitfest seems like a bad idea. Your portrayal of a rough and complicated transition is not terribly well supported by other projects' switches to git. I would encourage core, committers and contributors to start becoming familiar with git on the expectation that we'll be making that move when we open for 8.6/9.0. Ideally, there could be an official decision made about when it's going to happen followed by an announcment when 8.4 is released. Thoughts? Here's mine: Git delayed is git denied. 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 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] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
David Fetter wrote: On Mon, May 25, 2009 at 09:59:14AM -0400, Tom Lane wrote: Dimitri Fontaine dfonta...@hi-media.com writes: Tom Lane t...@sss.pgh.pa.us writes: The rearrangement might be marginally nicer from a code beautification point of view --- right now we're a bit inconsistent about whether datatype-specific hash functions live in hashfunc.c or in the datatype's utils/adt/ file. But I'm not sure that removing hashfunc.c altogether is an appropriate solution to that, not least because of the loss of CVS history for the functions. I'd be inclined to leave the core hash_any() code where it is, if not all of these functions altogether. I guess someone has to talk about it: git will follow the code even when the file hosting it changes. That might possibly be relevant a year from now; it is 100% irrelevant to a change being proposed for 8.4. It's pretty relevant as far as the schedule goes. I'm not alone thinking that the appropriate place to make this change, given buildfarm support, is at the transition to 8.5. CVS is dead. Long live git! :) That still misses Tom's point, since the change is proposed for 8.4 and at the earliest we would not change SCCMs until after 8.4 is released (and, notwithstanding your eagerness, I suspect it will be rather later). cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
David Fetter da...@fetter.org writes: On Mon, May 25, 2009 at 11:45:33AM -0400, Stephen Frost wrote: I'm all for moving to git, but not until at least the core folks are more familiar with it and have been using it. Which ones aren't familiar and haven't been using it for at least the past year? I count two. I'm not familiar with it, and neither is Bruce, and frankly that's entirely sufficient reason not to change now. What was more or less agreed to at the developer's meeting was that we would move towards git in an orderly fashion. I'm thinking something like six months to a year before cutting over the core repository. If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: David Fetter da...@fetter.org writes: On Mon, May 25, 2009 at 11:45:33AM -0400, Stephen Frost wrote: I'm all for moving to git, but not until at least the core folks are more familiar with it and have been using it. Which ones aren't familiar and haven't been using it for at least the past year? I count two. I'm not familiar with it, and neither is Bruce, and frankly that's entirely sufficient reason not to change now. What was more or less agreed to at the developer's meeting was that we would move towards git in an orderly fashion. The rest have already been moving to it in an orderly fashion, some for over than a year. I'm thinking something like six months to a year before cutting over the core repository. What would gate that? If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? I've been pestering them :) 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 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] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? It looks like this is doable with a suitable git configuration file such as $HOME/.gitconfig or (finer grain) a .git/config for the repository :) 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 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] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Zdenek Kotala zdenek.kot...@sun.com writes: Tom Lane pÃÅ¡e v ne 24. 05. 2009 v 18:46 -0400: In any case, the barriers to implementing 8.3-style hash indexes in 8.4 are pretty huge: you'd need to duplicate not only the hash AM code, but also all the hash functions, and therefore all of the hash pg_amop and pg_amproc entries. I'm not sure if I need duplicate functions. Generally yes but It seems to me that hash index does not changed functions behavior and they could be shared at this moment. No, the behavior of the hash functions themselves changed during 8.4. Twice, even: 2008-04-06 12:54 tgl * contrib/dblink/expected/dblink.out, contrib/dblink/sql/dblink.sql, src/backend/access/hash/hashfunc.c, src/include/catalog/catversion.h, src/test/regress/expected/portals.out, src/test/regress/sql/portals.sql: Improve hash_any() to use word-wide fetches when hashing suitably aligned data. This makes for a significant speedup at the cost that the results now vary between little-endian and big-endian machines; which forces us to add explicit ORDER BYs in a couple of regression tests to preserve machine-independent comparison results. Also, force initdb by bumping catversion, since the contents of hash indexes will change (at least on big-endian machines). Kenneth Marshall and Tom Lane, based on work from Bob Jenkins. This commit does not adopt Bob's new faster mix() algorithm, however, since we still need to convince ourselves that that doesn't degrade the quality of the hashing. 2009-02-09 16:18 tgl * src/: backend/access/hash/hashfunc.c, include/catalog/catversion.h, test/regress/expected/polymorphism.out, test/regress/expected/union.out, test/regress/sql/polymorphism.sql: Adopt Bob Jenkins' improved hash function for hash_any(). This changes the contents of hash indexes (again), so bump catversion. Kenneth Marshall So as far as I can see, you need completely separate copies of both hash_any() and the SQL-level functions that call it. I'm not really seeing that the proposed refactoring makes this any easier. You might as well just copy-and-paste all that old code into a separate set of files, and not worry about what is in access/hash.h. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
David Fetter wrote: On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? It looks like this is doable with a suitable git configuration file such as $HOME/.gitconfig or (finer grain) a .git/config for the repository :) Can you be more specific on the necessary contents of such file? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
David Fetter da...@fetter.org writes: On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? It looks like this is doable with a suitable git configuration file such as $HOME/.gitconfig or (finer grain) a .git/config for the repository :) Cool, let's see one. If we were to put it into a repository config file, that would more or less have the effect of enforcing a project style for diffs, no? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On 05/25/2009 07:20 PM, Alvaro Herrera wrote: David Fetter wrote: On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? It looks like this is doable with a suitable git configuration file such as $HOME/.gitconfig or (finer grain) a .git/config for the repository :) Can you be more specific on the necessary contents of such file? A very sketchy notion of it is at: http://wiki.postgresql.org/wiki/Talk:Working_with_Git I will try to correct the wording + windows information after eating. Andres -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On 05/25/2009 07:31 PM, Tom Lane wrote: David Fetterda...@fetter.org writes: On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? It looks like this is doable with a suitable git configuration file such as $HOME/.gitconfig or (finer grain) a .git/config for the repository :) Cool, let's see one. If we were to put it into a repository config file, that would more or less have the effect of enforcing a project style for diffs, no? Yes and no. You can define that a subset (or all) files use a specific diff driver in the repository - unfortunately the definition of that driver has to be done locally. Defining it currently involves installing a wrapper like the one on http://wiki.postgresql.org/wiki/Talk:Working_with_Git and doing Andres -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On 05/25/2009 07:53 PM, Andres Freund wrote: On 05/25/2009 07:31 PM, Tom Lane wrote: David Fetterda...@fetter.org writes: On Mon, May 25, 2009 at 12:24:05PM -0400, Tom Lane wrote: If you'd like to accomplish something *useful* about this, how about pestering git upstream to support diff -c output format? It looks like this is doable with a suitable git configuration file such as $HOME/.gitconfig or (finer grain) a .git/config for the repository :) Cool, let's see one. If we were to put it into a repository config file, that would more or less have the effect of enforcing a project style for diffs, no? Yes and no. You can define that a subset (or all) files use a specific diff driver in the repository - unfortunately the definition of that driver has to be done locally. Defining it currently involves installing a wrapper like the one on http://wiki.postgresql.org/wiki/Talk:Working_with_Git and doing Ugh, hit the wrong key: and executing `git config --global diff.context.command git-external-diff` Andres -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Andres Freund and...@anarazel.de writes: You can define that a subset (or all) files use a specific diff driver in the repository - unfortunately the definition of that driver has to be done locally. Defining it currently involves installing a wrapper like the one on http://wiki.postgresql.org/wiki/Talk:Working_with_Git and doing Ugh, hit the wrong key: and executing `git config --global diff.context.command git-external-diff` Okay, so it will more or less have to be a local option. That's okay ... all I really insist on is being able to get a readable diff out of it. I grant that not everyone may have the same opinion about what's readable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
On Sat, May 23, 2009 at 02:52:49PM -0400, Zdenek Kotala wrote: I forgot to fix contrib. Updated patch attached. Zdenek Zdenek Kotala pe v p?? 22. 05. 2009 v 16:23 -0400: Attached patch cleanups hash index headers to allow compile hasham for 8.3 version. It helps to improve pg_migrator with capability to migrate database with hash index without reindexing. I discussed this patch year ago with Alvaro when we tried to cleanup include bloating problem. It should reduce also number of including. The main point is that hash functions for datatypes are now in related data files in utils/adt directory. hash_any() and hash_uint32 it now in utils/hashfunc.c. It would be nice to have this in 8.4 because it allows to test index migration functionality. Thanks Zdenek How does that work with the updated hash functions without a reindex? Regards, Ken -- 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] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Kenneth Marshall k...@rice.edu writes: On Sat, May 23, 2009 at 02:52:49PM -0400, Zdenek Kotala wrote: Attached patch cleanups hash index headers to allow compile hasham for 8.3 version. It helps to improve pg_migrator with capability to migrate database with hash index without reindexing. How does that work with the updated hash functions without a reindex? I looked at this patch and I don't see how it helps pg_migrator at all. It's just pushing some code and function declarations around. The rearrangement might be marginally nicer from a code beautification point of view --- right now we're a bit inconsistent about whether datatype-specific hash functions live in hashfunc.c or in the datatype's utils/adt/ file. But I'm not sure that removing hashfunc.c altogether is an appropriate solution to that, not least because of the loss of CVS history for the functions. I'd be inclined to leave the core hash_any() code where it is, if not all of these functions altogether. What does seem useful is to refactor the headers so that datatype hash functions don't need to include all of the AM's implementation details. But this patch seems to do both more and less than that --- I don't think it's gotten rid of all external #includes of access/hash.h, and in any case moving the function code is not necessary to that goal. In any case, the barriers to implementing 8.3-style hash indexes in 8.4 are pretty huge: you'd need to duplicate not only the hash AM code, but also all the hash functions, and therefore all of the hash pg_amop and pg_amproc entries. Given the close-to-zero usefulness of hash indexes in production installations, I don't think it's worth the trouble. It would be much more helpful to look into supporting 8.3-compatible GIN indexes. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
I forgot to fix contrib. Updated patch attached. Zdenek Zdenek Kotala píše v pá 22. 05. 2009 v 16:23 -0400: Attached patch cleanups hash index headers to allow compile hasham for 8.3 version. It helps to improve pg_migrator with capability to migrate database with hash index without reindexing. I discussed this patch year ago with Alvaro when we tried to cleanup include bloating problem. It should reduce also number of including. The main point is that hash functions for datatypes are now in related data files in utils/adt directory. hash_any() and hash_uint32 it now in utils/hashfunc.c. It would be nice to have this in 8.4 because it allows to test index migration functionality. Thanks Zdenek hash_02.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)
Attached patch cleanups hash index headers to allow compile hasham for
8.3 version. It helps to improve pg_migrator with capability to migrate
database with hash index without reindexing.
I discussed this patch year ago with Alvaro when we tried to cleanup
include bloating problem. It should reduce also number of including.
The main point is that hash functions for datatypes are now in related
data files in utils/adt directory. hash_any() and hash_uint32 it now in
utils/hashfunc.c.
It would be nice to have this in 8.4 because it allows to test index
migration functionality.
Thanks Zdenek
diff -Nrc pgsql_indexcompat.5d4d60e3a557/src/backend/access/hash/hashfunc.c pgsql_indexcompat/src/backend/access/hash/hashfunc.c
*** pgsql_indexcompat.5d4d60e3a557/src/backend/access/hash/hashfunc.c 2009-05-22 15:56:34.409314434 -0400
--- pgsql_indexcompat/src/backend/access/hash/hashfunc.c 1969-12-31 19:00:00.0 -0500
***
*** 1,528
- /*-
- *
- * hashfunc.c
- * Support functions for hash access method.
- *
- * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
- * Portions Copyright (c) 1994, Regents of the University of California
- *
- *
- * IDENTIFICATION
- * $PostgreSQL: pgsql/src/backend/access/hash/hashfunc.c,v 1.57 2009/01/01 17:23:35 momjian Exp $
- *
- * NOTES
- * These functions are stored in pg_amproc. For each operator class
- * defined for hash indexes, they compute the hash value of the argument.
- *
- * Additional hash functions appear in /utils/adt/ files for various
- * specialized datatypes.
- *
- * It is expected that every bit of a hash function's 32-bit result is
- * as random as every other; failure to ensure this is likely to lead
- * to poor performance of hash joins, for example. In most cases a hash
- * function should use hash_any() or its variant hash_uint32().
- *-
- */
-
- #include postgres.h
-
- #include access/hash.h
-
-
- /* Note: this is used for both char and boolean datatypes */
- Datum
- hashchar(PG_FUNCTION_ARGS)
- {
- return hash_uint32((int32) PG_GETARG_CHAR(0));
- }
-
- Datum
- hashint2(PG_FUNCTION_ARGS)
- {
- return hash_uint32((int32) PG_GETARG_INT16(0));
- }
-
- Datum
- hashint4(PG_FUNCTION_ARGS)
- {
- return hash_uint32(PG_GETARG_INT32(0));
- }
-
- Datum
- hashint8(PG_FUNCTION_ARGS)
- {
- /*
- * The idea here is to produce a hash value compatible with the values
- * produced by hashint4 and hashint2 for logically equal inputs; this is
- * necessary to support cross-type hash joins across these input types.
- * Since all three types are signed, we can xor the high half of the int8
- * value if the sign is positive, or the complement of the high half when
- * the sign is negative.
- */
- #ifndef INT64_IS_BUSTED
- int64 val = PG_GETARG_INT64(0);
- uint32 lohalf = (uint32) val;
- uint32 hihalf = (uint32) (val 32);
-
- lohalf ^= (val = 0) ? hihalf : ~hihalf;
-
- return hash_uint32(lohalf);
- #else
- /* here if we can't count on x 32 to work sanely */
- return hash_uint32((int32) PG_GETARG_INT64(0));
- #endif
- }
-
- Datum
- hashoid(PG_FUNCTION_ARGS)
- {
- return hash_uint32((uint32) PG_GETARG_OID(0));
- }
-
- Datum
- hashenum(PG_FUNCTION_ARGS)
- {
- return hash_uint32((uint32) PG_GETARG_OID(0));
- }
-
- Datum
- hashfloat4(PG_FUNCTION_ARGS)
- {
- float4 key = PG_GETARG_FLOAT4(0);
- float8 key8;
-
- /*
- * On IEEE-float machines, minus zero and zero have different bit patterns
- * but should compare as equal. We must ensure that they have the same
- * hash value, which is most reliably done this way:
- */
- if (key == (float4) 0)
- PG_RETURN_UINT32(0);
-
- /*
- * To support cross-type hashing of float8 and float4, we want to return
- * the same hash value hashfloat8 would produce for an equal float8 value.
- * So, widen the value to float8 and hash that. (We must do this rather
- * than have hashfloat8 try to narrow its value to float4; that could fail
- * on overflow.)
- */
- key8 = key;
-
- return hash_any((unsigned char *) key8, sizeof(key8));
- }
-
- Datum
- hashfloat8(PG_FUNCTION_ARGS)
- {
- float8 key = PG_GETARG_FLOAT8(0);
-
- /*
- * On IEEE-float machines, minus zero and zero have different bit patterns
- * but should compare as equal. We must ensure that they have the same
- * hash value, which is most reliably done this way:
- */
- if (key == (float8) 0)
- PG_RETURN_UINT32(0);
-
- return hash_any((unsigned char *) key, sizeof(key));
- }
-
- Datum
- hashoidvector(PG_FUNCTION_ARGS)
- {
- oidvector *key = (oidvector *) PG_GETARG_POINTER(0);
-
- return hash_any((unsigned char *) key-values, key-dim1 * sizeof(Oid));
- }
-
- Datum
- hashint2vector(PG_FUNCTION_ARGS)
- {
- int2vector *key = (int2vector *) PG_GETARG_POINTER(0);
-
