Re: Truncate in synchronous logical replication failed

On Thu, 15 Apr 2021 at 18:22, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Apr 14, 2021 at 3:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>> On Tue, Apr 13, 2021 at 8:07 PM Petr Jelinek
>> <petr(dot)jelinek(at)enterprisedb(dot)com> wrote:
>> >
>> > > On 12 Apr 2021, at 08:58, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> > >
>> > > The problem happens only when we try to fetch IDENTITY_KEY attributes
>> > > because pgoutput uses RelationGetIndexAttrBitmap() to get that
>> > > information which locks the required indexes. Now, because TRUNCATE
>> > > has already acquired an exclusive lock on the index, it seems to
>> > > create a sort of deadlock where the actual Truncate operation waits
>> > > for logical replication of operation to complete and logical
>> > > replication waits for actual Truncate operation to finish.
>> > >
>> > > Do we really need to use RelationGetIndexAttrBitmap() to build
>> > > IDENTITY_KEY attributes? During decoding, we don't even lock the main
>> > > relation, we just scan the system table and build that information
>> > > using a historic snapshot. Can't we do something similar here?
>> > >
>> > > Adding Petr J. and Peter E. to know their views as this seems to be an
>> > > old problem (since the decoding of Truncate operation is introduced).
>> >
>> > We used RelationGetIndexAttrBitmap because it already existed, no other reason.
>> >
>>
>> Fair enough. But I think we should do something about it because using
>> the same (RelationGetIndexAttrBitmap) just breaks the synchronous
>> logical replication. I think this is broken since the logical
>> replication of Truncate is supported.
>>
>> > I am not sure what exact locking we need but I would have guessed at least AccessShareLock would be needed.
>> >
>>
>> Are you telling that we need AccessShareLock on the index? If so, why
>> is it different from how we access the relation during decoding
>> (basically in ReorderBufferProcessTXN, we directly use
>> RelationIdGetRelation() without any lock on the relation)? I think we
>> do it that way because we need it to process WAL entries and we need
>> the relation state based on the historic snapshot, so, even if the
>> relation is later changed/dropped, we are fine with the old state we
>> got. Isn't the same thing applies here in logicalrep_write_attrs? If
>> that is true then some equivalent of RelationGetIndexAttrBitmap where
>> we use RelationIdGetRelation instead of index_open should work?
>>
>
> Today, again I have thought about this and don't see a problem with
> the above idea. If the above understanding is correct, then I think
> for our purpose in pgoutput, we just need to call RelationGetIndexList
> and then build the idattr list for relation->rd_replidindex.

Sorry, I don't know how can we build the idattr without open the index.
If we should open the index, then we should use NoLock, since the TRUNCATE
side hold AccessExclusiveLock. As Osumi points out in [1], The NoLock mode
assumes that the appropriate lock on the index is already taken.

Please let me know if I have misunderstood.

[1] https://www.postgresql.org/message-id/OSBPR01MB488834BDBD67BFF2FB048B3DED4E9%40OSBPR01MB4888.jpnprd01.prod.outlook.com

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.