On Fri, Mar 18, 2016 at 1:19 PM, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote: > Please, find the new version of the patch attached. Now it has WAL > functionality. > > Detailed description of the feature you can find in README draft > https://goo.gl/50O8Q0 > > This patch is pretty complicated, so I ask everyone, who interested in this > feature, > to help with reviewing and testing it. I will be grateful for any feedback. > But please, don't complain about code style, it is still work in progress. > > Next things I'm going to do: > 1. More debugging and testing. I'm going to attach in next message couple of > sql scripts for testing. > 2. Fix NULLs processing > 3. Add a flag into pg_index, that allows to enable/disable compression for > each particular index. > 4. Recheck locking considerations. I tried to write code as less invasive as > possible, but we need to make sure that algorithm is still correct. > 5. Change BTMaxItemSize > 6. Bring back microvacuum functionality.
I really like this idea, and the performance results seem impressive, but I think we should push this out to 9.7. A btree patch that didn't have WAL support until two and a half weeks into the final CommitFest just doesn't seem to me like a good candidate. First, as a general matter, if a patch isn't code-complete at the start of a CommitFest, it's reasonable to say that it should be reviewed but not necessarily committed in that CommitFest. This patch has had some review, but I'm not sure how deep that review is, and I think it's had no code review at all of the WAL logging changes, which were submitted only a week ago, well after the CF deadline. Second, the btree AM is a particularly poor place to introduce possibly destabilizing changes. Everybody depends on it, all the time, for everything. And despite new tools like amcheck, it's not a particularly easy thing to debug.
It's all true. But:
1) It's a great feature many users dream about.
2) Patch is not very big.
3) Patch doesn't introduce significant infrastructural changes. It just change some well-isolated placed.
Let's give it a chance. I've signed as additional reviewer and I'll do my best in spotting all possible issues in this patch.
