Thread: now() vs transaction_timestamp()

Postgres documentation says that "now() is a traditional PostgreSQL equivalent to transaction_timestamp()".
Also both use the same implementation.
But them have different parallel safety property:

postgres=# \df+ now 
                                                                                         List of functions
   Schema   | Name |     Result data type     | Argument data types | Type | Volatility |  Parallel  |  Owner   | Security | Access privileges | Language |
 Source code |       Description       
------------+------+--------------------------+---------------------+------+------------+------------+----------+----------+-------------------+----------+
-------------+--------------------------
 pg_catalog | now  | timestamp with time zone |                     | func | stable     | restricted | knizhnik | invoker  |                   | internal |
 now         | current transaction time
(1 row)

postgres=# \df+ transaction_timestamp
                                                                                                List of functions
   Schema   |         Name          |     Result data type     | Argument data types | Type | Volatility | Parallel |  Owner   | Security | Access privileg
es | Language | Source code |       Description       
------------+-----------------------+--------------------------+---------------------+------+------------+----------+----------+----------+----------------
---+----------+-------------+--------------------------
 pg_catalog | transaction_timestamp | timestamp with time zone |                     | func | stable     | safe     | knizhnik | invoker  |               
   | internal | now         | current transaction time
(1 row)

As a result using now() in query disable parallel execution while transaction_timestamp allows it.
Was it done intentionally or it is just a bug?

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
> Postgres documentation says that |"now()| is a traditional PostgreSQL 
> equivalent to |transaction_timestamp()|".
> Also both use the same implementation.

Right.

> But them have different parallel safety property:

That seems like a bug/thinko.  I am not sure which property setting is
correct though.  It'd only be safe if the parallel-query infrastructure
transfers the relevant timestamp to workers, and I don't know if it does.

            regards, tom lane

I wrote:
> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>> Postgres documentation says that |"now()| is a traditional PostgreSQL 
>> equivalent to |transaction_timestamp()|".
>> Also both use the same implementation.

> Right.

>> But them have different parallel safety property:

> That seems like a bug/thinko.  I am not sure which property setting is
> correct though.  It'd only be safe if the parallel-query infrastructure
> transfers the relevant timestamp to workers, and I don't know if it does.

The answer is that it doesn't; indeed, xactStartTimestamp is local inside
xact.c, and it's easy to see by inspection that there is nothing at all
that sets it except StartTransaction().  What is happening, if you do

set force_parallel_mode to 1;
select transaction_timestamp();

is that the value you get back is the time that the parallel worker did
StartTransaction().  It is easy to show that this is utterly broken:
transaction_timestamp() holds still within a transaction until you set
force_parallel_mode, and then it does not.

regression=# begin; 
BEGIN
regression=# select transaction_timestamp();
     transaction_timestamp     
-------------------------------
 2018-10-05 17:00:11.764019-04
(1 row)

regression=# select transaction_timestamp();
     transaction_timestamp     
-------------------------------
 2018-10-05 17:00:11.764019-04
(1 row)

regression=# select transaction_timestamp();
     transaction_timestamp     
-------------------------------
 2018-10-05 17:00:11.764019-04
(1 row)

regression=# set force_parallel_mode to 1;
SET
regression=# select transaction_timestamp();
     transaction_timestamp     
-------------------------------
 2018-10-05 17:00:21.983122-04
(1 row)

Looking at the related functions, I see that now() and
statement_timestamp() are marked stable/restricted, which is OK, while
clock_timestamp() and timeofday() are marked volatile/safe which seems odd
but on reflection I think it's OK.  Their values wouldn't hold still in
the parent process either, so there's no reason to disallow workers from
running them.

So transaction_timestamp() is definitely buggy, but we're not out of the
woods yet: SQLValueFunction is treated as parallel-safe, but it also has
some instances that are equivalent to transaction_timestamp and so do not
work correctly.

regression=# begin;
BEGIN
regression=# select current_time;
    current_time    
--------------------
 17:12:35.942968-04
(1 row)

regression=# select current_time;
    current_time    
--------------------
 17:12:35.942968-04
(1 row)

regression=# set force_parallel_mode to 1;
SET
regression=# select current_time;
    current_time    
--------------------
 17:12:55.462141-04
(1 row)

regression=# set force_parallel_mode to 0;
SET
regression=# select current_time;
    current_time    
--------------------
 17:12:35.942968-04
(1 row)

Ain't that fun?

My initial thought was that we should just re-mark transaction_timestamp()
as parallel-restricted and call it a day, but we'd then have to do the
same for SQLValueFunction, which is not much fun because it does have
variants that are parallel safe (and teaching max_parallel_hazard_walker
which is which seems like a recipe for bugs).

Also, while it might not be quite too late to force a catversion bump
in v11, this is demonstrably also broken in v10, and we can't do that
there.

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.

Thoughts?

            regards, tom lane

I wrote:
> So transaction_timestamp() is definitely buggy, but we're not out of the
> woods yet: SQLValueFunction is treated as parallel-safe, but it also has
> some instances that are equivalent to transaction_timestamp and so do not
> work correctly.

Oh, and I notice that timestamp_in and related functions are marked
parallel safe, which is equally broken since the conversion of 'now'
also depends on xactStartTimestamp.

            regards, tom lane

On Sat, Oct 6, 2018 at 2:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> My initial thought was that we should just re-mark transaction_timestamp()
> as parallel-restricted and call it a day, but we'd then have to do the
> same for SQLValueFunction, which is not much fun because it does have
> variants that are parallel safe (and teaching max_parallel_hazard_walker
> which is which seems like a recipe for bugs).
>
> Also, while it might not be quite too late to force a catversion bump
> in v11, this is demonstrably also broken in v10, and we can't do that
> there.
>
> 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.
>

+1.  Sounds like a reasonable way to fix the problem.  I can take care
of it (though not immediately) if you want.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 06.10.2018 00:25, Tom Lane wrote:
> I wrote:
>> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>>> Postgres documentation says that |"now()| is a traditional PostgreSQL
>>> equivalent to |transaction_timestamp()|".
>>> Also both use the same implementation.
>> Right.
>>> But them have different parallel safety property:
>> That seems like a bug/thinko.  I am not sure which property setting is
>> correct though.  It'd only be safe if the parallel-query infrastructure
>> transfers the relevant timestamp to workers, and I don't know if it does.
> The answer is that it doesn't; indeed, xactStartTimestamp is local inside
> xact.c, and it's easy to see by inspection that there is nothing at all
> that sets it except StartTransaction().  What is happening, if you do
>
> set force_parallel_mode to 1;
> select transaction_timestamp();
>
> is that the value you get back is the time that the parallel worker did
> StartTransaction().  It is easy to show that this is utterly broken:
> transaction_timestamp() holds still within a transaction until you set
> force_parallel_mode, and then it does not.
>
> regression=# begin;
> BEGIN
> regression=# select transaction_timestamp();
>       transaction_timestamp
> -------------------------------
>   2018-10-05 17:00:11.764019-04
> (1 row)
>
> regression=# select transaction_timestamp();
>       transaction_timestamp
> -------------------------------
>   2018-10-05 17:00:11.764019-04
> (1 row)
>
> regression=# select transaction_timestamp();
>       transaction_timestamp
> -------------------------------
>   2018-10-05 17:00:11.764019-04
> (1 row)
>
> regression=# set force_parallel_mode to 1;
> SET
> regression=# select transaction_timestamp();
>       transaction_timestamp
> -------------------------------
>   2018-10-05 17:00:21.983122-04
> (1 row)
>
> Looking at the related functions, I see that now() and
> statement_timestamp() are marked stable/restricted, which is OK, while
> clock_timestamp() and timeofday() are marked volatile/safe which seems odd
> but on reflection I think it's OK.  Their values wouldn't hold still in
> the parent process either, so there's no reason to disallow workers from
> running them.
>
> So transaction_timestamp() is definitely buggy, but we're not out of the
> woods yet: SQLValueFunction is treated as parallel-safe, but it also has
> some instances that are equivalent to transaction_timestamp and so do not
> work correctly.
>
> regression=# begin;
> BEGIN
> regression=# select current_time;
>      current_time
> --------------------
>   17:12:35.942968-04
> (1 row)
>
> regression=# select current_time;
>      current_time
> --------------------
>   17:12:35.942968-04
> (1 row)
>
> regression=# set force_parallel_mode to 1;
> SET
> regression=# select current_time;
>      current_time
> --------------------
>   17:12:55.462141-04
> (1 row)
>
> regression=# set force_parallel_mode to 0;
> SET
> regression=# select current_time;
>      current_time
> --------------------
>   17:12:35.942968-04
> (1 row)
>
> Ain't that fun?
>
> My initial thought was that we should just re-mark transaction_timestamp()
> as parallel-restricted and call it a day, but we'd then have to do the
> same for SQLValueFunction, which is not much fun because it does have
> variants that are parallel safe (and teaching max_parallel_hazard_walker
> which is which seems like a recipe for bugs).
>
> Also, while it might not be quite too late to force a catversion bump
> in v11, this is demonstrably also broken in v10, and we can't do that
> there.
>
> 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.
>
> Thoughts?
>
>             regards, tom lane
Attached please find very small patch fixing the problem (propagating 
transaction and statement timestamps to workers).

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.

            regards, tom lane

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.   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.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Sat, Oct 6, 2018 at 9:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.   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.

That state is restored at least two transactions too late.

            regards, tom lane

On Sun, Oct 7, 2018 at 10:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Sat, Oct 6, 2018 at 9:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> 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.   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.
>
> That state is restored at least two transactions too late.
>

That is right, but I think we can perform shm_toc_lookup for
PARALLEL_KEY_TRANSACTION_STATE at some earlier point and then use the
variables from it to restore the respective state at the different
point of times.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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.
>

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Sun, Oct 7, 2018 at 10:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That state is restored at least two transactions too late.

> That is right, but I think we can perform shm_toc_lookup for
> PARALLEL_KEY_TRANSACTION_STATE at some earlier point and then use the
> variables from it to restore the respective state at the different
> point of times.

That hardly seems cleaner.

I think this is actually the right way, because
PARALLEL_KEY_TRANSACTION_STATE is (at least by the name) state associated
with the main transaction the worker is going to run.  But given what
I did to xact.c just now, xactStartTimestamp and stmtStartTimestamp are
*not* transaction-local state so far as the worker is concerned.  They
will persist throughout the lifetime of that process, just as the database
ID, user ID, etc, will.  So you might as well argue that none of
FixedParallelState should exist because it should all be in
PARALLEL_KEY_TRANSACTION_STATE, and that doesn't seem like much of
an improvement.

            regards, tom lane

On Sun, Oct 7, 2018 at 11:12 AM Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:
>
> 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.

Right, it seems using another format to add timestampTz in that
serialization routine might turn out to be more invasive especially
considering that we have to back-patch this fix.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com