Re: Confusing with commit time usage in logical decoding

On 01/03/16 18:18, Andres Freund wrote:
> Hi,
>
> On 2016-03-01 18:09:28 +0100, Petr Jelinek wrote:
>> On 01/03/16 17:57, Alvaro Herrera wrote:
>>> Artur Zakirov wrote:
>>>> Hello, Andres
>>>>
>>>> You have introduced a large replication progress tracking infrastructure
>>>> last year. And there is a problem described at the link in the quote below.
>>>>
>>>> Attached patch fix this issue. Is this patch correct? I will be grateful if
>>>> it is and if it will be committed.
>>>
>>> AFAICS this is clearly a bug introduced in 5aa235042:
>>>
>>>      /* replay actions of all transaction + subtransactions in order */
>>>      ReorderBufferCommit(ctx->reorder, xid, buf->origptr, buf->endptr,
>>> -                       parsed->xact_time);
>>> +                       commit_time, origin_id, origin_lsn);
>>>   }
>>>
>>
>> Well yeah but the commit_time is set few lines above as Artur pointed out, I
>> think the proposed fix is correct one.
>
> I'd rather just initialize commit_time to parsed->xact_time.
>
> This indeed is clearly a bug. I do wonder if anybody has a good idea
> about how to add regression tests for this? It's rather annoying that
> we have to suppress timestamps in the test_decoding tests, because
> they're obviously not reproducible...
>

The test for commit timestamps checks that the timestamps are within 
reasonable time frame (for example, bigger than value of a timestamp 
column in the table since that's assigned before commit obviously) , 
it's not perfect but similar approach should catch issues like this one.

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