On Thu, Mar 29, 2018 at 4:39 PM, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote:
> > > On 03/02/2018 12:16 PM, Magnus Hagander wrote: > > On Fri, Feb 23, 2018 at 7:55 PM, Magnus Hagander <mag...@hagander.net > > <mailto:mag...@hagander.net>> wrote: > > > > On Fri, Feb 23, 2018 at 7:52 PM, Tom Lane <t...@sss.pgh.pa.us > > <mailto:t...@sss.pgh.pa.us>> wrote: > > > > Magnus Hagander <mag...@hagander.net > > <mailto:mag...@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. > With no further feedback on this, I have pushed this version of the patch (rebased). Thanks! -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
