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

On Sat, Jul 8, 2017 at 8:52 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jul 6, 2017 at 5:54 AM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Check for INIT_FORKNUM appears both accompanied and not
>> accompanied by check for RELPER.._UNLOGGED, so I'm not sure which
>> is the convention here.
>
> Checking only for INIT_FORKNUM seems logical to me. Also checking for
> RELPERSISTENCE_UNLOGGED just makes the code longer to no benefit. I
> guess Amit copied the test from ATExecSetTablespace, which does it as
> he did, but it seems unnecessarily long-winded.
>

Okay. If you and Michael feel the check that way is better, I will
change and submit the patch.

>> The difference of the two is an init fork of TEMP
>> relations. However I believe that init fork is by definition a
>> part of an unlogged relation, it seems to me that it ought not to
>> be wal-logged if we had it. From this viewpoint, the middle line
>> makes sense.
>
> Actually, the init fork of an unlogged relation *must* be WAL-logged.
> All other forks are removed on a crash (and the main fork is copied
> anew from the init fork). But the init fork itself must survive -
> therefore it, and only it, must be WAL-logged and thus durable.
>
>> By the way the comment of the function ReadBufferWithoutRelcache
>> has the following sentense.
>>
>> | * NB: At present, this function may only be used on permanent relations, which
>> | * is OK, because we only use it during XLOG replay. If in the future we
>> | * want to use it on temporary or unlogged relations, we could pass additional
>> | * parameters.
>>
>> and does
>>
>> | return ReadBuffer_common(smgr, RELPERSISTENCE_PERMANENT, forkNum, blockNum,
>> mode, strategy, &hit);
>>
>> This surely works since BufferAlloc recognizes INIT_FORK. But
>> some adjustment may be needed around here.
>
> Yeah, it should probably mention that the init fork of an unlogged
> relation is also OK.
>

I think we should do that as a separate patch (I can write the same as
well) because that is not new behavior introduced by this patch, but
let me know if you think that we should add such a comment in this
patch itself.

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