Re: Improvements in psql hooks for variables

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: "Daniel Verite" <daniel(at)manitou-mail(dot)org>
Cc: "Rahila Syed" <rahilasyed90(at)gmail(dot)com>, "Stephen Frost" <sfrost(at)snowman(dot)net>, "Ashutosh Bapat" <ashutosh(dot)bapat(at)enterprisedb(dot)com>, "pgsql-hackers" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Improvements in psql hooks for variables
Date: 2017-02-01 18:32:53
Message-ID: 17516.1485973973@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Attached is a finished version that includes hooks for all the variables
> for which they were clearly sensible. (FETCH_COUNT doesn't seem to really
> need one, and I didn't do anything with HISTSIZE or IGNOREEOF either.
> It might be worth bringing the latter two into the hooks paradigm, but
> that seems like fit material for a separate patch.)

I got more excited about doing that after noticing that not only would
it clean up the behavior of those particular variables, but we could get
rid of some code. First, we'd not need the rather warty GetVariableNum()
anymore, and second, we'd then be almost at the point where every control
variable has a hook, and therefore we could drop tab-complete.c's private
list of known variable names. That was only ever needed to cover the
possibility of important variables not being present in the variables
list.

So the attached proposed patch does these things:

1. Modify FETCH_COUNT to always have a defined value, like other control
variables, mainly so it will always appear in "\set" output.

2. Add hooks to force HISTSIZE to be defined and require it to have an
integer value. (I don't see any point in allowing it to be set to
non-integral values.)

3. Add hooks to force IGNOREEOF to be defined and require it to have an
integer value. Unlike the other cases, here we're trying to be
bug-compatible with a rather bogus externally-defined behavior, so I think
we need to continue to allow "\set IGNOREEOF whatever". What I propose is
that the substitution hook silently replace non-numeric values with "10",
so that the stored value always reflects what we're really doing.

4. Add a dummy assign hook for HISTFILE, just so it's always in
variables.c's list. We can't require it to be defined always, because
that would break the interaction with the PSQL_HISTORY environment
variable, so there isn't any change in visible behavior here.

5. Remove tab-complete.c's private list of known variable names. Given
the other changes, there are no control variables it won't show anyway.
This does mean that if for some reason you've unset one of the status
variables (DBNAME, HOST, etc), that variable would not appear in tab
completion for \set. But I think that's fine, for at least two reasons:
we shouldn't be encouraging people to use those variables as regular
variables, and if someone does do so anyway, why shouldn't it act just
like a regular variable?

6. Remove no-longer-used-anywhere GetVariableNum().

Any objections?

regards, tom lane

Attachment Content-Type Size
more-psql-variable-cleanup.patch text/x-diff 13.6 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-02-01 19:00:31 Re: [PATCH] Add tab completion for DEALLOCATE
Previous Message Robert Haas 2017-02-01 18:27:31 Re: Time to up bgwriter_lru_maxpages?