Re: [HACKERS] heap_xlog_lock forgets to reset HEAP_XMAX_INVALID

2016-07-15 Thread Andres Freund
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, 
> > >t_infomask,
> >
> > >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


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


Re: [HACKERS] heap_xlog_lock forgets to reset HEAP_XMAX_INVALID

2016-07-15 Thread Alvaro Herrera
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, 
> >t_infomask,
>  
> >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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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


Re: [HACKERS] heap_xlog_lock forgets to reset HEAP_XMAX_INVALID

2016-07-15 Thread Andres Freund
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, 
>t_infomask,
   
>t_infomask2);

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

Andres


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


Re: [HACKERS] heap_xlog_lock forgets to reset HEAP_XMAX_INVALID

2016-07-15 Thread Alvaro Herrera
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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


[HACKERS] heap_xlog_lock forgets to reset HEAP_XMAX_INVALID

2016-07-15 Thread Andres Freund
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


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