heap_xlog_lock forgets to reset HEAP_XMAX_INVALID

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

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

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

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

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

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

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

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