On Wed, Nov 9, 2016 at 9:10 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Nov 9, 2016 at 9:04 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I think we can give a brief explanation right in the code comment. I
> referred to "decreasing the TIDs"; you are referring to "moving tuples
> around". But I think that moving the tuples to different locations is
> not the problem. I think the problem is that a tuple might be
> assigned a lower spot in the item pointer array
>
I think we both understand the problem and it is just matter of using
different words. I will go with your suggestion and will try to
slightly adjust the README as well so that both places use same
terminology.
>>> + /*
>>> + * Acquiring cleanup lock to clear the split-in-progress flag ensures that
>>> + * there is no pending scan that has seen the flag after it is cleared.
>>> + */
>>> + _hash_chgbufaccess(rel, bucket_obuf, HASH_NOLOCK, HASH_WRITE);
>>> + opage = BufferGetPage(bucket_obuf);
>>> + oopaque = (HashPageOpaque) PageGetSpecialPointer(opage);
>>>
>>> I don't understand the comment, because the code *isn't* acquiring a
>>> cleanup lock.
>>
>> Oops, this is ramnant from one of the design approach to clear these
>> flags which was later discarded due to issues. I will change this to
>> indicate Exclusive lock.
>
> Of course, an exclusive lock doesn't guarantee anything like that...
>
Right, but we don't need that guarantee (there is no pending scan that
has seen the flag after it is cleared) to clear the flags. It was
written in one of the previous patches where I was exploring the idea
of using cleanup lock to clear the flags and then don't use the same
during vacuum. However, there were some problems in that design and I
have changed the code, but forgot to update the comment.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com