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