Hi, I've spent a bit of time looking at this patch. It seems there's a clear consensus that having "owned schemas" for extensions would be good for security. To me it also seems as a convenient way to organize stuff. It was possible to create extensions in a separate schema before, ofc, but that's up to the DBA. With this the extension author to enforce that.
One thing that's not quite clear to me is what's the correct way for existing extensions to switch to an "owned schema". Let's say you have an extension. How do you transition to this? Can you just add it to the control file and then some magic happens? A couple minor comments: 1) doc/src/sgml/extend.sgml An extension is <firstterm>owned_schema</firstterm> if it requires a new dedicated schema for its objects. Such a requirement can make security concerns related to <literal>search_path</literal> injection much easier to reason about. The default is <literal>false</literal>, i.e., the extension can be installed into an existing schema. Doesn't "extension is owned_schema" sound a bit weird? I'd probably say "extension may own a schema" or something like that. Also, "requires a new dedicated schema" is a bit ambiguous. It's not clear if it means the schema is expected to exist, or if it creates the schema itself. And perhaps it should clarify what "much easier to reason about" means. That's pretty vague, and as a random extension author I wouldn't know about the risks to consider. Maybe there's a section about this stuff that we could reference? 2) doc/src/sgml/ref/create_extension.sgml relocated. The named schema must already exist if the extension's control file does not specify <literal>owned_schema</literal>. Seems a bit unclear, I'd say having "owned_schema = false" in the control file still qualifies as "specifies owned_schema". So might be better to say it needs to be set to true? Also, perhaps "dedicated_schema" would be better than "owned_schema"? I mean, the point is not that it's "owned" by the extension, but that there's nothing else in it. But that's nitpicking. 3) src/backend/commands/extension.c I'm not sure why AlterExtensionNamespace moves the privilege check. Why should it not check the privilege for owned schema too? 4) src/bin/pg_dump/pg_dump.c checkExtensionMembership has typo "owned_schem". Shouldn't binary_upgrade_extension_member still set ext=NULL in the for loop, the way the original code does that? The long if conditions might need some indentation, I guess. pgindent leaves them like this, but 100 columns seems a bit too much. I'd do a line break after each condition, I guess. regards -- Tomas Vondra
