Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Thu, Oct 28, 2021 at 8:15 AM Amit Kapila wrote: > > On Wed, Oct 27, 2021 at 4:39 PM Dilip Kumar wrote: > > > > On Tue, Oct 26, 2021 at 9:19 AM Amit Kapila wrote: > > > > > > > > > > > Thanks, both your patches look good to me except that we need to > > > > remove the sentence related to the revert of ade24dab97 from the > > > > commit message. I think we should backpatch the first patch to 14 > > > > where it was introduced and commit the second patch (related to moving > > > > code out of critical section) only to HEAD but we can even backpatch > > > > the second one till 9.6 for the sake of consistency. What do you guys > > > > think? > > > > > > > > > > The other option could be to just commit both these patches in HEAD as > > > there is no correctness issue here. > > > > Right, even I feel we should just commit it to the HEAD as there is no > > correctness issue. > > > > Thanks for your opinion. I'll commit it to the HEAD by next Tuesday > unless someone feels that we should backpatch this. > Pushed. -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Wed, Oct 27, 2021 at 4:39 PM Dilip Kumar wrote: > > On Tue, Oct 26, 2021 at 9:19 AM Amit Kapila wrote: > > > > > > > > Thanks, both your patches look good to me except that we need to > > > remove the sentence related to the revert of ade24dab97 from the > > > commit message. I think we should backpatch the first patch to 14 > > > where it was introduced and commit the second patch (related to moving > > > code out of critical section) only to HEAD but we can even backpatch > > > the second one till 9.6 for the sake of consistency. What do you guys > > > think? > > > > > > > The other option could be to just commit both these patches in HEAD as > > there is no correctness issue here. > > Right, even I feel we should just commit it to the HEAD as there is no > correctness issue. > Thanks for your opinion. I'll commit it to the HEAD by next Tuesday unless someone feels that we should backpatch this. -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Tue, Oct 26, 2021 at 9:19 AM Amit Kapila wrote: > > On Mon, Oct 25, 2021 at 4:21 PM Amit Kapila wrote: > > > > On Thu, Oct 21, 2021 at 11:20 AM Dilip Kumar wrote: > > > > > > On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila > > > wrote: > > > > > > > > > > v5-0001, incorporates all the comment fixes suggested by Alvaro. and > > > 0001 is an additional patch which moves > > > MarkCurrentTransactionIdLoggedIfAny(), out of the critical section. > > > > > > > Thanks, both your patches look good to me except that we need to > > remove the sentence related to the revert of ade24dab97 from the > > commit message. I think we should backpatch the first patch to 14 > > where it was introduced and commit the second patch (related to moving > > code out of critical section) only to HEAD but we can even backpatch > > the second one till 9.6 for the sake of consistency. What do you guys > > think? > > > > The other option could be to just commit both these patches in HEAD as > there is no correctness issue here. Right, even I feel we should just commit it to the HEAD as there is no correctness issue. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Mon, Oct 25, 2021 at 4:21 PM Amit Kapila wrote: > > On Thu, Oct 21, 2021 at 11:20 AM Dilip Kumar wrote: > > > > On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila wrote: > > > > > > > v5-0001, incorporates all the comment fixes suggested by Alvaro. and > > 0001 is an additional patch which moves > > MarkCurrentTransactionIdLoggedIfAny(), out of the critical section. > > > > Thanks, both your patches look good to me except that we need to > remove the sentence related to the revert of ade24dab97 from the > commit message. I think we should backpatch the first patch to 14 > where it was introduced and commit the second patch (related to moving > code out of critical section) only to HEAD but we can even backpatch > the second one till 9.6 for the sake of consistency. What do you guys > think? > The other option could be to just commit both these patches in HEAD as there is no correctness issue here. -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Thu, Oct 21, 2021 at 11:20 AM Dilip Kumar wrote: > > On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila wrote: > > > > v5-0001, incorporates all the comment fixes suggested by Alvaro. and > 0001 is an additional patch which moves > MarkCurrentTransactionIdLoggedIfAny(), out of the critical section. > Thanks, both your patches look good to me except that we need to remove the sentence related to the revert of ade24dab97 from the commit message. I think we should backpatch the first patch to 14 where it was introduced and commit the second patch (related to moving code out of critical section) only to HEAD but we can even backpatch the second one till 9.6 for the sake of consistency. What do you guys think? -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila wrote: > > On Wed, Oct 20, 2021 at 8:49 PM Dilip Kumar wrote: > > > > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera > > wrote: > > > > > > Does MarkTopTransactionIdLogged() have to be inside XLogInsertRecord's > > > critical section? > > > > I think this function is doing somewhat similar things to what we are > > doing in MarkCurrentTransactionIdLoggedIfAny() so put at the same > > place. But I don't see any reason for this to be in the critical > > section. > > > > Yeah, I also don't see any reason for this to be in the critical > section but it might be better to keep both together. So, if we want > to keep MarkTopTransactionIdLogged() out of the critical section in > this patch then we should move the existing function > MarkCurrentTransactionIdLoggedIfAny() in a separate patch so that > future readers doesn't get confused as to why one of these is in the > critical section and other is not. OTOH, we can move > MarkCurrentTransactionIdLoggedIfAny() out of the critical section in > this patch itself but that appears like an unrelated change and we may > or may not want to back-patch the same. > v5-0001, incorporates all the comment fixes suggested by Alvaro. and 0001 is an additional patch which moves MarkCurrentTransactionIdLoggedIfAny(), out of the critical section. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com From 797f274c395f56b72f1d7ec5ee20099f73d6fa9f Mon Sep 17 00:00:00 2001 From: Dilip Kumar Date: Thu, 21 Oct 2021 09:52:09 +0530 Subject: [PATCH v5 1/2] Replace XLOG_INCLUDE_XID flag with a more localized flag. Commit 0bead9af484c introduced XLOG_INCLUDE_XID flag to indicate that the WAL record contains subXID-to-topXID association. It uses that flag later to mark in CurrentTransactionState that top-xid is logged so that we should not try to log it again with the next WAL record in the current subtransaction. However, we can use a localized variable to pass that information. In passing, change the related function and variable names to make them consistent with what the code is actually doing. This also reverts the changes made by commit ade24dab97 where we have improved the documentation for XLOG_INCLUDE_XID flag. Author: Dilip Kumar Reviewed-by: Amit Kapila Discussion: https://postgr.es/m/e1msoyz-0007fh...@gemulon.postgresql.org --- src/backend/access/transam/xact.c | 103 src/backend/access/transam/xlog.c | 13 +++- src/backend/access/transam/xloginsert.c | 26 src/include/access/xact.h | 4 +- src/include/access/xlog.h | 4 +- 5 files changed, 82 insertions(+), 68 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index d96881c..90e689e 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -205,7 +205,7 @@ typedef struct TransactionStateData bool didLogXid; /* has xid been included in WAL record? */ int parallelModeLevel; /* Enter/ExitParallelMode counter */ bool chain; /* start a new block after this one */ - bool assigned; /* assigned to top-level XID */ + bool topXidLogged; /* for a subxact: is top-level XID logged? */ struct TransactionStateData *parent; /* back link to parent */ } TransactionStateData; @@ -238,7 +238,7 @@ typedef struct SerializedTransactionState static TransactionStateData TopTransactionStateData = { .state = TRANS_DEFAULT, .blockState = TBLOCK_DEFAULT, - .assigned = false, + .topXidLogged = false, }; /* @@ -529,6 +529,56 @@ MarkCurrentTransactionIdLoggedIfAny(void) CurrentTransactionState->didLogXid = true; } +/* + * IsSubxactTopXidLogPending + * + * This is used to decide whether we need to WAL log the top-level XID for + * operation in a subtransaction. We require that for logical decoding, see + * LogicalDecodingProcessRecord. + * + * This returns true if wal_level >= logical and we are inside a valid + * subtransaction, for which the assignment was not yet written to any WAL + * record. + */ +bool +IsSubxactTopXidLogPending(void) +{ + /* check whether it is already logged */ + if (CurrentTransactionState->topXidLogged) + return false; + + /* wal_level has to be logical */ + if (!XLogLogicalInfoActive()) + return false; + + /* we need to be in a transaction state */ + if (!IsTransactionState()) + return false; + + /* it has to be a subtransaction */ + if (!IsSubTransaction()) + return false; + + /* the subtransaction has to have a XID assigned */ + if (!TransactionIdIsValid(GetCurrentTransactionIdIfAny())) + return false; + + return true; +} + +/* + * MarkSubxactTopXidLogged + * + * Remember that the top transaction id for the current subtransaction is WAL + * logged now. + */ +void +MarkSubxactTopXidLogged(void) +{ + Assert(IsSubxactTopXidLogPending()); + + CurrentTransactionState->topXidLogged = true; +} /* * GetStableLatestTransactionId @@ -5168,7 +5218,7 @@ PushTransact
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Wed, Oct 20, 2021 at 8:49 PM Dilip Kumar wrote: > > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera > wrote: > > > > Does MarkTopTransactionIdLogged() have to be inside XLogInsertRecord's > > critical section? > > I think this function is doing somewhat similar things to what we are > doing in MarkCurrentTransactionIdLoggedIfAny() so put at the same > place. But I don't see any reason for this to be in the critical > section. > Yeah, I also don't see any reason for this to be in the critical section but it might be better to keep both together. So, if we want to keep MarkTopTransactionIdLogged() out of the critical section in this patch then we should move the existing function MarkCurrentTransactionIdLoggedIfAny() in a separate patch so that future readers doesn't get confused as to why one of these is in the critical section and other is not. OTOH, we can move MarkCurrentTransactionIdLoggedIfAny() out of the critical section in this patch itself but that appears like an unrelated change and we may or may not want to back-patch the same. -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Wed, Oct 20, 2021 at 9:46 PM Alvaro Herrera wrote: > > On 2021-Oct-20, Dilip Kumar wrote: > > > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera > > wrote: > > > > In IsTopTransactionIdLogPending(), I suggest to reorder the tests so > > > that the faster ones are first -- or at least, the last one should be at > > > the top, so in some cases we can return without additional function > > > calls. I don't think it'd be extremely noticeable, but as Tom likes to > > > say, a cycle shaved is a cycle earned. > > > > I don't think we can really move the last at top. Basically, we only > > want to log the top transaction id if all the above check passes and > > the top xid is not yet logged. For example, if the WAL level is not > > logical then we don't want to log the top xid even if it is not yet > > logged, similarly, if the current transaction is not a subtransaction > > then also we don't want to log the top transaction. > > Well, I don't suggest to move it verbatim, but ISTM the code can be > restructured so that we do that test first, and if we see that flag set > to true, we don't have to consider any of the other tests. That flag > can only be set true if we saw all the other checks pass in the same > subtransaction, right? Yeah you are right, I will change it. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On 2021-Oct-20, Dilip Kumar wrote: > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera > wrote: > > In IsTopTransactionIdLogPending(), I suggest to reorder the tests so > > that the faster ones are first -- or at least, the last one should be at > > the top, so in some cases we can return without additional function > > calls. I don't think it'd be extremely noticeable, but as Tom likes to > > say, a cycle shaved is a cycle earned. > > I don't think we can really move the last at top. Basically, we only > want to log the top transaction id if all the above check passes and > the top xid is not yet logged. For example, if the WAL level is not > logical then we don't want to log the top xid even if it is not yet > logged, similarly, if the current transaction is not a subtransaction > then also we don't want to log the top transaction. Well, I don't suggest to move it verbatim, but ISTM the code can be restructured so that we do that test first, and if we see that flag set to true, we don't have to consider any of the other tests. That flag can only be set true if we saw all the other checks pass in the same subtransaction, right? -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera wrote: > > API-wise, this seems a good improvement and it brings a lot of clarity > to what is really going on. Thanks for working on it. > > Some minor comments: Thanks for the review, most of the comments look fine, and will work on those, but I think some of them need more thoughts so replying to those. > In IsTopTransactionIdLogPending(), I suggest to reorder the tests so > that the faster ones are first -- or at least, the last one should be at > the top, so in some cases we can return without additional function > calls. I don't think it'd be extremely noticeable, but as Tom likes to > say, a cycle shaved is a cycle earned. I don't think we can really move the last at top. Basically, we only want to log the top transaction id if all the above check passes and the top xid is not yet logged. For example, if the WAL level is not logical then we don't want to log the top xid even if it is not yet logged, similarly, if the current transaction is not a subtransaction then also we don't want to log the top transaction. > > Does MarkTopTransactionIdLogged() have to be inside XLogInsertRecord's > critical section? I think this function is doing somewhat similar things to what we are doing in MarkCurrentTransactionIdLoggedIfAny() so put at the same place. But I don't see any reason for this to be in the critical section. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
API-wise, this seems a good improvement and it brings a lot of clarity to what is really going on. Thanks for working on it. Some minor comments: Please do not revert the comment change in xlogrecord.h. It is not wrong. The exceptions mentioned are values 252-255, so "a few" is better than "a couple". In IsTopTransactionIdLogPending(), I suggest to reorder the tests so that the faster ones are first -- or at least, the last one should be at the top, so in some cases we can return without additional function calls. I don't think it'd be extremely noticeable, but as Tom likes to say, a cycle shaved is a cycle earned. XLogRecordAssemble's comment should explain its new output argument. Maybe "*topxid_included is set if the topmost transaction ID is logged with the current subtransaction". I think these new routines IsTopTransactionIdLogPending and MarkTopTransactionIdLogged should not be at the end of the file; a location closer to where MarkCurrentTransactionIdLoggedIfAny() seems more appropriate. Keep related things closer. (In this case these are operating on TransactionStateData, and it looks right that they would appear somewhat about AssignTransactionId and GetStableLatestTransactionId.) Does MarkTopTransactionIdLogged() have to be inside XLogInsertRecord's critical section? The names IsTopTransactionIdLogPending() and MarkTopTransactionIdLogged() irk me somewhat. It's not at all obvious from these names that these routines are mostly about actions taken for a subtransaction. I propose IsSubxactTopXidLogPending() and MarkSubxactTopXidLogged(). I don't feel the need to expand "Xid" to "TransactionId" in these function names. In TransactionStateData, I propose this wording for the comment: bool topXidLogged;/* for a subxact: is top-level XID logged? */ Thanks! -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Wed, Oct 20, 2021 at 4:17 PM Amit Kapila wrote: > > On Wed, Oct 20, 2021 at 10:25 AM Dilip Kumar wrote: > > > > I have one comment here, basically, you have changed the function name > > to "IsTopTransactionIdLogged", but it still behaves like > > IsTopTransactionIdLogPending. Now with the new name, it should return > > (CurrentTransactionState->topXidLogged) instead of > > (!CurrentTransactionState->topXidLogged). > > > > Valid point but I think the change suggested by you won't be > sufficient. We also need to change all the other checks in that > function to return true which will make it a bit awkward. So instead, > we can change the function name to IsTopTransactionIdLogPending(). > Does that make sense? Yeah, that makes sense. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Wed, Oct 20, 2021 at 10:25 AM Dilip Kumar wrote: > > On Mon, Oct 18, 2021 at 10:48 AM Amit Kapila wrote: > > > > > Today, I have looked at this patch again and slightly changed a > > comment, one of the function name and variable name. Do, let me know > > if you or others have any suggestions for better names or otherwise? I > > think we should backpatch this to 14 as well where this code was > > introduced. > > > > bool > -IsSubTransactionAssignmentPending(void) > +IsTopTransactionIdLogged(void) > { > /* wal_level has to be logical */ > if (!XLogLogicalInfoActive()) > @@ -6131,19 +6131,20 @@ IsSubTransactionAssignmentPending(void) > if (!TransactionIdIsValid(GetCurrentTransactionIdIfAny())) > return false; > > - /* and it should not be already 'assigned' */ > - return !CurrentTransactionState->assigned; > + /* and it should not be already 'logged' */ > + return !CurrentTransactionState->topXidLogged; > } > > I have one comment here, basically, you have changed the function name > to "IsTopTransactionIdLogged", but it still behaves like > IsTopTransactionIdLogPending. Now with the new name, it should return > (CurrentTransactionState->topXidLogged) instead of > (!CurrentTransactionState->topXidLogged). > Valid point but I think the change suggested by you won't be sufficient. We also need to change all the other checks in that function to return true which will make it a bit awkward. So instead, we can change the function name to IsTopTransactionIdLogPending(). Does that make sense? -- With Regards, Amit Kapila. v4-0001-Replace-XLOG_INCLUDE_XID-flag-with-a-more-localiz.patch Description: Binary data
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Mon, Oct 18, 2021 at 10:48 AM Amit Kapila wrote: > > Today, I have looked at this patch again and slightly changed a > comment, one of the function name and variable name. Do, let me know > if you or others have any suggestions for better names or otherwise? I > think we should backpatch this to 14 as well where this code was > introduced. > bool -IsSubTransactionAssignmentPending(void) +IsTopTransactionIdLogged(void) { /* wal_level has to be logical */ if (!XLogLogicalInfoActive()) @@ -6131,19 +6131,20 @@ IsSubTransactionAssignmentPending(void) if (!TransactionIdIsValid(GetCurrentTransactionIdIfAny())) return false; - /* and it should not be already 'assigned' */ - return !CurrentTransactionState->assigned; + /* and it should not be already 'logged' */ + return !CurrentTransactionState->topXidLogged; } I have one comment here, basically, you have changed the function name to "IsTopTransactionIdLogged", but it still behaves like IsTopTransactionIdLogPending. Now with the new name, it should return (CurrentTransactionState->topXidLogged) instead of (!CurrentTransactionState->topXidLogged). And the caller should also be changed accordingly. Other changes look fine. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Thu, Oct 7, 2021 at 10:46 AM Amit Kapila wrote: > > On Sat, Oct 2, 2021 at 4:16 PM Dilip Kumar wrote: > > > > On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera > > wrote: > > > > > > On 2021-Oct-01, Amit Kapila wrote: > > > > > I think a straight standalone variable (probably a static boolean in > > > xloginsert.c) might be less confusing. > > > > I have written two patches, Approach1 is as you described using a > > static boolean and Approach2 as a local variable to XLogAssembleRecord > > as described by Amit, attached both of them for your reference. > > IMHO, either of these approaches looks cleaner. > > > > I have tried to improve some comments and a variable name in the > Approach-2 (use local variable) patch and also reverts one of the > comments introduced by the commit ade24dab97. I am fine if we decide > to go with Approach-1 as well but personally, I would prefer to keep > the code consistent with nearby code. > > Let me know what you think of the attached? > Today, I have looked at this patch again and slightly changed a comment, one of the function name and variable name. Do, let me know if you or others have any suggestions for better names or otherwise? I think we should backpatch this to 14 as well where this code was introduced. -- With Regards, Amit Kapila. v3-0001-Replace-XLOG_INCLUDE_XID-flag-with-a-more-localiz.patch Description: Binary data
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Sat, Oct 2, 2021 at 4:16 PM Dilip Kumar wrote: > > On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera wrote: > > > > On 2021-Oct-01, Amit Kapila wrote: > > > I think a straight standalone variable (probably a static boolean in > > xloginsert.c) might be less confusing. > > I have written two patches, Approach1 is as you described using a > static boolean and Approach2 as a local variable to XLogAssembleRecord > as described by Amit, attached both of them for your reference. > IMHO, either of these approaches looks cleaner. > I have tried to improve some comments and a variable name in the Approach-2 (use local variable) patch and also reverts one of the comments introduced by the commit ade24dab97. I am fine if we decide to go with Approach-1 as well but personally, I would prefer to keep the code consistent with nearby code. Let me know what you think of the attached? With Regards, Amit Kapila. v2-0001-Replace-XLOG_INCLUDE_XID-flag-with-a-more-localiz.patch Description: Binary data
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Sat, Oct 2, 2021 at 6:46 AM Dilip Kumar wrote: > I have written two patches, Approach1 is as you described using a > static boolean and Approach2 as a local variable to XLogAssembleRecord > as described by Amit, attached both of them for your reference. > IMHO, either of these approaches looks cleaner. I agree, and I don't have a strong preference between them. If I were writing code like this from scratch, I would do what 0001 does. But 0002 is arguably more consistent with the existing style. It's kind of hard to judge, at least for me, which is to be preferred. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Fri, Oct 1, 2021 at 6:23 PM Alvaro Herrera wrote: > > On 2021-Oct-01, Amit Kapila wrote: > > > AFAICS, there are two possibilities w.r.t global variables: (a) use > > curinsert_flags which we are doing now, (b) another is to introduce a > > new global variable, set it after we make the association, and then > > reset it after we mark SubTransaction assigned and on error. I have > > also thought of passing it via XLogCtlInsert but as that is shared by > > different processes, it can be set by one process and be read by > > another process which we don't want here. > > So, in my mind, curinsert_flags is a way for the high-level user of > XLogInsert to pass info about the record being inserted to the low-level > xloginsert.c infrastructure. In contrast, XLOG_INCLUDE_XID is being > used solely within xloginsert.c, by one piece of low-level > infrastructure to communicate to another piece of low-level > infrastructure that some cleanup is needed. Nothing outside of > xloginsert.c needs to know anything about XLOG_INCLUDE_XID, in contrast > with the other bits that can be set by XLogSetRecordFlags. You could > move the #define to xloginsert.c and everything would compile fine. > > Another tell-tale sign that things are not fitting right is that > XLOG_INCLUDE_XID is not set via XLogSetRecordFlags, contrary the comment > above those defines. > > (Aside: I wonder why do we have XLogSetRecordFlags at all -- I think we > could just pass the other two flags via XLogBeginInsert). > Agreed, I think we can do that if we want but we still need to set curinsert_flags or some other similar variable in xloginsert.c so that we can later use and reset it. > Regarding XLOG_INCLUDE_XID, I don't think replacing it with a bit in > shared memory is a good idea, since it only applies to the insertion > being carried out by the current process, right? > Right. Ideally, we can set this in a local variable via XLogRecordAssemble() and then use it in XLogInsertRecord() as is done in 0001-Refactor-code-for-logging-the-top-transaction-id-in-Approach2. Basically, we just need to ensure that we mark the CurrentTransactionState for this flag once we are sure that the function XLogInsertRecord() will perform the insertion and won't return InvalidXLogRecPtr. OTOH, I see the point in using a global static variable to achieve this purpose as that allows to do the required work only in xloginsert.c. > I think a straight standalone variable (probably a static boolean in > xloginsert.c) might be less confusing. > > ... so, reading the xact.c code again, TransactionState->assigned really > means "whether the subXID-to-topXID association has been wal-logged", > which is a completely different meaning from what the term 'assigned' > means in all other comments in xact.c ... > I think you have interpreted it correctly and we make this association logged with the first WAL of each subtransaction if the WAL level is logical. -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Sun, Oct 3, 2021 at 5:05 PM Dilip Kumar wrote: > > On Sat, Oct 2, 2021 at 8:10 PM Alvaro Herrera wrote: > > > > On 2021-Oct-02, Dilip Kumar wrote: > > > > > I have written two patches, Approach1 is as you described using a > > > static boolean and Approach2 as a local variable to XLogAssembleRecord > > > as described by Amit, attached both of them for your reference. > > > IMHO, either of these approaches looks cleaner. > > > > Thanks! I haven't read these patches carefully, but I think the > > variable is about assigning the *subxid*, not the topxid. Amit can > > confirm ... > > IIRC, this variable is for logging the top xid in the first WAL by > each subtransaction. So that during logical decoding, while creating > the ReorderBufferTxn for the subtransaction we can associate it to the > top transaction without seeing the commit WAL. > This is correct. -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Sat, Oct 2, 2021 at 8:10 PM Alvaro Herrera wrote: > > On 2021-Oct-02, Dilip Kumar wrote: > > > I have written two patches, Approach1 is as you described using a > > static boolean and Approach2 as a local variable to XLogAssembleRecord > > as described by Amit, attached both of them for your reference. > > IMHO, either of these approaches looks cleaner. > > Thanks! I haven't read these patches carefully, but I think the > variable is about assigning the *subxid*, not the topxid. Amit can > confirm ... IIRC, this variable is for logging the top xid in the first WAL by each subtransaction. So that during logical decoding, while creating the ReorderBufferTxn for the subtransaction we can associate it to the top transaction without seeing the commit WAL. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On 2021-Oct-02, Dilip Kumar wrote: > I have written two patches, Approach1 is as you described using a > static boolean and Approach2 as a local variable to XLogAssembleRecord > as described by Amit, attached both of them for your reference. > IMHO, either of these approaches looks cleaner. Thanks! I haven't read these patches carefully, but I think the variable is about assigning the *subxid*, not the topxid. Amit can confirm ... -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Aprender sin pensar es inútil; pensar sin aprender, peligroso" (Confucio)
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera wrote: > > On 2021-Oct-01, Amit Kapila wrote: > I think a straight standalone variable (probably a static boolean in > xloginsert.c) might be less confusing. I have written two patches, Approach1 is as you described using a static boolean and Approach2 as a local variable to XLogAssembleRecord as described by Amit, attached both of them for your reference. IMHO, either of these approaches looks cleaner. > > ... so, reading the xact.c code again, TransactionState->assigned really > means "whether the subXID-to-topXID association has been wal-logged", > which is a completely different meaning from what the term 'assigned' > means in all other comments in xact.c ... and I think the subroutine > name MarkSubTransactionAssigned() is not a great choice either. I have also renamed the variable and functions as per the actual usage. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com From 3d4fb94fdeef2ae46511291f144b6305c9eea9f7 Mon Sep 17 00:00:00 2001 From: Dilip Kumar Date: Sat, 2 Oct 2021 15:21:52 +0530 Subject: [PATCH] Refactor code for logging the top transaction id in WAL --- src/backend/access/transam/xact.c | 22 +++--- src/backend/access/transam/xlog.c | 7 ++- src/backend/access/transam/xloginsert.c | 23 ++- src/include/access/xact.h | 4 ++-- src/include/access/xlog.h | 4 ++-- 5 files changed, 31 insertions(+), 29 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 4cc38f0..893147649 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -204,7 +204,7 @@ typedef struct TransactionStateData bool didLogXid; /* has xid been included in WAL record? */ int parallelModeLevel; /* Enter/ExitParallelMode counter */ bool chain; /* start a new block after this one */ - bool assigned; /* assigned to top-level XID */ + bool topXidLogged; /* top-level XID is logged?*/ struct TransactionStateData *parent; /* back link to parent */ } TransactionStateData; @@ -237,7 +237,7 @@ typedef struct SerializedTransactionState static TransactionStateData TopTransactionStateData = { .state = TRANS_DEFAULT, .blockState = TBLOCK_DEFAULT, - .assigned = false, + .topXidLogged = false, }; /* @@ -5159,7 +5159,7 @@ PushTransaction(void) GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext); s->prevXactReadOnly = XactReadOnly; s->parallelModeLevel = 0; - s->assigned = false; + s->topXidLogged = false; CurrentTransactionState = s; @@ -6093,7 +6093,7 @@ xact_redo(XLogReaderState *record) } /* - * IsSubTransactionAssignmentPending + * IsLogTopTransactionIdPending * * This is used to decide whether we need to WAL log the top-level XID for * operation in a subtransaction. We require that for logical decoding, see @@ -6104,7 +6104,7 @@ xact_redo(XLogReaderState *record) * record. */ bool -IsSubTransactionAssignmentPending(void) +IsLogTopTransactionIdPending(void) { /* wal_level has to be logical */ if (!XLogLogicalInfoActive()) @@ -6123,18 +6123,18 @@ IsSubTransactionAssignmentPending(void) return false; /* and it should not be already 'assigned' */ - return !CurrentTransactionState->assigned; + return !CurrentTransactionState->topXidLogged; } /* - * MarkSubTransactionAssigned + * MarkTopTransactionIdLogged * - * Mark the subtransaction assignment as completed. + * Mark top transaction id is WAL logged for the current subtransaction. */ void -MarkSubTransactionAssigned(void) +MarkTopTransactionIdLogged(void) { - Assert(IsSubTransactionAssignmentPending()); + Assert(IsLogTopTransactionIdPending()); - CurrentTransactionState->assigned = true; + CurrentTransactionState->topXidLogged = true; } diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index f8c714b..78232aa 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -1010,7 +1010,8 @@ XLogRecPtr XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn, uint8 flags, - int num_fpi) + int num_fpi, + bool topxid_included) { XLogCtlInsert *Insert = &XLogCtl->Insert; pg_crc32c rdata_crc; @@ -1163,6 +1164,10 @@ XLogInsertRecord(XLogRecData *rdata, MarkCurrentTransactionIdLoggedIfAny(); + /* Mark top transaction id is logged (if needed) */ + if (topxid_included) + MarkTopTransactionIdLogged(); + END_CRIT_SECTION(); /* diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index b492c65..6b5925e 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -123,7 +123,8 @@ static MemoryContext xloginsert_cxt; static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info, XLogRecPtr RedoRecPtr, bool doPageWrites, - XLogRecPtr *fpw_lsn, int *num_fpi); + XLogRe
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Fri, Oct 1, 2021 at 8:53 AM Alvaro Herrera wrote: > I think a straight standalone variable (probably a static boolean in > xloginsert.c) might be less confusing. +1. > ... so, reading the xact.c code again, TransactionState->assigned really > means "whether the subXID-to-topXID association has been wal-logged", > which is a completely different meaning from what the term 'assigned' > means in all other comments in xact.c ... and I think the subroutine > name MarkSubTransactionAssigned() is not a great choice either. +1. -- Robert Haas EDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On 2021-Oct-01, Amit Kapila wrote: > AFAICS, there are two possibilities w.r.t global variables: (a) use > curinsert_flags which we are doing now, (b) another is to introduce a > new global variable, set it after we make the association, and then > reset it after we mark SubTransaction assigned and on error. I have > also thought of passing it via XLogCtlInsert but as that is shared by > different processes, it can be set by one process and be read by > another process which we don't want here. So, in my mind, curinsert_flags is a way for the high-level user of XLogInsert to pass info about the record being inserted to the low-level xloginsert.c infrastructure. In contrast, XLOG_INCLUDE_XID is being used solely within xloginsert.c, by one piece of low-level infrastructure to communicate to another piece of low-level infrastructure that some cleanup is needed. Nothing outside of xloginsert.c needs to know anything about XLOG_INCLUDE_XID, in contrast with the other bits that can be set by XLogSetRecordFlags. You could move the #define to xloginsert.c and everything would compile fine. Another tell-tale sign that things are not fitting right is that XLOG_INCLUDE_XID is not set via XLogSetRecordFlags, contrary the comment above those defines. (Aside: I wonder why do we have XLogSetRecordFlags at all -- I think we could just pass the other two flags via XLogBeginInsert). Regarding XLOG_INCLUDE_XID, I don't think replacing it with a bit in shared memory is a good idea, since it only applies to the insertion being carried out by the current process, right? I think a straight standalone variable (probably a static boolean in xloginsert.c) might be less confusing. ... so, reading the xact.c code again, TransactionState->assigned really means "whether the subXID-to-topXID association has been wal-logged", which is a completely different meaning from what the term 'assigned' means in all other comments in xact.c ... and I think the subroutine name MarkSubTransactionAssigned() is not a great choice either. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Fri, Oct 1, 2021 at 12:32 AM Robert Haas wrote: > > On Thu, Sep 30, 2021 at 6:08 AM Amit Kapila wrote: > > I think we can do better than using XLOG_INCLUDE_XID flag in the > > record being inserted. We need this flag so that we can mark > > SubTransaction assigned after XLogInsertRecord() is successful. We > > can instead output a flag (say sub_xact_assigned) from > > XLogRecordAssemble() and pass it to XLogInsertRecord(). Then in > > XLogInsertRecord(), we can mark SubTransactionAssigned once the record > > is inserted (after or before calling > > MarkCurrentTransactionIdLoggedIfAny()). > > Isn't there other communication between these routines that just uses > global variables? > AFAICS, there are two possibilities w.r.t global variables: (a) use curinsert_flags which we are doing now, (b) another is to introduce a new global variable, set it after we make the association, and then reset it after we mark SubTransaction assigned and on error. I have also thought of passing it via XLogCtlInsert but as that is shared by different processes, it can be set by one process and be read by another process which we don't want here. I am not sure if any of these is better than doing this communication via local variable. Do you have something specific in mind? -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Thu, Sep 30, 2021 at 6:08 AM Amit Kapila wrote: > I think we can do better than using XLOG_INCLUDE_XID flag in the > record being inserted. We need this flag so that we can mark > SubTransaction assigned after XLogInsertRecord() is successful. We > can instead output a flag (say sub_xact_assigned) from > XLogRecordAssemble() and pass it to XLogInsertRecord(). Then in > XLogInsertRecord(), we can mark SubTransactionAssigned once the record > is inserted (after or before calling > MarkCurrentTransactionIdLoggedIfAny()). Isn't there other communication between these routines that just uses global variables? -- Robert Haas EDB: http://www.enterprisedb.com
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Wed, Sep 29, 2021 at 8:50 PM Robert Haas wrote: > > On Tue, Sep 21, 2021 at 6:48 PM Alvaro Herrera > wrote: > > Document XLOG_INCLUDE_XID a little better > > > > I noticed that commit 0bead9af484c left this flag undocumented in > > XLogSetRecordFlags, which led me to discover that the flag doesn't > > actually do what the one comment on it said it does. Improve the > > situation by adding some more comments. > > > > Backpatch to 14, where the aforementioned commit appears. > > I'm not sure that saying something is a "hack" is really all that > useful as documentation. > > But more to the point, I think this hack is ugly and needs to be > replaced with something less hacky. > I think we can do better than using XLOG_INCLUDE_XID flag in the record being inserted. We need this flag so that we can mark SubTransaction assigned after XLogInsertRecord() is successful. We can instead output a flag (say sub_xact_assigned) from XLogRecordAssemble() and pass it to XLogInsertRecord(). Then in XLogInsertRecord(), we can mark SubTransactionAssigned once the record is inserted (after or before calling MarkCurrentTransactionIdLoggedIfAny()). The other idea could be that in XLogInsertRecord(), we check IsSubTransactionAssignmentPending() after the record is successfully inserted and then mark SubTransaction assigned but I think the previous one is better. What do you think? -- With Regards, Amit Kapila.
Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Tue, Sep 21, 2021 at 6:48 PM Alvaro Herrera wrote: > Document XLOG_INCLUDE_XID a little better > > I noticed that commit 0bead9af484c left this flag undocumented in > XLogSetRecordFlags, which led me to discover that the flag doesn't > actually do what the one comment on it said it does. Improve the > situation by adding some more comments. > > Backpatch to 14, where the aforementioned commit appears. I'm not sure that saying something is a "hack" is really all that useful as documentation. But more to the point, I think this hack is ugly and needs to be replaced with something less hacky. Amit? -- Robert Haas EDB: http://www.enterprisedb.com