Re: [HACKERS] Re: pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

On Thu, Jun 9, 2016 at 10:28 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> [Thanks to Robert to stepping up to keep this moving while I was
> down yesterday with a minor injury. I'm back on it today.]
>> Generally, I think I see the hazard you're concerned about: I had
>> failed to realize, as you mentioned upthread, that new index pages
>> would have an LSN of 0. So if a tuple is pruned early and then the
>> index is reindexed, old snapshots won't realize that data is missing.
>> What I'm less certain about is whether you can actually get by with
>> reusing ii_BrokenHotChain to handle this case.
>
> v2 and later does not do that. v1 did, but that was a more blunt
> instrument.
>
>> For example, note this comment:
>>
>> * However, when reindexing an existing index, we should do nothing here.
>> * Any HOT chains that are broken with respect to the index must predate
>> * the index's original creation, so there is no need to change the
>> * index's usability horizon. Moreover, we *must not* try to change the
>> * index's pg_index entry while reindexing pg_index itself, and this
>> * optimization nicely prevents that.
>>
>> This logic doesn't apply to the old snapshot case; there, you'd need
>> to distrust the index whether it was an initial build or a REINDEX,
>> but it doesn't look like that's what the patch does. I'm worried
>> there could be other places where we rely on ii_BrokenHotChain to
>> detect only one specific condition that isn't quite the same as what
>> you're trying to use it for here.
>
> Well spotted. I had used a lot of discreet calls to get that
> reindex logic right, but it was verbose and ugly, so I had just
> added the new macros in this patch and started to implement them
> before I knocked off for the day. At handover I was too distracted
> to remember where I was with it. :-( See if it looks right to you
> now.
>
> Attached is v3. I will commit this patch to resolve this issue
> tomorrow, barring any objections before then.

So I like the idea of centralizing checks in
RelationAllowsEarlyVacuum, but shouldn't it really be called
RelationAllowsEarlyPruning?

Will look at this a bit more if I get time.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company