On Wed, Feb 22, 2017 at 8:39 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Feb 22, 2017 at 6:57 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Feb 22, 2017 at 4:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Fujii Masao <fujii@postgresql.org> writes:
>>>> Fix connection leak in DROP SUBSCRIPTION command.
>>>> Previously the command forgot to close the connection to the publisher
>>>> when it failed to drop the replication slot.
>>>
>>> If there's a bug here, this seems like an extremely unreliable way of
>>> fixing it. What if an error gets thrown before you reach that ereport?
>>>
>>> In other words, this coding is assuming that the walrcv_command()
>>> subroutine cannot throw an error,
>
> Yes, but I agree that walrcv_command() may be changed in the future so that
> an error is thrown and current coding is not reliable in that case.
>
>>> which I would consider dangerous
>>> even if it were a fixed subroutine. If it's a hook that's doing
>>> unknown stuff, that seems a completely untenable assumption. You
>>> really need either to hook the cleanup action into normal error
>>> recovery, or to use a PG_TRY block.
>>
>> To be honest, I have thought about using PG_ENSURE_ERROR_CLEANUP()
>> when seeing the thread. If other ERROR messages are generated in the
>> future that the current fix would be unreliable.
>
> What about the attached patch?
Thanks for the patch. That looks good to me.
--
Michael