Re: now() vs transaction_timestamp()
On Sun, Oct 7, 2018 at 11:12 AM Konstantin Knizhnik wrote: > > On 07.10.2018 07:58, Amit Kapila wrote: > > On Sat, Oct 6, 2018 at 9:40 PM Tom Lane wrote: > >> Konstantin Knizhnik 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
Re: now() vs transaction_timestamp()
Amit Kapila writes: > On Sun, Oct 7, 2018 at 10:36 AM Tom Lane 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
Re: now() vs transaction_timestamp()
On 07.10.2018 07:58, Amit Kapila wrote: On Sat, Oct 6, 2018 at 9:40 PM Tom Lane wrote: Konstantin Knizhnik 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.
Re: now() vs transaction_timestamp()
On Sun, Oct 7, 2018 at 10:36 AM Tom Lane wrote: > > Amit Kapila writes: > > On Sat, Oct 6, 2018 at 9:40 PM Tom Lane 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
Re: now() vs transaction_timestamp()
Amit Kapila writes: > On Sat, Oct 6, 2018 at 9:40 PM Tom Lane 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
Re: now() vs transaction_timestamp()
On Sat, Oct 6, 2018 at 9:40 PM Tom Lane wrote: > > Konstantin Knizhnik 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
Re: now() vs transaction_timestamp()
Konstantin Knizhnik 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
Re: now() vs transaction_timestamp()
On 06.10.2018 00:25, Tom Lane wrote: I wrote: Konstantin Knizhnik 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). diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index cdaa32e..abc41e7 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -87,6 +87,8 @@ typedef struct FixedParallelState PGPROC *parallel_master_pgproc; pid_t parallel_master_pid; BackendId parallel_master_backend_id; + TimestampTz xact_ts; + TimestampTz stmt_ts; /* Mutex protects remaining fields. */ slock_t mutex; @@ -318,6 +320,8 @@ InitializeParallelDSM(ParallelContext *pcxt) fps->parallel_master_pgproc = MyProc; fps->parallel_master_pid = MyProcPid; fps->parallel_master_backend_id = MyBackendId; + fps->xact_ts = GetCurrentTransactionStartTimestamp(); + fps->stmt_ts = GetCurrentStatementStartTimestamp(); SpinLockInit(&fps->mutex); fps->last_xlog_end = 0; shm_toc_insert(pcxt->toc, PARALLEL_KEY_FIXED, fps); @@ -1351,6 +1355,9 @@ ParallelWorkerMain(Datum main_arg) tstatespace = shm_toc_lookup(toc, PARALLEL_KEY_TRANSACTION_STATE, false); StartParallelWorkerTransaction(tstatespace); + /* Restore current transaction and statement timestamps */ + SetCurrentStartTimestamps(fps->xact_ts, fps->stmt_ts); + /* Restore combo CID s
Re: now() vs transaction_timestamp()
so 6. 10. 2018 v 13:47 odesílatel Amit Kapila napsal: > On Sat, Oct 6, 2018 at 2:55 AM Tom Lane 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 > >
Re: now() vs transaction_timestamp()
On Sat, Oct 6, 2018 at 2:55 AM Tom Lane 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
Re: now() vs transaction_timestamp()
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
Re: now() vs transaction_timestamp()
I wrote: > Konstantin Knizhnik 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
Re: now() vs transaction_timestamp()
Konstantin Knizhnik 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
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
