Обсуждение: Re: pgsql: Document XLOG_INCLUDE_XID a little better
On Tue, Sep 21, 2021 at 6:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On Wed, Sep 29, 2021 at 8:50 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Tue, Sep 21, 2021 at 6:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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.
On Thu, Sep 30, 2021 at 6:08 AM Amit Kapila <amit.kapila16@gmail.com> 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
On Fri, Oct 1, 2021 at 12:32 AM Robert Haas <robertmhaas@gmail.com> wrote: > > On Thu, Sep 30, 2021 at 6:08 AM Amit Kapila <amit.kapila16@gmail.com> 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.
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)
On Fri, Oct 1, 2021 at 8:53 AM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
Вложения
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)
On Sat, Oct 2, 2021 at 8:10 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On Sun, Oct 3, 2021 at 5:05 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Sat, Oct 2, 2021 at 8:10 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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.
On Fri, Oct 1, 2021 at 6:23 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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.
On Sat, Oct 2, 2021 at 6:46 AM Dilip Kumar <dilipbalaut@gmail.com> 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
On Sat, Oct 2, 2021 at 4:16 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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.
Вложения
On Thu, Oct 7, 2021 at 10:46 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Sat, Oct 2, 2021 at 4:16 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Fri, Oct 1, 2021 at 6:24 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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.
Вложения
On Mon, Oct 18, 2021 at 10:48 AM Amit Kapila <amit.kapila16@gmail.com> 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
On Wed, Oct 20, 2021 at 10:25 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Oct 18, 2021 at 10:48 AM Amit Kapila <amit.kapila16@gmail.com> 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.
Вложения
On Wed, Oct 20, 2021 at 4:17 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 10:25 AM Dilip Kumar <dilipbalaut@gmail.com> 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
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)
On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On 2021-Oct-20, Dilip Kumar wrote: > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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/
On Wed, Oct 20, 2021 at 9:46 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Oct-20, Dilip Kumar wrote: > > > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On Wed, Oct 20, 2021 at 8:49 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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.
On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Oct 20, 2021 at 8:49 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Wed, Oct 20, 2021 at 7:09 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
Вложения
On Thu, Oct 21, 2021 at 11:20 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila <amit.kapila16@gmail.com> 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.
On Mon, Oct 25, 2021 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Oct 21, 2021 at 11:20 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila <amit.kapila16@gmail.com> 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.
On Tue, Oct 26, 2021 at 9:19 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 4:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Thu, Oct 21, 2021 at 11:20 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > On Thu, Oct 21, 2021 at 9:11 AM Amit Kapila <amit.kapila16@gmail.com> 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
On Wed, Oct 27, 2021 at 4:39 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 9:19 AM Amit Kapila <amit.kapila16@gmail.com> 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.
On Thu, Oct 28, 2021 at 8:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Oct 27, 2021 at 4:39 PM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > On Tue, Oct 26, 2021 at 9:19 AM Amit Kapila <amit.kapila16@gmail.com> 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.