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

On Sat, Jul 8, 2017 at 9:06 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.
>

Changed as per suggestion.

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

Attached a separate patch to adjust the comment atop ReadBufferWithoutRelcache.

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