Обсуждение: [COMMITTERS] pgsql: Fix connection leak in DROP SUBSCRIPTION command.
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. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/898a792eb8283e31efc0b6fcbc03bbcd5f7df667 Modified Files -------------- src/backend/commands/subscriptioncmds.c | 4 ++++ 1 file changed, 4 insertions(+)
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, 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.
regards, tom lane
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, 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. -- Michael
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? Regards, -- Fujii Masao
Вложения
On 22/02/17 00:39, Fujii Masao 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? > Looks more or less like what we do in CREATE SUBSCRIPTION for this, so I guess it's okay. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
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
On Wed, Feb 22, 2017 at 1:27 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > 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. Petr and Michael, Thanks for the review! Pushed. Regards, -- Fujii Masao