Re: Truncate in synchronous logical replication failed

On Fri, Apr 23, 2021 at 12:04 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Thursday, April 22, 2021 6:33 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Tue, Apr 20, 2021 at 9:00 AM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> > >
> > > On Tuesday, April 20, 2021 10:53 AM Ajin Cherian <itsajin(at)gmail(dot)com>
> > wrote:
> > > >
> > > > I reviewed the patch, ran make check, no issues. One minor comment:
> > > >
> > > > Could you add the comment similar to RelationGetIndexAttrBitmap() on
> > > > why the redo, it's not very obvious to someone reading the code, why
> > > > we are refetching the index list here.
> > > >
> > > > + /* Check if we need to redo */
> > > >
> > > > + newindexoidlist = RelationGetIndexList(relation);
> > > Yeah, makes sense. Fixed.
> >
> > I don't think here we need to restart to get a stable list of indexes as we do in
> > RelationGetIndexAttrBitmap. The reason is here we build the cache entry
> > using a historic snapshot and all the later changes are absorbed while
> > decoding WAL.
> I rechecked this and I agree with this.
> I don't see any problem to remove the redo check.
> Based on this direction, I'll share my several minor comments.
>
> [1] a typo of RelationGetIdentityKeyBitmap's comment
>
> + * This is a special purpose function used during logical replication. Here,
> + * unlike RelationGetIndexAttrBitmap(), we don't a acquire lock on the required
>
> We have "a" in an unnatural place. It should be "we don't acquire...".
>
> [2] suggestion to fix RelationGetIdentityKeyBitmap's comment
>
> + * later changes are absorbed while decoding WAL. Due to this reason, we don't
> + * need to retry here in case of a change in the set of indexes.
>
> I think it's better to use "Because of" instead of "Due to".
> This patch works to solve the deadlock.
>

I am not sure which one is better. I would like to keep it as it is
unless you feel strongly about point 2.

> [3] wrong comment in RelationGetIdentityKeyBitmap
>
> + /* Save some values to compare after building attributes */
>
> I wrote this comment for the redo check
> that has been removed already. We can delete it.
>
> [4] suggestion to remove local variable relreplindex in RelationGetIdentityKeyBitmap
>
> I think we don't need relreplindex.
> We can just pass relaton->rd_replidindex to RelationIdGetRelation().
> There is no more reference of the variable.
>
> [5] suggestion to fix the place to release indexoidlist
>
> I thought we can do its list_free() ealier,
> after checking if there is no indexes.
>

Hmm, why? If there are no indexes then we wouldn't have allocated any memory.

> [6] suggestion to remove period in one comment.
>
> + /* Quick exit if we already computed the result. */
>
> This comes from RelationGetIndexAttrBitmap (and my posted versions)
> but I think we can remove the period to give better alignment shared with other comments in the function.
>

Can you please update the patch except for the two points to which I
responded above?

--
With Regards,
Amit Kapila.