Обсуждение: now() vs transaction_timestamp()
Postgres documentation says that
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?
"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
so 6. 10. 2018 v 13:47 odesílatel Amit Kapila <amit.kapila16@gmail.com> napsal:
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.
+1
Pavel
--
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