On Mon, Apr 13, 2020 at 05:20:39PM +0530, Dilip Kumar wrote:
On Mon, Apr 13, 2020 at 4:14 PM Kuntal Ghosh <kuntalghosh.2...@gmail.com> wrote:On Thu, Apr 9, 2020 at 2:40 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > I have rebased the patch on the latest head. I haven't yet changed > anything for xid assignment thing because it is not yet concluded. > Some review comments from 0001-Immediately-WAL-log-*.patch, +bool +IsSubTransactionAssignmentPending(void) +{ + 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; + + /* and it needs to have 'assigned' */ + return !CurrentTransactionState->assigned; + +} IMHO, it's important to reduce the complexity of this function since it's been called for every WAL insertion. During the lifespan of a transaction, any of these if conditions will only be evaluated if previous conditions are true. So, we can maintain some state machine to avoid multiple evaluation of a condition inside a transaction. But, if the overhead is not much, it's not worth I guess.Yeah maybe, in some cases we can avoid checking multiple conditions by maintaining that state. But, that state will have to be at the transaction level. But, I am not sure how much worth it will be to add one extra condition to skip a few if checks and it will also add the code complexity. And, in some cases where logical decoding is not enabled, it may add one extra check? I mean first check the state and that will take you to the first if check.
Perhaps. I think we should only do that if we can demonstrate it's an issue in practice. Otherwise it's just unnecessary complexity. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
