Horton

Thread

Commits

  1. Move MarkCurrentTransactionIdLoggedIfAny() out of the critical section.

  2. Replace XLOG_INCLUDE_XID flag with a more localized flag.

  3. Document XLOG_INCLUDE_XID a little better

  4. Immediately WAL-log subtransaction and top-level XID association.

  1. pgsql: Document XLOG_INCLUDE_XID a little better

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-09-21T22:48:33Z

    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.

Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/202109212119.c3nhfp64t2ql@alvherre.pgsql

Branch
------
REL_14_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/c1d1ae1db23796e4d1b04f5c087944722cf1446a

Modified Files
--------------
src/backend/access/transam/xloginsert.c | 2 ++
src/include/access/xlog.h               | 2 +-
src/include/access/xlogrecord.h         | 5 +++--
3 files changed, 6 insertions(+), 3 deletions(-)

  2. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Robert Haas <robertmhaas@gmail.com> — 2021-09-29T15:20:29Z

    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

  3. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-09-30T10:07:47Z

    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.

  4. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Robert Haas <robertmhaas@gmail.com> — 2021-09-30T19:02:04Z

    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

  5. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-01T04:51:16Z

    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.

  6. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-01T12:53:48Z

    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)

  7. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Robert Haas <robertmhaas@gmail.com> — 2021-10-01T13:45:47Z

    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

  8. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-02T10:46:31Z

    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

  9. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-02T14:40:10Z

    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)

  10. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-03T11:35:24Z

    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

  11. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-04T04:26:29Z

    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.

  12. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-04T05:04:22Z

    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.

  13. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Robert Haas <robertmhaas@gmail.com> — 2021-10-04T17:13:20Z

    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

  14. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-07T05:16:03Z

    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.

  15. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-18T05:17:56Z

    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.

  16. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-20T04:55:05Z

    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

  17. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-20T10:47:26Z

    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.

  18. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-20T11:47:02Z

    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

  19. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-20T13:39:15Z

    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)

  20. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-20T15:19:36Z

    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

  21. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-10-20T16:16:47Z

    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/

  22. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-21T03:10:12Z

    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

  23. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-21T03:40:55Z

    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.

  24. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-21T05:50:23Z

    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

  25. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-25T10:51:26Z

    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.

  26. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-26T03:49:03Z

    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.

  27. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-10-27T11:09:09Z

    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

  28. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-10-28T02:45:24Z

    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.

  29. Re: pgsql: Document XLOG_INCLUDE_XID a little better

    Amit Kapila <amit.kapila16@gmail.com> — 2021-11-03T03:10:26Z

    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.