Re: Support logical replication of DDLs

On Wed, Jun 29, 2022 at 3:25 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Wednesday, June 29, 2022 11:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 28, 2022 at 5:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > wrote:
> > >
> > > 5.
> > > +static ObjTree *
> > > +deparse_CreateStmt(Oid objectId, Node *parsetree)
> > > {
> > > ...
> > > + tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0); if
> > > + (node->tablespacename) append_string_object(tmp, "tablespace",
> > > + node->tablespacename); else { append_null_object(tmp, "tablespace");
> > > + append_bool_object(tmp, "present", false); }
> > > + append_object_object(createStmt, "tablespace", tmp);
> > > ...
> > > }
> > >
> > > Why do we need to append the objects (tablespace, with clause, etc.)
> > > when they are not present in the actual CREATE TABLE statement? The
> > > reason to ask this is that this makes the string that we want to send
> > > downstream much longer than the actual statement given by the user on
> > > the publisher.
> > >
> >
> > After thinking some more on this, it seems the user may want to optionally
> > change some of these attributes, for example, on the subscriber, it may want to
> > associate the table with a different tablespace. I think to address that we can
> > append these additional attributes optionally, say via an additional parameter
> > (append_all_options/append_all_attributes or something like that) in exposed
> > APIs like deparse_utility_command().
>
> I agree and will research this part.
>
> And here is the new version patch set.
> Most of changes are in the deparser which include:
>
> support CREATE PARTITIONED TABLE
> support ALTER ATTACH/DETACH PARTITION
> support CREATE/ALTER TABLE with ACCESS METHOD
> support CREATE TABLE OF
> support CREATE/ALTER TABLE with GENERATED COLUMN
> support CREATE/ALTER TABLE with DENTITY COLUMN
> support CREATE/ALTER TABLE with COMPRESSION METHOD
> support ALTER COLUMN numofcol SET STATISTICS (mentioned by sawada-san [1])
> support ALTER SCHEMA
> support CRAETE/DROP INDEX
>
> Note that, for ATTACH/DETACH PARTITION, I haven't added extra logic on
> subscriber to handle the case where the table on publisher is a PARTITIONED
> TABLE while the target table on subscriber side is NORMAL table. We will
> research this more and improve this later.
>
> Besides, the new version event trigger won't WAL log the DDL whose target
> table is a temporary table so that the problem reported by Vignesh[2] is
> fixed.
>
> About the recent comment from Amit[3] and Vignesh[4], I will investigate the
> comments and address them in next version.
>
> [1] https://www.postgresql.org/message-id/CAD21AoBVCoPPRKvU_5-%3DwEXsa92GsNJFJOcYyXzvoSEJCx5dKw%40mail.gmail.com
> [2] https://www.postgresql.org/message-id/CALDaNm33W35pcBE3zOpJhwnYBdBoZDpKxssemAN21NwVhJuong%40mail.gmail.com
> [3] https://www.postgresql.org/message-id/CAA4eK1K88SMoBq%3DDRA4XU-F3FG6qyzCjGMMKsPpcRBPRcrELrw%40mail.gmail.com
> [4] https://www.postgresql.org/message-id/CALDaNm3rEA_zmnDMOCT7NqK4aAffhAgooLf8rXjUN%3DYwA8ASFw%40mail.gmail.com

Thanks for the updated patch.
Few comments on 0002 patch:
1) When we create a subscription for a publication with the existing
default PUBLISH parameter having default value as
'insert,update,delete,truncate', we do an initial table sync to get
the initial table data from the publisher to the subscriber. But in
case of a publication created with 'ddl', the subscription expects the
existing initial tables present in the publisher to be created
beforehand in the subscriber. Should this be the default behavior?
Should we do a ddl dump for all the tables and restore the ddl to the
subscription while creating the subscription? Or is this planned as an
option for the later version. If we could do this as part of ddl
logical replication, it will help in reducing the steps further for
logical replication setup. If this will not be supported in this patch
series, probably we could document the behavior and add comments for
this at an appropriate place.

2) When a publication is created with ddl enabled, event triggers
will be created implicitly. Currently these event triggers are also
getting dumped. These should not be dumped. Currently while the
publication is restored, the event trigger will be created and also
the explicit event triggers which were dumped will also get created
resulting in multiple event triggers. The event trigger should not be
dumped.
@@ -4016,6 +4026,15 @@ dumpPublication(Archive *fout, const
PublicationInfo *pubinfo)
first = false;
}

+ if (pubinfo->pubddl)
+ {
+ if (!first)
+ appendPQExpBufferStr(query, ", ");
+
+ appendPQExpBufferStr(query, "ddl");
+ first = false;
+ }
+

3) SYNTAX Support:
Currently creation of "FOR TABLE" publication with ddl is supported.
Should we allow support of ddl for "FOR TABLE" publication. The
creation of the subscription will fail saying the table does not exist
while creating the subscription. Users will have to create all the
tables before creating the subscription. Is this syntax supported for
the use case where the table is altered after the subscription is
created.
ex: create publication pub3 for table t1 with ( PUBLISH= ddl);

4) Few includes can be removed:
4.a) This change is not required, it compiles ok for me without this:
diff --git a/src/backend/access/transam/rmgr.c
b/src/backend/access/transam/rmgr.c
index 8ed6924..312f117 100644
--- a/src/backend/access/transam/rmgr.c
+++ b/src/backend/access/transam/rmgr.c
@@ -27,6 +27,7 @@
#include "fmgr.h"
#include "funcapi.h"
#include "miscadmin.h"
+#include "replication/ddlmessage.h"
#include "replication/decode.h"
#include "replication/message.h"

4.b) This change is not required, it compiles ok for me without this change:
diff --git a/src/backend/utils/adt/ri_triggers.c
b/src/backend/utils/adt/ri_triggers.c
index 51b3fdc..7d60aac 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -33,6 +33,7 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_type.h"
#include "commands/trigger.h"
+#include "commands/event_trigger.h"
#include "executor/executor.h"
#include "executor/spi.h"
#include "lib/ilist.h"
@@ -40,6 +41,7 @@
#include "parser/parse_coerce.h"
#include "parser/parse_relation.h"
#include "storage/bufmgr.h"
+#include "tcop/ddl_deparse.h"

5) The changes are not required for the patch, it can be removed:
5.a) This empty line can be removed
@@ -2153,6 +2329,7 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
entry->pubactions.pubupdate |=
pub->pubactions.pubupdate;
entry->pubactions.pubdelete |=
pub->pubactions.pubdelete;
entry->pubactions.pubtruncate |=
pub->pubactions.pubtruncate;
+ entry->pubactions.pubddl |=
pub->pubactions.pubddl;

/*
* We want to publish the changes as
the top-most ancestor
@@ -2338,6 +2515,7 @@ rel_sync_cache_publication_cb(Datum arg, int
cacheid, uint32 hashvalue)
{
entry->replicate_valid = false;
}
+
}

5.b) This change is done by mistake, this change can be removed:
@@ -4282,8 +4391,7 @@ ReorderBufferRestoreChange(ReorderBuffer *rb,
ReorderBufferTXN *txn,
/* read prefix */
memcpy(&prefix_size, data, sizeof(Size));
data += sizeof(Size);
- change->data.msg.prefix =
MemoryContextAlloc(rb->context,
-
prefix_size);
+ change->data.msg.prefix =
MemoryContextAlloc(rb->context, prefix_size);

5.c) This change is done by mistake, this change can be removed:
diff --git a/src/backend/replication/logical/proto.c
b/src/backend/replication/logical/proto.c
index ff8513e..eaec031 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -640,8 +640,8 @@ logicalrep_read_truncate(StringInfo in,
*/
void
logicalrep_write_message(StringInfo out, TransactionId xid, XLogRecPtr lsn,
- bool transactional,
const char *prefix, Size sz,
- const char *message)
+ bool transactional,
const char *prefix,
+ Size sz, const char *message)

Regards,
Vignesh