Thread: Allow workers to override datallowconn
In working on the checksumhelper patch, we came across wanting a background worker to be allowed to bypass datallowconn for a database. Right now we didn't take care of that, and just said "you have to ALTER TABLE" first.
Specifically for this usecase that is OK, but not paticularly user friendly. And I think this is a use that can also be useful for other things.
Attached is a patch that adds new Override versions of the functions to connect to a database from a background worker.
Specifically for this usecase that is OK, but not paticularly user friendly. And I think this is a use that can also be useful for other things.
Attached is a patch that adds new Override versions of the functions to connect to a database from a background worker.
Another option would be to just add the parameter directly to the regular connection function, and not create separate functions. But that would make it an incompatible change. And since background workers are commonly used in extensions, that would break a lot of extensions out there. I figured it's probably not worth doing that, and thus added the new functions. What do others think about that?
--
Are there any other caveats in doing that this actually makes it dangerous to just allow bypassing it for extensions?
Attachment
Magnus Hagander <magnus@hagander.net> writes:
> Attached is a patch that adds new Override versions of the functions to
> connect to a database from a background worker.
> Another option would be to just add the parameter directly to the regular
> connection function, and not create separate functions. But that would make
> it an incompatible change. And since background workers are commonly used
> in extensions, that would break a lot of extensions out there. I figured
> it's probably not worth doing that, and thus added the new functions. What
> do others think about that?
Meh. We change exported APIs in new major versions all the time. As
long as it's just a question of an added parameter, people can deal
with it. You could take the opportunity to future-proof a little by
making this option be the first bit in a flags parameter, so that at
least future boolean option additions don't require another API break
or a whole new set of redundant functions.
> Are there any other caveats in doing that this actually makes it dangerous
> to just allow bypassing it for extensions?
Don't think so; we autovacuum such DBs anyway don't we?
regards, tom lane
On Thu, Feb 22, 2018 at 7:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
> Attached is a patch that adds new Override versions of the functions to
> connect to a database from a background worker.
> Another option would be to just add the parameter directly to the regular
> connection function, and not create separate functions. But that would make
> it an incompatible change. And since background workers are commonly used
> in extensions, that would break a lot of extensions out there. I figured
> it's probably not worth doing that, and thus added the new functions. What
> do others think about that?
Meh. We change exported APIs in new major versions all the time. As
long as it's just a question of an added parameter, people can deal
with it. You could take the opportunity to future-proof a little by
making this option be the first bit in a flags parameter, so that at
least future boolean option additions don't require another API break
or a whole new set of redundant functions.
Fair enough. In that case, something like the attached?
> Are there any other caveats in doing that this actually makes it dangerous
> to just allow bypassing it for extensions?
Don't think so; we autovacuum such DBs anyway don't we?
Yes. We set the freeze ages to 0 but we do run on it.
Attachment
Hi, On 2018-02-22 19:01:35 +0100, Magnus Hagander wrote: > In working on the checksumhelper patch, we came across wanting a background > worker to be allowed to bypass datallowconn for a database. Right now we > didn't take care of that, and just said "you have to ALTER TABLE" first. I suspect you mean ALTER DATABASE, rather than table? ;) I wonder if we don't want this to be a slightly more generic facility. E.g. pg_upgrade has the same need. Perhaps we whould superuser-only connection-establishment only option that allows connecting to a database even if datallowconn = false, and then additionally allow to pass connection arguments to BackgroundWorkerInitializeConnection()? It seems fairly reasonable to want to establish different GUCs via normal GUC-y mechanisms when establishing a bgworker connection... Regards, Andres
On 22 February 2018 at 18:24, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Are there any other caveats in doing that this actually makes it dangerous >> to just allow bypassing it for extensions? > > Don't think so; we autovacuum such DBs anyway don't we? Yeh, there is already precedent that should mean it is easy/default for background workers to ignore datallowcon. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Feb 22, 2018 at 8:17 PM, Andres Freund <andres@anarazel.de> wrote:
-- Hi,
On 2018-02-22 19:01:35 +0100, Magnus Hagander wrote:
> In working on the checksumhelper patch, we came across wanting a background
> worker to be allowed to bypass datallowconn for a database. Right now we
> didn't take care of that, and just said "you have to ALTER TABLE" first.
I suspect you mean ALTER DATABASE, rather than table? ;)
Um. Yes :)
I wonder if we don't want this to be a slightly more generic
facility. E.g. pg_upgrade has the same need. Perhaps we whould
superuser-only connection-establishment only option that allows
connecting to a database even if datallowconn = false, and then
additionally allow to pass connection arguments to
BackgroundWorkerInitializeConnection()? It seems fairly reasonable to
want to establish different GUCs via normal GUC-y mechanisms when
establishing a bgworker connection...
In a background worker you can just set the parameter using SetConfigOption(), no? That seems a lot easier than turning things in to a kv pair and back...
I can see the point for having such a parameter for pg_upgrade, but I'm not sure we'd necessarily want to overload them.
Hi, On 2018-02-22 20:24:03 +0100, Magnus Hagander wrote: > In a background worker you can just set the parameter using > SetConfigOption(), no? That seems a lot easier than turning things in to a > kv pair and back... Sure, but, it doesn't seem bad to offer the option to only allow this for code running as superuser. > I can see the point for having such a parameter for pg_upgrade, but I'm not > sure we'd necessarily want to overload them. What's the argument against? Greetings, Andres Freund
On Thu, Feb 22, 2018 at 8:26 PM, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2018-02-22 20:24:03 +0100, Magnus Hagander wrote:
> In a background worker you can just set the parameter using
> SetConfigOption(), no? That seems a lot easier than turning things in to a
> kv pair and back...
Sure, but, it doesn't seem bad to offer the option to only allow this
for code running as superuser.
> I can see the point for having such a parameter for pg_upgrade, but I'm not
> sure we'd necessarily want to overload them.
What's the argument against?
Complexity for the bgw usecase. It now has to construct a key/value pair with proper escaping (well, for this one flag it would be easy, but if we do that wouldn't we also support the other config params? Were you thinking we'd have *just* tihs one?). Basically taking a data that you have in a "structured format" in the btw already turning it to a string and then parsing it back into structured data in the same string.
I think it'd be cleaner to let the bgw initializer pass those as flags. A "user connection" parameter could still use the booelan in InitPostgres() of course, and not invent a new things there, but the entry point API could stay simpler.
I haven't actually looked into what it would look like, so it could be that I'm overestimating what it'd mean of course.
On 2018-02-22 20:30:02 +0100, Magnus Hagander wrote: > Complexity for the bgw usecase. It now has to construct a key/value pair > with proper escaping (well, for this one flag it would be easy, but if we > do that wouldn't we also support the other config params? Were you thinking > we'd have *just* tihs one?). Basically taking a data that you have in a > "structured format" in the btw already turning it to a string and then > parsing it back into structured data in the same string. The serialization problem seems out of scope, I don't see why this code would need to deal with it. Receiving the options would be pretty easy, we pretty much have the relevant code for PGOPTIONS handling. The more important part I think is that we solve it via a GUC that can be used outside of bgworkers. Whether we require setting it via set_config() for bgworkers or have option parsing doesn't matter that much. > I think it'd be cleaner to let the bgw initializer pass those as flags. A > "user connection" parameter could still use the booelan in InitPostgres() > of course, and not invent a new things there, but the entry point API could > stay simpler. I'm pretty strongly against having a special case flag for bgw initialization for this. Greetings, Andres Freund
Magnus Hagander <magnus@hagander.net> writes:
> On Thu, Feb 22, 2018 at 8:26 PM, Andres Freund <andres@anarazel.de> wrote:
>> What's the argument against?
> Complexity for the bgw usecase.
They'd be completely different implementations and code paths, no?
For pg_upgrade to use such a thing it'd need to be a connection parameter
of some sort (implying, eg, infrastructure in libpq), while for a bgworker
there's no such animal as connection parameters because there's no
connection.
Certainly what pg_upgrade has to do is a bit ugly, but you'd be adding
an awful lot of code to get rid of a small amount of code. Doesn't
seem like a great tradeoff. Even if it is a good tradeoff, it seems
entirely unrelated to the bgworker's problem.
regards, tom lane
Andres Freund <andres@anarazel.de> writes:
> The more important part I think is that we solve it via a GUC that can
> be used outside of bgworkers.
Are you proposing an "ignore_datallowconn" GUC? That's a remarkably
bad idea. We don't have infrastructure that would allow it to be set
at an appropriate scope. I can't imagine any good use-case for allowing
it to be on globally; you'd want it to be per-session (or per-bgworker).
But I don't think there's any way to change the system default setting
in time for the setting to take effect during connection startup or
bgworker startup.
Magnus' most recent patch in this thread seems like a fine answer for
bgworkers. You've moved the goalposts into the next county, and the
design you're proposing to satisfy that goal is lousy. It will end
up being a large amount of additional work with no benefit except
being able to use an arguably less ugly (but almost certainly no
shorter) datallowconn override method in pg_upgrade.
regards, tom lane
On Thu, Feb 22, 2018 at 9:09 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
> The more important part I think is that we solve it via a GUC that can
> be used outside of bgworkers.
Are you proposing an "ignore_datallowconn" GUC? That's a remarkably
bad idea. We don't have infrastructure that would allow it to be set
at an appropriate scope. I can't imagine any good use-case for allowing
it to be on globally; you'd want it to be per-session (or per-bgworker).
But I don't think there's any way to change the system default setting
in time for the setting to take effect during connection startup or
bgworker startup.
I hacked up an attempt to do this. It does seem to work in the very simple case, but it does requiring changing the order in InitPostgres() to load the startup packet before validating those.
PFA WIP, which also shows how to do it in the background worker.
This one also lets me do
PGOPTIONS="-c ignore_connection_restrictionon" psql template0
to bypass the restriction, which is what pg_upgrade would basically do.
Magnus' most recent patch in this thread seems like a fine answer for
bgworkers. You've moved the goalposts into the next county, and the
design you're proposing to satisfy that goal is lousy. It will end
up being a large amount of additional work with no benefit except
being able to use an arguably less ugly (but almost certainly no
shorter) datallowconn override method in pg_upgrade.
*If* the moving of the startup packet processing to one step earlier in InitPostgres is safe, then it is actually less code this way right now. From a quick look I think it's safe to move it, but I haven't looked in detail.
I also haven't checked if this actually helps in the pg_upgrade case, but if bypassing datallowconn is what's needed there then it shuld.
Attachment
Magnus Hagander <magnus@hagander.net> writes:
> I hacked up an attempt to do this. It does seem to work in the very simple
> case, but it does requiring changing the order in InitPostgres() to load
> the startup packet before validating those.
I doubt that's safe. It requires, to name just one thing, an assumption
that no processing done in process_startup_options has any need to know
the database encoding, which is established by CheckMyDatabase. Thus
for instance, if any GUC settings carried in the startup packet include
non-ASCII characters, the wrong things will happen.
You could possibly make it work with more aggressive refactoring, but
I remain of the opinion that this is a fundamentally bad idea anyhow.
A GUC of this kind is just ripe for abuse, and I don't think it's solving
any problem we really need solved.
regards, tom lane
On 2018-02-22 15:24:50 -0500, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > I hacked up an attempt to do this. It does seem to work in the very simple > > case, but it does requiring changing the order in InitPostgres() to load > > the startup packet before validating those. > > I doubt that's safe. It requires, to name just one thing, an assumption > that no processing done in process_startup_options has any need to know > the database encoding, which is established by CheckMyDatabase. Thus > for instance, if any GUC settings carried in the startup packet include > non-ASCII characters, the wrong things will happen. I think those are effectively ascii only anyway. We process them with pretty much ascii (or well 8 byte ascii compatible) only logic afaict. C.f. pg_split_opts(). > You could possibly make it work with more aggressive refactoring, but > I remain of the opinion that this is a fundamentally bad idea anyhow. > A GUC of this kind is just ripe for abuse, and I don't think it's > solving any problem we really need solved. How's that any less safe than allowing to load libraries, disabling system indexes, and reams of other things we allow via GUCs? Greetings, Andres Freund
Hi, On 2018-02-22 15:24:50 -0500, Tom Lane wrote: > You could possibly make it work with more aggressive refactoring, but > I remain of the opinion that this is a fundamentally bad idea anyhow. > A GUC of this kind is just ripe for abuse, and I don't think it's solving > any problem we really need solved. Oh, and I actually find this a thing that I needed a couple times before. Databases used as templates are occasionally useful. To fill their contents one usually wants to connect to them occasionally, but most of the time connections ought to not be allowed so creating a database with them as a template actually works. It's solvable via other means, but not conveniently. Greetings, Andres Freund
On Thu, Feb 22, 2018 at 9:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
> I hacked up an attempt to do this. It does seem to work in the very simple
> case, but it does requiring changing the order in InitPostgres() to load
> the startup packet before validating those.
I doubt that's safe. It requires, to name just one thing, an assumption
that no processing done in process_startup_options has any need to know
the database encoding, which is established by CheckMyDatabase. Thus
for instance, if any GUC settings carried in the startup packet include
non-ASCII characters, the wrong things will happen.
You could possibly make it work with more aggressive refactoring, but
I remain of the opinion that this is a fundamentally bad idea anyhow.
A GUC of this kind is just ripe for abuse, and I don't think it's solving
any problem we really need solved.
Here's another attempt at moving this one forward. Basically this adds a new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set, any other parameters are ignored (without error). For now, only the ignore_connection_restriction is allowed at this stage. Then it runs CheckMyDatabase(), and after that it runs through all the parameters again, now with the GUC_S_CLIENT source as usual, which will now process all other variables.
Attachment
Magnus Hagander <magnus@hagander.net> writes:
> Here's another attempt at moving this one forward. Basically this adds a
> new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters
> once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this
> source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set,
> any other parameters are ignored (without error). For now, only the
> ignore_connection_restriction is allowed at this stage. Then it runs
> CheckMyDatabase(), and after that it runs through all the parameters again,
> now with the GUC_S_CLIENT source as usual, which will now process all
> other variables.
Ick. This is an improvement over the other way of attacking the problem?
I do not think so.
regards, tom lane
On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
> Here's another attempt at moving this one forward. Basically this adds a
> new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters
> once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this
> source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set,
> any other parameters are ignored (without error). For now, only the
> ignore_connection_restriction is allowed at this stage. Then it runs
> CheckMyDatabase(), and after that it runs through all the parameters again,
> now with the GUC_S_CLIENT source as usual, which will now process all
> other variables.
Ick. This is an improvement over the other way of attacking the problem?
I do not think so.
Nope, I'm far from sure that it is. I just wanted to show what it'd look like.
I personally think the second patch (the one adding a parameter to BackendWorkerInitializeConnection) is the cleanest one. It doesn't solve Andres' problem, but perhaps that should be the job of a different patch.
On Fri, Feb 23, 2018 at 7:55 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:Magnus Hagander <magnus@hagander.net> writes:
> Here's another attempt at moving this one forward. Basically this adds a
> new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters
> once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this
> source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set,
> any other parameters are ignored (without error). For now, only the
> ignore_connection_restriction is allowed at this stage. Then it runs
> CheckMyDatabase(), and after that it runs through all the parameters again,
> now with the GUC_S_CLIENT source as usual, which will now process all
> other variables.
Ick. This is an improvement over the other way of attacking the problem?
I do not think so.Nope, I'm far from sure that it is. I just wanted to show what it'd look like.I personally think the second patch (the one adding a parameter to BackendWorkerInitializeConnection) is the cleanest one. It doesn't solve Andres' problem, but perhaps that should be the job of a different patch.
Andres, do you have any other ideas of directions to look that would make you withdraw your objection? I'm happy to try to write up a patch that solves it in a way that everybody can agree with. But right now it seems to be stuck between one way that's strongly objected to by you and one way that's strongly objected to by Tom. And I'd rather not have that end up with not getting the problem solved at all for *any* of the usecases...
--
On 03/02/2018 12:16 PM, Magnus Hagander wrote: > On Fri, Feb 23, 2018 at 7:55 PM, Magnus Hagander <magnus@hagander.net > <mailto:magnus@hagander.net>> wrote: > > On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us > <mailto:tgl@sss.pgh.pa.us>> wrote: > > Magnus Hagander <magnus@hagander.net > <mailto:magnus@hagander.net>> writes: > > Here's another attempt at moving this one forward. Basically this adds a > > new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters > > once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this > > source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set, > > any other parameters are ignored (without error). For now, only the > > ignore_connection_restriction is allowed at this stage. Then it runs > > CheckMyDatabase(), and after that it runs through all the parameters again, > > now with the GUC_S_CLIENT source as usual, which will now process all > > other variables. > > Ick. This is an improvement over the other way of attacking the > problem? > I do not think so. > > > Nope, I'm far from sure that it is. I just wanted to show what it'd > look like. > > I personally think the second patch (the one adding a parameter to > BackendWorkerInitializeConnection) is the cleanest one. It doesn't > solve Andres' problem, but perhaps that should be the job of a > different patch. > > > > FWIW, I just realized that thue poc patch that adds the GUC also breaks > a large part of the regression tests. As a side-effect of it breaking > how DateStyle works. That's obviously a fixable problem, but it seems > not worth spending time on if that's not the way forward anyway. > > Andres, do you have any other ideas of directions to look that would > make you withdraw your objection? I'm happy to try to write up a patch > that solves it in a way that everybody can agree with. But right now it > seems to be stuck between one way that's strongly objected to by you and > one way that's strongly objected to by Tom. And I'd rather not have that > end up with not getting the problem solved at all for *any* of the > usecases... > My 2c: I think we should just go with the simplest solution, that is the patch sent on 22/2 19:54 (i.e. BackgroundWorkerInitializeConnectionByOid with an extra parameter). It would be nice to have something more generic that also works for the Andres' use case, but the patches submitted to this thread are not particularly pretty. Also, Tom suggested there may be safety issues when the GUCs are processed earlier - I agree Andres is right the GUCs are effectively ASCII-only so the encoding is not an issue, but perhaps there are other issues (Tom suggested this merely as an example). So I agree with Magnus, the extra flag seems to be perfectly fine for bgworkers, and I'd leave the more generic solution for a future patch if anyone wants to hack on it. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Mar 29, 2018 at 4:39 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
On 03/02/2018 12:16 PM, Magnus Hagander wrote:
> On Fri, Feb 23, 2018 at 7:55 PM, Magnus Hagander <magnus@hagander.net
> <mailto:magnus@hagander.net>> wrote:
>
> On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>
> Magnus Hagander <magnus@hagander.netMy 2c: I think we should just go with the simplest solution, that is the> <mailto:magnus@hagander.net>> writes:
> > Here's another attempt at moving this one forward. Basically this adds a
> > new GucSource being GUC_S_CLIENT_EARLY. It now runs through the parameters
> > once before CheckMyDatabase, with source set to GUC_S_CLIENT_EARLY. In this
> > source, *only* parameters that are flagged as GUC_ALLOW_EARLY will be set,
> > any other parameters are ignored (without error). For now, only the
> > ignore_connection_restriction is allowed at this stage. Then it runs
> > CheckMyDatabase(), and after that it runs through all the parameters again,
> > now with the GUC_S_CLIENT source as usual, which will now process all
> > other variables.
>
> Ick. This is an improvement over the other way of attacking the
> problem?
> I do not think so.
>
>
> Nope, I'm far from sure that it is. I just wanted to show what it'd
> look like.
>
> I personally think the second patch (the one adding a parameter to
> BackendWorkerInitializeConnection) is the cleanest one. It doesn't
> solve Andres' problem, but perhaps that should be the job of a
> different patch.
>
>
>
> FWIW, I just realized that thue poc patch that adds the GUC also breaks
> a large part of the regression tests. As a side-effect of it breaking
> how DateStyle works. That's obviously a fixable problem, but it seems
> not worth spending time on if that's not the way forward anyway.
>
> Andres, do you have any other ideas of directions to look that would
> make you withdraw your objection? I'm happy to try to write up a patch
> that solves it in a way that everybody can agree with. But right now it
> seems to be stuck between one way that's strongly objected to by you and
> one way that's strongly objected to by Tom. And I'd rather not have that
> end up with not getting the problem solved at all for *any* of the
> usecases...
>
patch sent on 22/2 19:54 (i.e. BackgroundWorkerInitializeConnectionByOid
with an extra parameter).
It would be nice to have something more generic that also works for the
Andres' use case, but the patches submitted to this thread are not
particularly pretty. Also, Tom suggested there may be safety issues when
the GUCs are processed earlier - I agree Andres is right the GUCs are
effectively ASCII-only so the encoding is not an issue, but perhaps
there are other issues (Tom suggested this merely as an example).
So I agree with Magnus, the extra flag seems to be perfectly fine for
bgworkers, and I'd leave the more generic solution for a future patch if
anyone wants to hack on it.
With no further feedback on this, I have pushed this version of the patch (rebased). Thanks!