Обсуждение: [PATCH] SET search_path += octopus

Hi.

The first attached patch
(0001-Accept-SET-xyz-pqr-to-add-pqr-to-the-current-setting.patch) adds
support for commands like the following, applicable to any configuration
settings that are represented as a comma-separated list of strings
(i.e., GUC_LIST_INPUT):

    postgres=# SET search_path += octopus;
    SET
    postgres=# SET search_path += "giant squid", kraken, narwhal; -- [1]
    SET
    postgres=# SET search_path -= public, narwhal;
    SET
    postgres=# SHOW search_path;
    ┌─────────────────────────────────────────┐
    │               search_path               │
    ├─────────────────────────────────────────┤
    │ "$user", octopus, "giant squid", kraken │
    └─────────────────────────────────────────┘
    (1 row)

The implementation extends to ALTER SYSTEM SET with next to no effort,
so you can also add entries to shared_preload_libraries without having
to know its current value:

    ALTER SYSTEM SET shared_preload_libraries += auto_explain;

The second patch
(0002-Support-SET-syntax-for-numeric-configuration-setting.patch) adds
support to modify numeric configuration settings:

    postgres=# SET cpu_tuple_cost += 0.02;
    SET
    postgres=# SET effective_cache_size += '2GB';
    SET
    postgres=# SHOW effective_cache_size;
    ┌──────────────────────┐
    │ effective_cache_size │
    ├──────────────────────┤
    │ 6GB                  │
    └──────────────────────┘
    (1 row)
    postgres=# ALTER SYSTEM SET max_worker_processes += 4;
    ALTER SYSTEM

Being able to safely modify shared_preload_libraries (in particular) and
max_worker_processes during automated extension deployments is a problem
I've struggled with more than once in the past.

These patches do not affect configuration file parsing in any way: its
use is limited to "SET" and "ALTER xxx SET". (After I started working on
this, I came to know that this idea has been proposed in different forms
in the past, and objections to those proposals centred around various
difficulties involved in adding this syntax to configuration files. I'm
not particularly fond of that idea, and it's not what I've done here.)

(Another feature that could be implemented using this framework is to
ensure the current setting is at least as large as a given value:

    ALTER SYSTEM SET shared_buffers >= '8GB';

This would not change shared_buffers if it were already larger than 8GB.
I have not implemented this, pending feedback on what's already there,
but it would be simple to do.)

Comments welcome.

-- Abhijit

1. This feature supports a wide variety of marine creatures, with no
   implied judgement about their status, real or mythical; however,
   adding them to shared_preload_libraries is not advisable.

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Thanks for the patch. The patch works on my machine as per specs on the master branch
(26ec6b5948a73d0e07ed9435ee4554594acdf34f).

On 2020-10-20 13:02, Ibrar Ahmed wrote:
> The following review has been posted through the commitfest 
> application:
> make installcheck-world:  tested, passed
> Implements feature:       tested, passed
> Spec compliant:           tested, passed
> Documentation:            not tested
> 
> Thanks for the patch. The patch works on my machine as per specs on
> the master branch (26ec6b5948a73d0e07ed9435ee4554594acdf34f).

FWIW, I checked the same steps,
including doc-builds of PDF-A4, and .html (on debian 9/stretch, with gcc 
10.1)

I found no problems.

Nice feature!


Erik Rijkers

Hi,

On 2020-09-28 09:09:24 +0530, Abhijit Menon-Sen wrote:
>     postgres=# SET search_path += octopus;
>     SET

Yea, that would be quite useful!



> The second patch
> (0002-Support-SET-syntax-for-numeric-configuration-setting.patch) adds
> support to modify numeric configuration settings:
> 
>     postgres=# SET cpu_tuple_cost += 0.02;
>     SET
>     postgres=# SET effective_cache_size += '2GB';
>     SET
>     postgres=# SHOW effective_cache_size;
>     ┌──────────────────────┐
>     │ effective_cache_size │
>     ├──────────────────────┤
>     │ 6GB                  │
>     └──────────────────────┘
>     (1 row)
>     postgres=# ALTER SYSTEM SET max_worker_processes += 4;
>     ALTER SYSTEM

Much less clear that this is a good idea...

It seems to me that appending and incrementing using the same syntax is
a) confusing b) will be a limitation before long.


> These patches do not affect configuration file parsing in any way: its
> use is limited to "SET" and "ALTER xxx SET".

Are you including user / database settings as part of ALTER ... SET? Or
just SYSTEM?


I'm not convinced that having different features for the SQL level is a
good idea.



> (Another feature that could be implemented using this framework is to
> ensure the current setting is at least as large as a given value:
> 
>     ALTER SYSTEM SET shared_buffers >= '8GB';

Given that this is just SQL level, I don't see why we'd need a special
type of language here. You can just use DO etc.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Given that this is just SQL level, I don't see why we'd need a special
> type of language here. You can just use DO etc.

I'd make that point against the whole proposal.  There's nothing here that
can't be done with current_setting() + set_config().  I'm pretty dubious
about layering extra functionality into such a fundamental utility command
as SET; and the fact that we've gone twenty-odd years without similar
previous proposals doesn't speak well for it being really useful.

            regards, tom lane

Hi,

On 2020-10-20 14:16:12 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Given that this is just SQL level, I don't see why we'd need a special
> > type of language here. You can just use DO etc.
> 
> I'd make that point against the whole proposal.  There's nothing here that
> can't be done with current_setting() + set_config().  I'm pretty dubious
> about layering extra functionality into such a fundamental utility command
> as SET; and the fact that we've gone twenty-odd years without similar
> previous proposals doesn't speak well for it being really useful.

From my POV it'd make sense to have SET support mirroring config file
syntax if we had it. And there've certainly been requests for
that...

The one case where I can see SET support being useful even without
config support is to allow for things like
ALTER DATABASE somedatabase SET search_path += 'myapp';

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2020-10-20 14:16:12 -0400, Tom Lane wrote:
>> I'd make that point against the whole proposal.  There's nothing here that
>> can't be done with current_setting() + set_config().

> The one case where I can see SET support being useful even without
> config support is to allow for things like
> ALTER DATABASE somedatabase SET search_path += 'myapp';

Hmm, yeah, that's fractionally less easy to build from spare parts
than the plain SET case.

But I think there are more definitional hazards than you are letting
on.  If there's no existing pg_db_role_setting entry, what value
exactly are we += 'ing onto, and why?

            regards, tom lane

On Tue, Oct 20, 2020 at 2:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2020-Oct-20, Tom Lane wrote:
> > and the fact that we've gone twenty-odd years without similar
> > previous proposals doesn't speak well for it being really useful.
>
> Actually, there's at least two threads about this:
>
> https://postgr.es/m/flat/86d2ceg611.fsf@jerry.enova.com
> https://postgr.es/m/flat/74af1f60-34af-633e-ee8a-310d40c741a7%402ndquadrant.com

Yeah, I think this is clearly useful. ALTER SYSTEM
shared_preload_libraries += direshark seems like a case with a lot of
obvious practical application, and adding things to search_path seems
compelling, too.

FWIW, I also like the chosen syntax. I think it's more useful with
lists than with numbers, but maybe it's at least somewhat useful for
both. However, I do wonder if it'd be good to have a list-manipulation
syntax that allows you to control where the item gets inserted --
start and end being the cases that people would want most, I suppose.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2020-10-20 14:39:42 -0400, Robert Haas wrote:
> On Tue, Oct 20, 2020 at 2:34 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2020-Oct-20, Tom Lane wrote:
> > > and the fact that we've gone twenty-odd years without similar
> > > previous proposals doesn't speak well for it being really useful.
> >
> > Actually, there's at least two threads about this:
> >
> > https://postgr.es/m/flat/86d2ceg611.fsf@jerry.enova.com
> > https://postgr.es/m/flat/74af1f60-34af-633e-ee8a-310d40c741a7%402ndquadrant.com
> 
> Yeah, I think this is clearly useful.

The proposal in this thread doesn't really allow what those mails
request. It explicitly just adds SET += to SQL, *not* to the config
file.


> ALTER SYSTEM
> shared_preload_libraries += direshark seems like a case with a lot of
> obvious practical application, and adding things to search_path seems
> compelling, too.

As far as I understand what the proposal does, if you were to do do an
ALTER SYSTEM like you do, it'd actually write an "absolute"
shared_preload_libraries value to postgresql.auto.conf. So if you then
subsequently modified the shared_preload_libraries in postgresql.conf
it'd be overwritten by the postgresql.auto.conf value, not "newly"
appended to.

Greetings,

Andres Freund

On Tue, Oct 20, 2020 at 3:24 PM Andres Freund <andres@anarazel.de> wrote:
> As far as I understand what the proposal does, if you were to do do an
> ALTER SYSTEM like you do, it'd actually write an "absolute"
> shared_preload_libraries value to postgresql.auto.conf. So if you then
> subsequently modified the shared_preload_libraries in postgresql.conf
> it'd be overwritten by the postgresql.auto.conf value, not "newly"
> appended to.

Right. So, it only really works if you either use ALTER SYSTEM for
everything or manual config file edits for everything. But, that can
still be useful.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

At 2020-10-20 10:53:04 -0700, andres@anarazel.de wrote:
>
> >     postgres=# ALTER SYSTEM SET max_worker_processes += 4;
> >     ALTER SYSTEM
> 
> Much less clear that this is a good idea...

I agree it's less clear. I still think it might be useful in some cases
(such as the example with max_worker_processes quoted above), but it's
not as compelling as altering search_path/shared_preload_libraries.

(That's partly why I posted it as a separate patch.)

> It seems to me that appending and incrementing using the same syntax
> is a) confusing b) will be a limitation before long.

I understand (a), but what sort of limitation do you foresee in (b)?

Do you think both features should be implemented, but with a different
syntax, or are you saying incrementing should not be implemented now?

> > These patches do not affect configuration file parsing in any way:
> > its use is limited to "SET" and "ALTER xxx SET".
> 
> Are you including user / database settings as part of ALTER ... SET?
> Or just SYSTEM?

Yes, it works the same for all of the ALTER … SET variants, including
users and databases.

-- Abhijit

Hi,

On 2020-10-21 10:05:40 +0530, Abhijit Menon-Sen wrote:
> > It seems to me that appending and incrementing using the same syntax
> > is a) confusing b) will be a limitation before long.
> 
> I understand (a), but what sort of limitation do you foresee in (b)?
> 
> Do you think both features should be implemented, but with a different
> syntax, or are you saying incrementing should not be implemented now?

I'm not sure, it just seems likely to me. Consider e.g. user defined
GUCs, where the type isn't yet known - just having type based dispatch
won't work well there.

For lists it also seems like you'd sometimes want prepend and sometimes
append.

After pondering this in the back of my mind for a while, I think my gut
feeling about this still is that it's not worth implementing something
that doesn't work in postgresql.conf. The likelihood of ending up with
something that makes it hard to to eventually implement proper
postgresql.conf seems high.

Greetings,

Andres Freund

At 2020-10-28 18:29:30 -0700, andres@anarazel.de wrote:
>
> I think my gut feeling about this still is that it's not worth
> implementing something that doesn't work in postgresql.conf. The
> likelihood of ending up with something that makes it hard to to
> eventually implement proper postgresql.conf seems high.

OK, thanks for explaining. That seems a reasonable concern.

I can't think of a reasonable way to accommodate the variety of possible
modifications to settings in postgresql.conf without introducing some
kind of functional syntax:

    shared_preload_libraries =
        list('newval', $shared_preload_libraries, 'otherval')

I rather doubt my ability to achieve anything resembling consensus on a
feature like that, even if it were restricted solely to list operations
on a few settings to begin with. I am also concerned that such a feature
would make it substantially harder to understand where and how the value
of a particular setting is derived (even if I do find some way to record
multiple sources in pg_settings—a problem that was brought up in earlier
discussions).

I'm certainly not in favour of introducing multiple operators to express
the various list operations, like += for prepend and =+ for append. (By
the standard of assembling such things from spare parts, the right thing
to do would be to add M4 support to postgresql.conf, but I'm not much of
a fan of that idea either.)

Therefore, for lack of any plausible way to proceed, I do not plan to
work on this feature any more.

-- Abhijit

On Tue, Dec  1, 2020 at 08:19:34PM +0530, Abhijit Menon-Sen wrote:
> I'm certainly not in favour of introducing multiple operators to express
> the various list operations, like += for prepend and =+ for append. (By
> the standard of assembling such things from spare parts, the right thing
> to do would be to add M4 support to postgresql.conf, but I'm not much of
> a fan of that idea either.)

Yeah, when M4 is your next option, you are in trouble.  ;-)

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee