Hi,

On 2013-12-17 16:00:14 -0500, Robert Haas wrote:
> @@ -5874,19 +5858,27 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, 
> TransactionId cutoff_xid,
>  void
>  heap_execute_freeze_tuple(HeapTupleHeader tuple, xl_heap_freeze_tuple *frz)
>  {
> +     tuple->t_infomask = frz->t_infomask;
> +     tuple->t_infomask2 = frz->t_infomask2;
> +
>       if (frz->frzflags & XLH_FREEZE_XMIN)
> -             HeapTupleHeaderSetXmin(tuple, FrozenTransactionId);
> +             HeapTupleHeaderSetXminFrozen(tuple);
>  
>       HeapTupleHeaderSetXmax(tuple, frz->xmax);
>  
>       if (frz->frzflags & XLH_FREEZE_XVAC)
> +     {
>               HeapTupleHeaderSetXvac(tuple, FrozenTransactionId);
> +             /* If we somehow haven't hinted the tuple previously, do it 
> now. */
> +             HeapTupleHeaderSetXminCommitted(tuple);
> +     }

What's the reasoning behind adding HeapTupleHeaderSetXminCommitted()
here?

> @@ -823,14 +823,14 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
>                                        * NB: Like with per-tuple hint bits, 
> we can't set the
>                                        * PD_ALL_VISIBLE flag if the inserter 
> committed
>                                        * asynchronously. See SetHintBits for 
> more info. Check
> -                                      * that the HEAP_XMIN_COMMITTED hint 
> bit is set because of
> -                                      * that.
> +                                      * that the tuple is hinted 
> xmin-committed hint bit because
> +                                      * of that.
>                                        */

Looks like there's a "hint bit" too much here.

> @@ -65,6 +65,9 @@ manage to be a conflict it would merely mean that one 
> bit-update would
>  be lost and need to be done again later.  These four bits are only hints
>  (they cache the results of transaction status lookups in pg_clog), so no
>  great harm is done if they get reset to zero by conflicting updates.
> +Note, however, that a tuple is frozen by setting both HEAP_XMIN_INVALID
> +and HEAP_XMIN_COMMITTED; this is a critical update and accordingly requires
> +an exclusive buffer lock.

I think it'd be appropriate to mention that this needs to be preserved
via WAL logging, it sounds like it's suficient to set a hint bit without
persistenc guarantees.

(not sure if I already wrote this, but whatever)

Looking good.

Greetings,

Andres Freund

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to