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