Re: hash index on unlogged tables doesn't behave as expected

On Sat, Jul 15, 2017 at 4:25 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sat, Jul 15, 2017 at 6:27 AM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)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