Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }

On Tue, 2023-09-19 at 11:41 -0400, Robert Haas wrote:
> I agree this is a mess, and that documenting the mess better would be
> good. But instead of saying not to do something, we need to say what
> will happen if you do the thing. I'm regularly annoyed when somebody
> reports that "I tried to do X and it didn't work," instead of saying
> what happened when they tried, and this situation is another form of
> the same thing. "If you do X, then Y will or can occur" is much
> better
> than "do not do X".

Good documentation includes some guidance. Sure, it should describe the
system behavior, but without anchoring it to some kind of expected use
case, it can be equally frustrating.

Allow me to pick on this example which came up in a recent thread:

"[password_required] Specifies whether connections to the publisher
made as a result of this subscription must use password authentication.
This setting is ignored when the subscription is owned by a superuser.
The default is true. Only superusers can set this value to false."
 -- https://www.postgresql.org/docs/16/sql-createsubscription.html

Only superusers can set it, and it's ignored for superusers. That does
a good job of describing the actual behavior, but is a bit puzzling.

I guess what the user is supposed to do is one of:
  1. Create a subscription as a superuser with the right connection
string (without a password) and password_required=false, then reassign
it to a non-superuser; or
  2. Create a subscription as a non-superuser member of
pg_create_subscription using a bogus connection string, then a
superuser can alter it to set password_required=false, then alter the
connection string; or
  3. Create a superuser, let the new superuser create a subscription
with password_required=false, and then remove their superuser status.

so why not just document one of those things as the expected thing to
do? Not a whole section or anything, but a sentence to suggest what
they should do or where else they should look.

I don't mean to set some major new standard in the documentation that
should apply to everything; but for the privilege system, even hackers
are having trouble keeping up (myself included). A bit of guidance
toward supported use cases helps a lot.

> I fear it will be hard to come up with something that is
> clear, that highlights the severity of the problems, and that does
> not
> veer off into useless vitriol against the status quo, but if we can
> get there, that would be good.

I hope what I'm saying is not useless vitriol. I am offering the best
solutions I see in a bad situation. And I believe I've uncovered some
emergent behaviors that are not well-understood even among prominent
hackers.

> That leads to a second idea, which is having it continue
> to be a GUC but only affect directly-entered SQL, with all
> indirectly-entered SQL either being stored as a node tree or having a
> search_path property attached somewhere.

That's not too far from the proposed patch and I'd certainly be
interested to hear more and/or adapt my patch towards this idea.

>  Or, as a third idea, suppose
> we leave it a GUC but start breaking semantics around where and how
> that GUC gets set, e.g. by changing CREATE FUNCTION to capture the
> prevailing search_path by default unless instructed otherwise.

How would one instruct otherwise?

> Personally I feel like we'd need pretty broad consensus for any of
> these kinds of changes

+1

>  because it would break a lot of stuff for a lot
> of people, but if we could get that then I think we could maybe
> emerge
> in a better spot once the pain of the compatibility break receded.

Are there ways we can soften this a bit? I know compatibility GUCs are
not to be added lightly, but perhaps one is justified here?

> Another option is something around sandboxing and/or function trust.
> The idea here is to not care too much about the search_path behavior
> itself, and instead focus on the consequences, namely what code is
> getting executed as which user and perhaps what kinds of operations
> it's performing.

I'm open to discussing that further, and it certainly may solve some
problems, but it does not seem to solve the fundamental problem with
search_path: that the caller can (intentionally or unintentionally)
cause a function to do unexpected things.

Sometimes an unexpected thing is not a the kind of thing that would be
caught by a sandbox, e.g. just an unexpected function result. But if
that function is used in a constraint or expression index, that
unexpected result can lead to a violated constraint or a bad index
(that will later cause wrong results). The owner of the table might
reasonably consider that a privilege problem, if the user who causes
the trouble had only INSERT privileges.

> Are there any other categories of things we can do? More specific
> kinds of things we can do in each category? I don't really see an
> option other than (1) "change something in the system design so that
> people use search_path wrongly less often" or (2) "make it so that it
> doesn't matter as much if people using the wrong search_path" but
> maybe I'm missing a clever idea.

Perhaps there are some clever ideas about maintaining compatibility
within the approaches (1) or (2), which might make one of them more
appealing.

Regards,
    Jeff Davis