Re: now() vs transaction_timestamp()

I wrote:
> Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)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