On 11 Feb 2021, at 10:42, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Feb 11, 2021 at 1:51 PM Petr Jelinek > <petr.jeli...@enterprisedb.com> wrote: >> >> On 10 Feb 2021, at 06:32, Amit Kapila <amit.kapil...@gmail.com> wrote: >>> >>> On Wed, Feb 10, 2021 at 7:41 AM Peter Smith <smithpb2...@gmail.com> wrote: >>>> >>>> On Tue, Feb 9, 2021 at 10:38 AM Peter Smith <smithpb2...@gmail.com> wrote: >>>>> >>>> >>>> PSA v2 of this WalRcvExceResult patch (it is same as v1 but includes >>>> some PG doc updates). >>>> This applies OK on top of v30 of the main patch. >>>> >>> >>> Thanks, I have integrated these changes into the main patch and >>> additionally made some changes to comments and docs. I have also fixed >>> the function name inconsistency issue you reported and ran pgindent. >> >> One thing: >> >>> + else if (res->status == WALRCV_ERROR && >>> + missing_ok && >>> + res->sqlstate == ERRCODE_UNDEFINED_OBJECT) >>> + { >>> + /* WARNING. Error, but missing_ok = true. */ >>> + ereport(WARNING, >>> (errmsg("could not drop the >>> replication slot \"%s\" on publisher", >>> slotname), >>> errdetail("The error was: %s", >>> res->err))); >> >> Hmm, why is this WARNING, we mostly call it with missing_ok = true when the >> slot is not expected to be there, so it does not seem correct to report it >> as warning? >> > > WARNING is for the cases where we don't always expect slots to exist > and we don't want to stop the operation due to it. For example, in > DropSubscription, for some of the rel states like (SUBREL_STATE_INIT > and SUBREL_STATE_DATASYNC), the slot won't exist. Similarly, say if we > fail (due to network error) after removing some of the slots, next > time, it will again try to drop already dropped slots and fail. For > these reasons, we need to use WARNING. Similarly for tablesync workers > when we are trying to initially drop the slot there is no certainty > that it exists, so we can't throw ERROR and stop the operation there. > There are other cases like when the table sync worker has finished > syncing the table, there we will raise an ERROR if the slot doesn't > exist. Does this make sense?
Well, I was thinking it could be NOTICE or LOG to be honest, WARNING seems unnecessarily scary for those usecases to me. — Petr