MIME-Version: 1.0
References: <20181212204154.nsxf3gzqv3gesl32@alap3.anarazel.de>
 <CA+TgmobS142wm_zZj-WCgDZts9qk=YfsN8iNBgXam_L3S7ny6g@mail.gmail.com>
 <20181214005521.jaty2d24lz4nroil@alap3.anarazel.de>
 <CAJ3gD9d8Af6Fjhkon33kNVSFhdNXJQds=aepGFxWMmh0YMk8Lg@mail.gmail.com>
 <CAJ3gD9dN_PZ+uEPxYHAUstKR2fnGge84OqzEtfNw1kGrTwUfSQ@mail.gmail.com>
 <CAJ3gD9fwvgXO9L+gcoqj-XNuHxFR+iw10GiuoB7ytnUVWMeXeg@mail.gmail.com>
 <CAJ3gD9c4NUBEYJu2wurSTb+ONAmce+SCQCZoA8w2-tx4fNYoqQ@mail.gmail.com>
 <20190402160414.h4wwhetcpggmy3tv@alap3.anarazel.de>
 <CAJ3gD9dnEn6fKXCwdCZF6HGC15oMKaAqEas6_v_psYk0HCZwmw@mail.gmail.com>
 <CAJ3gD9cFg-FgG5P=p7yym2RMQ0cz9bKkOQ+Mp9p6vxm2se1=FA@mail.gmail.com>
 <20190405231553.njjcu7igoyuk2hx5@alap3.anarazel.de>
 <CAJ3gD9f+aFrq_fOuUqdtjapHZKP8V8feXZVE2u4PiEp=32BkEw@mail.gmail.com>
In-Reply-To: 
 <CAJ3gD9f+aFrq_fOuUqdtjapHZKP8V8feXZVE2u4PiEp=32BkEw@mail.gmail.com>
From: Amit Khandekar <amitdkhan.pg@gmail.com>
Date: Wed, 22 May 2019 15:05:29 +0530
Message-ID: 
 <CAJ3gD9dC9oMGXqCAeisNOK2BkeaiOTYO2aTMFzi9SHnKtu4PJg@mail.gmail.com>
Subject: Re: Minimal logical decoding on standbys
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>, Craig Ringer <craig@2ndquadrant.com>,
	Petr Jelinek <petr@2ndquadrant.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 9 Apr 2019 at 22:23, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>
> On Sat, 6 Apr 2019 at 04:45, Andres Freund <andres@anarazel.de> wrote:
> > > diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
> > > index 006446b..5785d2f 100644
> > > --- a/src/backend/replication/slot.c
> > > +++ b/src/backend/replication/slot.c
> > > @@ -1064,6 +1064,85 @@ ReplicationSlotReserveWal(void)
> > >       }
> > >  }
> > >
> > > +void
> > > +ResolveRecoveryConflictWithSlots(Oid dboid, TransactionId xid)
> > > +{
> > > +     int                     i;
> > > +     bool            found_conflict = false;
> > > +
> > > +     if (max_replication_slots <= 0)
> > > +             return;
> > > +
> > > +restart:
> > > +     if (found_conflict)
> > > +     {
> > > +             CHECK_FOR_INTERRUPTS();
> > > +             /*
> > > +              * Wait awhile for them to die so that we avoid flooding an
> > > +              * unresponsive backend when system is heavily loaded.
> > > +              */
> > > +             pg_usleep(100000);
> > > +             found_conflict = false;
> > > +     }
> > > +
> > > +     LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> > > +     for (i = 0; i < max_replication_slots; i++)
> > > +     {
> > > +             ReplicationSlot *s;
> > > +             NameData        slotname;
> > > +             TransactionId slot_xmin;
> > > +             TransactionId slot_catalog_xmin;
> > > +
> > > +             s = &ReplicationSlotCtl->replication_slots[i];
> > > +
> > > +             /* cannot change while ReplicationSlotCtlLock is held */
> > > +             if (!s->in_use)
> > > +                     continue;
> > > +
> > > +             /* not our database, skip */
> > > +             if (s->data.database != InvalidOid && s->data.database != dboid)
> > > +                     continue;
> > > +
> > > +             SpinLockAcquire(&s->mutex);
> > > +             slotname = s->data.name;
> > > +             slot_xmin = s->data.xmin;
> > > +             slot_catalog_xmin = s->data.catalog_xmin;
> > > +             SpinLockRelease(&s->mutex);
> > > +
> > > +             if (TransactionIdIsValid(slot_xmin) && TransactionIdPrecedesOrEquals(slot_xmin, xid))
> > > +             {
> > > +                     found_conflict = true;
> > > +
> > > +                     ereport(WARNING,
> > > +                                     (errmsg("slot %s w/ xmin %u conflicts with removed xid %u",
> > > +                                                     NameStr(slotname), slot_xmin, xid)));
> > > +             }
> > > +
> > > +             if (TransactionIdIsValid(slot_catalog_xmin) && TransactionIdPrecedesOrEquals(slot_catalog_xmin, xid))
> > > +             {
> > > +                     found_conflict = true;
> > > +
> > > +                     ereport(WARNING,
> > > +                                     (errmsg("slot %s w/ catalog xmin %u conflicts with removed xid %u",
> > > +                                                     NameStr(slotname), slot_catalog_xmin, xid)));
> > > +             }
> > > +
> > > +
> > > +             if (found_conflict)
> > > +             {
> > > +                     elog(WARNING, "Dropping conflicting slot %s", s->data.name.data);
> > > +                     LWLockRelease(ReplicationSlotControlLock);      /* avoid deadlock */
> > > +                     ReplicationSlotDropPtr(s);
> > > +
> > > +                     /* We released the lock above; so re-scan the slots. */
> > > +                     goto restart;
> > > +             }
> > > +     }
> > >
> > I think this should be refactored so that the two found_conflict cases
> > set a 'reason' variable (perhaps an enum?) to the particular reason, and
> > then only one warning should be emitted.  I also think that LOG might be
> > more appropriate than WARNING - as confusing as that is, LOG is more
> > severe than WARNING (see docs about log_min_messages).
>
> What I have in mind is :
>
> ereport(LOG,
> (errcode(ERRCODE_INTERNAL_ERROR),
> errmsg("Dropping conflicting slot %s", s->data.name.data),
> errdetail("%s, removed xid %d.", conflict_str, xid)));
> where conflict_str is a dynamically generated string containing
> something like : "slot xmin : 1234, slot catalog_xmin: 5678"
> So for the user, the errdetail will look like :
> "slot xmin: 1234, catalog_xmin: 5678, removed xid : 9012"
> I think the user can figure out whether it was xmin or catalog_xmin or
> both that conflicted with removed xid.
> If we don't do this way, we may not be able to show in a single
> message if both xmin and catalog_xmin are conflicting at the same
> time.
>
> Does this message look good to you, or you had in mind something quite
> different ?

The above one is yet another point that needs to be concluded on. Till
then I will use the above way to display the error message in the
upcoming patch version.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company