Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-11-02 Thread Amit Kapila
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 r

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-27 Thread Amit Kapila
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-27 Thread Dilip Kumar
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 f

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-25 Thread Amit Kapila
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 >

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-25 Thread Amit Kapila
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.

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Dilip Kumar
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Amit Kapila
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Dilip Kumar
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Alvaro Herrera
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Dilip Kumar
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 t

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Alvaro Herrera
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Dilip Kumar
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 ne

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-20 Thread Amit Kapila
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-19 Thread Dilip Kumar
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-17 Thread Amit Kapila
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-06 Thread Amit Kapila
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 patche

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-04 Thread Robert Haas
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-03 Thread Amit Kapila
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 associat

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-03 Thread Amit Kapila
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 XLogAssembleR

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-03 Thread Dilip Kumar
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 re

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-02 Thread Alvaro Herrera
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. Thank

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-02 Thread Dilip Kumar
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 Approac

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-01 Thread Robert Haas
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 ha

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-10-01 Thread Alvaro Herrera
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 an

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-09-30 Thread Amit Kapila
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

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-09-30 Thread Robert Haas
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_assig

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-09-30 Thread Amit Kapila
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 do

Re: pgsql: Document XLOG_INCLUDE_XID a little better

2021-09-29 Thread Robert Haas
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. Impro