Subject: Re: now() vs transaction_timestamp()
To: Amit Kapila <amit.kapila16@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
 Robert Haas <robertmhaas@gmail.com>
References: <6406dbd2-5d37-4cb6-6eb2-9c44172c7e7c@postgrespro.ru>
 <4268.1538747795@sss.pgh.pa.us> <4911.1538774725@sss.pgh.pa.us>
 <9c78b24f-4ec9-e1dd-bdf6-e09e9b5f0e31@postgrespro.ru>
 <14825.1538842224@sss.pgh.pa.us>
 <CAA4eK1LOQs6mBUHi7dttD_cy3Rq4K7inQR1rhzKv_DGGvYHXVQ@mail.gmail.com>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <3096a889-7ad2-94fa-1c58-468444690d3e@postgrespro.ru>
Date: Sun, 7 Oct 2018 08:42:02 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: 
 <CAA4eK1LOQs6mBUHi7dttD_cy3Rq4K7inQR1rhzKv_DGGvYHXVQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 07.10.2018 07:58, Amit Kapila wrote:
> On Sat, Oct 6, 2018 at 9:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>>> On 06.10.2018 00:25, Tom Lane wrote:
>>>> So maybe the right answer is to change the parallel mode infrastructure
>>>> so it transmits xactStartTimestamp, making transaction_timestamp()
>>>> retroactively safe, and then in HEAD only we could re-mark now() as
>>>> safe.  We might as well do the same for statement_timestamp as well.
>>> Attached please find very small patch fixing the problem (propagating
>>> transaction and statement timestamps to workers).
>> That's a bit too small ;-) ... one demonstrable problem with it is
>> that the parallel worker will report the wrong xactStartTimestamp
>> to pgstat_report_xact_timestamp(), since you aren't jamming the
>> transmitted value in soon enough.  Also, I found that ParallelWorkerMain
>> executes at least two transactions before it ever gets to the "main"
>> transaction that does real work, and I didn't much care for the fact
>> that those were running with worker-local values of xactStartTimestamp
>> and stmtStartTimestamp.  So I rearranged things a bit to ensure that
>> parallel workers wouldn't generate their own values for either
>> timestamp, and pushed it.
>>
> Currently, we serialize the other transaction related stuff via
> PARALLEL_KEY_TRANSACTION_STATE.
Yes, it was my first though to add serialization of timestamps to 
SerializeTransactionState.
But it performs serialization into array of TransactionId, which is 
32-bit (except PGProEE), and so too small for TimestampTz. Certainly it 
is possible to store timestamp into pair of words, but frankly speaking 
I do not like approach used in SerializeTransactionState: IMHO 
serializer should either produce stream of bytes, either use some structure.
Serialization to array of words combines drawbacks of both approaches. 
Also using type TransactionId for something very different from XIDs 
seems to be not so good idea.


>     However, this patch has serialized
> xact_ts via PARALLEL_KEY_FIXED which appears okay, but I think it
> would have been easier for future readers of the code if all the
> similar state variables have been serialized by using the same key.
>