RE: Support logical replication of DDLs

On Friday, June 3, 2022 7:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Thu, Jun 2, 2022 at 5:44 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > The main idea of replicating the CREATE TABLE AS is that we deprase
> > the CREATE TABLE AS into a simple CREATE TABLE(without subquery)
> > command and WAL log it after creating the table and before writing
> > data into the table and replicate the incoming writes later as normal
> > INSERTs. In this apporach, we don't execute the subquery on subscriber
> > so that don't need to make sure all the objects referenced in the
> > subquery also exists in subscriber. And This approach works for all
> > kind of commands(e.g. CRAETE TABLE AS [SELECT][EXECUTE][VALUES])
> >
> > One problem of this approach is that we cannot use the current trigger
> > to deparse or WAL log the CREATE TABLE. Because none of the even
> > trigger is fired after creating the table and before inserting the
> > data. To solve this, one idea is that we could directly add some code
> > at the end of create_ctas_internal() to deparse and WAL log it.
> > Moreover, we could even introduce a new type of event
> > trigger(table_create) which would be fired at the expected timing so
> > that we can use the trigger function to deparse and WAL log. I am not
> > sure which way is better.
> >
> 
> I am also not able to think of a better way to replicate CREATE TABLE AS/SELECT
> INTO other than to use a new type of event trigger. I think it is better to discuss
> this new type of event trigger in a separate thread with the use case of DDL
> replication unless we have a better idea to achieve this.
> 
> Few comments:
> ===============
> 1. Why not capture the CREATE TABLE/CREATE TABLE AS ... command with
> EventTriggerCollectSimpleCommand instead of using
> EventTriggerCreateTableStart?

After thinking more, I remove this part as it seems enough for the new type
trigger to only catch the CTAS and SELECT INTO.

> 2.
> + /*
> + * Use PG_TRY to ensure parsetree is reset even when one trigger
> + * fails. (This is perhaps not necessary, as the currentState variable
> + will
> + * be removed shortly by our caller, but it seems better to play safe.)
> + */ old_parsetree =
> + currentEventTriggerState->currentCommand->parsetree;
> + currentEventTriggerState->currentCommand->d.simple.address = address;
> + currentEventTriggerState->currentCommand->parsetree = parsetree;
> 
> Instead of doing this can't we use the parsetree stored in trigdata to deparse
> the statement?

We need to use the real createstmt which contains the actual column info. But I agree
It looks hacky and I adjusted this part in the patch.

> 3. Is there a reason to invoke the trigger after defining the relation instead of
> doing it before similar to table_rewrite trigger (EventTriggerTableRewrite).

To deparse the createstmt we need to access the catalog info about the newly
created table, so I invoke this after defining table.

> 4. It should be published for all tables publication similar to 'create table'
Agreed.

> 5. The new code in CreatePublication looks quite haphazard, can we improve it
> by moving it into separate functions?

Changed.

Thanks for the comments.

Here is the new version POC patch set for DDL replication.

For V7-0002, I tried to improve the code in publicationcmds.c by introducing a
common function to create the event trigger which can avoid some duplicate
code. Besides, I merged Ajin's DROP CASCADE patch into it. Also adjusted some
other code style.

For V7-0003, I changed the new type trigger to only catch the CREATE TABLE
AS/SELECT INTO. And rename it to "table_init_write" trigger which seems
consistent for its usage. I kept the function
EventTriggerTableInitWriteStart/End as we need these to store the original
parsetree which is needed to filter the tagname(We need to use the tag in
original parsetree to judge which command should fire the event trigger, see
function EventTriggerCommonSetup()). Besides, I also adjusted code try to make
it look less hacky.

Best regards,
Hou zj