Обсуждение: heap_xlog_lock forgets to reset HEAP_XMAX_INVALID

Hi,

While validating my patch for
http://archives.postgresql.org/message-id/20160714060607.klwgq2qr7egt3zrr%40alap3.anarazel.de
I noticed that the standby's infomask still had HEAP_XMAX_INVALID set
after replaying a XLOG_HEAP_LOCK record.

That's bad, but not really commonly fatal, because unless prepared
transactions are used, locks don't need to be present / valid after
crash-recovery. But it's clearly something we need to fix.

Given that it's been this way for ages, it's not a blocker for
committing the fix for above URL, but I'll try to get in something today
for that as well.  Looks like the minimal fix is just to add    htup->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
htup->t_infomask2&= ~HEAP_KEYS_UPDATED;
 
to heap_xlog_lock and heap_xlog_lock_updated.

Regards,

Andres

Andres Freund wrote:

> While validating my patch for
http://archives.postgresql.org/message-id/20160714060607.klwgq2qr7egt3zrr%40alap3.anarazel.de
> I noticed that the standby's infomask still had HEAP_XMAX_INVALID set
> after replaying a XLOG_HEAP_LOCK record.
> 
> That's bad, but not really commonly fatal, because unless prepared
> transactions are used, locks don't need to be present / valid after
> crash-recovery. But it's clearly something we need to fix.
> 
> Given that it's been this way for ages, it's not a blocker for
> committing the fix for above URL, but I'll try to get in something today
> for that as well.  

Yes, it looks broken, and the consequences for prepared xacts should be
pretty obvious.  Other than those, I think it's pretty innocuous.

> Looks like the minimal fix is just to add
>         htup->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
>         htup->t_infomask2 &= ~HEAP_KEYS_UPDATED;
> to heap_xlog_lock and heap_xlog_lock_updated.

Maybe we should change fix_infomask_from_infobits() to reset
HEAP_XMAX_BITS | HEAP_MOVED too (and HEAP_KEYS_UPDATED I suppose), to
avoid doing it in each callsite.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-07-15 17:23:57 -0400, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > While validating my patch for
http://archives.postgresql.org/message-id/20160714060607.klwgq2qr7egt3zrr%40alap3.anarazel.de
> > I noticed that the standby's infomask still had HEAP_XMAX_INVALID set
> > after replaying a XLOG_HEAP_LOCK record.
> > 
> > That's bad, but not really commonly fatal, because unless prepared
> > transactions are used, locks don't need to be present / valid after
> > crash-recovery. But it's clearly something we need to fix.
> > 
> > Given that it's been this way for ages, it's not a blocker for
> > committing the fix for above URL, but I'll try to get in something today
> > for that as well.  
> 
> Yes, it looks broken, and the consequences for prepared xacts should be
> pretty obvious.  Other than those, I think it's pretty innocuous.

Yea, I can't see anything else right now.


> > Looks like the minimal fix is just to add
> >         htup->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
> >         htup->t_infomask2 &= ~HEAP_KEYS_UPDATED;
> > to heap_xlog_lock and heap_xlog_lock_updated.
> 
> Maybe we should change fix_infomask_from_infobits() to reset
> HEAP_XMAX_BITS | HEAP_MOVED too (and HEAP_KEYS_UPDATED I suppose), to
> avoid doing it in each callsite.

Yea, I was thinking of that as well. But there's code like    htup->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
htup->t_infomask2&= ~HEAP_KEYS_UPDATED;    if (hot_update)        HeapTupleHeaderSetHotUpdated(htup);    else
HeapTupleHeaderClearHotUpdated(htup);   fix_infomask_from_infobits(xlrec->old_infobits_set, &htup->t_infomask,
                    &htup->t_infomask2);
 

so I'd rather only clean this up in master.

Andres

Andres Freund wrote:
> On 2016-07-15 17:23:57 -0400, Alvaro Herrera wrote:

> > Maybe we should change fix_infomask_from_infobits() to reset
> > HEAP_XMAX_BITS | HEAP_MOVED too (and HEAP_KEYS_UPDATED I suppose), to
> > avoid doing it in each callsite.
> 
> Yea, I was thinking of that as well. But there's code like
>         htup->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
>         htup->t_infomask2 &= ~HEAP_KEYS_UPDATED;
>         if (hot_update)
>             HeapTupleHeaderSetHotUpdated(htup);
>         else
>             HeapTupleHeaderClearHotUpdated(htup);
>         fix_infomask_from_infobits(xlrec->old_infobits_set, &htup->t_infomask,
>                                    &htup->t_infomask2);
> 
> so I'd rather only clean this up in master.

Mumble.  I don't see any way that this would matter, but I don't object
to doing the cleanup in master only.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2016-07-15 17:43:44 -0400, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2016-07-15 17:23:57 -0400, Alvaro Herrera wrote:
> 
> > > Maybe we should change fix_infomask_from_infobits() to reset
> > > HEAP_XMAX_BITS | HEAP_MOVED too (and HEAP_KEYS_UPDATED I suppose), to
> > > avoid doing it in each callsite.
> > 
> > Yea, I was thinking of that as well. But there's code like
> >         htup->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
> >         htup->t_infomask2 &= ~HEAP_KEYS_UPDATED;
> >         if (hot_update)
> >             HeapTupleHeaderSetHotUpdated(htup);
> >         else
> >             HeapTupleHeaderClearHotUpdated(htup);
> >         fix_infomask_from_infobits(xlrec->old_infobits_set, &htup->t_infomask,
> >                                    &htup->t_infomask2);
> > 
> > so I'd rather only clean this up in master.

Well, I think we should move setting hot updated into infomask as well,
then rename fix_infomask_from_infobits to set_infomask_from_infobits and
such. I want to get this fix and the heap_update stuff in now, before
the beta, with time to fix potential fallout. So it's the minimal fix,
if I do it...

Andres