Обсуждение: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
[HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
Hi, Currently, clients wishing to know when the server exits hot standby have to resort to polling, which is often suboptimal. This adds the new "in_hot_standby" GUC variable that is reported via a ParameterStatus message. This allows the clients to: (a) know right away that they are connected to a server in hot standby; and (b) know immediately when a server exits hot standby. This change will be most beneficial to various connection pooling systems (pgpool etc.) Elvis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Michael Paquier
Дата:
On Sat, Mar 18, 2017 at 2:56 AM, Elvis Pranskevichus <elprans@gmail.com> wrote: > Currently, clients wishing to know when the server exits hot standby > have to resort to polling, which is often suboptimal. > > This adds the new "in_hot_standby" GUC variable that is reported via a > ParameterStatus message. This allows the clients to: > > (a) know right away that they are connected to a server in hot > standby; and > > (b) know immediately when a server exits hot standby. > > This change will be most beneficial to various connection pooling > systems (pgpool etc.) Why adding a good chunk of code instead of using pg_is_in_recovery(), which switches to false once a server exits recovery? -- Michael
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote: > On Sat, Mar 18, 2017 at 2:56 AM, Elvis Pranskevichus <elprans@gmail.com> wrote: > > Currently, clients wishing to know when the server exits hot standby > > have to resort to polling, which is often suboptimal. > > > > This adds the new "in_hot_standby" GUC variable that is reported via > > a> > > ParameterStatus message. This allows the clients to: > > (a) know right away that they are connected to a server in hot > > > > standby; and > > > > (b) know immediately when a server exits hot standby. > > > > This change will be most beneficial to various connection pooling > > systems (pgpool etc.) > > Why adding a good chunk of code instead of using pg_is_in_recovery(), > which switches to false once a server exits recovery? That requires polling the database continuously, which may not be possible or desirable. My main motivation here is to gain the ability to manage a pool of connections in asyncpg efficiently. A part of the connection release protocol is "UNLISTEN *;", which the server in Hot Standby would fail to process. Polling the database for pg_is_in_recovery() is not feasible in this case, unfortunately. Elvis
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUCpseudo-variable.
От
Peter Eisentraut
Дата:
On 3/17/17 13:56, Elvis Pranskevichus wrote: > Currently, clients wishing to know when the server exits hot standby > have to resort to polling, which is often suboptimal. > > This adds the new "in_hot_standby" GUC variable that is reported via a > ParameterStatus message. The terminology chosen here is not very clear. What is the opposite of "in hot standby"? Warm standby? Cold standby? Not standby at all? Promoted to primary (writable)? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Tuesday, March 21, 2017 11:50:38 PM EDT Peter Eisentraut wrote: > On 3/17/17 13:56, Elvis Pranskevichus wrote: > > Currently, clients wishing to know when the server exits hot standby > > have to resort to polling, which is often suboptimal. > > > > This adds the new "in_hot_standby" GUC variable that is reported via > > a ParameterStatus message. > > The terminology chosen here is not very clear. What is the opposite > of "in hot standby"? Warm standby? Cold standby? Not standby at > all? Promoted to primary (writable)? The opposite means primary. I can flip the GUC name to "is_primary", if that's clearer. Elvis
On Wed, Mar 22, 2017 at 8:25 AM, Elvis Pranskevichus <elprans@gmail.com> wrote: > On Tuesday, March 21, 2017 11:50:38 PM EDT Peter Eisentraut wrote: >> On 3/17/17 13:56, Elvis Pranskevichus wrote: >> > Currently, clients wishing to know when the server exits hot standby >> > have to resort to polling, which is often suboptimal. >> > >> > This adds the new "in_hot_standby" GUC variable that is reported via >> > a ParameterStatus message. >> >> The terminology chosen here is not very clear. What is the opposite >> of "in hot standby"? Warm standby? Cold standby? Not standby at >> all? Promoted to primary (writable)? > > The opposite means primary. I can flip the GUC name to "is_primary", if > that's clearer. Hmm, I don't find that clearer. "hot standby" has a very specific meaning; "primary" isn't vague, but I would say it's less specific. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Jaime Casanova
Дата:
On 18 March 2017 at 14:01, Elvis Pranskevichus <elprans@gmail.com> wrote: > On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote: >> >> Why adding a good chunk of code instead of using pg_is_in_recovery(), >> which switches to false once a server exits recovery? > > That requires polling the database continuously, which may not be > possible or desirable. > > My main motivation here is to gain the ability to manage a pool of > connections in asyncpg efficiently. A part of the connection release > protocol is "UNLISTEN *;", which the server in Hot Standby would fail to > process. Polling the database for pg_is_in_recovery() is not feasible > in this case, unfortunately. > Sorry, i still don't understand the motivation for this. At one point you're going to poll for the value of the GUC in pg_settings, no? Or how are you going to know the current value of the GUC that makes it different to just poll for pg_is_in_recovery()? -- Jaime Casanova www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Wednesday, March 22, 2017 2:17:27 PM EDT Jaime Casanova wrote: > On 18 March 2017 at 14:01, Elvis Pranskevichus <elprans@gmail.com> wrote: > > On Saturday, March 18, 2017 3:33:16 AM EDT Michael Paquier wrote: > >> Why adding a good chunk of code instead of using > >> pg_is_in_recovery(), > >> which switches to false once a server exits recovery? > > > > That requires polling the database continuously, which may not be > > possible or desirable. > > > > My main motivation here is to gain the ability to manage a pool of > > connections in asyncpg efficiently. A part of the connection > > release > > protocol is "UNLISTEN *;", which the server in Hot Standby would > > fail to process. Polling the database for pg_is_in_recovery() is > > not feasible in this case, unfortunately. > > Sorry, i still don't understand the motivation for this. > At one point you're going to poll for the value of the GUC in > pg_settings, no? Or how are you going to know the current value of > the GUC that makes it different to just poll for pg_is_in_recovery()? It is marked as GUC_REPORT and is reported by the server on connection and on every change: https://www.postgresql.org/docs/current/static/protocol-flow.html#PROTOCOL-ASYNC Elvis
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUCpseudo-variable.
От
Peter Eisentraut
Дата:
On 3/22/17 14:09, Robert Haas wrote: >> The opposite means primary. I can flip the GUC name to "is_primary", if >> that's clearer. > Hmm, I don't find that clearer. "hot standby" has a very specific > meaning; "primary" isn't vague, but I would say it's less specific. The problem I have is that there is already a GUC named "hot_standby", which determines whether an instance is in hot (as opposed to warm?) mode if it is a standby. This is proposing to add a setting named "in_hot_standby" which says nothing about the hotness, but something about the standbyness. Note that these are all in the same namespace. I think we could use "in_recovery", which would be consistent with existing naming. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Mar 22, 2017 at 4:00 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/22/17 14:09, Robert Haas wrote: >>> The opposite means primary. I can flip the GUC name to "is_primary", if >>> that's clearer. >> Hmm, I don't find that clearer. "hot standby" has a very specific >> meaning; "primary" isn't vague, but I would say it's less specific. > > The problem I have is that there is already a GUC named "hot_standby", > which determines whether an instance is in hot (as opposed to warm?) > mode if it is a standby. This is proposing to add a setting named > "in_hot_standby" which says nothing about the hotness, but something > about the standbyness. Note that these are all in the same namespace. Good point. > I think we could use "in_recovery", which would be consistent with > existing naming. True. (Jaime's question is also on point, I think.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Wednesday, March 22, 2017 4:28:18 PM EDT Robert Haas wrote: > On Wed, Mar 22, 2017 at 4:00 PM, Peter Eisentraut > > I think we could use "in_recovery", which would be consistent with > > existing naming. > > True. Ironically, that was the name I originally used. I'll update the patch. > (Jaime's question is also on point, I think.) The main (and only) point of this patch is to avoid polling. Since "in_recovery" is marked as GUC_REPORT, it will be sent to the client asynchronously in a ParamStatus message. Other GUC_REPORT variables set a good precedent. My argument is that Hot Standby is a major mode of operation, which significantly alters how connected clients work with the server, and this bit of knowledge is no less important than the other GUC_REPORT vars. Elvis
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Magnus Hagander
Дата:
On Wed, Mar 22, 2017 at 9:00 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 3/22/17 14:09, Robert Haas wrote:
>> The opposite means primary. I can flip the GUC name to "is_primary", if
>> that's clearer.
> Hmm, I don't find that clearer. "hot standby" has a very specific
> meaning; "primary" isn't vague, but I would say it's less specific.
The problem I have is that there is already a GUC named "hot_standby",
which determines whether an instance is in hot (as opposed to warm?)
mode if it is a standby. This is proposing to add a setting named
"in_hot_standby" which says nothing about the hotness, but something
about the standbyness. Note that these are all in the same namespace.
I think we could use "in_recovery", which would be consistent with
existing naming.
One thing we might want to consider around this -- in 10 we have target_session_attrs=read-write (since 721f7bd3cbccaf8c07cad2707826b83f84694832), which will issue a SHOW transaction_read_only on the connection.
We should probably consider if there is some way we can implement these two things the same way. If we're inventing a new variable that gets pushed on each connection, perhaps we can use that one and avoid the SHOW command? Is the read-write thing really interesting in the aspect of the general case, or is it more about detectinv readonly standbys as well? Or to flip that, would sending the transaction_read_only parameter be enough for the usecase in this thread, without having to invent a new variable?
(I haven't thought it through all the way, but figured I should mention the thought as I'm working through my email backlog.)
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Thursday, March 23, 2017 4:50:20 AM EDT Magnus Hagander wrote: > We should probably consider if there is some way we can implement > these two things the same way. If we're inventing a new variable that > gets pushed on each connection, perhaps we can use that one and avoid > the SHOW command? Is the read-write thing really interesting in the > aspect of the general case, or is it more about detectinv readonly > standbys as well? Or to flip that, would sending the > transaction_read_only parameter be enough for the usecase in this > thread, without having to invent a new variable? Hot standby differs from regular read-only transactions in a number of ways. Most importantly, it prohibits LISTEN/NOTIFY. Grep for PreventCommandDuringRecovery() if you're interested in the scope. Elvis
On Thu, Mar 23, 2017 at 4:50 AM, Magnus Hagander <magnus@hagander.net> wrote: > One thing we might want to consider around this -- in 10 we have > target_session_attrs=read-write (since > 721f7bd3cbccaf8c07cad2707826b83f84694832), which will issue a SHOW > transaction_read_only on the connection. > > We should probably consider if there is some way we can implement these two > things the same way. If we're inventing a new variable that gets pushed on > each connection, perhaps we can use that one and avoid the SHOW command? I think that would be a good idea. It was, in fact, proposed to do exactly that as part of the patch that added target_session_attrs=read-write, but we ended up not doing anything about it because the SHOW mechanism would still be needed when connecting to pre-10 versions of PostgreSQL. Therefore, it seemed like a separate improvement. But if we're adding a GUC_REPORT value that could be used for the same or a similar purpose, I think it would make sense to consider revising that mechanism to leverage it as well, obviously only on releases that have the GUC. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Magnus Hagander
Дата:
On Wed, Apr 5, 2017 at 6:22 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Mar 23, 2017 at 4:50 AM, Magnus Hagander <magnus@hagander.net> wrote:
> One thing we might want to consider around this -- in 10 we have
> target_session_attrs=read-write (since
> 721f7bd3cbccaf8c07cad2707826b83f84694832), which will issue a SHOW
> transaction_read_only on the connection.
>
> We should probably consider if there is some way we can implement these two
> things the same way. If we're inventing a new variable that gets pushed on
> each connection, perhaps we can use that one and avoid the SHOW command?
I think that would be a good idea. It was, in fact, proposed to do
exactly that as part of the patch that added
target_session_attrs=read-write, but we ended up not doing anything
about it because the SHOW mechanism would still be needed when
connecting to pre-10 versions of PostgreSQL. Therefore, it seemed
like a separate improvement. But if we're adding a GUC_REPORT value
that could be used for the same or a similar purpose, I think it would
make sense to consider revising that mechanism to leverage it as well,
obviously only on releases that have the GUC.
Based on that we seem to agree here, should we add this as an open item? Clearly if we want to change this, we should do so before 10.
I also came up with another case where the current one won't work but it could be really useful -- if we make a replication connection (with say pg_receivewal) it would be good to be able to say "i want the master" (or "i want a standby") the same way. And that will fail today if it needs SHOW to work, right?
So having it send that information across in the startup package when talking to a 10 server, but falling back to using SHOW if talking to an earlier server, would make a lot of sense I think.
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Michael Paquier
Дата:
On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net> wrote: > Based on that we seem to agree here, should we add this as an open item? > Clearly if we want to change this, we should do so before 10. This really is a new feature, so as the focus is to stabilize things I think that we should not make the code more complicated because... > I also came up with another case where the current one won't work but it > could be really useful -- if we make a replication connection (with say > pg_receivewal) it would be good to be able to say "i want the master" (or "i > want a standby") the same way. And that will fail today if it needs SHOW to > work, right? > > So having it send that information across in the startup package when > talking to a 10 server, but falling back to using SHOW if talking to an > earlier server, would make a lot of sense I think. Of this reason, as libpq needs to be compliant with past server versions as well we are never going to save a set of version-dependent if/else code to handle target_session_attrs properly using either a SHOW or a new mechanism. -- Michael
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Magnus Hagander
Дата:
On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Based on that we seem to agree here, should we add this as an open item?
> Clearly if we want to change this, we should do so before 10.
This really is a new feature, so as the focus is to stabilize things I
think that we should not make the code more complicated because...
The part I'm talking about is the potential adjustment of the patch that's already committed. That's not a new feature, that's exactly the sort of thing we'd want to adjust before we get to release. Because once released we really can't change it.
> I also came up with another case where the current one won't work but it
> could be really useful -- if we make a replication connection (with say
> pg_receivewal) it would be good to be able to say "i want the master" (or "i
> want a standby") the same way. And that will fail today if it needs SHOW to
> work, right?
>
> So having it send that information across in the startup package when
> talking to a 10 server, but falling back to using SHOW if talking to an
> earlier server, would make a lot of sense I think.
Of this reason, as libpq needs to be compliant with past server
versions as well we are never going to save a set of version-dependent
if/else code to handle target_session_attrs properly using either a
SHOW or a new mechanism.
We'd have to cache the status recived yes. I don't see how that makes it a "set of" if/else code when there is only one if/else now, though? Though admittedly I haven't looked at the actual code for it.
On 11 April 2017 at 09:05, Magnus Hagander <magnus@hagander.net> wrote: > On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <michael.paquier@gmail.com> > wrote: >> >> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net> >> wrote: >> > Based on that we seem to agree here, should we add this as an open item? >> > Clearly if we want to change this, we should do so before 10. >> >> This really is a new feature, so as the focus is to stabilize things I >> think that we should not make the code more complicated because... > > > The part I'm talking about is the potential adjustment of the patch that's > already committed. That's not a new feature, that's exactly the sort of > thing we'd want to adjust before we get to release. Because once released we > really can't change it. I agree if we introduce target_session_attrs it would be better to have a complete feature in one release. It does seem quite strange to have target_session_attrs=read-write but not target_session_attrs=read-only And it would be even better to have these session attrs as well notify-on-promote - sent when standby is promoted notify-on-write- sent when an xid is assigned "notify-on-promotion" being my suggested name for the feature being discussed here. In terms of the feature as submitted, I wonder whether having a GUC parameter like this makes sense, but I think its ok for us to send a protocol message, maybe a NotificationResponse, but there isn't any material difference between those two protocol messages. Rather than the special case code in the patch, I imagine more generic code like this... if (sessionInterruptPending) ProcessSessionInterrupt(); I'm happy to work on the patch, if that's OK. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Magnus Hagander
Дата:
On Tue, Apr 11, 2017 at 2:38 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
On 11 April 2017 at 09:05, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net>
>> wrote:
>> > Based on that we seem to agree here, should we add this as an open item?
>> > Clearly if we want to change this, we should do so before 10.
>>
>> This really is a new feature, so as the focus is to stabilize things I
>> think that we should not make the code more complicated because...
>
>
> The part I'm talking about is the potential adjustment of the patch that's
> already committed. That's not a new feature, that's exactly the sort of
> thing we'd want to adjust before we get to release. Because once released we
> really can't change it.
I agree if we introduce target_session_attrs it would be better to
have a complete feature in one release.
It does seem quite strange to have
target_session_attrs=read-write
but not
target_session_attrs=read-only
And it would be even better to have these session attrs as well
notify-on-promote - sent when standby is promoted
notify-on-write - sent when an xid is assigned
Well, one of those could come automatically with a GUC_REPORT variable of the correct type, no? So if we were to use the transaction_read_only one, you'd get a notification on promotion because your transaction became read/write, wouldn't it?
"notify-on-promotion" being my suggested name for the feature being
discussed here. In terms of the feature as submitted, I wonder whether
having a GUC parameter like this makes sense, but I think its ok for
us to send a protocol message, maybe a NotificationResponse, but there
isn't any material difference between those two protocol messages.
Rather than the special case code in the patch, I imagine more generic
code like this...
if (sessionInterruptPending)
ProcessSessionInterrupt();
I'm happy to work on the patch, if that's OK.
I think going through all those steps is moving the goalposts a bit too far for the 10 release.
But if adjustment to the already applied patch is needed to make sure we can improve on it to get to this point in 11, that's more on topic I think.
On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander <magnus@hagander.net> wrote: > On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <michael.paquier@gmail.com> wrote: >> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net> >> wrote: >> > Based on that we seem to agree here, should we add this as an open item? >> > Clearly if we want to change this, we should do so before 10. >> >> This really is a new feature, so as the focus is to stabilize things I >> think that we should not make the code more complicated because... > > The part I'm talking about is the potential adjustment of the patch that's > already committed. That's not a new feature, that's exactly the sort of > thing we'd want to adjust before we get to release. Because once released we > really can't change it. I don't really agree. I think if we go and install a GUC_REPORT GUC now, we're much less likely to flush out the bugs in the 'show transaction_read_only' mechanism. Also, now that I think about, the reason why we settled on 'show transaction_read_only' against alternate queries is because there's some ability for the DBA to make that return 'false' rather than 'true' even when not in recovery, so that if for example you are using logical replication rather than physical replication, you have a way to make target_session_attrs=read-write still do something useful. If you add an in_hot_standby GUC that's used instead, you lose that. Now, we can decide what we want to do about that, but I don't see that a change in this area *must* go into v10. Maybe the answer is that target_session_attrs grows additional values like 'primary' and 'standby' alongside 'read-write' (and Simon's suggested 'read-only'). Or maybe we have another idea. But I don't really see the urgency of whacking this around right this minute. There's nothing broken here; there's just more stuff people would like to have. It can be added next time around. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Magnus Hagander
Дата:
On Wed, Apr 12, 2017 at 2:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
-- On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Tue, Apr 11, 2017 at 3:26 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>> On Mon, Apr 10, 2017 at 5:47 PM, Magnus Hagander <magnus@hagander.net>
>> wrote:
>> > Based on that we seem to agree here, should we add this as an open item?
>> > Clearly if we want to change this, we should do so before 10.
>>
>> This really is a new feature, so as the focus is to stabilize things I
>> think that we should not make the code more complicated because...
>
> The part I'm talking about is the potential adjustment of the patch that's
> already committed. That's not a new feature, that's exactly the sort of
> thing we'd want to adjust before we get to release. Because once released we
> really can't change it.
I don't really agree. I think if we go and install a GUC_REPORT GUC
now, we're much less likely to flush out the bugs in the 'show
transaction_read_only' mechanism. Also, now that I think about, the
reason why we settled on 'show transaction_read_only' against
alternate queries is because there's some ability for the DBA to make
that return 'false' rather than 'true' even when not in recovery, so
that if for example you are using logical replication rather than
physical replication, you have a way to make
target_session_attrs=read-write still do something useful. If you add
an in_hot_standby GUC that's used instead, you lose that.
Now, we can decide what we want to do about that, but I don't see that
a change in this area *must* go into v10. Maybe the answer is that
target_session_attrs grows additional values like 'primary' and
'standby' alongside 'read-write' (and Simon's suggested 'read-only').
Or maybe we have another idea. But I don't really see the urgency of
whacking this around right this minute. There's nothing broken here;
there's just more stuff people would like to have. It can be added
next time around.
Fair enough, sounds reasonable. I wasn't engaged in the original thread, so you clearly have thought about this more than I have. I just wanted to make sure we're not creating something that's going to cause a head-ache for such a feature in the future.
(And this is why I was specifically asking you if you wanted it on the open items list or not!)
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby"GUC pseudo-variable.
От
"Tsunakawa, Takayuki"
Дата:
Hello, I didn't realize that my target_session_attrs naming proposal was committed. I didn't think half way it would be adopted,because the name is less intuitive than the original target_server_type, and is different from the PgJDBC's targetServerType. From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Simon Riggs > I agree if we introduce target_session_attrs it would be better to have > a complete feature in one release. > > It does seem quite strange to have > target_session_attrs=read-write > but not > target_session_attrs=read-only I totally agree. I'm a bit disappointed with and worried about the current situation. I could easily imagine that peoplearound me would say a stern opinion on the specification... I think these are necessary in descending order of priority, if based on target_session_attrs: [PG10] 1. target_session_attrs=read-only This is mainly to connect to the standby. People will naturally expect that this is available, because PostgreSQL provideshot standby feature, and other DBMSs and even PgJDBC has the functionality. 2. Make transaction_read_only GUC_REPORT This is to avoid the added round-trip by SHOW command. It also benefits client apps that want to know when the server getspromoted? And this may simplify the libpq code. I don't understand yet why we need to provide this feature for older servers by using SHOW. Those who are already using<= 9.6 in production completed the system or application, and their business is running. Why would they want to justreplace libpq and use this feature? [PG 11] 3. target_session_attrs=prefer-read-only This is mainly to prefer standbys, but allows to connect to the primary if no standby is available. Honestly, this is alsorequired in PG 10 because PgJDBC already provides this by "preferSlave". From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas > On Tue, Apr 11, 2017 at 4:05 AM, Magnus Hagander <magnus@hagander.net> > wrote: > > The part I'm talking about is the potential adjustment of the patch > > that's already committed. That's not a new feature, that's exactly the > > sort of thing we'd want to adjust before we get to release. Because > > once released we really can't change it. > > I don't really agree. I think if we go and install a GUC_REPORT GUC now, > we're much less likely to flush out the bugs in the 'show > transaction_read_only' mechanism. I'm sorry I couldn't get this part (maybe purely English nuance. Are you concerned about some bugs? We can't do anythingif we fear of bugs. Is it OK instead to make transaction_read_only GUC_REPORT? > Also, now that I think about, the reason > why we settled on 'show transaction_read_only' against alternate queries > is because there's some ability for the DBA to make that return 'false' > rather than 'true' even when not in recovery, so that if for example you > are using logical replication rather than physical replication, you have > a way to make target_session_attrs=read-write still do something useful. > If you add an in_hot_standby GUC that's used instead, you lose that. Agreed. Again, is this satisfied by GUC_REPORTing transaction_read_only as well? > Now, we can decide what we want to do about that, but I don't see that a > change in this area *must* go into v10. Maybe the answer is that > target_session_attrs grows additional values like 'primary' and 'standby' > alongside 'read-write' (and Simon's suggested 'read-only'). > Or maybe we have another idea. But I don't really see the urgency of > whacking this around right this minute. There's nothing broken here; > there's just more stuff people would like to have. It can be added next > time around. But if completeness of the functionality is below people's expectations, it may unnecessarily compromise the reputation ofPostgreSQL. Is there any chance to incorporate a patch into PG 10? May I add this as a PG 10 open item? Regards Takayuki Tsunakawa
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Craig Ringer
Дата:
On 13 April 2017 at 14:59, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > 2. Make transaction_read_only GUC_REPORT > This is to avoid the added round-trip by SHOW command. It also benefits client apps that want to know when the servergets promoted? And this may simplify the libpq code. > I don't understand yet why we need to provide this feature for older servers by using SHOW. Those who are already using<= 9.6 in production completed the system or application, and their business is running. Why would they want to justreplace libpq and use this feature? I think "transaction_read_only" is a bit confusing for something we're expecting to change under us. To me, a "read only" xact is one created with BEGIN READ ONLY TRANSACTION; .... which I would not expect to become read/write under me, since I explicitly asked for read-only. It's more like "session read only" that we're interested in IMO.
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby"GUC pseudo-variable.
От
"Tsunakawa, Takayuki"
Дата:
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Craig Ringer > On 13 April 2017 at 14:59, Tsunakawa, Takayuki > <tsunakawa.takay@jp.fujitsu.com> wrote: > > > 2. Make transaction_read_only GUC_REPORT This is to avoid the added > > round-trip by SHOW command. It also benefits client apps that want to > know when the server gets promoted? And this may simplify the libpq code. > > I don't understand yet why we need to provide this feature for older servers > by using SHOW. Those who are already using <= 9.6 in production completed > the system or application, and their business is running. Why would they > want to just replace libpq and use this feature? > > I think "transaction_read_only" is a bit confusing for something we're > expecting to change under us. > > To me, a "read only" xact is one created with > > BEGIN READ ONLY TRANSACTION; > > .... which I would not expect to become read/write under me, since I > explicitly asked for read-only. > > It's more like "session read only" that we're interested in IMO. Are you suggest thating we provide a GUC_REPORT read-only variable "session_read_only" and the libpq should use it? Anyway, I added this item in the PostgreSQL 10 Open Items page under "Design Decisions to Recheck Mid-Beta". I'm willing to submit a patch for PG10. Regards Takayuki Tsunakawa
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby"GUC pseudo-variable.
От
"Tsunakawa, Takayuki"
Дата:
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Craig Ringer > On 13 April 2017 at 14:59, Tsunakawa, Takayuki > <tsunakawa.takay@jp.fujitsu.com> wrote: > > > 2. Make transaction_read_only GUC_REPORT This is to avoid the added > > round-trip by SHOW command. It also benefits client apps that want to > know when the server gets promoted? And this may simplify the libpq code. > > I don't understand yet why we need to provide this feature for older servers > by using SHOW. Those who are already using <= 9.6 in production completed > the system or application, and their business is running. Why would they > want to just replace libpq and use this feature? > > I think "transaction_read_only" is a bit confusing for something we're > expecting to change under us. > > To me, a "read only" xact is one created with > > BEGIN READ ONLY TRANSACTION; > > .... which I would not expect to become read/write under me, since I > explicitly asked for read-only. > > It's more like "session read only" that we're interested in IMO. I confirmed that the attached patch successfully provides: * target_session_attrs=read-only * If the server is >= 10, avoid the round-trip for SHOW transaction_read_only. For this, I added a GUC_REPORT variable session_read_only which indicates the session's default read-only status. The characteristicsare: * It cannot be changed directly by the user (postgresql.conf, SET, etc.) * Its value is the same as default_transaction_read_only when not in recovery. * Its value is false during recovery. Could you include this in PG 10? I think these are necessary as the bottom line to meet the average expectation of users(please don't ask me what's the average; the main reasons are that PostgreSQL provides hot standby, PgJDBC enables connectionto the standby (targetServerType=slave), and PostgreSQL emphasizes performance.) Ideally, I wanted to add otherfeatures of PgJDBC (e.g. targetServerType=preferSlave), but I thought this is the limit not to endanger the qualityof the final release. Regards Takayuki Tsunakawa -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On Wed, May 24, 2017 at 3:16 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > I confirmed that the attached patch successfully provides: > > * target_session_attrs=read-only > * If the server is >= 10, avoid the round-trip for SHOW transaction_read_only. > > For this, I added a GUC_REPORT variable session_read_only which indicates the session's default read-only status. Thecharacteristics are: > > * It cannot be changed directly by the user (postgresql.conf, SET, etc.) > * Its value is the same as default_transaction_read_only when not in recovery. > * Its value is false during recovery. > > Could you include this in PG 10? I think these are necessary as the bottom line to meet the average expectation of users(please don't ask me what's the average; the main reasons are that PostgreSQL provides hot standby, PgJDBC enables connectionto the standby (targetServerType=slave), and PostgreSQL emphasizes performance.) Ideally, I wanted to add otherfeatures of PgJDBC (e.g. targetServerType=preferSlave), but I thought this is the limit not to endanger the qualityof the final release. I've already stated my position on this, which is that: * target_session_attrs=read-only is a perfectly good new feature, but we're past feature freeze, so it's material for v11. * I'm not opposed to adding a GUC_REPORT GUC of some kind, but I see no urgency about that either. The feature works fine as it is. The fact that it could possibly be made to work more efficiently is not a critical bug. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, May 24, 2017 at 3:16 AM, Tsunakawa, Takayuki > <tsunakawa.takay@jp.fujitsu.com> wrote: >> For this, I added a GUC_REPORT variable session_read_only which indicates the session's default read-only status. Thecharacteristics are: >> >> * It cannot be changed directly by the user (postgresql.conf, SET, etc.) >> * Its value is the same as default_transaction_read_only when not in recovery. I didn't look at exactly how you tried to do that, but GUCs whose values depend on other GUCs generally don't work well at all. >> * Its value is false during recovery. [ scratches head... ] Surely this should read as "true" during recovery? Also, what happens if the standby server goes live mid-session? >> Could you include this in PG 10? I concur with Robert that this is too late for v10, and the argument to shove it in anyway is not compelling. regards, tom lane
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby"GUC pseudo-variable.
От
"Tsunakawa, Takayuki"
Дата:
From: Robert Haas [mailto:robertmhaas@gmail.com] > I've already stated my position on this, which is that: > > * target_session_attrs=read-only is a perfectly good new feature, but we're > past feature freeze, so it's material for v11. > > * I'm not opposed to adding a GUC_REPORT GUC of some kind, but I see no > urgency about that either. The feature works fine as it is. The fact that > it could possibly be made to work more efficiently is not a critical bug. I see. I'll add this in the next CF for PG 11. I'd be glad if you could review the patch in PG 11 development. Also, I'llupdate the PostgreSQL 10 Open Items page that way. Regards Takayuki Tsunakawa
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby"GUC pseudo-variable.
От
"Tsunakawa, Takayuki"
Дата:
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane > I didn't look at exactly how you tried to do that, but GUCs whose values > depend on other GUCs generally don't work well at all. transaction_read_only and transaction_isolation depends on default_transaction_read_only and default_transaction_isolationrespectively. But I feel you are concerned about something I'm not aware of. Could you shareyour worries? I haven't found a problem so far. > >> * Its value is false during recovery. > > [ scratches head... ] Surely this should read as "true" during recovery? Ouch, you are right. > Also, what happens if the standby server goes live mid-session? The clients will know the change of session_read_only when they do something that calls RecoveryInProgress(). Currently,RecoveryInProgress() seems to be the only place where the sessions notice the promotion, so I set session_read_onlyto the value of default_transaction_read_only there. I think that there is room for discussion here. Itwould be ideal for the sessions to notice the server promotion promptly and notify the clients of the change. I have noidea to do that well. Regards Takayuki Tsunakawa
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUCpseudo-variable.
От
Michael Banck
Дата:
Hi, On Wed, May 24, 2017 at 07:16:06AM +0000, Tsunakawa, Takayuki wrote: > I confirmed that the attached patch successfully provides: I was going to take a look at this, but the patch no longer applies cleanly for me: Hunk #1 succeeded at 1474 (offset 49 lines). Hunk #2 succeeded at 1762 (offset 49 lines). Hunk #3 succeeded at 1773 (offset 49 lines). patching file doc/src/sgml/protocol.sgml patching file src/backend/access/transam/xlog.c Hunk #1 succeeded at 7909 (offset 5 lines). patching file src/backend/tcop/postgres.c Hunk #1 succeeded at 3737 (offset 15 lines). patching file src/backend/utils/misc/check_guc patching file src/backend/utils/misc/guc.c Hunk #1 succeeded at 147 with fuzz 1. Hunk #5 succeeded at 10062 (offset 11 lines). patching file src/interfaces/libpq/fe-connect.c Hunk #1 succeeded at 1178 (offset 112 lines). Hunk #2 succeeded at 2973 (offset 124 lines). Hunk #3 succeeded at 3003 (offset 124 lines). Hunk #4 succeeded at 3067 (offset 124 lines). Hunk #5 FAILED at 3022. Hunk #6 succeeded at 3375 (offset 144 lines). 1 out of 6 hunks FAILED -- saving rejects to file src/interfaces/libpq/fe-connect.c.rej patching file src/interfaces/libpq/fe-exec.c patching file src/interfaces/libpq/libpq-int.h Hunk #1 succeeded at 361 (offset 1 line). Hunk #2 succeeded at 421 (offset -1 lines). Can you rebase it, please? Best Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.banck@credativ.de credativ GmbH, HRB Mönchengladbach 12080 USt-ID-Nummer: DE204566209 Trompeterallee 108, 41189 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Wednesday, May 24, 2017 9:38:36 PM EDT Tsunakawa, Takayuki wrote: > The clients will know the change of session_read_only when they do > something that calls RecoveryInProgress(). Currently, > RecoveryInProgress() seems to be the only place where the sessions > notice the promotion, so I set session_read_only to the value of > default_transaction_read_only there. I think that there is room for > discussion here. It would be ideal for the sessions to notice the > server promotion promptly and notify the clients of the change. I > have no idea to do that well. My original patch did that via the new SendSignalToAllBackends() helper, which is called whenever the postmaster exits hot stanby. I incorporated those bits into your patch and rebased in onto master. Please see attached. FWIW, I think that mixing the standby status and the default transaction writability is suboptimal. They are related, yes, but not the same thing. It is possible to have a master cluster in the read-only mode, and with this patch it would be impossible to distinguish from a hot-standby replica without also polling pg_is_in_recovery(), which defeats the purpose of having to do no database roundtrips. Elvis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Thomas Munro
Дата:
On Wed, Sep 13, 2017 at 3:48 AM, Elvis Pranskevichus <elprans@gmail.com> wrote: > I incorporated those bits into your patch and rebased in onto master. > Please see attached. > > FWIW, I think that mixing the standby status and the default > transaction writability is suboptimal. They are related, yes, but not > the same thing. It is possible to have a master cluster in the > read-only mode, and with this patch it would be impossible to > distinguish from a hot-standby replica without also polling > pg_is_in_recovery(), which defeats the purpose of having to do no > database roundtrips. Hi Elvis, FYI the recovery test 001_stream_rep.pl fails with this patch applied. You can see that if you configure with --enable-tap-tests, build and then cd into src/test/recovery and "make check". -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Melanie Plageman
Дата:
On Tue, Sep 12, 2017 at 6:11 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
On Wed, Sep 13, 2017 at 3:48 AM, Elvis Pranskevichus <elprans@gmail.com> wrote:
> I incorporated those bits into your patch and rebased in onto master.
> Please see attached.
>
> FWIW, I think that mixing the standby status and the default
> transaction writability is suboptimal. They are related, yes, but not
> the same thing. It is possible to have a master cluster in the
> read-only mode, and with this patch it would be impossible to
> distinguish from a hot-standby replica without also polling
> pg_is_in_recovery(), which defeats the purpose of having to do no
> database roundtrips.
Hi Elvis,
FYI the recovery test 001_stream_rep.pl fails with this patch applied.
You can see that if you configure with --enable-tap-tests, build and
then cd into src/test/recovery and "make check".
The latest patch applies cleanly and builds (I am also seeing the failing TAP tests), however, I have a concern. With a single server set up, when I attempt to make a connection with target_session_attrs=read-write, I get the message
psql: could not make a suitable connection to server "localhost:5432"
Whereas, when I attempt to make a connection with target_session_attrs=read-only, it is successful.
I might be missing something, but this seems somewhat counter-intuitive. I would expect to specify read-write as target_session_attrs and successfully connect to a server on which read and write operations are permitted. I see this behavior implemented in src/interfaces/libpq/fe-connect.c
Is there a reason to reject a connection to a primary server when I specify 'read-write'? Is this intentional?
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Melanie Plageman
Дата:
On Tue, Sep 19, 2017 at 4:15 PM, Melanie Plageman <melanieplageman@gmail.com> wrote:
Hi Elvis,The latest patch applies cleanly and builds (I am also seeing the failing TAP tests), however, I have a concern. With a single server set up, when I attempt to make a connection with target_session_attrs=read-write, I get the message psql: could not make a suitable connection to server "localhost:5432"Whereas, when I attempt to make a connection with target_session_attrs=read-only, it is successful. I might be missing something, but this seems somewhat counter-intuitive. I would expect to specify read-write as target_session_attrs and successfully connect to a server on which read and write operations are permitted. I see this behavior implemented in src/interfaces/libpq/fe-connect.c Is there a reason to reject a connection to a primary server when I specify 'read-write'? Is this intentional?
Making an assumption about the intended functionality mentioned above, I swapped the 'not' to the following lines of src/interfaces/libpq/fe-connect.c ~ line 3005
if (conn->target_session_attrs != NULL &&
((strcmp(conn->target_session_attrs, "read-write") == 0 && conn->session_read_only) ||
(strcmp(conn->target_session_attrs, "read-only") == 0 && !conn->session_read_only)))
I rebased and built with this change locally.
The review below is based on the patch with that change.
Also, the following comment has what looks like a copy-paste error and the first line should be deleted
in src/backend/utils/misc/guc.c ~ line 10078
assign_default_transaction_read_only
+assign_default_transaction_read_only(bool newval, void *extra)
...
+ /*
- * We clamp manually-set values to at least 1MB. Since
+ * Also set the session read-only parameter. We only need
+ * to set the correct value in processes that have database
+ * sessions, but there's no mechanism to know that there's
patch applies cleanly: yes
installcheck: passed
installcheck-world: passed
feature works as expected: yes (details follow)
With two servers, one configured as the primary and one configured to run in Hot Standby mode, I was able to observe that the value of session_read_only changed after triggering failover once the standby server exited recovery
When attempting to connect to a primary server with target_session_attrs=read-write, I was successful and when attempting to connect with target_session_attrs=read-only, the connection was closed and the expected message was produced
Thanks!
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Daniel Gustafsson
Дата:
> On 22 Sep 2017, at 18:57, Melanie Plageman <melanieplageman@gmail.com> wrote: > > On Tue, Sep 19, 2017 at 4:15 PM, Melanie Plageman <melanieplageman@gmail.com <mailto:melanieplageman@gmail.com>> wrote: > The latest patch applies cleanly and builds (I am also seeing the failing TAP tests), however, I have a concern. With asingle server set up, when I attempt to make a connection with target_session_attrs=read-write, I get the message > psql: could not make a suitable connection to server "localhost:5432" > Whereas, when I attempt to make a connection with target_session_attrs=read-only, it is successful. > > I might be missing something, but this seems somewhat counter-intuitive. I would expect to specify read-write as target_session_attrsand successfully connect to a server on which read and write operations are permitted. I see this behaviorimplemented in src/interfaces/libpq/fe-connect.c > Is there a reason to reject a connection to a primary server when I specify 'read-write'? Is this intentional? > > Hi Elvis, > > Making an assumption about the intended functionality mentioned above, I swapped the 'not' to the following lines of src/interfaces/libpq/fe-connect.c~ line 3005 > > if (conn->target_session_attrs != NULL && > ((strcmp(conn->target_session_attrs, "read-write") == 0 && conn->session_read_only) || > (strcmp(conn->target_session_attrs, "read-only") == 0 && !conn->session_read_only))) > > I rebased and built with this change locally. > The review below is based on the patch with that change. > > Also, the following comment has what looks like a copy-paste error and the first line should be deleted > in src/backend/utils/misc/guc.c ~ line 10078 > assign_default_transaction_read_only > > > +assign_default_transaction_read_only(bool newval, void *extra) > ... > + /* > - * We clamp manually-set values to at least 1MB. Since > + * Also set the session read-only parameter. We only need > + * to set the correct value in processes that have database > + * sessions, but there's no mechanism to know that there's > > patch applies cleanly: yes > installcheck: passed > installcheck-world: passed > feature works as expected: yes (details follow) > > With two servers, one configured as the primary and one configured to run in Hot Standby mode, I was able to observe thatthe value of session_read_only changed after triggering failover once the standby server exited recovery > > When attempting to connect to a primary server with target_session_attrs=read-write, I was successful and when attemptingto connect with target_session_attrs=read-only, the connection was closed and the expected message was produced Based on the unaddressed questions raised in this thread, I’m marking this patch Returned with Feedback. Please re-submit a new version of the patch to a future commitfest. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUC pseudo-variable.
От
Michael Paquier
Дата:
On Mon, Oct 2, 2017 at 4:16 PM, Daniel Gustafsson <daniel@yesql.se> wrote: > Based on the unaddressed questions raised in this thread, I’m marking this > patch Returned with Feedback. Please re-submit a new version of the patch to a > future commitfest. For some reason this patch has been moved to CF 2017-11, but there has been no real updates since the last version in https://www.postgresql.org/message-id/flat/1572601.d6Y6hSXNBC%40hammer.magicstack.net#1572601.d6Y6hSXNBC@hammer.magicstack.net was sent. The patch fails to apply as well now. I am marking it as returned with feedback. I am not seeing any replies to the requests done as well. -- Michael
Re: [HACKERS] [PATCH v1] Add and report the new "in_hot_standby" GUCpseudo-variable.
От
Andres Freund
Дата:
On 2017-11-28 10:55:34 +0900, Michael Paquier wrote: > On Mon, Oct 2, 2017 at 4:16 PM, Daniel Gustafsson <daniel@yesql.se> wrote: > > Based on the unaddressed questions raised in this thread, I’m marking this > > patch Returned with Feedback. Please re-submit a new version of the patch to a > > future commitfest. > > For some reason this patch has been moved to CF 2017-11, but there has > been no real updates since the last version in > https://www.postgresql.org/message-id/flat/1572601.d6Y6hSXNBC%40hammer.magicstack.net#1572601.d6Y6hSXNBC@hammer.magicstack.net > was sent. The patch fails to apply as well now. I am marking it as > returned with feedback. I am not seeing any replies to the requests > done as well. A CF entry for this patch has been created yesterday, without any changes having happend since the above status update. Given that there's been no progress for multiple commitfests, and this is the last CF, this doesn't seem appropriate. Therefore marked as returned with feedback. Greetings, Andres Freund
Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Thursday, March 1, 2018 4:25:16 AM EDT Andres Freund wrote: > A CF entry for this patch has been created yesterday, without any > changes having happend since the above status update. Given that > there's been no progress for multiple commitfests, and this is the > last CF, this doesn't seem appropriate. Therefore marked as returned > with feedback. Sorry for the long silence on this. I'm attaching a rebased and cleaned-up patch with the earlier review issues addressed. I've checked all relevant tests and verified manually. Elvis
Вложения
Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only"GUC pseudo-variable.
От
Melanie Plageman
Дата:
On Mon, Sep 24, 2018 at 9:28 AM Elvis Pranskevichus <elprans@gmail.com> wrote:
On Thursday, March 1, 2018 4:25:16 AM EDT Andres Freund wrote:
> A CF entry for this patch has been created yesterday, without any
> changes having happend since the above status update. Given that
> there's been no progress for multiple commitfests, and this is the
> last CF, this doesn't seem appropriate. Therefore marked as returned
> with feedback.
Sorry for the long silence on this. I'm attaching a rebased and
cleaned-up patch with the earlier review issues addressed. I've checked
all relevant tests and verified manually.
Elvis
After taking a look at the updated patch, it appears to address the review
feedback which I provided. However, the patch didn't apply cleanly for me, so
it likely needs a quick look along with a rebase.
After looking at the patch [1] to add a "prefer-read" option to
target-session-attrs which is also up for consideration this commitfest, I was
wondering, assuming it is desirable to have both "prefer-read" and "read-only"
modes, does it make sense for the "prefer-read" option to also be implemented
with a ParameterStatus message instead of using polling?
[1] https://commitfest.postgresql.org/20/1677/
feedback which I provided. However, the patch didn't apply cleanly for me, so
it likely needs a quick look along with a rebase.
After looking at the patch [1] to add a "prefer-read" option to
target-session-attrs which is also up for consideration this commitfest, I was
wondering, assuming it is desirable to have both "prefer-read" and "read-only"
modes, does it make sense for the "prefer-read" option to also be implemented
with a ParameterStatus message instead of using polling?
[1] https://commitfest.postgresql.org/20/1677/
Melanie Plageman <melanieplageman@gmail.com> writes: > On Mon, Sep 24, 2018 at 9:28 AM Elvis Pranskevichus <elprans@gmail.com> > wrote: >> Sorry for the long silence on this. I'm attaching a rebased and >> cleaned-up patch with the earlier review issues addressed. I've checked >> all relevant tests and verified manually. > After taking a look at the updated patch, it appears to address the review > feedback which I provided. However, the patch didn't apply cleanly for me, > so it likely needs a quick look along with a rebase. It looks like the one apply failure is due to the patch having corrected a comment that somebody else already corrected. So no big deal there. Looking through the thread, it seems like there's a pretty fundamental design issue that hasn't been resolved, namely whether and how this ought to interact with default_transaction_read_only. The patch as it stands seems to be trying to implement the definition that the transmittable variable session_read_only is equal to "!(DefaultXactReadOnly || RecoveryInProgress())". I doubt that that's a good idea. In the first place, it's not at all clear that such a flag is sufficient for all use-cases. In the second, it's somewhere between difficult and impossible to correctly implement GUCs that are interdependent in that way; the current patch certainly fails to do so. (It will not correctly track intra-session changes to DefaultXactReadOnly, for instance.) I think that we'd be better off maintaining a strict separation between "in hot standby" and default_transaction_read_only. If there are use cases in which people would like to reject connections based on either one being true, let's handle that by marking them both GUC_REPORT, and having libpq check both. (So there need to be two flavors of target-session-attrs libpq parameter, depending on whether you want "in hot standby" or "currently read only" to be checked.) That puts us back to having to choose an appropriate GUC name, because "session_read_only" isn't it :-(. Don't have any better ideas about that than what's been discussed in the thread. I'm also not terribly happy with the patch adding a whole new cross-backend signaling mechanism for this; surely we don't really need that. Perhaps it'd be sufficient to transmit SIGHUP and embed the check for "just exited hot standby" in the handling for that? regards, tom lane
Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only" GUC pseudo-variable.
От
Elvis Pranskevichus
Дата:
On Monday, November 12, 2018 1:08:37 PM EST Tom Lane wrote: > Looking through the thread, it seems like there's a pretty fundamental > design issue that hasn't been resolved, namely whether and how this > ought to interact with default_transaction_read_only. The patch as > it stands seems to be trying to implement the definition that the > transmittable variable session_read_only is equal to > "!(DefaultXactReadOnly || RecoveryInProgress())". I doubt that > that's a good idea. In the first place, it's not at all clear that > such a flag is sufficient for all use-cases. In the second, it's > somewhere between difficult and impossible to correctly implement > GUCs that are interdependent in that way; the current patch certainly > fails to do so. (It will not correctly track intra-session changes > to DefaultXactReadOnly, for instance.) > > I think that we'd be better off maintaining a strict separation > between "in hot standby" and default_transaction_read_only. If there > are use cases in which people would like to reject connections based > on either one being true, let's handle that by marking them both > GUC_REPORT, and having libpq check both. (So there need to be two > flavors of target-session-attrs libpq parameter, depending on whether > you want "in hot standby" or "currently read only" to be checked.) I agree that the original design with the separate "in_hot_standby" GUC is better. Hot standby and read-only session are distinct modes, not all commands that are OK in a read-only session will succeed on a hot- standby backend. > I'm also not terribly happy with the patch adding a whole new > cross-backend signaling mechanism for this; surely we don't really > need that. Perhaps it'd be sufficient to transmit SIGHUP and embed > the check for "just exited hot standby" in the handling for that? That seems doable. Is there an existing check that is a good candidate for "just exited hot standby"? Elvis
Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only"GUC pseudo-variable.
От
Dmitry Dolgov
Дата:
> On Mon, Nov 12, 2018 at 8:30 PM Elvis Pranskevichus <elprans@gmail.com> wrote: > > On Monday, November 12, 2018 1:08:37 PM EST Tom Lane wrote: > > Looking through the thread, it seems like there's a pretty fundamental > > design issue that hasn't been resolved, namely whether and how this > > ought to interact with default_transaction_read_only. The patch as > > it stands seems to be trying to implement the definition that the > > transmittable variable session_read_only is equal to > > "!(DefaultXactReadOnly || RecoveryInProgress())". I doubt that > > that's a good idea. In the first place, it's not at all clear that > > such a flag is sufficient for all use-cases. In the second, it's > > somewhere between difficult and impossible to correctly implement > > GUCs that are interdependent in that way; the current patch certainly > > fails to do so. (It will not correctly track intra-session changes > > to DefaultXactReadOnly, for instance.) > > > > I think that we'd be better off maintaining a strict separation > > between "in hot standby" and default_transaction_read_only. If there > > are use cases in which people would like to reject connections based > > on either one being true, let's handle that by marking them both > > GUC_REPORT, and having libpq check both. (So there need to be two > > flavors of target-session-attrs libpq parameter, depending on whether > > you want "in hot standby" or "currently read only" to be checked.) > > I agree that the original design with the separate "in_hot_standby" GUC > is better. Hot standby and read-only session are distinct modes, not > all commands that are OK in a read-only session will succeed on a hot- > standby backend. > > > I'm also not terribly happy with the patch adding a whole new > > cross-backend signaling mechanism for this; surely we don't really > > need that. Perhaps it'd be sufficient to transmit SIGHUP and embed > > the check for "just exited hot standby" in the handling for that? > > That seems doable. Is there an existing check that is a good candidate > for "just exited hot standby"? Apparently due the minor conflict, mentioned above, the patch was in "Waiting on author" state, which probably is not exactly correct. I'm moving it to the next CF, but still, it would be nice if you post an updated version without conflicts.
Re: [HACKERS] [PATCH v2] Add and report the new "session_read_only"GUC pseudo-variable.
От
Michael Paquier
Дата:
On Fri, Nov 30, 2018 at 05:35:21PM +0100, Dmitry Dolgov wrote: > Apparently due the minor conflict, mentioned above, the patch was in "Waiting > on author" state, which probably is not exactly correct. I'm moving it to the > next CF, but still, it would be nice if you post an updated version without > conflicts. And nothing has happened since, so I have marked the entry as returned with feedback. -- Michael