Thread: Simplify set of flags used by MyXactFlags
Hi all, c5660e0 has introduced a new flag for MyXactFlags to restrict two-phase commit from working with temporary objects, and as a matter of fact XACT_FLAGS_ACCESSEDTEMPREL has been kept around to keep the error handling message compatible with past versions, still it is weird to keep both ACCESSEDTEMPNAMESPACE and ACCESSEDTEMPREL as the former implies the latter, so attached is a cleanup patch for HEAD. Keeping both messages makes the error handling at PREPARE time perhaps a bit cleaner to make the difference about schema-level access or table-level access, still I'd rather simplify the code and just only keep the schema-level change as something we complain about. Another thing is that ACCESSEDTEMPREL is used in PreCommit_on_commit_actions() to avoid the truncates of ON COMMIT DELETE ROWS if no temporary tables have been accessed, still it would just mean that truncation would be tried on empty tables for nothing even if say a function is created in pg_temp. Thoughts? -- Michael
Attachment
On 2019-Jan-18, Michael Paquier wrote: > Keeping both messages makes the error handling at PREPARE time perhaps > a bit cleaner to make the difference about schema-level access or > table-level access, still I'd rather simplify the code and just only > keep the schema-level change as something we complain about. Another > thing is that ACCESSEDTEMPREL is used in PreCommit_on_commit_actions() > to avoid the truncates of ON COMMIT DELETE ROWS if no temporary tables > have been accessed, still it would just mean that truncation would be > tried on empty tables for nothing even if say a function is created in > pg_temp. "... operated on temp namespace" doesn't look good; seems to me to be missing an article, for one thing, but really I'm not sure that 'namespace' is the term to be using here. I'd say "... operated on temporary objects" instead (the plural avoids the need for the article; and the user doesn't really care about the namespace itself but rather about the objects it contains.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jan 21, 2019 at 02:58:39PM -0300, Alvaro Herrera wrote: > "... operated on temp namespace" doesn't look good; seems to me to be > missing an article, for one thing, but really I'm not sure that > 'namespace' is the term to be using here. I'd say "... operated on > temporary objects" instead (the plural avoids the need for the article; > and the user doesn't really care about the namespace itself but rather > about the objects it contains.) Thanks for the input. Would you prefer something like the attached then? I have switched the error message to "temporary objects", and renamed the flag to XACT_FLAGS_ACCESSEDTEMPOBJECTS. -- Michael
Attachment
On 2019-Jan-22, Michael Paquier wrote: > On Mon, Jan 21, 2019 at 02:58:39PM -0300, Alvaro Herrera wrote: > > "... operated on temp namespace" doesn't look good; seems to me to be > > missing an article, for one thing, but really I'm not sure that > > 'namespace' is the term to be using here. I'd say "... operated on > > temporary objects" instead (the plural avoids the need for the article; > > and the user doesn't really care about the namespace itself but rather > > about the objects it contains.) > > Thanks for the input. Would you prefer something like the attached > then? I have switched the error message to "temporary objects", and > renamed the flag to XACT_FLAGS_ACCESSEDTEMPOBJECTS. Uh, I didn't think it was necessary nor desirable to rename the flag, only the user-visible message. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jan 24, 2019 at 08:56:05AM -0300, Alvaro Herrera wrote: > Uh, I didn't think it was necessary nor desirable to rename the flag, > only the user-visible message. Oh, OK. I have overstated your comment then. Are you fine with the attached instead? The flag name remains the same, and the comment is updated. -- Michael
Attachment
On 2019-Jan-25, Michael Paquier wrote: > On Thu, Jan 24, 2019 at 08:56:05AM -0300, Alvaro Herrera wrote: > > Uh, I didn't think it was necessary nor desirable to rename the flag, > > only the user-visible message. > > Oh, OK. I have overstated your comment then. Are you fine with the > attached instead? The flag name remains the same, and the comment is > updated. I am, except that the change of "table" to "object" in xact.c line 2262 makes the new paragraph read a bit weird -- it's now saying "if we've added a temp object ... Other objects have the same problem". Doesn't make sense. If you keep that word as "table", it works fine. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 25, 2019 at 12:30:39PM -0300, Alvaro Herrera wrote: > I am, except that the change of "table" to "object" in xact.c line 2262 > makes the new paragraph read a bit weird -- it's now saying "if we've > added a temp object ... Other objects have the same problem". Doesn't > make sense. If you keep that word as "table", it works fine. Indeed. Committed with this suggestion. Thanks for the review. -- Michael