Hi, On Tue, 9 Aug 2022 00:21:02 +0900 Yugo NAGATA <nag...@sraoss.co.jp> wrote:
> On Wed, 27 Jul 2022 22:50:55 -0400 > Tom Lane <t...@sss.pgh.pa.us> wrote: > > > Yugo NAGATA <nag...@sraoss.co.jp> writes: > > > I've looked at the commited fix. What I wonder is whether a change in > > > IsInTransactionBlock() is necessary or not. > > > > I've not examined ANALYZE's dependencies on this closely, but it doesn't > > matter really, because I'm not willing to assume that ANALYZE is the > > only caller. There could be external modules with stronger assumptions > > that IsInTransactionBlock() yielding false provides guarantees equivalent > > to PreventInTransactionBlock(). It did before this patch, so I think > > it needs to still do so after. > > Thank you for your explanation. I understood that IsInTransactionBlock() > and PreventInTransactionBlock() share the equivalent assumption. > > As to ANALYZE, after investigating the code more, I found that setting > XACT_FLAGS_NEEDIMMEDIATECOMMIT in IsInTransactionBlock() is needed indeed. > That is, some flags in pg_class such as relhasindex can be safely updated > only if ANALYZE is not in a transaction block and never rolled back. So, > in a pipeline, ANALYZE must be immediately committed. > > However, I think we need more comments on these functions to clarify what > users can expect or not for them. It is ensured that the statement that > calls PreventInTransactionBlock() or receives false from > IsInTransactionBlock() never be rolled back if it finishes successfully. > This can eliminate the harmful influence of non-rollback-able side effects. > > On the other hand, it cannot ensure that the statement calling these > functions is the first or only one in the transaction in pipelining. If > there are preceding statements in a pipeline, they are committed in the > same transaction of the current statement. > > The attached patch tries to add comments explaining it on the functions. I forward it to the hackers list because the patch is to fix comments. Also, I'll register it to commitfest. The past discussion is here. https://www.postgresql.org/message-id/flat/17434-d9f7a064ce2a88a3%40postgresql.org > > > > In fact, the result of IsInTransactionBlock does not make senses at > > > all in pipe-line mode regardless to the fix. ANALYZE could commit all > > > previous commands in pipelining, and this may not be user expected > > > behaviour. > > > > This seems pretty much isomorphic to the fact that CREATE DATABASE > > will commit preceding steps in the pipeline. > > I am not sure if we can think CREATE DATABASE case and ANLALYZE case > similarly. First, CREATE DATABASE is one of the commands that cannot be > executed inside a transaction block, but ANALYZE can be. So, users would > not be able to know ANALYZE in a pipeline causes a commit from the > documentation. Second, ANALYZE issues a commit internally in an early > stage not only after it finished successfully. For example, even if > ANALYZE is failing because a not-existing column name is specified, it > issues a commit before checking the column name. This makes more hard > to know which statements will be committed and which statements not > committed in a pipeline. Also, as you know, there are other commands > that issue internal commits. > > > That's not great, > > I admit; we'd not have designed it like that if we'd had complete > > understanding of the behavior at the beginning. But it's acted > > like that for a couple of decades now, so changing it seems far > > more likely to make people unhappy than happy. The same for > > ANALYZE in a pipeline. > > > > If the first command in a pipeline is DDL commands such as CREATE > > > DATABASE, this is allowed and immediately committed after success, as > > > same as the current behavior. Executing such commands in the middle of > > > pipeline is not allowed because the pipeline is regarded as "an implicit > > > transaction block" at that time. Similarly, ANALYZE in the middle of > > > pipeline can not close and open transaction. > > > > I'm not going there. If you can persuade some other committer that > > this is worth breaking backward compatibility for, fine; the user > > complaints will be their problem. > > I don't have no idea how to reduce the complexity explained above and > clarify the transactional behavior of pipelining to users other than the > fix I proposed in the previous post. However, I also agree that such > changing may make some people unhappy. If there is no good way and we > would not like to change the behavior, I think it is better to mention > the effects of commands that issue internal commits in the documentation > at least. > > Regards, > Yugo Nagata > > -- > Yugo NAGATA <nag...@sraoss.co.jp> -- Yugo NAGATA <nag...@sraoss.co.jp>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 50f092d7eb..ee9f0fafdd 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -3455,6 +3455,10 @@ AbortCurrentTransaction(void) * * We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ensure * that postgres.c follows through by committing after the statement is done. + * This ensures that the statement that calls this function never be rolled + * back if it finishes successfully. Note that if there are preceding statements + * in a pipeline, they are committed in the same transaction of the current + * statement. * * isTopLevel: passed down from ProcessUtility to determine whether we are * inside a function. (We will always fail if this is false, but it's @@ -3573,6 +3577,10 @@ CheckTransactionBlock(bool isTopLevel, bool throwError, const char *stmtType) * a transaction block than when running as single commands. ANALYZE is * currently the only example. * + * Returning false ensures that the statement that calls this function never + * be rolled back if it finishes successfully. See comments on + * PreventInTransactionBlock. + * * isTopLevel: passed down from ProcessUtility to determine whether we are * inside a function. */
