Обсуждение: 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 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.
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
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 ?
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
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 Wed, 10 Jul 2019 at 16:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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
See attached for an initial patch. If this is an acceptable way to go I will add tests and documentation
Вложения
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
On Thu, 11 Jul 2019 at 10:06, Robert Haas <robertmhaas@gmail.com> wrote:
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:
Sure, of course
1. I don't really like putting 'guc' into an externally visible name;
that's why I suggested 'report'.
sure, no problem
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.
Well we haven't even established the connection. I don't really see a way to find extensions
I thought about checking the available GUC's. I'll add that.
Thanks for your quick response
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, 11 Jul 2019 at 10:19, Robert Haas <robertmhaas@gmail.com> wrote:
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.
This suggests creating a list in guc.c instead. I'm unclear as to the visibility of variables in there
How do I make this list visible only to the session ?
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 Wed, 10 Jul 2019 at 16:22, Robert Haas <robertmhaas@gmail.com> wrote:
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.
Not being intimately familiar with the backend the implications of above just struck home.
So if I understand this correctly if user bob has altered his search path and there is a security-definer function called owned by him then
the search path will be changed for the duration of the function and reported for every iteration? The implications of this are "interesting" to say the least.
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, 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 called owned by him then
> the search path will be changed for the duration of the function and reported for every iteration? The implications of 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 ???
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
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.
On Thu, 11 Jul 2019 at 04:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.
Yep. For search_path I definitely agree.
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.
With that said, most of the time I think we could argue that it's reasonable to fire a GUC_REPORT if the *top-level* GUC nestlevel values change. That'd solve the search_path spam issue while still giving Dave what he wants, amongst other things.
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.
On Tue, 8 Oct 2019 at 11:57, Craig Ringer <craig@2ndquadrant.com> wrote:
On Thu, 11 Jul 2019 at 04:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: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.Yep. For search_path I definitely agree.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.With that said, most of the time I think we could argue that it's reasonable to fire a GUC_REPORT if the *top-level* GUC nestlevel values change. That'd solve the search_path spam issue while still giving Dave what he wants, amongst other things.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.
This in fact is the number one reason I want to do this. NPGSQL also benefits from this as well.
On Wed, 9 Oct 2019 at 08:15, Dave Cramer <pg@fastcrypt.com> wrote:
On Tue, 8 Oct 2019 at 11:57, Craig Ringer <craig@2ndquadrant.com> wrote:On Thu, 11 Jul 2019 at 04:34, Tom Lane <tgl@sss.pgh.pa.us> wrote: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.Yep. For search_path I definitely agree.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.With that said, most of the time I think we could argue that it's reasonable to fire a GUC_REPORT if the *top-level* GUC nestlevel values change. That'd solve the search_path spam issue while still giving Dave what he wants, amongst other things.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.This in fact is the number one reason I want to do this. NPGSQL also benefits from this as well.
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.
See attached patch
Вложения
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 Thu, 10 Oct 2019 at 23:45, Andres Freund <andres@anarazel.de> wrote:
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.
True. I remember dealing with (or trying to deal with) that mess in BDR's DDL replication.
I guess the question becomes "what is required to to permit apps, clients or drivers to safely and efficiently cache mappings of object names to server side metadata".
E.g. if I have a non-schema-qualified relation name what server side help is needed to make it possible to safely cache its column names, column order, and column types? Or for a given function name, cache whether it's a function or procedure, its IN, INOUT and OUT parameter names, positions and types, etc?
You sensibly point out that being notified of search_path changes is not sufficient. It might still be useful.
We have comprehensive server-side cache invalidation support. I wonder if it's reasonable to harness that. Let clients request that we accumulate invalidations and deliver them on the wire to the client at the end of each statement, before ReadyForQuery, along with a backend-local invalidation epoch that increments each time we send invalidations to the client.
The problem is that the client would have to send the invalidation epoch along with each query so the server could tell it to flush its cache and retry if there were invalidations since the end of the last query.
None of that is likely to be pretty given that we've been unable to agree on any way to extend the protocol at client request.
Anyone have any ideas for "near enough is good enough" approaches or methods that'd work with some client assistance / extension support / etc?
On Thu, 10 Oct 2019 at 12:05, Andres Freund <andres@anarazel.de> wrote:
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.
So off the top of my head providing a system function seems like the way to go here unless you were contemplating adding something to the protocol ?
> @@ -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()?
Fair enough
> + 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.
Upon review I'm left with "what was I thinking?"
> +/*
> + * 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.
I'm open to suggestions here, although I'm not sure what your concern is?
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 Fri, 11 Oct 2019 at 16:41, Chapman Flack <chap@anastigmatix.net> wrote:
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]
Correct me if I'm wrong but I'm pretty sure nothing in the protocol currently would allow this
While I see the utility at least for connection pools this does seem to be scope creep at the moment
Dave
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 Sat, 12 Oct 2019 at 05:05, Tom Lane <tgl@sss.pgh.pa.us> 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.
regards, tom lane
Here's an updated patch that addresses some of Andres' concerns specifically does not use strtok.
As far as addressing connection poolers goes; one thought is to use the cancellation key to "validate" the SQL.
This should be known to all drivers and pool implementations. Thoughts ?
Dave
Вложения
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 Sun, 3 Nov 2019 at 19:40, Thomas Munro <thomas.munro@gmail.com> wrote:
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
Thomas,
Thanks for the review.
see attached
Вложения
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 Wed, 6 Nov 2019 at 11:09, Robert Haas <robertmhaas@gmail.com> wrote:
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.
So where are we on this patch ? AFAICT using _pq is a protocol level option.
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