Date: Sun, 29 Mar 2020 01:59:05 +0100
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: PATCH: logical_work_mem and logical streaming of large
 in-progress transactions
Message-ID: <20200329005905.ooxq25qyjwn2mvxz@development>
References: 
 <CAFiTN-uNKhtscqUvHokdxykM8w4J5aLEgSx+_Xx2O-sBdfc=tA@mail.gmail.com>
 <CAA4eK1LT2UArE9FyPqZcmV2kALhEmcXnZGdreYWCdCJ9vRUd8Q@mail.gmail.com>
 <CAFiTN-t1YNmoBf7k1kUrUue4q1Tf3GXjGwZTseNLivKmr9sz1Q@mail.gmail.com>
 <CAFiTN-vR7jQBP75FpvE=-4Ca-3jqsJA55ZDct0B8UtE0x13gCQ@mail.gmail.com>
 <CAFiTN-uNzEyY3YL1EZabWcG=Bah1nO8hno+-UaAkCDcfCGYC0g@mail.gmail.com>
 <20200303214646.6adway2z5wnapjem@development>
 <CAFiTN-sn0Bd4Q72Cn9t8-mZp1sxWRv2SmUoeR_7J05SqNTR--g@mail.gmail.com>
 <CAA4eK1KyrP6m53FQV5v0gysEnaTHPdc2xYT0KeNN+fhOcedRwA@mail.gmail.com>
 <CAFiTN-uBd3p50s=kfEiSbg93_Fnf_VU+haS4FqBZAm7PmbX69g@mail.gmail.com>
 <CAA4eK1LkLU5frY4kZGO38d8m6R504H+GbfkwsQLAp7sQ-4oqbw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAA4eK1LkLU5frY4kZGO38d8m6R504H+GbfkwsQLAp7sQ-4oqbw@mail.gmail.com>
Precedence: bulk

On Sat, Mar 28, 2020 at 03:29:34PM +0530, Amit Kapila wrote:
>On Sat, Mar 28, 2020 at 2:19 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>
>> On Sat, Mar 28, 2020 at 11:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >
>> >
>> > I have looked at the solution proposed and I would like to share my
>> > findings.  I think calling ProcArrayApplyXidAssignment for each
>> > subtransaction is not a good idea for a couple of reasons:
>> > (a) It will just beat the purpose of maintaining KnowAssignedXids
>> > array which is to avoid looking at pg_subtrans in
>> > TransactionIdIsInProgress() on standby.  Basically, if we remove it
>> > for each subXid, it will consider the KnowAssignedXids to be
>> > overflowed and check pg_subtrans frequently.
>>
>> Right, I also think this is a problem with this solution.  I think we
>> may try to avoid this by caching this information.  But, then we will
>> have to maintain this in some dimensional array which stores
>> sub-transaction ids per top transaction or we can maintain a list of
>> sub-transaction for each transaction.  I haven't thought about how
>> much complexity this solution will add.
>>
>
>How about if instead of writing an XLOG_XACT_ASSIGNMENT WAL, we set a
>flag in TransactionStateData and then log that as special information
>whenever we write next WAL record for a new subtransaction?  Then
>during recovery, we can only call ProcArrayApplyXidAssignment when we
>find that special flag is set in a WAL record.  One idea could be to
>use a flag bit in XLogRecord.xl_info.  If that is feasible then the
>solution can work as it is now, without any overhead or change in the
>way we maintain KnownAssignedXids.
>

Ummm, how is that different from what the patch is doing now? I mean, we
only write the top-level XID for the first WAL record in each subxact,
right? Or what would be the difference with your approach?

Anyway, I think you're right the ProcArrayApplyXidAssignment call was
done too early, but I think that can be fixed by moving it until after
the RecordKnownAssignedTransactionIds call, no? Essentially, right
before rm_redo().

You're right calling ProcArrayApplyXidAssignment() may be an issue,
because it exclusively acquires the ProcArrayLock. I've actually hinted
that might be an issue in my original message, suggesting we might add a
local cache of assigned XIDs (a small static array, doing essentially
the same thing we used to do on the upstream node). I haven't done that
in my WIP patch to keep it simple, but AFACS it'd work.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services