let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|

let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
On Tue, Jan 9, 2018 at 3:36 PM, Peter Eisentraut
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Simon Riggs
On 10 January 2018 at 16:08, Robert Haas <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Tom Lane-2
In reply to this post by Robert Haas
Robert Haas <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Joshua D. Drake
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.   *****


Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Tom Lane-2
"Joshua D. Drake" <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Joshua D. Drake
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.   *****


Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Chapman Flack
In reply to this post by Robert Haas
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
https://www.postgresql.org/message-id/11465.1339163239%40sss.pgh.pa.us,
in a thread about making Connection.setReadOnly() work right in PGJDBC.

-Chap

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
In reply to this post by Tom Lane-2
On Wed, Jan 10, 2018 at 12:36 PM, Tom Lane <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Tom Lane-2
Robert Haas <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Chapman Flack
In reply to this post by Robert Haas
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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
In reply to this post by Tom Lane-2
On Wed, Jan 10, 2018 at 3:22 PM, Tom Lane <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
In reply to this post by Chapman Flack
On Wed, Jan 10, 2018 at 3:32 PM, Chapman Flack <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Tom Lane-2
In reply to this post by Robert Haas
Robert Haas <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
On Wed, Jan 10, 2018 at 3:55 PM, Tom Lane <[hidden email]> wrote:

> Robert Haas <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Craig Ringer-3
In reply to this post by Tom Lane-2
On 11 January 2018 at 09:55, Tom Lane <[hidden email]> wrote:
Robert Haas <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
On Sun, Jan 21, 2018 at 5:41 PM, Craig Ringer <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Dave Cramer-8


On Mon, 22 Jan 2018 at 07:39, Robert Haas <[hidden email]> wrote:
On Sun, Jan 21, 2018 at 5:41 PM, Craig Ringer <[hidden email]> 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 ?
Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
On Tue, Jul 9, 2019 at 2:32 PM Dave Cramer <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Dave Cramer-8


On Wed, 10 Jul 2019 at 09:11, Robert Haas <[hidden email]> wrote:
On Tue, Jul 9, 2019 at 2:32 PM Dave Cramer <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: let's make the list of reportable GUCs configurable (was Re: Add %r substitution for psql prompts to show recovery status)

Robert Haas
On Wed, Jul 10, 2019 at 9:59 AM Dave Cramer <[hidden email]> 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


123