Re: Support logical replication of DDLs

On 2/19/23 11:14 PM, Amit Kapila wrote:
> On Sun, Feb 19, 2023 at 7:50 AM Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>
>> On 2/17/23 4:15 AM, Amit Kapila wrote:
>>
>>> This happens because of the function used in the index expression.
>>> Now, this is not the only thing, the replication can even fail during
>>> DDL replication when the function like above is IMMUTABLE and used as
>>> follows: ALTER TABLE tbl ADD COLUMN d int DEFAULT t(1);
>>>
>>> Normally, it is recommended that users can fix such errors by
>>> schema-qualifying affected names. See commits 11da97024a and
>>> 582edc369c.
>>
>> I'm very familiar with those CVEs, but even though these are our
>> recommended best practices, there is still a lot of code that does not
>> schema-qualify the names of functions (including many of our own examples ;)
>>
>> If we're going to say "You can use logical replication to replicate
>> functions, but you have to ensure you've schema-qualified any function
>> calls within them," I think that will prevent people from being able to
>> use this feature, particularly on existing applications.
>>
> 
> I agree with this statement.
> 
>> I guess there's a connection I'm missing here. For the failing examples
>> above, I look at the pg_proc entries on both the publisher and the
>> subscriber and they're identical. I'm not understanding why creating and
>> executing the functions works on the publisher, but it does not on the
>> subscriber.
>>
> 
> It is because on the subscriber, in apply worker, we override the
> search path to "". See
> 
> InitializeApplyWorker()
> {
> ...
> /*
> * Set always-secure search path, so malicious users can't redirect user
> * code (e.g. pg_index.indexprs).
> */
> SetConfigOption("search_path", "", PGC_SUSET, PGC_S_OVERRIDE);
> ...
> 
> This has been done to ensure that apply worker doesn't execute
> arbitrary expressions as currently, it works with the privileges of
> the subscription owner which would be a superuser.

Got it, thanks!

>> What additional info would the subscriber need to be able to
>> successfully run these functions? Would we need to pass in some
>> additional context, e.g. what the search_path was at the time the
>> publisher created the function?
>>
> 
> Yeah, I think search_path is required. I think we need some way to
> avoid breaking what we have done in commit 11da97024a and that also
> allows us to refer to objects without schema qualification in
> functions. Will it be sane to allow specifying search_path for a
> subscription via Alter Subscription? Can we think of any other way
> here?

Presumably CREATE SUBSCRIPTION as well -- and are you thinking on one of 
the WITH options? Maybe it's also possible with a GUC on the subscriber 
side that sets a default search path to use during apply. If not set, it 
will use what 11da97024a specified. Regardless, one or both of these are 
opt-in on the subscriber, so the subscriber can make the call what level 
of permissiveness to have in the search_path.

We can then combine this with documentation, i.e. emphasize the 
importance of the best-practice to schema qualify functions. 
Additionally, we can also (strongly?) recommend for users to use SQL 
standard function bodies (BEGIN ATOMIC) for SQL-based functions.

I want to be mindful of our security recommendations the work we've done 
to harden the search_path and don't want to weaken anything there. At 
the same time, I also want to ensure we don't make it a nonstarter to 
use logical replication in the new set of use cases this and other work 
is opening up.

Thanks,

Jonathan