Thread: let's make the list of reportable GUCs configurable (was Re: Add %rsubstitution 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
https://www.postgresql.org/message-id/11465.1339163239%40sss.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 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 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, 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

Robert Haas <robertmhaas@gmail.com> writes:
> 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.

And, even more to the point, what's the client side going to do with
the information?  If there was a security problem inside the
security-definer function, it's too late.  And the client can't do
much about it anyway.

If we have a configurable GUC_REPORT list, and somebody thinks it's useful
to them to report search_path, I don't wish to stand in their way.
But the argument that this is useful is so tissue-thin that we have no
business imposing the overhead on everybody, much less back-patching it.

            regards, tom lane

On Thu, Jul 11, 2019 at 8:23 AM Dave Cramer <pg@fastcrypt.com> wrote:
> See attached for an initial patch. If this is an acceptable way to go I will add tests and documentation

And clean up the code?  Doesn't look crazy on a quick glance but I
think I see at least half a dozen coding style problems.  More
substantively:

1. I don't really like putting 'guc' into an externally visible name;
that's why I suggested 'report'.

2. I haven't really scrutinized whether what SetConfigReport is an OK
way of implementing this.  That probably needs some study.  It may be
fine.

3. I'm not sure that just ignoring any GUCs we don't find is the right
thing.  I'm also not sure that it's the wrong thing, but it might be.
My question is: what if there's an extension-owned GUC in play? The
library probably isn't even loaded at this stage, unless it's in
shared_preload_libraries.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> 3. I'm not sure that just ignoring any GUCs we don't find is the right
> thing.  I'm also not sure that it's the wrong thing, but it might be.
> My question is: what if there's an extension-owned GUC in play? The
> library probably isn't even loaded at this stage, unless it's in
> shared_preload_libraries.

Gut reaction is that define_custom_variable would need to consult
the list to see if a newly-defined variable should be marked GUC_REPORT.

Therefore, at least for qualified GUC names, we can't issue an error
for unrecognized names.  But maybe it should complain about unrecognized
unqualified names.

            regards, tom lane

On Thu, Jul 11, 2019 at 10:11 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > 3. I'm not sure that just ignoring any GUCs we don't find is the right
> > thing.  I'm also not sure that it's the wrong thing, but it might be.
> > My question is: what if there's an extension-owned GUC in play? The
> > library probably isn't even loaded at this stage, unless it's in
> > shared_preload_libraries.
>
> Gut reaction is that define_custom_variable would need to consult
> the list to see if a newly-defined variable should be marked GUC_REPORT.

Yeah, that seems like a good idea.

> Therefore, at least for qualified GUC names, we can't issue an error
> for unrecognized names.  But maybe it should complain about unrecognized
> unqualified names.

I had the same thought, but I just realized that's probably
unfriendly: at the point when the client is assembling the list of
names to send to the server, it doesn't know the server version.  I
think we're probably best off assuming that any names we don't
recognize are something that got added in a newer server version and
just ignoring them. The client is not powerless to sort this out
after-the-fact: once the connection is made, they'll know the server
version and also have the option to interrogate pg_settings if they
wish.

We also need to think about how to write a test for this patch...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> I had the same thought, but I just realized that's probably
> unfriendly: at the point when the client is assembling the list of
> names to send to the server, it doesn't know the server version.  I
> think we're probably best off assuming that any names we don't
> recognize are something that got added in a newer server version and
> just ignoring them. The client is not powerless to sort this out
> after-the-fact: once the connection is made, they'll know the server
> version and also have the option to interrogate pg_settings if they
> wish.

> We also need to think about how to write a test for this patch...

All of the above is based on the assumption that this isn't a plain
old USERSET GUC, which I'm not really seeing the argument for.
OK, there might be *implementation* reasons why we would rather not
deal with on-the-fly changes to the list, but there's no advantage
to users in it.

            regards, tom lane

On Thu, Jul 11, 2019 at 10:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> All of the above is based on the assumption that this isn't a plain
> old USERSET GUC, which I'm not really seeing the argument for.
> OK, there might be *implementation* reasons why we would rather not
> deal with on-the-fly changes to the list, but there's no advantage
> to users in it.

It's pretty important that this value remain fixed for the lifetime of
a session.  If it can be changed during the session via SQL, or via
SET commands attached to a function for example, then driver authors
can't count on it to really be set to the value that they requested.

Also, think about something like a connection pooler. The user might
request one list of GUCs from the driver, and the driver might request
a different (probably larger) from the connection pooler, and the
connection pooler might then request a different (and presumably still
larger) list of GUCs from the server. If the end-user can shoot a SET
command through and change the setting on the server, the connection
pooler and the driver will break, because they were presumably
counting on getting the reports for which they asked (and forwarding
the ones someone else asked).

We have steadfastly refused to provide protocol-level tools for things
like "please change my user ID, and don't let anyone change it again
via SQL," and that's a huge problem for things like connection poolers
which can't parse all the SQL flowing through the connection (because
figuring out what it does requires solving the Halting Problem) and
wouldn't want to if they could for performance reasons. I think that's
a huge mistake. The FEBE protocol endpoint is not the application, but
some intermediate piece of software that needs control over its own
destiny. Even when there is no pooling involved, the FEBE endpoint is
the driver, not the application. That distinction is subtle, but it is
real, and it matters here too.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 11, 2019 at 2:29 PM Dave Cramer <pg@fastcrypt.com> wrote:
> So if I understand this correctly if user bob has altered his search path and there is a security-definer function
calledowned by him then
 
> the search path will be changed for the duration of the function and reported for every iteration? The implications
ofthis are "interesting" to say the least.
 

I don't believe that it matters what search path has been set using
ALTER USER bob.  But it does matter whether there is a SET function
attached to the function definition.  If you're not familiar with
this, reread the CREATE FUNCTION docs... it's a cool feature.  And of
course the function could also have an explicit SET inside of it, or
several of them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jul 11, 2019 at 3:16 PM Dave Cramer <pg@fastcrypt.com> wrote:
> On Thu, 11 Jul 2019 at 15:07, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Jul 11, 2019 at 2:29 PM Dave Cramer <pg@fastcrypt.com> wrote:
>> > So if I understand this correctly if user bob has altered his search path and there is a security-definer function
calledowned by him then
 
>> > the search path will be changed for the duration of the function and reported for every iteration? The
implicationsof this are "interesting" to say the least.
 
>>
>> I don't believe that it matters what search path has been set using
>> ALTER USER bob.
>
> Why wouldn't it ???

Because the search_path used to execute a security definer function
has nothing to do with the search_path that would be applied to a new
session created by bob.

Unless I am confused.  But you can easily test this.  Just make a
security-definer function that prints its search_path and experiment.
I think what's going to happen is that it'll inherit the search_path
from the surrounding session unless you use ALTER FUNCTION .. SET, and
that ALTER USER will make no difference.  Sometimes I'm wrong, though.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2019-10-08 23:57:24 +0800, Craig Ringer wrote:
> In other places I've (ab)used GUC_REPORT to push information back to the
> client as a workaround for the lack of protocol extensibility / custom
> messages. It's a little less ugly than abusing NOTICE messages. I'd prefer
> a real way to add optional/extension messages, but in the absence of that
> extension-defined GUCs may have good reasons to want to report multiple
> changes within a single statement/function/etc.

FWIW, custom messages strike me as a terrible idea leading to a lot of
poorly thought ought extensions that various drivers have to implement,
without there being an authoritative source of what needs to be
implemented from a practical point of view.


> BTW, a good argument for the client wanting to be notified of search_path
> changes might be for clients that want to cache name=>oid mappings for
> types, relations, etc. The JDBC driver would be able to benefit from that
> if it could trust that the same name would map to the same type (for a
> top-level statement) in future, but currently it can't.

Unless schema qualified you can't rely on that even without search_path
changing. Consider an object in schema b existing, with a search_path of
a,b. Even without the search path changing, somebody could create a type
in a, "masking" the one in b.  I'm somewhat convinced that search_path
has no role in this type of caching, unless you want to do it
wrong. There's a separate issue of needing cache invalidation for such
caches to cover some edge cases, but that's unrelated to search_path
imo.

Greetings,

Andres Freund

Hi,

On 2019-10-09 16:29:07 -0400, Dave Cramer wrote:
> I've added functionality into libpq to be able to set this STARTUP
> parameter as well as changed it to _pq_.report.
> Still need to document this and figure out how to test it.


> From 85de9f48f80a3bfd9e8bdd4f1ba6b177b1ff9749 Mon Sep 17 00:00:00 2001
> From: Dave Cramer <davecramer@gmail.com>
> Date: Thu, 11 Jul 2019 08:20:14 -0400
> Subject: [PATCH] Add a STARTUP packet option to set GUC_REPORT on GUC's that
>  currently do not have that option set. There is a facility to add protocol
>  options using _pq_.<newoption> The new option name is report and takes a
>  comma delmited string of GUC names which will have GUC_REPORT set. Add
>  functionality into libpq to accept this new option key

I don't think it's good to only be able to change this at connection
time. Especially for poolers this ought to be configurable at any
time. I do think startup message support makes sense (especially to
avoid race conditions around to-be-reported gucs changing soon after
connecting), don't get me wrong, I just don't think it's sufficient.

> @@ -2094,6 +2094,7 @@ retry1:
>           * zeroing extra byte above.
>           */
>          port->guc_options = NIL;
> +        port->guc_report = NIL;
>  
>          while (offset < len)
>          {
> @@ -2138,13 +2139,34 @@ retry1:
>              }
>              else if (strncmp(nameptr, "_pq_.", 5) == 0)
>              {
> -                /*
> -                 * Any option beginning with _pq_. is reserved for use as a
> -                 * protocol-level option, but at present no such options are
> -                 * defined.
> -                 */
> -                unrecognized_protocol_options =
> -                    lappend(unrecognized_protocol_options, pstrdup(nameptr));
> +                if (strncasecmp(nameptr + 5, "report", 6) == 0)
> +                {
> +                    char sep[3] = " ,";
> +
> +                    /* avoid scribbling on valptr */
> +                    char *temp_val = pstrdup(valptr);
> +
> +                    /* strtok is going to scribble on temp_val */
> +                    char *freeptr = temp_val;
> +                    char *guc_report = strtok(temp_val, sep);
> +                    while (guc_report)
> +                    {
> +                        port->guc_report = lappend(port->guc_report,
> +                                                   pstrdup(guc_report));
> +                        guc_report = strtok(NULL, sep);
> +                    }
> +                    pfree(freeptr);
> +                }

I don't think it's good to open-code this inside
ProcessStartupPacket(). Should be moved into its own function. I'd
probably even move all of the option handling out of
ProcessStartupPacket() before expanding it further.

I don't like the use of strtok, nor the type of parsing done
here. Perhaps we could just use SplitGUCList()?


> +                else
> +                {
> +                    /*
> +                     * Any option beginning with _pq_. is reserved for use as a
> +                     * protocol-level option, but at present no such options are
> +                     * defined.
> +                     */
> +                    unrecognized_protocol_options =
> +                            lappend(unrecognized_protocol_options, pstrdup(nameptr));
> +                }
>              }

You can't just move a comment explaining what _pq_ is into the else,
especially not without adjusting the contents.





> +/*
> + * Set the option to be GUC_REPORT
> + */
> +
> +bool
> +SetConfigReport(const char *name, bool missing_ok)
> +{
> +    struct config_generic *record;
>  
> +    record = find_option(name, false, WARNING);
> +    if (record == NULL)
> +    {
> +        if (missing_ok)
> +            return 0;
> +        ereport(ERROR,
> +                (errcode(ERRCODE_UNDEFINED_OBJECT),
> +                 errmsg("unrecognized configuration parameter \"%s\"",
> +                        name)));
> +    }
> +    record->flags |= GUC_REPORT;
> +
> +    return 0;
> +}

This way we loose track which gucs originally were marked as REPORT,
that strikes me as bad. We'd imo want to be able to debug this by
querying pg_settings.


Greetings,

Andres Freund

On Fri, Oct 11, 2019 at 8:21 AM Dave Cramer <pg@fastcrypt.com> wrote:
> So off the top of my head providing a system function seems like the way to go here unless you were contemplating
addingsomething to the protocol ?
 

Since the list of reportable GUCs is for the benefit of the driver,
I'm not sure why this would need to be changed after the connection
has been established.

But, if it does need to be changed, it seems like a terrible idea to
allow it to be done via SQL. Otherwise, the user can break the driver
by using SQL to set the list to something that the driver's not
expecting, and there's nothing the driver can do to prevent it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/11/19 4:30 PM, Robert Haas wrote:

> But, if it does need to be changed, it seems like a terrible idea to
> allow it to be done via SQL. Otherwise, the user can break the driver
> by using SQL to set the list to something that the driver's not
> expecting, and there's nothing the driver can do to prevent it.

... unless there is something like a (perhaps more generally useful)
BECAUSE I HAVE :cookie AND I SAY SO clause.[1]

Regards,
-Chap

[1]
https://www.postgresql.org/message-id/59127E4E.8090705%40anastigmatix.net

On 10/11/19 4:44 PM, Dave Cramer wrote:
> On Fri, 11 Oct 2019 at 16:41, Chapman Flack <chap@anastigmatix.net> 
>> On 10/11/19 4:30 PM, Robert Haas wrote:
>>> allow it to be done via SQL. Otherwise, the user can break the driver
>>> by using SQL to set the list to something that the driver's not
>>> expecting, and there's nothing the driver can do to prevent it.
>>
>> ... unless there is something like a (perhaps more generally useful)
>> BECAUSE I HAVE :cookie AND I SAY SO clause.[1]
> 
> Correct me if I'm wrong but I'm pretty sure nothing in the protocol
> currently would allow this

Nothing in the protocol to support: a post-connection-establishment
changeable list of reportable GUCs? Or some sort of cookie-based
arrangement to lock something down?

If you meant the cookie scheme, the way I had envisioned it in that
earlier message, I didn't see anything about it that would require
protocol support. I was tempted to do a PoC implementation as a plain
old extension. I can't say that I have, yet, but I'm still tempted.

-Chap

Hi,

On 2019-10-11 16:30:17 -0400, Robert Haas wrote:
> On Fri, Oct 11, 2019 at 8:21 AM Dave Cramer <pg@fastcrypt.com> wrote:
> > So off the top of my head providing a system function seems like the way to go here unless you were contemplating
addingsomething to the protocol ?
 
> 
> Since the list of reportable GUCs is for the benefit of the driver,
> I'm not sure why this would need to be changed after the connection
> has been established.

Because of connection pooling. Consider the pooler inbetween the client
and the server. The pooler can't report back settings changes it doesn't
get itself. And just asking for all settings upfront seems problematic.


> But, if it does need to be changed, it seems like a terrible idea to
> allow it to be done via SQL. Otherwise, the user can break the driver
> by using SQL to set the list to something that the driver's not
> expecting, and there's nothing the driver can do to prevent it.

Uhm. The driver can just ignore GUCs it's not interested in, like our
docs have told them for a long time? And I mean if somebody is able to
issue sql, then they likely have some control over the driver as well. I
don't understand the problem here.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-10-11 16:30:17 -0400, Robert Haas wrote:
>> But, if it does need to be changed, it seems like a terrible idea to
>> allow it to be done via SQL. Otherwise, the user can break the driver
>> by using SQL to set the list to something that the driver's not
>> expecting, and there's nothing the driver can do to prevent it.

> Uhm. The driver can just ignore GUCs it's not interested in, like our
> docs have told them for a long time?

Certainly it should do that; but the problematic case is where it
*doesn't* get told about something it's depending on knowing about.

            regards, tom lane

On Wed, Oct 16, 2019 at 6:49 PM Dave Cramer <pg@fastcrypt.com> wrote:
> Here's an updated patch that addresses some of Andres' concerns specifically does not use strtok.

Hi Dave,

I think you need to s/strncasecmp/pg_strncasecmp/ to make this build on Windows.

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.63963

On 2019-10-12 05:05, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2019-10-11 16:30:17 -0400, Robert Haas wrote:
>>> But, if it does need to be changed, it seems like a terrible idea to
>>> allow it to be done via SQL. Otherwise, the user can break the driver
>>> by using SQL to set the list to something that the driver's not
>>> expecting, and there's nothing the driver can do to prevent it.
> 
>> Uhm. The driver can just ignore GUCs it's not interested in, like our
>> docs have told them for a long time?
> 
> Certainly it should do that; but the problematic case is where it
> *doesn't* get told about something it's depending on knowing about.

Yeah, the problem I see here is this:  Client 1 uses language driver A, 
client 2 uses language driver B.  Connection pooling is in use, and they 
both connect to the same pool.  Everyone is happy.

Now this feature gets introduced.  Language driver A is updated to 
transparently ask for GUC "foo" to be reported, because it uses it 
internally.  Now how it connection pooling supposed to work in this 
situation?

There either needs to be a way to change reported parameters during a 
session that this can be run when an existing backend connection is 
assigned to a pooler client.  Or the connection pooler would need to be 
changed to create separate pools for each different setting of the 
to-be-reported list, just like it already creates separate pools for 
different users and databases, since you can't change those after 
session start either.  Both of these options are not without problems. 
We should have a complete plan for this before implementing the feature 
in the server.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Oct-08, Craig Ringer wrote:

> On Fri, 12 Jul 2019 at 01:27, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> > We have steadfastly refused to provide protocol-level tools for things
> > like "please change my user ID, and don't let anyone change it again
> > via SQL," and that's a huge problem for things like connection poolers
> > which can't parse all the SQL flowing through the connection (because
> > figuring out what it does requires solving the Halting Problem) and
> > wouldn't want to if they could for performance reasons. I think that's
> > a huge mistake.
> 
> I very strongly agree. The inability to limit SET and RESET of SESSION
> AUTHORIZATION and ROLE is a huge pain point and it's far from the only one.

There's a reason the SQL standard defines SET SESSION AUTHORIZATION but
no RESET SESSION AUTHORIZATION: once you enter a security context, you
cannot escape it.  ISTM that essentially we broke feature F321 "User
authorization" by adding RESET into the mix.  (I think RESET ROLE breaks
the spirit of feature T331 too.)  The SQL:2016 standard describes how
this is supposed to work in Foundation "4.40.1.1 SQL-session
authorization identifiers" (same section is numbered 4.35.1.1 in
SQL:2011), and ISTM we made a huge mess of it.

I don't see how to fix it, though.  If we were to adopt the standard's
mechanism, we'd probably break tons of existing code.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Nov 5, 2019 at 9:06 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Yeah, the problem I see here is this:  Client 1 uses language driver A,
> client 2 uses language driver B.  Connection pooling is in use, and they
> both connect to the same pool.  Everyone is happy.
>
> Now this feature gets introduced.  Language driver A is updated to
> transparently ask for GUC "foo" to be reported, because it uses it
> internally.  Now how it connection pooling supposed to work in this
> situation?

Good point.

> There either needs to be a way to change reported parameters during a
> session that this can be run when an existing backend connection is
> assigned to a pooler client.  Or the connection pooler would need to be
> changed to create separate pools for each different setting of the
> to-be-reported list, just like it already creates separate pools for
> different users and databases, since you can't change those after
> session start either.  Both of these options are not without problems.
> We should have a complete plan for this before implementing the feature
> in the server.

+1.

Based on this, it seems to me that we actually do need a way to change
the list of reportable GUCs. However, it also seems to me that it
needs to be done via a protocol level mechanism (which is also how
RESET SESSION AUTHORIZATION ought to work). Drivers can avoid letting
protocol messages be sent if the effect of those messages will create
a configuration they can't support. They can't prevent SQL from being
executed, unless they can solve the halting problem.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 5, 2019 at 10:02 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> There's a reason the SQL standard defines SET SESSION AUTHORIZATION but
> no RESET SESSION AUTHORIZATION: once you enter a security context, you
> cannot escape it.  ISTM that essentially we broke feature F321 "User
> authorization" by adding RESET into the mix.  (I think RESET ROLE breaks
> the spirit of feature T331 too.)  The SQL:2016 standard describes how
> this is supposed to work in Foundation "4.40.1.1 SQL-session
> authorization identifiers" (same section is numbered 4.35.1.1 in
> SQL:2011), and ISTM we made a huge mess of it.
>
> I don't see how to fix it, though.  If we were to adopt the standard's
> mechanism, we'd probably break tons of existing code.

It wouldn't be difficult to introduce a new protocol-level option that
prohibits RESET SESSION AUTHORIZATION; and it would also be possible
to introduce a new protocol message that has the same effect as RESET
SESSION AUTHORIZATION. If you do those two things, then it's possible
to create a sandbox which the end client cannot escape but which the
pooler can escape easily.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Dec 29, 2019 at 8:35 AM Dave Cramer <pg@fastcrypt.com> wrote:
> So where are we on this patch ? AFAICT using _pq is a protocol level option.

It is, but it only lets you set the initial value, not change things
later. Fixing that is probably going to require introducing a new
protocol message with careful thinking about backward compatibility.
I'm somewhat keen to work on that if nobody else wants to do it, but
I'm not sure exactly when I'll have time.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company