On 11 Feb 2021, at 10:56, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Feb 11, 2021 at 3:20 PM Petr Jelinek > <petr.jeli...@enterprisedb.com> wrote: >> >> 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. >> > > I am fine with LOG and will make that change. Do you have any more > comments or want to spend more time on this patch before we call it > good?
I am good, thanks! — Petr