On Thu, Dec 19, 2013 at 5:44 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-12-18 21:42:25 -0500, Robert Haas wrote:
>> On Wed, Dec 18, 2013 at 5:54 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> >> 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?
>>
>> I'm just copying the existing logic. See the final stanza of
>> heap_prepare_freeze_tuple.
>
> Yes, but why don't you keep that in heap_prepare_freeze_tuple()? Just
> because of HeapTupleHeaderSetXminCommitted()?
Yes, that's pretty much it.
> I dislike transporting the
> infomask in the wal record and then changing it away from that again afterwards.
I don't really see a problem with it. Relying on the macros to tweak
the bits seems more future-proof than inventing some other way to do
it (that might even get copied into other parts of the code where it's
even less safe). I actually think transporting the infomask is kind
of a funky way to handle this in the first instance, but I don't think
it's this patch's job to kibitz that decision.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company