Re: Page Scan Mode in Hash Index

On Tue, Aug 22, 2017 at 3:55 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, Aug 22, 2017 at 2:28 PM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>> On Sat, Aug 19, 2017 at 11:37 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Fri, Aug 11, 2017 at 6:51 PM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>>>>>
>>>>> 7.
>>>>> _hash_kill_items(IndexScanDesc scan)
>>>>> {
>>>>> ..
>>>>> + /*
>>>>> + * If page LSN differs it means that the page was modified since the
>>>>> + * last read. killedItems could be not valid so LP_DEAD hints apply-
>>>>> + * ing is not safe.
>>>>> + */
>>>>> + page = BufferGetPage(buf);
>>>>> + if (PageGetLSN(page) != so->currPos.lsn)
>>>>> + {
>>>>> + _hash_relbuf(rel, buf);
>>>>> + return;
>>>>> + }
>>>>> ..
>>>>> }
>>>>>
>>>>> How does this check cover the case of unlogged tables?
>>>>
>>>> Thanks for putting that point. It doesn't cover the case for unlogged
>>>> tables. As suggested by you in one of your email in this mailing list, i am
>>>> now not allowing vacuum to release lock on current page before acquiring
>>>> lock on next page for unlogged tables. This will ensure that scan is always
>>>> behind vacuum if they are running on the same bucket simultaneously.
>>>> Therefore, there is danger in marking tuples as dead for unlogged pages even
>>>> if they are not having any lsn.
>>>>
>>>
>>
>> Once again, Thank you for reviewing my patches.
>>
>>> In the last line, I guess you wanted to say "there is *no* danger
>>> ..."?
>>
>> Yes, i meant that because, it ensures that scan will always be following VACUUM.
>>
>> Today, while again thinking about this part of the patch
>>> (_hash_kill_items) it occurred to me that we can't rely on a pin on an
>>> overflow page to guarantee that it is not modified by Vacuum.
>>> Consider a case where vacuum started vacuuming the bucket before the
>>> scan and then in-between scan overtakes it. Now, it is possible that
>>> even if a scan has a pin on a page (and no lock), vacuum might clean
>>> that page, if that happens, then we can't prevent the reuse of TID.
>>> What do you think?
>>>
>>
>> I think, you are talking about non-mvcc scan case, because in case of
>> mvcc scans, even if we have released both pin and lock on a page,
>> VACUUM can't remove tuples from a page if it is visible to some
>> concurrently running transactions (mvcc scan in our case).
>>
>
> I am talking about tuples that are marked as dead in heap. It has
> nothing to do with the visibility of tuple or type of scan (mvcc or
> non-mvcc).
>

Okay, I got your point now. I think, currently in _hash_kill_items(),
if an overflow page is pinned we do not check if it got modified since
the last read or
not. Hence, if the vacuum runs on an overflow page that is pinned and
also has some dead tuples in it then it could create a problem for
scan basically,
when scan would attempt to mark the killed items as dead. To get rid
of such problem, i think, even if an overflow page is pinned we should
check if it got
modified or not since the last read was performed on the page. If yes,
then do not allow scan to mark killed items as dead. Attached is the
newer version with these changes along with some other cosmetic
changes mentioned in your earlier email. Thanks.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com