Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions

On Mon, Apr 13, 2020 at 05:20:39PM +0530, Dilip Kumar wrote:
>On Mon, Apr 13, 2020 at 4:14 PM Kuntal Ghosh  wrote:
>>
>> On Thu, Apr 9, 2020 at 2:40 PM Dilip Kumar  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