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

On Mon, Jul 17, 2017 at 10:21 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Jul 17, 2017 at 6:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> 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.
>
> Things are centralized in XLogReadBufferForRedoExtended() for init
> fork flushes, which is not something bad in itself as it is the unique
> place doing this work normally for other AMs. And I have to admit that
> the current coding for hash index WAL has the advantage to create less
> WAL quantity as the bitmap and metadata pages get initialized using
> the data of the records themselves. One idea I can think of would be
> to create a README in src/backend/access for index AMs that summarizes
> a basic set of recommendations for each routine used.
>

We already have quite a decent amount of information about indexes in
our docs [1][2].  We might want to extend that as well.

>> 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).
>

Thanks.  Do you have any suggestion for back-branches?  As of now, it
fails badly with below kind of error:

test=> SELECT * FROM t_u_hash;
ERROR:  could not open file "base/16384/16392": No such file or directory

It is explained in another thread [3] where it has been found that the
reason for such an error is that hash indexes are not WAL logged prior
to 10.  Now, we can claim that we don't recommend hash indexes to be
used prior to 10 in production, so such an error is okay even if there
is no crash has happened in the system.

[1] - https://www.postgresql.org/docs/devel/static/indexam.html
[2] - https://www.postgresql.org/docs/devel/static/index-functions.html
[3] - https://www.postgresql.org/message-id/20170630005634.GA4448%40momjian.us

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com