Re: Providing catalog view to pg_hba.conf file - Patch submission

2015-02-27 17:59 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> All,
>
> * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> > On 28.1.2015 23:01, Jim Nasby wrote:
> > > On 1/28/15 12:46 AM, Haribabu Kommi wrote:
> > >>> >Also, what happens if someone reloads the config in the middle of
> > >>> running
> > >>> >the SRF?
> > >> hba entries are reloaded only in postmaster process, not in every
> > >> backend.
> > >> So there shouldn't be any problem with config file reload. Am i
> > >> missing something?
> > >
> > > Ahh, good point. That does raise another issue though... there might
> > > well be some confusion about that. I think people are used to the
> > > varieties of how settings come into effect that perhaps this isn't an
> > > issue with pg_settings, but it's probably worth at least mentioning in
> > > the docs for a pg_hba view.
> >
> > I share this fear, and it's the main problem I have with this patch.
>
> Uh, yeah, agreed.
>

yes, good notice. I was blind.

>
> > Currently, the patch always does load_hba() every time pg_hba_settings
> > is accessed, which loads the current contents of the config file into
> > the backend process, but the postmaster still uses the original one.
> > This makes it impossible to look at currently effective pg_hba.conf
> > contents. Damn confusing, I guess.
>
> Indeed. At a *minimum*, we'd need to have some way to say "what you're
> seeing isn't what's actually being used".
>
> > Also, I dare to say that having a system view that doesn't actually show
> > the system state (but contents of a config file that may not be loaded
> > yet), is wrong.
>
> Right, we need to show what's currently active in PG-land, not just spit
> back whatever the on-disk contents currently are.
>
> > Granted, we don't modify pg_hba.conf all that often, and it's usually
> > followed by a SIGHUP to reload the contents, so both the backend and
> > postmaster should see the same stuff. But the cases when I'd use this
> > pg_hba_settings view usually turned out to be 'forgot to SIGHUP', so
> > this would not help, actually.
>
> Agreed.
>
> > What I can imagine is keeping the original list (parsed by postmaster,
> > containing the effective pg_hba.conf contents), and keeping another list
> > of 'current contents' within backends. And having a 'reload' flag for
> > pg_hba_settings, determining which list to use.
> >
> > pg_hba_settings(reload=false) -> old list (from postmaster)
> > pg_hba_settings(reload=true) -> new list (from backend)
> >
> > The pg_hba_settings view should use 'reload=false' (i.e. show effective
> > contents of the hba).
>
> I don't think we actually care what the "current contents" are from the
> backend's point of view- after all, when does an individual backend ever
> use the contents of pg_hba.conf after it's up and running? What would
> make sense, to me at least, would be:
>
> pg_hba_configured() -- spits back whatever the config file has
> pg_hba_active() -- shows what the postmaster is using currently
>

I disagree and I dislike this direction. It is looks like over engineering.

* load every time is wrong, because you will see possibly not active data.

* ignore reload is a attack to mental health of our users.

It should to work like "pg_settings". I need to see "what is wrong in this
moment" in pg_hba.conf, not what was or what will be wrong.

We can load any config files via admin contrib module - so there is not
necessary repeat same functionality

Regards

Pavel

> Or something along those lines. Note that I'd want pg_hba_configured()
> to throw an error if the config file can't be parsed (which would be
> quite handy to make sure that you didn't goof up the config..). This,
> combined with pg_reload_conf() would provide a good way to review
> changes before putting them into place.
>
> > The other feature that'd be cool to have is a debugging function on top
> > of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd)
> > showing which hba rule matched. But that's certainly nontrivial.
>
> I'm not sure that I see why, offhand, it'd be much more than trivial..
>
> Thanks!
>
> Stephen
>