RE: Support logical replication of DDLs

On Thursday, June 23, 2022 6:22 AM Zheng Li <zhengli10@gmail.com> wrote:

Hi,

> 
> > Here are some points in my mind about the two approaches discussed here.
> >
> > 1) search_patch vs schema qualify
> >
> > Again, I still think it will bring more flexibility and security by
> > schema qualify the objects in DDL command as mentioned before[1].
> 
> I wonder what security concerns you have? We certainly don't want to log the
> search_path if there are serious security issues.

I was thinking the case when the publisher has two schema "s1, s2" while
subscriber only has schema "s2". If we set publisher's search_patch to
's1, s2' and execute CREATE TABLE xxx (); If we replicate the original SQL
with search_path to subcriber, it would silently create the table on
schema s2 instead of reporting an error "schema s1 doesn't exist" which
looks dangerous to me.


> 
> > > "Create Table As .." is already handled by setting the skipData flag
> > > of the statement parsetreee before replay:
> >
> > 2) About the handling of CREATE TABLE AS:
> >
> > I think it's not a appropriate approach to set the skipdata flag on
> > subscriber as it cannot handle EXECUTE command in CTAS.
> >
> > CREATE TABLE q5_prep_results AS EXECUTE q5(200, 'DTAAAA');
> >
> > The Prepared statement is a temporary object which we don't replicate.
> > So if you directly execute the original SQL on subscriber, even if you
> > set skipdata it will fail.
> >
> > I think it difficult to make this work as you need handle the
> > create/drop of this prepared statement. And even if we extended
> > subscriber's code to make it work, it doesn't seems like a standard and
> elegant approach.
> 
> This is indeed an interesting case, thanks for pointing this out. One light weight
> solution I can think of is to directly deparse the parsetree on the publisher into
> a simple CREATE TABLE statement without the prepared statement and then
> replicate the simple CREATE TABLE statement .
> This doesn't have to involve the json format though.

I thought about this solution as well. But I am not very sure about this,
I feel it looks a bit hacky to directly do this instead of using a standard
event trigger(Or introduce a new type event trigger).


> > > "Alter Table .. " that rewrites with volatile expressions can also
> > > be handled without any syntax change, by enabling the table rewrite
> > > replication and converting the rewrite inserts to updates. ZJ's patch
> introduced this solution.
> >
> > 3) About the handling of ALTER TABLE rewrite.
> >
> > The approach I proposed before is based on the event trigger +
> > deparser approach. We were able to improve that approach as we don't
> > need to replicate the rewrite in many cases. For example: we don't
> > need to replicate rewrite dml if there is no volatile/mutable
> > function. We should check and filter these case at publisher (e.g. via
> deparser) instead of checking that at subscriber.
> 
> Surely we can make the check about volatile/mutable functions on the
> publisher side as well. It doesn't have to be done via the deparser.
> 
> > Besides, as discussed, we need to give warning or error for the cases
> > when DDL contains volatile function which would be executed[2]. We
> > should check this at publisher as well(via deparser).
> 
> Again, I think the check doesn't have to be done via the deparser.

Personally, I think it's not great to add lots of logical replication
related code(check for rewrite DDL, check for function volatility) in
utility.c or tablecmds.c which seems a bit ad-hoc.


Best regards,
Hou zj