Обсуждение: UNLISTEN, DISCARD ALL and readonly standby
Hi hackers.
The documentation for DISCARD ALL[1] state that it is equivalent to a series of commands which includes UNLISTEN *. On the other hand, the docs for hot standby mode[1], state that UNLISTEN * is unsupported while DISCARD is (although the docs don't specify whether this includes DISCARD ALL). I haven't done a test, but this seems to indicate that while DISCARD ALL is supported in hot standby mode, UNLISTEN is not, although its functionality is included in the former.
If this is indeed the case, is there any specific reason UNLISTEN can't be supported? This is actually quite important in the case of Npgsql (.NET driver). When a connection is returned to the connection pool, its state is reset - usually with a DISCARD ALL. However, if prepared statements exist, we avoid DISCARD ALL since it destroys those (the DEALLOCATE ALL component of DISCARD ALL), and we want to keep prepared statements across connection lifecycles for performance. So instead of issuing DISCARD ALL, Npgsql sends the equivalent commands excluding DEALLOCATE ALL - but UNLISTEN * fails when in recovery.
So ideally UNLISTEN would be made to work in standby model, just like DISCARD ALL. If DISCARD ALL is in fact unsupported in hot standby mode, then the docs should probably be updated to reflect that.
Thanks for any information or advice on this!
Shay Rojansky <roji@roji.org> writes:
> The documentation for DISCARD ALL[1] state that it is equivalent to a
> series of commands which includes UNLISTEN *. On the other hand, the docs
> for hot standby mode[1], state that UNLISTEN * is unsupported while DISCARD
> is (although the docs don't specify whether this includes DISCARD ALL). I
> haven't done a test, but this seems to indicate that while DISCARD ALL is
> supported in hot standby mode, UNLISTEN is not, although its functionality
> is included in the former.
Well, if there weren't
PreventCommandDuringRecovery("UNLISTEN");
in it, UNLISTEN on a standby would just be a no-op, because there'd be
nothing for it to do. DISCARD lacks that error check, but its call to
the UNLISTEN support code is still a no-op.
The reason for disallowing LISTEN/UNLISTEN on a standby is that they
wouldn't behave as a reasonable person might expect, ie seeing NOTIFY
events from the master. At some point we might allow that to happen, and
we don't want to have complaints about backwards compatibility if we do.
I guess there's an argument to be made that it'd be OK to allow UNLISTEN
but not LISTEN, but I find that argument fairly fishy. I think issuing
UNLISTEN to a standby is a likely sign of application misdesign, so I'm
not convinced we'd be doing anyone any favors by not throwing an error.
> If this is indeed the case, is there any specific reason UNLISTEN can't be
> supported? This is actually quite important in the case of Npgsql (.NET
> driver). When a connection is returned to the connection pool, its state is
> reset - usually with a DISCARD ALL. However, if prepared statements exist,
> we avoid DISCARD ALL since it destroys those (the DEALLOCATE ALL component
> of DISCARD ALL), and we want to keep prepared statements across connection
> lifecycles for performance. So instead of issuing DISCARD ALL, Npgsql sends
> the equivalent commands excluding DEALLOCATE ALL - but UNLISTEN * fails
> when in recovery.
Can't you skip that step when talking to a read-only session? I think
you're going to end up writing that code anyway, even if we accepted
this request, because it'd be a long time before you could count on
all servers having absorbed the patch.
regards, tom lane
Thanks for explanation.
> I guess there's an argument to be made that it'd be OK to allow UNLISTEN
> but not LISTEN, but I find that argument fairly fishy. I think issuing
> UNLISTEN to a standby is a likely sign of application misdesign, so I'm
> not convinced we'd be doing anyone any favors by not throwing an error.
> but not LISTEN, but I find that argument fairly fishy. I think issuing
> UNLISTEN to a standby is a likely sign of application misdesign, so I'm
> not convinced we'd be doing anyone any favors by not throwing an error.
I understand the argument. It would definitely be a fix for my specific problem, and I do think in some application designs it may make sense for an application to say "UNLISTEN from everything" regardless of whether any LISTENs were previously issued - this could make things easier as the application doesn't need to be aware of the backend it's connected to (master, standby) from the place where UNLISTEN is managed So I don't think it *necessarily* points to bad application design.
> Can't you skip that step when talking to a read-only session? I think
> you're going to end up writing that code anyway, even if we accepted
> this request, because it'd be a long time before you could count on
> all servers having absorbed the patch.
> you're going to end up writing that code anyway, even if we accepted
> this request, because it'd be a long time before you could count on
> all servers having absorbed the patch.
That possibility was raised, but the problem is how to manage the client's idea of whether the backend is read-only. If we call pg_is_in_recovery() every time a connection is returned to the pool, that's a performance killer. If we cache that information, we miss any state changes that may happen along the way. I'm open to any suggestions on this though.
Regarding older versions of PostgreSQL, I hoping this is small enough to be backported to at least active branches. In any case, a workaround already exists to tell Npgsql to not reset connection state at all when returning to the pool. This is what I'm recommending to the affected user in the meantime.
On Thu, Oct 25, 2018 at 10:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Shay Rojansky <roji@roji.org> writes:
> The documentation for DISCARD ALL[1] state that it is equivalent to a
> series of commands which includes UNLISTEN *. On the other hand, the docs
> for hot standby mode[1], state that UNLISTEN * is unsupported while DISCARD
> is (although the docs don't specify whether this includes DISCARD ALL). I
> haven't done a test, but this seems to indicate that while DISCARD ALL is
> supported in hot standby mode, UNLISTEN is not, although its functionality
> is included in the former.
Well, if there weren't
PreventCommandDuringRecovery("UNLISTEN");
in it, UNLISTEN on a standby would just be a no-op, because there'd be
nothing for it to do. DISCARD lacks that error check, but its call to
the UNLISTEN support code is still a no-op.
The reason for disallowing LISTEN/UNLISTEN on a standby is that they
wouldn't behave as a reasonable person might expect, ie seeing NOTIFY
events from the master. At some point we might allow that to happen, and
we don't want to have complaints about backwards compatibility if we do.
I guess there's an argument to be made that it'd be OK to allow UNLISTEN
but not LISTEN, but I find that argument fairly fishy. I think issuing
UNLISTEN to a standby is a likely sign of application misdesign, so I'm
not convinced we'd be doing anyone any favors by not throwing an error.
> If this is indeed the case, is there any specific reason UNLISTEN can't be
> supported? This is actually quite important in the case of Npgsql (.NET
> driver). When a connection is returned to the connection pool, its state is
> reset - usually with a DISCARD ALL. However, if prepared statements exist,
> we avoid DISCARD ALL since it destroys those (the DEALLOCATE ALL component
> of DISCARD ALL), and we want to keep prepared statements across connection
> lifecycles for performance. So instead of issuing DISCARD ALL, Npgsql sends
> the equivalent commands excluding DEALLOCATE ALL - but UNLISTEN * fails
> when in recovery.
Can't you skip that step when talking to a read-only session? I think
you're going to end up writing that code anyway, even if we accepted
this request, because it'd be a long time before you could count on
all servers having absorbed the patch.
regards, tom lane