Re: RFC: Making TRUNCATE more "MVCC-safe"

On Sun, Mar 4, 2012 at 2:28 AM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
> On Sat, Mar 3, 2012 at 14:53, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Thanks Noah for drawing attention to this thread. I hadn't been
>> watching. As you say, this work would allow me to freeze rows at load
>> time and avoid the overhead of hint bit setting, which avoids
>> performance issues from hint bit setting in checksum patch.
>>
>> I've reviewed this patch and it seems OK to me. Good work Marti.
>
> Thanks! This approach wasn't universally liked, but if it gets us
> tangible benefits (COPY with frozenxid) then I guess it's a reason to
> reconsider.

Comments I see support this idea. If we did this purely for truncate
correctness I probably wouldn't care either, which is why I didn't
read this thread in the first place.

>> v2 patch attached, updated to latest HEAD. Patch adds
>> * relvalidxid is reset by VACUUM/ANALYZE to avoid wraparound failure
>
> Personally I'd rather keep this out of ANALYZE -- since its purpose is
> to collect stats; VACUUM is responsible for correctness (xid
> wraparound etc). But I don't feel strongly about this.

If there were a reason to do it, then I would agree. Later you point
out a reason, so I will make this change.

> A more important consideration is how this interacts with hot standby.
> Currently you compare OldestXmin to relvalidxmin to decide when to
> reset it. But the standby's OldestXmin may be older than the master's.
> (If VACUUM removes rows then this causes a recovery conflict, but
> AFAICT that won't happen if only relvalidxmin changes)

As of 9.1, the standby's oldestxmin is incorporated into the master's
via hot_standby_feedback, so it wouldn't typically be a problem these
days. It's possible that the standby has this set off by choice, in
which case anomalies could exist in the case that a VACUUM doesn't
clean any rows, as you say.

So we'll use vacrelstats->latestRemovedXid instead of OldestXmin when
we call vac_update_relstats()
which means ANALYZE should always pass InvalidTransactionId.

> It might be more robust to wait until relfrozenxid exceeds
> relvalidxmin -- by then, recovery conflict mechanisms will have taken
> care of killing all older snapshots, or am I mistaken?

It would be better to set it as early as possible to reduce the cost
of the test in heap_begin_scan()

> And a few typos the code...
>
> + gettext_noop("When enabled viewing a truncated or newly created table "
> + "will throw a serialization error to prevent MVCC "
> + "discrepancies that were possible priot to 9.2.")
>
> "prior" not "priot"

Yep

> + * Reset relvalidxmin if its old enough
>
> Should be "it's" in this context.

Cool, thanks for the review.

v3 attached.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services