Re: Single transaction in the tablesync worker?

On 11 Feb 2021, at 10:56, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 11, 2021 at 3:20 PM Petr Jelinek
> <petr.jelinek@enterprisedb.com> wrote:
>>
>> On 11 Feb 2021, at 10:42, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>>
>>> On Thu, Feb 11, 2021 at 1:51 PM Petr Jelinek
>>> <petr.jelinek@enterprisedb.com> wrote:
>>>>
>>>> On 10 Feb 2021, at 06:32, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>>>>
>>>>> On Wed, Feb 10, 2021 at 7:41 AM Peter Smith <smithpb2250@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Feb 9, 2021 at 10:38 AM Peter Smith <smithpb2250@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
itdoes 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