let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)
On Tue, Jan 9, 2018 at 3:36 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
I agree a backend status message is the right way to do this.
We could perhaps report transaction_read_only, if we don't want to add a
new one.
That's not really the same thing, though.
I think that we really need to think about allowing clients to tell
the server which GUCs they'd like reported, instead of having a single
list to which everyone is bound. A new protocol option using the
facilities added in commit ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed,
like _pq_.report=work_mem might be one way, though that doesn't give a
psql session the option of changing its mind in mid-session. If we
wanted to allow that, we could teach the server to understand a new
message to change protocol options and let psql send it to
sufficiently-new servers.
Way back in the day, we used to reject every proposed new EXPLAIN
option because we didn't want to add keywords for all of those things;
nowadays, we reject every new GUC_REPORT GUC because it adds overhead
for everyone. The solution there was to change the rules of the game
so that EXPLAIN options didn't have to be keywords, and I think the
solution here is to change the game so it doesn't add overhead for
everyone.
We might also want to consider de-GUC-reportify some things that are
GUC_REPORT now. DateStyle, IntervalStyle, TimeZone, application_name
aren't used by anything in core. Apparently we have application_name
as GUC_REPORT because pgbouncer wants it
(59ed94ad0c9f74a3f057f359316c845cedc4461e, 2009), TimeZone because
JDBC wants it (b5ae0d69da8f83e400921fcdd171e5bdadb45db3, 2004),
IntervalStyle I suppose because it was copied from DateStyle
(df7641e25ab4da6f3a48222cbda0e121ccb32ad5, 2008) and DateStyle from
the very beginning for unspecified reasons
(b700a672feadbb6f122b7c7249967fb0f58dda2b, 2003). If clients can
request the GUCs they care about, it becomes much less important for
the default list to be comprehensive.
As a side benefit, then Craig and Tom can stop arguing about whether
server_version_num should be GUC_REPORT. Under this proposal, you can
have it whichever way you like.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 10 January 2018 at 16:08, Robert Haas <robertmhaas@gmail.com> wrote:
I think that we really need to think about allowing clients to tell
the server which GUCs they'd like reported, instead of having a single
list to which everyone is bound.
+1
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Robert Haas <robertmhaas@gmail.com> writes:
I think that we really need to think about allowing clients to tell
the server which GUCs they'd like reported, instead of having a single
list to which everyone is bound.
Interesting idea ...
As a side benefit, then Craig and Tom can stop arguing about whether
server_version_num should be GUC_REPORT. Under this proposal, you can
have it whichever way you like.
... but I don't think it fixes that, because you couldn't send this new
request without making an assumption about the server version being
new enough to support it. My entire beef with making server_version_num
be GUC_REPORT is that it would encourage people to write client code that
fails outright against older servers. I'm afraid what you are suggesting
will be an equally attractive nuisance.
regards, tom lane
On 01/10/2018 09:22 AM, Tom Lane wrote:
... but I don't think it fixes that, because you couldn't send this new
request without making an assumption about the server version being
new enough to support it. My entire beef with making server_version_num
be GUC_REPORT is that it would encourage people to write client code that
fails outright against older servers. I'm afraid what you are suggesting
will be an equally attractive nuisance.
It seems to me that is not our problem. Why do we care if some developer
says, "I only work with 9.6"? If I am understanding your complaint.
JD
regards, tom lane
--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
***** Unless otherwise stated, opinions are my own. *****
"Joshua D. Drake" <jd@commandprompt.com> writes:
On 01/10/2018 09:22 AM, Tom Lane wrote:
... but I don't think it fixes that, because you couldn't send this new
request without making an assumption about the server version being
new enough to support it. My entire beef with making server_version_num
be GUC_REPORT is that it would encourage people to write client code that
fails outright against older servers. I'm afraid what you are suggesting
will be an equally attractive nuisance.
It seems to me that is not our problem. Why do we care if some developer
says, "I only work with 9.6"? If I am understanding your complaint.
I don't care at all if J. Random Developer's homegrown code only works
with the PG version he's using. The concern I have is that unwanted
server version dependencies will sneak into widely used code, like
psql, or libpq, or jdbc. Or another way of putting it: Robert's proposal
is a protocol version break, just like most stuff at this level. Trying
to pretend it isn't doesn't make it not one.
regards, tom lane
On 01/10/2018 09:36 AM, Tom Lane wrote:
It seems to me that is not our problem. Why do we care if some developer
says, "I only work with 9.6"? If I am understanding your complaint.
I don't care at all if J. Random Developer's homegrown code only works
with the PG version he's using. The concern I have is that unwanted
server version dependencies will sneak into widely used code, like
psql, or libpq, or jdbc. Or another way of putting it: Robert's proposal
is a protocol version break, just like most stuff at this level. Trying
to pretend it isn't doesn't make it not one.
That makes sense, thanks for clarifying.
JD
regards, tom lane
--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
***** Unless otherwise stated, opinions are my own. *****
On 01/10/2018 11:08 AM, Robert Haas wrote:
I think that we really need to think about allowing clients to tell
the server which GUCs they'd like reported, instead of having a single
list to which everyone is bound.
+1
That already sounded like a good idea back in
/messages/by-id/11465.1339163239@sss.pgh.pa.us,
in a thread about making Connection.setReadOnly() work right in PGJDBC.
-Chap
On Wed, Jan 10, 2018 at 12:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I don't care at all if J. Random Developer's homegrown code only works
with the PG version he's using. The concern I have is that unwanted
server version dependencies will sneak into widely used code, like
psql, or libpq, or jdbc. Or another way of putting it: Robert's proposal
is a protocol version break, just like most stuff at this level. Trying
to pretend it isn't doesn't make it not one.
Your argument here sounds suspiciously like "If we add a new feature
and people use it in a stupid way then it may cause their stuff not to
work".
Everything that worked before adding an option like _pq_.report
continues to work afterward. Granted, if they try to use the option,
it will only work on versions that support that option, but that is
true of any new feature. Furthermore, they will easily be able to
tell based on the reported server version whether or not their request
for different behavior was accepted by the server. Therefore, if they
write their code well, there should be no danger of a client thinking
that they are getting behavior A while actually getting behavior B.
If we suppose that they write their code poorly, then there could well
be a problem, but drivers that contain bad code are fairly serious
problem with or without this feature. It's not like checking the
server version to see whether it's new enough to know about
_pq_.report is rocket science.
I agree that my follow-on proposal of dropping GUC_REPORT for some
GUCs IS a protocol version break, and that may be a good reason to
reject that part of the proposal, or postpone it until we bump to 3.1
for some other reason. Note that commit
ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed contemplates a specific
scheme for making servers that speak 3.1 and later backward-compatible
with existing clients that want 3.0, so any client that requests 3.1
can be assumed to be comfortable with all 3.1 behaviors. Of course,
it's not like we've always been similarly careful in the past: you
yourself committed patches adding GUC_REPORT reporting to various GUCs
over the years, and if you worried about what impact that would have
on naive clients, the commit log does not record it, and we certainly
didn't bump the protocol version to account for it.
If you want an example of a much more serious protocol version break,
you don't have to look any further than the major features section of
the v10 release notes. With SCRAM, we don't have to fall back on weak
arguments like "driver authors might use it wrong!" to see how things
get broken. They're just wildly broken, and we don't care, because
SCRAM is a good feature. v10 also broke the replication sub-protocol
both by adding facilities for logical replication (which is arguably a
backward-compatible change) and by changing the format of hot standby
feedback messages (which isn't, unless we think clients ought to
ignore differences in the message length and just look at the initial
bytes, but I'm not sure we're particularly careful about extending
replication messages only at the end, so that sounds like a risky
strategy).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
Your argument here sounds suspiciously like "If we add a new feature
and people use it in a stupid way then it may cause their stuff not to
work".
I think you're attacking a straw man ...
Everything that worked before adding an option like _pq_.report
continues to work afterward. Granted, if they try to use the option,
it will only work on versions that support that option, but that is
true of any new feature. Furthermore, they will easily be able to
tell based on the reported server version whether or not their request
for different behavior was accepted by the server.
My point is specifically that that reasoning fails for features that you
might try to use to determine what the server version is, or that you
might try to use before finding out what the server version is. For
example somebody might get cute and put an attempt to set _pq_.report
into their connection request packet. It'll work fine as long as they
don't test against old servers.
Yes, you can code correctly if you recognize that the hazard exists,
I'm just worried about people failing to recognize that. We don't
have any infrastructure for systematic testing of newer client code
against older server code, so it's something that bears worrying IMO.
So mostly what I'm objecting to is your claim that applying this feature
to server_version_num will do anything useful. Leaving that aside,
it might well be a worthwhile idea.
regards, tom lane
On 01/10/2018 03:11 PM, Robert Haas wrote:
it will only work on versions that support that option, but that is
true of any new feature. Furthermore, they will easily be able to
tell based on the reported server version whether or not their request
for different behavior was accepted by the server. Therefore, if they
write their code well, there should be no danger of a client thinking
that they are getting behavior A while actually getting behavior B.
SSL certificates support a notion of 'extension' where a certificate
can include beyond-the-standard doodads that the party on the other
end might or might not understand, and they can be marked either
'critical' ("please refuse my connection if you don't understand
this one") or not ("we'll muddle along if you don't understand
that one").
Is there a notion like that in the pq protocol now? If not, and
a protocol bump becomes necessary to meet some need, would it be
worth adding such a notion at the same time, to simplify future
evolution?
-Chap
On Wed, Jan 10, 2018 at 3:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
My point is specifically that that reasoning fails for features that you
might try to use to determine what the server version is, or that you
might try to use before finding out what the server version is.
OK, I didn't understand that your objection was that narrow.
For
example somebody might get cute and put an attempt to set _pq_.report
into their connection request packet. It'll work fine as long as they
don't test against old servers.
Even though I've referred to commit
ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed twice in this email thread so
far, this comment makes it look as though you haven't read it, or even
the commit message. The startup packet would be the only place you
*could* put it.
Yes, you can code correctly if you recognize that the hazard exists,
I'm just worried about people failing to recognize that. We don't
have any infrastructure for systematic testing of newer client code
against older server code, so it's something that bears worrying IMO.So mostly what I'm objecting to is your claim that applying this feature
to server_version_num will do anything useful. Leaving that aside,
it might well be a worthwhile idea.
Well, the reason Craig wants to server_version_num to be GUC_REPORT is
that it's simpler to parse than server_version, and therefore less
error-prone. I discovered today that Craig Sabino Mullane had it as
GUC_REPORT in the 2006 patch that originally added it. That was
commit 04912899e792094ed00766b99b6c604cadf9edf7, which articulated the
parsing-is-simpler justification explicitly. You forced the removal
of GUC_REPORT back then, too (commit
5086dfceba79ecd5d1eb28b8f4ed5221838ff3a6).
But if we add this feature and somebody wants to use it for
server_version_num, it's really pretty simple. In the startup packet,
you say _pq_.report=server_version_num. Then, you call
PQparameterStatus(conn, "server_version_num"). If you don't get a
value, you try calling PQparameterStatus(conn, "server_version") and
extracting the second word. If that still doesn't work then you give
up. That doesn't seem either useless or difficult to implement
correctly from here.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Jan 10, 2018 at 3:32 PM, Chapman Flack <chap@anastigmatix.net> wrote:
Is there a notion like that in the pq protocol now? If not, and
a protocol bump becomes necessary to meet some need, would it be
worth adding such a notion at the same time, to simplify future
evolution?
For the fourth time in this thread,
https://git.postgresql.org/pg/commitdiff/ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
But if we add this feature and somebody wants to use it for
server_version_num, it's really pretty simple. In the startup packet,
you say _pq_.report=server_version_num. Then, you call
PQparameterStatus(conn, "server_version_num"). If you don't get a
value, you try calling PQparameterStatus(conn, "server_version") and
extracting the second word. If that still doesn't work then you give
up. That doesn't seem either useless or difficult to implement
correctly from here.
Yeah, but what's the point? If yuou have to maintain the server_version
parsing code anyway, you're not saving any complexity with this. You're
just creating a code backwater that you probably won't test adequately.
regards, tom lane
On Wed, Jan 10, 2018 at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
But if we add this feature and somebody wants to use it for
server_version_num, it's really pretty simple. In the startup packet,
you say _pq_.report=server_version_num. Then, you call
PQparameterStatus(conn, "server_version_num"). If you don't get a
value, you try calling PQparameterStatus(conn, "server_version") and
extracting the second word. If that still doesn't work then you give
up. That doesn't seem either useless or difficult to implement
correctly from here.Yeah, but what's the point? If yuou have to maintain the server_version
parsing code anyway, you're not saving any complexity with this. You're
just creating a code backwater that you probably won't test adequately.
Well, you obviously don't buy the idea that parsing server_version_num
might be more reliable than parsing server_version. If you did buy
that idea, you might want to use the more-reliable technique when
possible and fall back otherwise, but I think you've made up your mind
about this. Anyway, a proposal like this gets us out of the business
of legislating what Everyone Must Do, which I think can only be a
plus.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 11 January 2018 at 09:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
But if we add this feature and somebody wants to use it for
server_version_num, it's really pretty simple. In the startup packet,
you say _pq_.report=server_version_num. Then, you call
PQparameterStatus(conn, "server_version_num"). If you don't get a
value, you try calling PQparameterStatus(conn, "server_version") and
extracting the second word. If that still doesn't work then you give
up. That doesn't seem either useless or difficult to implement
correctly from here.Yeah, but what's the point? If yuou have to maintain the server_version
parsing code anyway, you're not saving any complexity with this. You're
just creating a code backwater that you probably won't test adequately.
If we'd done server_version_num in 9.5, for example, less stuff would've
broken with pg10.
I just don't get it. The argument you use can be applied to almost any
change, to say "why bother making change <x> because people can't rely on
it for <y> years, so it's pointless to have it at all". Why add protocol v3?
PostgreSQL is a stable, long term project, and I'd like to plan for the
future. I also think people are way more likely to handle things like
--with-extra-version correctly when dealing with server_version_num, where
I don't much *care* if code parsing old server versions breaks.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Sun, Jan 21, 2018 at 5:41 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
If we'd done server_version_num in 9.5, for example, less stuff would've
broken with pg10.
Yeah, and if Tom hadn't forced it to be reverted from *8.2*, then
every version anyone still cares about would now have support for it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Mon, 22 Jan 2018 at 07:39, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jan 21, 2018 at 5:41 PM, Craig Ringer <craig@2ndquadrant.com>
wrote:If we'd done server_version_num in 9.5, for example, less stuff would've
broken with pg10.Yeah, and if Tom hadn't forced it to be reverted from *8.2*, then
every version anyone still cares about would now have support for it.--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
So did this die from lack of interest?
I have proposed in another thread adding more GUC REPORT variables, but I
see this as a much better way.
I'm willing to code the patch if we can get some buy in here ?
Dave Cramer
davec@postgresintl.com
www.postgresintl.com
On Tue, Jul 9, 2019 at 2:32 PM Dave Cramer <pg@fastcrypt.com> wrote:
So did this die from lack of interest?
I have proposed in another thread adding more GUC REPORT variables, but I see this as a much better way.
I'm willing to code the patch if we can get some buy in here ?
It seemed like most people at least didn't hate the idea completely,
and some liked it, so I think it would be worth revisiting. If you
decide to write a patch, I'll try to help review.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, 10 Jul 2019 at 09:11, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 9, 2019 at 2:32 PM Dave Cramer <pg@fastcrypt.com> wrote:
So did this die from lack of interest?
I have proposed in another thread adding more GUC REPORT variables, but
I see this as a much better way.
I'm willing to code the patch if we can get some buy in here ?
It seemed like most people at least didn't hate the idea completely,
and some liked it, so I think it would be worth revisiting. If you
decide to write a patch, I'll try to help review.
Awesome! I've already started working on the patch.
I'm still a bit conflicted about what to do with search_path as I do
believe this is potentially a security issue.
It may be that we always want to report that and possibly back patch it.
Dave
On Wed, Jul 10, 2019 at 9:59 AM Dave Cramer <pg@fastcrypt.com> wrote:
I'm still a bit conflicted about what to do with search_path as I do believe this is potentially a security issue.
It may be that we always want to report that and possibly back patch it.
I don't see that as a feasible option unless we make the logic that
does the reporting smarter. If it changes transiently inside of a
security-definer function, and then changes back, my recollection is
that right now we would report both changes. I think that could cause
a serious efficiency problem if you are calling such a function in a
loop.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
