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

On Sat, Jul 8, 2017 at 9:06 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Jul 8, 2017 at 8:52 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Jul 6, 2017 at 5:54 AM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@lab.ntt.co.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