On Sat, Jul 15, 2017 at 4:25 AM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Sat, Jul 15, 2017 at 6:27 AM, Ashutosh Sharma <ashu.coe...@gmail.com> > wrote: >> I do agree with Amit. I think hash index is slightly trickier (in >> terms of how it is organised) than other indexes and that could be the >> reason for maintaining common code for hashbuild and hashbuildempty. > > Well, you both and Robert worked more on this code for PG10 than I > did, so I am fine to rely on your judgement for the final result. > Still I find this special handling quite surprising. All other AMs > just always log FPWs for the init fork pages so I'd rather not break > this treaty, but that's one against the world as things stand > currently on this thread ;)
It seems to me that this area might benefit from a broader rethink. It's not very nice to impose a restriction that init forks can only be constructed using log_newpage(); on the other hand, it is also not very nice that Amit's patch needs to recapitulate the mysterious incantation used by XLogReadBufferForRedoExtended in several more places. The basic problem here is that it would be really easy for the next person who adds an index AM to make the exact same mistake that Amit made here and that I failed to spot during code review. It would be nice to come up with some more generic solution to the problem rather than relying on each AM to do insert this FlushOneBuffer() incantation in each place where it's needed. I think there are probably several ways to do that; one idea might be to flush all init-fork buffers in bulk at the end of recovery. However, it doesn't really seem urgent to tinker with that; while I think the fact that existing AMs use log_newpage() to populate the init fork is mostly just luck, it might well be 10 or 20 years before somebody adds another AM that happens to use any other technique. Moreover, at the moment, we're trying to get a release out the door, and to me that argues for keeping structural changes to a minimum. Amit's patch seems like a pretty surgical fix to this problem with minimal chance of breaking anything new, and that seems like the right kind of fix for July. So I plan to commit what Amit proposed (with a few grammar corrections). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
