Re: Support logical replication of DDLs

Hello,

One of the most frequently requested improvements from our customers
is to reduce downtime associated with software updates (both major and
minor versions). To do this, we have reviewed potential contributions to
improving logical replication.

I’m working on a patch to support logical replication of data
definition language statements (DDLs). This is a useful feature when a
database in logical replication has lots of tables, functions and
other objects that change over time, such as in online cross major
version upgrade.

I put together a prototype that replicates DDLs using the generic
messages for logical decoding. The idea is to log the candidate DDL
string in ProcessUtilitySlow() using LogLogicalMessge() with a new
flag in WAL record type xl_logical_message indicating it’s a DDL
message. The xl_logical_message record is decoded and sent to the
subscriber via pgoutput. The logical replication worker process is
dispatched for this new DDL message type and executes the command
accordingly.

However, there are still many edge cases to sort out because not every
DDL statement can/should be replicated. Some of these include:

1. DDL involving multiple tables where only some tables are replicated, e.g.

DROP TABLE replicated_foo, notreplicated_bar;

This statement will fail on the subscriber and block logical
replication. It can be detected and filtered on the publisher.

2. Any DDL that calls a volatile function, such as NOW() or RAND(), is
likely to generate a different value on each replica. It is possible
to work around these issues—for example, the publisher can replace any
volatile function calls with a fixed return value when the statement
is logged so that the subscribers all get the same value. We will have
to consider some other cases.

3. CREATE TABLE AS and SELECT INTO, For example:

CREATE TABLE foo AS
SELECT field_1, field_2 FROM bar;

There are a few issues that can occur here. For one, it’s possible
that table bar doesn't exist on the subscriber. Even if “bar” does
exist, it may not be fully up-to-date with the publisher, which would
cause a data mismatch on “foo” between the publisher and subscriber.

4. Statements that have nondeterministic side effects (e.g., as caused
by triggers, stored procedures, user-defined functions) may result in
different side effects occurring on each subscriber.

Whether a DDL should be replicated also depends on what granularity do
we define DDL replication. For example, we can define DDL replication
on these levels:

1. Database level
Allows all DDLs for a database to be replicated except for certain
edge cases (refer to the edge cases mentioned above).
This is likely a major use case, such as in online major version upgrade.

2. Table level
Allows DDLs on the published tables to be replicated except for
certain edge cases.
This is useful for consolidating multiple databases into a single one,
e.g. for analytics.

3. Other fine-grained levels base on the object type such as index,
function, procedure and view etc.
Allows DDLs on certain object types to be replicated. At the moment
I’m unsure of a use case for this.

To implement such DDL replication levels, we need to modify the CREATE
PUBLICATION syntax. For example, to help starting the discussion on
the granularity of DDL replication, we can add a new option list ‘ddl’
in the WITH definition:

CREATE PUBLICATION mypub FOR ALL TABLES with
(publish = ‘insert, update, delete, truncate’, ddl = ‘database’)

CREATE PUBLICATION mypub FOR TABLE T1, T2 with
(publish = ‘insert, update, delete, truncate’, ddl = ‘table’)

CREATE PUBLICATION mypub FOR TABLE T1, T2 with
(publish = ‘insert, update, delete, truncate’, ddl = ‘table, function,
procedure’)

We can probably make “ddl = ‘database’” valid only for the FOR ALL
TABLES publication, because it doesn’t really make sense to replicate
all DDLs for a database when only a subset of tables are being
replicated (which can cause edge case 1 to occur frequently). “ddl =
‘database’” + FOR ALL TABLES is likely where logical replication of
DDL is most useful, i.e. for online major versions upgrades.

Based on the DDL publication levels we can further implement the logic
to conditionally log the DDL commands or to conditionally decode/ship
the logical DDL message.

Thoughts? Your feedback is appreciated.

Thanks,
Zheng Li
Amazon RDS/Aurora for PostgreSQL

Hi Zheng,

> I’m working on a patch to support logical replication of data
> definition language statements (DDLs).

That's great!

> However, there are still many edge cases to sort out because not every
> DDL statement can/should be replicated.

Maybe the first implementation shouldn't be perfect as long as known
limitations are documented and the future improvements are unlikely to
break anything for the users. Committing an MVP and iterating on this is
much simpler in terms of development and code review. Also, it will deliver
value to the users and give us feedback sooner.

> 1. DDL involving multiple tables where only some tables are replicated,
e.g.
>
> DROP TABLE replicated_foo, notreplicated_bar;
>

I would add DROP TABLE ... CASCADE to the list. Also, let's not forget that
PostgreSQL supports table inheritance and table partitioning.

> 2. Any DDL that calls a volatile function, such as NOW() or RAND(), is
> likely to generate a different value on each replica. It is possible
> to work around these issues—for example, the publisher can replace any
> volatile function calls with a fixed return value when the statement
> is logged so that the subscribers all get the same value. We will have
> to consider some other cases.

That would make sense.

> [...]
> Whether a DDL should be replicated also depends on what granularity do
> we define DDL replication. For example, we can define DDL replication
> on these levels:
>
> 1. Database level
> Allows all DDLs for a database to be replicated except for certain
> edge cases (refer to the edge cases mentioned above).
> This is likely a major use case, such as in online major version upgrade.

To my knowledge, this is not a primary use case for logical replication.
Also, I suspect that implementing it may be a bit challenging. What if we
focus on table-level replication for now?

--
Best regards,
Aleksander Alekseev

Em seg., 21 de fev. de 2022 às 13:13, Zheng Li <zhengli10(at)gmail(dot)com>
escreveu:

>
> 2. Table level
> Allows DDLs on the published tables to be replicated except for
> certain edge cases.
>
> Think how to handle triggers and functions with same name but different
purpose.

Publisher
create function public.audit() returns trigger language plpgsql as $$
begin
new.Audit_User = current_user();
new.Audit_Date_Time = now();
return new;
end;$$
create trigger audit before insert or update on foo for each row execute
procedure public.audit();

Subscriber
create function public.audit() returns trigger language plpgsql as $$
begin
insert into Audit(Audit_Date_Time, Audit_User, Schema_Name, Table_Name,
Audit_Action, Field_Values) values(new.Audit_ts, new.Audit_User,
tg_table_schema, tg_table_name, tg_op, row_to_json(case when tg_op =
'DELETE' then old.* else new.* end));
return null;
end;$$
create trigger audit after insert or update or delete on foo for each row
execute procedure public.audit();

regards,
Marcos

Hi Aleksander,

>That's great!
Thanks!

>Maybe the first implementation shouldn't be perfect as long as known
>limitations are documented and the future improvements are unlikely to
>break anything for the users. Committing an MVP and iterating on this is
>much simpler in terms of development and code review. Also, it will deliver
>value to the users and give us feedback sooner.

Agree, I’ll post the prototype when it’s in a bit better shape. Also
I’d like to hear
some more feedback from the community on whether I’m heading the right
direction.

>I would add DROP TABLE ... CASCADE to the list. Also, let's not forget that
>PostgreSQL supports table inheritance and table partitioning.

Thanks, I will keep this in mind.

>> 1. Database level
>> Allows all DDLs for a database to be replicated except for certain
>> edge cases (refer to the edge cases mentioned above).
>> This is likely a major use case, such as in online major version upgrade.
>To my knowledge, this is not a primary use case for logical replication.
>Also, I suspect that implementing it may be a bit challenging. What if we
>focus on table-level replication for now?

I think it is due to the fact that the current limitations in logical
replication are
holding it back in major version upgrade (MVU). Online / reduced downtime MVU
is a popular request from customers, and why we should enhance logical
replication
to support this use case.

Also I think table-level DDL replication is actually more challenging,
especially in
the FOR TABLE case, due to the fact that differences are expected to
occur between the
source and target database. Marcos’ comment also justifies the complexity
in this case. Whereas database-level DDL replication in the FOR ALL
TABLE case is
relatively simple because the source and target database are (almost) identical.

Regards,
Zheng

Hi Zheng,

> >Also, I suspect that implementing it may be a bit challenging. What if we
> >focus on table-level replication for now?
>
> I think it is due to the fact that the current limitations in logical
> replication are
> holding it back in major version upgrade (MVU). Online / reduced downtime MVU
> is a popular request from customers, and why we should enhance logical
> replication
> to support this use case.
>
> Also I think table-level DDL replication is actually more challenging,
> especially in
> the FOR TABLE case, due to the fact that differences are expected to
> occur between the
> source and target database. Marcos’ comment also justifies the complexity
> in this case. Whereas database-level DDL replication in the FOR ALL
> TABLE case is
> relatively simple because the source and target database are (almost) identical.

Sure, I don't insist on implementing table-level replication first.
It's up to you. My point was that it's not necessary to implement
everything at once.

--
Best regards,
Aleksander Alekseev

On Mon, Feb 21, 2022 at 9:43 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> Hello,
>
> One of the most frequently requested improvements from our customers
> is to reduce downtime associated with software updates (both major and
> minor versions). To do this, we have reviewed potential contributions to
> improving logical replication.
>
> I’m working on a patch to support logical replication of data
> definition language statements (DDLs). This is a useful feature when a
> database in logical replication has lots of tables, functions and
> other objects that change over time, such as in online cross major
> version upgrade.

+1

> I put together a prototype that replicates DDLs using the generic
> messages for logical decoding. The idea is to log the candidate DDL
> string in ProcessUtilitySlow() using LogLogicalMessge() with a new
> flag in WAL record type xl_logical_message indicating it’s a DDL
> message. The xl_logical_message record is decoded and sent to the
> subscriber via pgoutput. The logical replication worker process is
> dispatched for this new DDL message type and executes the command
> accordingly.

If you don't mind, would you like to share the POC or the branch for this work?

> However, there are still many edge cases to sort out because not every
> DDL statement can/should be replicated. Some of these include:

> 3. CREATE TABLE AS and SELECT INTO, For example:
>
> CREATE TABLE foo AS
> SELECT field_1, field_2 FROM bar;
>
> There are a few issues that can occur here. For one, it’s possible
> that table bar doesn't exist on the subscriber. Even if “bar” does
> exist, it may not be fully up-to-date with the publisher, which would
> cause a data mismatch on “foo” between the publisher and subscriber.

In such cases why don't we just log the table creation WAL for DDL
instead of a complete statement which creates the table and inserts
the tuple? Because we are already WAL logging individual inserts and
once you make sure of replicating the table creation I think the exact
data insertion on the subscriber side will be taken care of by the
insert WALs no?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

+

On Sun, Mar 13, 2022 at 5:05 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Feb 21, 2022 at 9:43 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> >
> > Hello,
> >
> > One of the most frequently requested improvements from our customers
> > is to reduce downtime associated with software updates (both major and
> > minor versions). To do this, we have reviewed potential contributions to
> > improving logical replication.
> >
> > I’m working on a patch to support logical replication of data
> > definition language statements (DDLs). This is a useful feature when a
> > database in logical replication has lots of tables, functions and
> > other objects that change over time, such as in online cross major
> > version upgrade.
>
> +1
+1

> > I put together a prototype that replicates DDLs using the generic
> > messages for logical decoding. The idea is to log the candidate DDL
> > string in ProcessUtilitySlow() using LogLogicalMessge() with a new
> > flag in WAL record type xl_logical_message indicating it’s a DDL
> > message. The xl_logical_message record is decoded and sent to the
> > subscriber via pgoutput. The logical replication worker process is
> > dispatched for this new DDL message type and executes the command
> > accordingly.
>
> If you don't mind, would you like to share the POC or the branch for this work?
I would love to try this patch out, would you like to share branch or POC ?

Hi,

>If you don't mind, would you like to share the POC or the branch for this work?

The POC patch is attached. It currently supports the following functionalities:
1. Configure either database level or table level DDL replication via
the CREATE PUBLICATION command.

2.Supports replication of DDL of the following types when database
level DDL replication is turned on. Other less common DDL types could
be added later.
T_CreateSchemaStmt
T_CreateStmt
T_CreateForeignTableStmt
T_AlterDomainStmt
T_DefineStmt
T_CompositeTypeStmt
T_CreateEnumStmt
T_CreateRangeStmt
T_AlterEnumStmt
T_ViewStmt
T_CreateFunctionStmt
T_AlterFunctionStmt
T_CreateTrigStmt
T_CreateDomainStmt
T_CreateCastStmt
T_CreateOpClassStmt
T_CreateOpFamilyStmt
T_AlterOpFamilyStmt
T_AlterOperatorStmt
T_AlterTypeStmt
T_GrantStmt
T_AlterCollationStmt
T_AlterTableStmt
T_IndexStmt

3.Supports replication of DDLs of the following types if only table
level DDL replication is turned on.
T_AlterTableStmt
T_IndexStmt

4.Supports seamless DML replication of new tables created via DDL
replication without having to manually running “ALTER SUBSCRIPTION ...
REFRESH PUBLICATION" on the subscriber.

Here is a demo:
source_db=# create publication mypub FOR ALL TABLES with (ddl = ‘database’);

target_db=# create subscription mysub CONNECTION 'dbname=source_db
host=localhost user=myuser port=5432' PUBLICATION mypub;

source_db=#
BEGIN;
CREATE TABLE foo (a int);
CREATE INDEX foo_idx ON foo (a);
ALTER TABLE foo ADD COLUMN b timestamptz;
CREATE FUNCTION foo_ts()
RETURNS trigger AS $$
BEGIN
NEW.b := current_timestamp;
RETURN NEW;
END;
$$
LANGUAGE plpgsql;

CREATE TRIGGER foo_ts BEFORE INSERT OR UPDATE ON foo
FOR EACH ROW EXECUTE FUNCTION foo_ts();
INSERT INTO foo VALUES (1);
COMMIT;

source_db=# select * from foo;
a | b
---+-------------------------------
1 | 2022-03-15 19:07:53.005215+00
(1 row)

target_db=# select * from foo;
a | b
---+-------------------------------
1 | 2022-03-15 19:07:53.005215+00
(1 row)

Here is the remaining work for this patch:

1. Test and handle corner case DDLs, such as the ones listed in my
first email[1].

2. Investigate less common DDL types (categorized in function
LogLogicalDDLCommand) and decide if and under what conditions they
should be replicated.

3. Determine if we want to include table-level DDL replication. If so,
we need to continue to enhance and test this feature.

I think this will be ready to consider for commit once 1 and 2 are
resolved. I think 3 can be done in the second phase.

If you are reviewing the code, I’ve provided a detailed description of
the patch below:
1. Syntax, pg_publication and pg_dump change:
Allows the user to configure either database level or table level DDL
replication via the CREATE PUBLICATION command as proposed in my first
email. Two new columns are added to the pg_publication catalog to show
the DDL replication levels, test output publication.out is updated
accordingly. pg_dump is also modified to accommodate the
pg_publication catalog change.

2. Logical logging change
a. A new WAL record type xl_logical_ddl_message is introduced to
support logical logging of DDL command. xl_logical_ddl_message is
similar to the existing xl_logical_message for generic message
logging. The reason for not using xl_logical_message directly as
proposed initially is I found out we need to log more information
(such as user role, search path and potentially more in the future)
than just one string, and we don’t want to make too much changes to
the existing xl_logical_message which may break its current consumers.
b. The logging of DDL command string is processed in function
LogLogicalDDLCommand. We categorize DDL command types into three
categories in this function:
1. replicated in database level replication only (such as CREATE
TABLE, CREATE FUNCTION).
2. replicated in database or table level replication depending on the
configuration (such as ALTER TABLE).
3. not supported for replication or pending investigation.

3. Logical decoding and Reorderbuffer change
Supports logical decoding of the new WAL record
xl_logical_ddl_message. This is similar to the logical decoding of
xl_logical_message. Tests for this change are added in the
test_decoding plugin.

4. Integration with pgoutput
Supports sending and receiving the DDL message using the logical
replication wire protocol. A new LogicalRepMsgType is introduced for
this purpose: LOGICAL_REP_MSG_DDLMESSAGE = 'L'.

5. Logical replication worker change
Supports execution of the DDL command in the original user role and
search_path. For any new table created this way, we also set its
srsubstate in the pg_subscription_rel catalog to SUBREL_STATE_INIT, So
that DML replication can progress on this new table without manually
running "ALTER SUBSCRIPTION ... REFRESH PUBLICATION".

6. TAP test
A new TAP test 030_rep_ddl.pl is added. We mainly focused on testing
the happy path of database level replication so far. Corner case DDLs
and table level DDL replication are still to be carefully tested.

>> However, there are still many edge cases to sort out because not every
>> DDL statement can/should be replicated. Some of these include:

>> 3. CREATE TABLE AS and SELECT INTO, For example:
>>
>> CREATE TABLE foo AS
>> SELECT field_1, field_2 FROM bar;
>>
>> There are a few issues that can occur here. For one, it’s possible
>> that table bar doesn't exist on the subscriber. Even if “bar” does
>> exist, it may not be fully up-to-date with the publisher, which would
>> cause a data mismatch on “foo” between the publisher and subscriber.

>In such cases why don't we just log the table creation WAL for DDL
>instead of a complete statement which creates the table and inserts
>the tuple? Because we are already WAL logging individual inserts and
>once you make sure of replicating the table creation I think the exact
>data insertion on the subscriber side will be taken care of by the
>insert WALs no?

The table creation WAL and table insert WAL are available. The tricky
part is how do we break down this command into two parts (a normal
CREATE TABLE followed by insertions) either from the parsetree or the
WALs. I’ll have to dig more on this.

I look forward to your feedback.

Regards,
Zheng

[1] https://www.postgresql.org/message-id/CAAD30U+pVmfKwUKy8cbZOnUXyguJ-uBNejwD75Kyo=OjdQGJ9g@mail.gmail.com

On Sun, Mar 13, 2022 at 7:35 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Feb 21, 2022 at 9:43 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> >
> > Hello,
> >
> > One of the most frequently requested improvements from our customers
> > is to reduce downtime associated with software updates (both major and
> > minor versions). To do this, we have reviewed potential contributions to
> > improving logical replication.
> >
> > I’m working on a patch to support logical replication of data
> > definition language statements (DDLs). This is a useful feature when a
> > database in logical replication has lots of tables, functions and
> > other objects that change over time, such as in online cross major
> > version upgrade.
>
> +1
>
> > I put together a prototype that replicates DDLs using the generic
> > messages for logical decoding. The idea is to log the candidate DDL
> > string in ProcessUtilitySlow() using LogLogicalMessge() with a new
> > flag in WAL record type xl_logical_message indicating it’s a DDL
> > message. The xl_logical_message record is decoded and sent to the
> > subscriber via pgoutput. The logical replication worker process is
> > dispatched for this new DDL message type and executes the command
> > accordingly.
>
> If you don't mind, would you like to share the POC or the branch for this work?
>
> > However, there are still many edge cases to sort out because not every
> > DDL statement can/should be replicated. Some of these include:
>
> > 3. CREATE TABLE AS and SELECT INTO, For example:
> >
> > CREATE TABLE foo AS
> > SELECT field_1, field_2 FROM bar;
> >
> > There are a few issues that can occur here. For one, it’s possible
> > that table bar doesn't exist on the subscriber. Even if “bar” does
> > exist, it may not be fully up-to-date with the publisher, which would
> > cause a data mismatch on “foo” between the publisher and subscriber.
>
> In such cases why don't we just log the table creation WAL for DDL
> instead of a complete statement which creates the table and inserts
> the tuple? Because we are already WAL logging individual inserts and
> once you make sure of replicating the table creation I think the exact
> data insertion on the subscriber side will be taken care of by the
> insert WALs no?
>
>
> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com

Hello

I think this is a pretty interesting and useful feature.

Did you see some old code I wrote towards this goal?
https://www.postgresql.org/message-id/[email protected]
The intention was that DDL would produce some JSON blob that accurately
describes the DDL that was run; the caller can acquire that and use it
to produce working DDL that doesn't depend on runtime conditions. There
was lots of discussion on doing things this way. It was ultimately
abandoned, but I think it's valuable.

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?" (Mafalda)

Hi, Zhang Li

On Thu, 17 Mar 2022 at 05:17, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> Hi,
>
>>If you don't mind, would you like to share the POC or the branch for this work?
>
> The POC patch is attached. It currently supports the following functionalities:
> 1. Configure either database level or table level DDL replication via
> the CREATE PUBLICATION command.
>
> 2.Supports replication of DDL of the following types when database
> level DDL replication is turned on. Other less common DDL types could
> be added later.
> T_CreateSchemaStmt
> T_CreateStmt
> T_CreateForeignTableStmt
> T_AlterDomainStmt
> T_DefineStmt
> T_CompositeTypeStmt
> T_CreateEnumStmt
> T_CreateRangeStmt
> T_AlterEnumStmt
> T_ViewStmt
> T_CreateFunctionStmt
> T_AlterFunctionStmt
> T_CreateTrigStmt
> T_CreateDomainStmt
> T_CreateCastStmt
> T_CreateOpClassStmt
> T_CreateOpFamilyStmt
> T_AlterOpFamilyStmt
> T_AlterOperatorStmt
> T_AlterTypeStmt
> T_GrantStmt
> T_AlterCollationStmt
> T_AlterTableStmt
> T_IndexStmt
>
> 3.Supports replication of DDLs of the following types if only table
> level DDL replication is turned on.
> T_AlterTableStmt
> T_IndexStmt
>
> 4.Supports seamless DML replication of new tables created via DDL
> replication without having to manually running “ALTER SUBSCRIPTION ...
> REFRESH PUBLICATION" on the subscriber.
>

Great! I think we can split the patch into several pieces (at least
implementation and test cases) for easier review.

+ * Simiarl to the generic logical messages, These DDL messages can be either
+ * transactional or non-transactional. Note by default DDLs in PostgreSQL are
+ * transactional.

There is a typo, s/Simiarl/Similar/.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, 17 Mar 2022 at 05:17, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> Hi,
>
>>If you don't mind, would you like to share the POC or the branch for this work?
>
> The POC patch is attached. It currently supports the following functionalities:

Hi,

When I try to run regression test, there has some errors.

make[2]: Entering directory '/home/px/Codes/postgresql/Debug/src/test/subscription'
rm -rf '/home/px/Codes/postgresql/Debug/src/test/subscription'/tmp_check && /usr/bin/mkdir -p '/home/px/Codes/postgresql/Debug/src/test/subscription'/tmp_check && cd /home/px/Codes/postgresql/Debug/../src
/test/subscription && TESTDIR='/home/px/Codes/postgresql/Debug/src/test/subscription' PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/bin:/home/px/Codes/postgresql/Debu
g/src/test/subscription:$PATH" LD_LIBRARY_PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/lib:$LD_LIBRARY_PATH" PGPORT='65432' PG_REGRESS='/home/px/Codes/postgresql/De
bug/src/test/subscription/../../../src/test/regress/pg_regress' /usr/bin/prove -I /home/px/Codes/postgresql/Debug/../src/test/perl/ -I /home/px/Codes/postgresql/Debug/../src/test/subscription t/*.pl
t/001_rep_changes.pl ............... ok
t/002_types.pl ..................... ok
t/003_constraints.pl ............... ok
t/004_sync.pl ...................... 6/? # Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 29 just after 7.
t/004_sync.pl ...................... Dubious, test returned 29 (wstat 7424, 0x1d00)
All 7 subtests passed
t/005_encoding.pl .................. ok
t/006_rewrite.pl ................... 1/? # poll_query_until timed out executing this query:
# SELECT '0/14A8648' <= replay_lsn AND state = 'streaming' FROM pg_catalog.pg_stat_replication WHERE application_name = 'mysub';
# expecting this output:
# t
# last actual query output:
#
# with stderr:
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 29 just after 1.
t/006_rewrite.pl ................... Dubious, test returned 29 (wstat 7424, 0x1d00)
All 1 subtests passed
t/007_ddl.pl ....................... ok
t/008_diff_schema.pl ............... 1/? # Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 29 just after 4.
t/008_diff_schema.pl ............... Dubious, test returned 29 (wstat 7424, 0x1d00)
All 4 subtests passed
t/009_matviews.pl .................. Dubious, test returned 29 (wstat 7424, 0x1d00)
No subtests run

The new patch change the behavior of publication, when we drop a table
on publisher, the table also be dropped on subscriber, and this made the
regression testa failed, since the regression test will try to drop the
table on subscriber.

IMO, we can disable the DDLs replication by default, which makes the
regression test happy. Any thoughts?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Hello Alvaro,

> I think this is a pretty interesting and useful feature.
>
> Did you see some old code I wrote towards this goal?
> https://www.postgresql.org/message-id/[email protected]
> The intention was that DDL would produce some JSON blob that accurately
> describes the DDL that was run; the caller can acquire that and use it
> to produce working DDL that doesn't depend on runtime conditions. There
> was lots of discussion on doing things this way. It was ultimately
> abandoned, but I think it's valuable.

Thanks for pointing this out. I'm looking into it.

Hello Japin,
>The new patch change the behavior of publication, when we drop a table
>on publisher, the table also be dropped on subscriber, and this made the
>regression testa failed, since the regression test will try to drop the
>table on subscriber.

>IMO, we can disable the DDLs replication by default, which makes the
>regression test happy. Any thoughts?

Good catch, I forgot to run the whole TAP test suite. I think for now
we can simply disable DDL replication for the failing tests when
creating publication: $node_publisher->safe_psql('postgres',
"CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
I'll fix it in the next version of patch. I'll also work on breaking
down the patch into smaller pieces for ease of review.

Regards,
Zheng

Hello,

Attached please find the broken down patch set. Also fixed the failing
TAP tests Japin reported.

Regards,
Zheng Li
Amazon RDS/Aurora for PostgreSQL

On Fri, 18 Mar 2022 at 00:22, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> Hello Japin,
>>The new patch change the behavior of publication, when we drop a table
>>on publisher, the table also be dropped on subscriber, and this made the
>>regression testa failed, since the regression test will try to drop the
>>table on subscriber.
>
>>IMO, we can disable the DDLs replication by default, which makes the
>>regression test happy. Any thoughts?
>
> Good catch, I forgot to run the whole TAP test suite. I think for now
> we can simply disable DDL replication for the failing tests when
> creating publication: $node_publisher->safe_psql('postgres',
> "CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
> I'll fix it in the next version of patch. I'll also work on breaking
> down the patch into smaller pieces for ease of review.
>

Oh, it doesn't work.

postgres=# CREATE PUBLICATION s1 FOR ALL TABLES WITH(ddl = '');;
ERROR: conflicting or redundant options

Here is the code, I think, you might mean `if (ddl_level_given == NULL)`.

+ if (*ddl_level_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options")));
+
+ /*
+ * If publish option was given only the explicitly listed actions
+ * should be published.
+ */
+ pubactions->pubddl_database = false;
+ pubactions->pubddl_table = false;
+
+ *ddl_level_given = true;

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Fri, 18 Mar 2022 at 08:18, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> Hello,
>
> Attached please find the broken down patch set. Also fixed the failing
> TAP tests Japin reported.
>

Thanks for updating the patchset, I will try it later.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Fri, 18 Mar 2022 at 08:22, Japin Li <japinli(at)hotmail(dot)com> wrote:
> On Fri, 18 Mar 2022 at 00:22, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>> Hello Japin,
>>>The new patch change the behavior of publication, when we drop a table
>>>on publisher, the table also be dropped on subscriber, and this made the
>>>regression testa failed, since the regression test will try to drop the
>>>table on subscriber.
>>
>>>IMO, we can disable the DDLs replication by default, which makes the
>>>regression test happy. Any thoughts?
>>
>> Good catch, I forgot to run the whole TAP test suite. I think for now
>> we can simply disable DDL replication for the failing tests when
>> creating publication: $node_publisher->safe_psql('postgres',
>> "CREATE PUBLICATION tap_pub FOR ALL TABLES WITH (ddl='')");
>> I'll fix it in the next version of patch. I'll also work on breaking
>> down the patch into smaller pieces for ease of review.
>>
>
> Oh, it doesn't work.
>
> postgres=# CREATE PUBLICATION s1 FOR ALL TABLES WITH(ddl = '');;
> ERROR: conflicting or redundant options
>
>
> Here is the code, I think, you might mean `if (ddl_level_given == NULL)`.
>
> + if (*ddl_level_given)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("conflicting or redundant options")));
> +
> + /*
> + * If publish option was given only the explicitly listed actions
> + * should be published.
> + */
> + pubactions->pubddl_database = false;
> + pubactions->pubddl_table = false;
> +
> + *ddl_level_given = true;

Oh, Sorry, I misunderstand it, it just like you forget initialize
*ddl_level_given to false when enter parse_publication_options().

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Fri, 18 Mar 2022 at 08:18, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> Hello,
>
> Attached please find the broken down patch set. Also fixed the failing
> TAP tests Japin reported.
>

Here are some comments for the new patches:

Seems like you forget initializing the *ddl_level_given after entering the
parse_publication_options(), see [1].

+ if (*ddl_level_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or redundant options")));

We can use the errorConflictingDefElem() to replace the ereport() to make the
error message more readable.

I also think that ddl = '' isn't a good way to disable DDL replication, how
about using ddl = 'none' or something else?

The test_decoding test case cannot work as expected, see below:

diff -U3 /home/px/Codes/postgresql/contrib/test_decoding/expected/ddlmessages.out /home/px/Codes/postgresql/Debug/contrib/test_decoding/results/ddlmessages.out
--- /home/px/Codes/postgresql/contrib/test_decoding/expected/ddlmessages.out 2022-03-18 08:46:57.653922671 +0800
+++ /home/px/Codes/postgresql/Debug/contrib/test_decoding/results/ddlmessages.out 2022-03-18 17:34:33.411563601 +0800
@@ -25,8 +25,8 @@
SELECT pg_drop_replication_slot('regression_slot');
DROP TABLE tab1;
DROP publication mypub;
- data
----------------------------------------------------------------------------------------------------------------------------------------------------
+ data
+-----------------------------------------------------------------------------------------------------------------------------------------------
DDL message: transactional: 1 prefix: role: redacted, search_path: "$user", public, sz: 47 content:CREATE TABLE tab1 (id serial unique, data int);
BEGIN
sequence public.tab1_id_seq: transactional:1 last_value: 1 log_cnt: 0 is_called:0

Since the DDL message contains the username, and we try to replace the username with 'redacted' to avoid this problem,

\o | sed 's/role.*search_path/role: redacted, search_path/g'

However, the title and dash lines may have different lengths for different
usernames. To avoid this problem, how about using a specified username when
initializing the database for this regression test?

I try to disable the ddlmessage regression test,

make[2]: Entering directory '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'
rm -rf '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'/tmp_check && /usr/bin/mkdir -p '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'/tmp_check && cd /home/px/Codes/postgresql/Debug/../src/bin/pg_dump
&& TESTDIR='/home/px/Codes/postgresql/Debug/src/bin/pg_dump' PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/bin:/home/px/Codes/postgresql/Debug/src/bin/pg_dump:$PATH"
LD_LIBRARY_PATH="/home/px/Codes/postgresql/Debug/tmp_install/home/px/Codes/postgresql/Debug/pg/lib:$LD_LIBRARY_PATH" PGPORT='65432' PG_REGRESS='/home/px/Codes/postgresql/Debug/src/bin/pg_dump/../../../s
rc/test/regress/pg_regress' /usr/bin/prove -I /home/px/Codes/postgresql/Debug/../src/test/perl/ -I /home/px/Codes/postgresql/Debug/../src/bin/pg_dump t/*.pl
t/001_basic.pl ................ ok
t/002_pg_dump.pl .............. 13/?
# Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub1'
# at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI

# Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub2'
# at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI

# Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub3'
# at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI

# Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub4'
# at t/002_pg_dump.pl line 3916.
# Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI
t/002_pg_dump.pl .............. 240/?
[...]
# Review section_post_data results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI
t/002_pg_dump.pl .............. 7258/? # Looks like you failed 84 tests of 7709.
t/002_pg_dump.pl .............. Dubious, test returned 84 (wstat 21504, 0x5400)
Failed 84/7709 subtests
t/003_pg_dump_with_server.pl .. ok
t/010_dump_connstr.pl ......... ok

Test Summary Report
-------------------
t/002_pg_dump.pl (Wstat: 21504 Tests: 7709 Failed: 84)
Failed tests: 136-139, 362-365, 588-591, 1040-1043, 1492-1495
1719-1722, 1946-1949, 2177-2180, 2407-2410
2633-2636, 2859-2862, 3085-3088, 3537-3540
3763-3766, 3989-3992, 4215-4218, 4441-4444
5119-5122, 5345-5348, 6702-6705, 7154-7157
Non-zero exit status: 84
Files=4, Tests=7808, 26 wallclock secs ( 0.46 usr 0.05 sys + 7.98 cusr 2.80 csys = 11.29 CPU)
Result: FAIL
make[2]: *** [Makefile:55: check] Error 1
make[2]: Leaving directory '/home/px/Codes/postgresql/Debug/src/bin/pg_dump'
make[1]: *** [Makefile:43: check-pg_dump-recurse] Error 2
make[1]: Leaving directory '/home/px/Codes/postgresql/Debug/src/bin'
make: *** [GNUmakefile:71: check-world-src/bin-recurse] Error 2

And, when I try to use git am to apply the patch, it complains:

$ git am ~/0001-syntax-pg_publication-pg_dump-ddl_replication.patch
Patch format detection failed.

[1] https://www.postgresql.org/message-id/MEYP282MB1669DDF788C623B7F8B64C2EB6139%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Hello,

Thanks for the quick review!

> And, when I try to use git am to apply the patch, it complains:
>
> $ git am ~/0001-syntax-pg_publication-pg_dump-ddl_replication.patch
> Patch format detection failed.

git apply works for me. I'm not sure why git am complains.
I also created a git branch to make code sharing easier, please try this out:
https://github.com/zli236/postgres/tree/ddl_replication

> Seems like you forget initializing the *ddl_level_given after entering the
> parse_publication_options(), see [1].
>
>
> + if (*ddl_level_given)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("conflicting or redundant options")));
>
> We can use the errorConflictingDefElem() to replace the ereport() to make the
> error message more readable.

Agreed. Fixed in the latest version.

> I also think that ddl = '' isn't a good way to disable DDL replication, how
> about using ddl = 'none' or something else?

The syntax follows the existing convention of the WITH publish option.
For example in CREATE PUBLICATION mypub WITH (publish = '')
it means don't publish any DML change. So I'd leave it as is for now.

> The test_decoding test case cannot work as expected, see below:
.....
> DDL message: transactional: 1 prefix: role: redacted, search_path: "$user", public, sz: 47 content:CREATE TABLE tab1 (id serial unique, data int);
> BEGIN
> sequence public.tab1_id_seq: transactional:1 last_value: 1 log_cnt: 0 is_called:0
>
> Since the DDL message contains the username, and we try to replace the username with 'redacted' to avoid this problem,
>
> \o | sed 's/role.*search_path/role: redacted, search_path/g'
>
> However, the title and dash lines may have different lengths for different
> usernames. To avoid this problem, how about using a specified username when
> initializing the database for this regression test?

I don't understand the question, do you have an example of when the
test doesn't work? It runs fine for me.

> t/002_pg_dump.pl .............. 13/?
> # Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub1'
> # at t/002_pg_dump.pl line 3916.
> # Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI
......
> Failed 84/7709 subtests
> t/003_pg_dump_with_server.pl .. ok
> t/010_dump_connstr.pl ......... ok
>
> Test Summary Report
> -------------------
> t/002_pg_dump.pl (Wstat: 21504 Tests: 7709 Failed: 84)

This is fixed in the latest version. I need to remind myself to run
make check-world in the future.

Regards,
Zheng Li

git branch of the change:
https://github.com/zli236/postgres/tree/ddl_replication

On Sat, 19 Mar 2022 at 01:25, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> Hello,
>
> Thanks for the quick review!
>
>> And, when I try to use git am to apply the patch, it complains:
>>
>> $ git am ~/0001-syntax-pg_publication-pg_dump-ddl_replication.patch
>> Patch format detection failed.
>
> git apply works for me. I'm not sure why git am complains.
> I also created a git branch to make code sharing easier, please try this out:
> https://github.com/zli236/postgres/tree/ddl_replication
>

Yeah, I can use git apply, I'm not sure how did you generate the patch.

>> I also think that ddl = '' isn't a good way to disable DDL replication, how
>> about using ddl = 'none' or something else?
>
> The syntax follows the existing convention of the WITH publish option.
> For example in CREATE PUBLICATION mypub WITH (publish = '')
> it means don't publish any DML change. So I'd leave it as is for now.
>

Agreed.

>> The test_decoding test case cannot work as expected, see below:
> .....
>> DDL message: transactional: 1 prefix: role: redacted, search_path: "$user", public, sz: 47 content:CREATE TABLE tab1 (id serial unique, data int);
>> BEGIN
>> sequence public.tab1_id_seq: transactional:1 last_value: 1 log_cnt: 0 is_called:0
>>
>> Since the DDL message contains the username, and we try to replace the username with 'redacted' to avoid this problem,
>>
>> \o | sed 's/role.*search_path/role: redacted, search_path/g'
>>
>> However, the title and dash lines may have different lengths for different
>> usernames. To avoid this problem, how about using a specified username when
>> initializing the database for this regression test?
>
> I don't understand the question, do you have an example of when the
> test doesn't work? It runs fine for me.
>

You should use a different user that has different length from your current one.
For example:

px(at)localhost$ make check-world

>
>> t/002_pg_dump.pl .............. 13/?
>> # Failed test 'binary_upgrade: should dump CREATE PUBLICATION pub1'
>> # at t/002_pg_dump.pl line 3916.
>> # Review binary_upgrade results in /home/px/Codes/postgresql/Debug/src/bin/pg_dump/tmp_check/tmp_test_jgRI
> ......
>> Failed 84/7709 subtests
>> t/003_pg_dump_with_server.pl .. ok
>> t/010_dump_connstr.pl ......... ok
>>
>> Test Summary Report
>> -------------------
>> t/002_pg_dump.pl (Wstat: 21504 Tests: 7709 Failed: 84)
>
> This is fixed in the latest version. I need to remind myself to run
> make check-world in the future.
>

Thanks for updating the patch.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Thu, Mar 17, 2022 at 2:47 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> Hi,
>
> >If you don't mind, would you like to share the POC or the branch for this work?
>
> The POC patch is attached. It currently supports the following functionalities:

Thanks for sharing, I will look into it.

> >In such cases why don't we just log the table creation WAL for DDL
> >instead of a complete statement which creates the table and inserts
> >the tuple? Because we are already WAL logging individual inserts and
> >once you make sure of replicating the table creation I think the exact
> >data insertion on the subscriber side will be taken care of by the
> >insert WALs no?
>
> The table creation WAL and table insert WAL are available. The tricky
> part is how do we break down this command into two parts (a normal
> CREATE TABLE followed by insertions) either from the parsetree or the
> WALs. I’ll have to dig more on this.

I agree that this is a bit tricky, anyway I will also put more thoughts on this.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Hi Japin,

> You should use a different user that has different length from your current one.
> For example:
>
> px(at)localhost$ make check-world

This is fixed in the latest commit:
https://github.com/zli236/postgres/commits/ddl_replication

Thanks,
Zheng

On Tue, 22 Mar 2022 at 04:56, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> Hi Japin,
>
>> You should use a different user that has different length from your current one.
>> For example:
>>
>> px(at)localhost$ make check-world
>
> This is fixed in the latest commit:
> https://github.com/zli236/postgres/commits/ddl_replication
>

Thanks for fixing this. I rebase the patchset on current master (383f222119)
and attach here for further review.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Mar 21, 2022 at 1:43 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Mar 17, 2022 at 2:47 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > >If you don't mind, would you like to share the POC or the branch for this work?
> >
> > The POC patch is attached. It currently supports the following functionalities:
>
> Thanks for sharing, I will look into it.
>
> > >In such cases why don't we just log the table creation WAL for DDL
> > >instead of a complete statement which creates the table and inserts
> > >the tuple? Because we are already WAL logging individual inserts and
> > >once you make sure of replicating the table creation I think the exact
> > >data insertion on the subscriber side will be taken care of by the
> > >insert WALs no?
> >
> > The table creation WAL and table insert WAL are available. The tricky
> > part is how do we break down this command into two parts (a normal
> > CREATE TABLE followed by insertions) either from the parsetree or the
> > WALs. I’ll have to dig more on this.
>
> I agree that this is a bit tricky, anyway I will also put more thoughts on this.

I had put some more thought about this, basically, during CTAS we are
generating the CreateStmt inside "create_ctas_internal" and executing
it first before inserting the tuple, so can't we generate the
independent sql just for creating the tuple maybe using deparsing or
something?

Apart from that I have one more question, basically if you are
directly logging the sql query then how you are identifying under
which schema you need to create that table, are you changing the sql
and generating schema-qualified name?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Mar 24, 2022 at 3:32 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Mar 21, 2022 at 1:43 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 17, 2022 at 2:47 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > >
> > > Hi,
> > >
> > > >If you don't mind, would you like to share the POC or the branch for this work?
> > >
> > > The POC patch is attached. It currently supports the following functionalities:
> >
> > Thanks for sharing, I will look into it.
> >
> > > >In such cases why don't we just log the table creation WAL for DDL
> > > >instead of a complete statement which creates the table and inserts
> > > >the tuple? Because we are already WAL logging individual inserts and
> > > >once you make sure of replicating the table creation I think the exact
> > > >data insertion on the subscriber side will be taken care of by the
> > > >insert WALs no?
> > >
> > > The table creation WAL and table insert WAL are available. The tricky
> > > part is how do we break down this command into two parts (a normal
> > > CREATE TABLE followed by insertions) either from the parsetree or the
> > > WALs. I’ll have to dig more on this.
> >
> > I agree that this is a bit tricky, anyway I will also put more thoughts on this.
>
> I had put some more thought about this, basically, during CTAS we are
> generating the CreateStmt inside "create_ctas_internal" and executing
> it first before inserting the tuple, so can't we generate the
> independent sql just for creating the tuple maybe using deparsing or
> something?
>
> Apart from that I have one more question, basically if you are
> directly logging the sql query then how you are identifying under
> which schema you need to create that table, are you changing the sql
> and generating schema-qualified name?

I was going through the patch and it seems you are logging the search
path as well along with the query so I think this will probably work.
I have got one more query while looking into the code. In the below
code snippet you are logging DDL command only if it is a top level
query but there are no comments explaining what sort of queries we
don't want to log. Suppose I am executing a DDL statement inside a PL
then that will not be a top level statement so is your intention to
block that as well or that is an unintentional side effect?

+ /*
+ * Consider logging the DDL command if logical logging is
enabled and this is
+ * a top level query.
+ */
+ if (XLogLogicalInfoActive() && isTopLevel)
+ LogLogicalDDLCommand(parsetree, queryString);
+

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Hi Dilip,

Thanks for the feedback.

> > > > The table creation WAL and table insert WAL are available. The tricky
> > > > part is how do we break down this command into two parts (a normal
> > > > CREATE TABLE followed by insertions) either from the parsetree or the
> > > > WALs. I’ll have to dig more on this.

> > I had put some more thought about this, basically, during CTAS we are
> > generating the CreateStmt inside "create_ctas_internal" and executing
> > it first before inserting the tuple, so can't we generate the
> > independent sql just for creating the tuple maybe using deparsing or
> > something?

Yes, deparsing might help for edge cases like this. However I found
a simple solution for this specific case:

The idea is to force skipping any direct data population (which can
potentially cause data inconsistency on the subscriber)
in CREATE AS and SELECT INTO command on the subscriber by forcing the
skipData flag in the intoClause of the parsetree after
the logical replication worker parses the command. The data sync will
be taken care of by the DML replication after the DDL replication
finishes.

This is implemented in the latest commit:
https://github.com/zli236/postgres/commit/116c33451da8d44577b8d6fdb05c4b6998cd0167

> > Apart from that I have one more question, basically if you are
> > directly logging the sql query then how you are identifying under
> > which schema you need to create that table, are you changing the sql
> > and generating schema-qualified name?
>
> I was going through the patch and it seems you are logging the search
> path as well along with the query so I think this will probably work.

Yes, currently we log the search path as well as the user name. And we
enforce the same search path and user name when applying the DDL command
on the subscriber.

> I have got one more query while looking into the code. In the below
> code snippet you are logging DDL command only if it is a top level
> query but there are no comments explaining what sort of queries we
> don't want to log. Suppose I am executing a DDL statement inside a PL
> then that will not be a top level statement so is your intention to
> block that as well or that is an unintentional side effect?
>
> + /*
> + * Consider logging the DDL command if logical logging is
> enabled and this is
> + * a top level query.
> + */
> + if (XLogLogicalInfoActive() && isTopLevel)
> + LogLogicalDDLCommand(parsetree, queryString);

Good catch. The reason for having isTopLevel in the condition is
because I haven't decided if a DDL statement inside a PL should
be replicated from the user point of view. For example, if I execute a
plpgsql function or a stored procedure which creates a table under the hood,
does it always make sense to replicate the DDL without running the same
function or stored procedure on the subscriber? It probably depends on
the specific
use case. Maybe we can consider making this behavior configurable by the user.

Thanks,
Zheng

On Thu, Mar 24, 2022 at 11:24 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> Hi Dilip,
>
> Thanks for the feedback.
>
> > > > > The table creation WAL and table insert WAL are available. The tricky
> > > > > part is how do we break down this command into two parts (a normal
> > > > > CREATE TABLE followed by insertions) either from the parsetree or the
> > > > > WALs. I’ll have to dig more on this.
>
> > > I had put some more thought about this, basically, during CTAS we are
> > > generating the CreateStmt inside "create_ctas_internal" and executing
> > > it first before inserting the tuple, so can't we generate the
> > > independent sql just for creating the tuple maybe using deparsing or
> > > something?
>
> Yes, deparsing might help for edge cases like this. However I found
> a simple solution for this specific case:
>
> The idea is to force skipping any direct data population (which can
> potentially cause data inconsistency on the subscriber)
> in CREATE AS and SELECT INTO command on the subscriber by forcing the
> skipData flag in the intoClause of the parsetree after
> the logical replication worker parses the command. The data sync will
> be taken care of by the DML replication after the DDL replication
> finishes.

Okay, something like that should work, I am not sure it is the best
design though.

> This is implemented in the latest commit:
> https://github.com/zli236/postgres/commit/116c33451da8d44577b8d6fdb05c4b6998cd0167
>
> > > Apart from that I have one more question, basically if you are
> > > directly logging the sql query then how you are identifying under
> > > which schema you need to create that table, are you changing the sql
> > > and generating schema-qualified name?
> >
> > I was going through the patch and it seems you are logging the search
> > path as well along with the query so I think this will probably work.
>
> Yes, currently we log the search path as well as the user name. And we
> enforce the same search path and user name when applying the DDL command
> on the subscriber.

Yeah this looks fine to me.

>
> > I have got one more query while looking into the code. In the below
> > code snippet you are logging DDL command only if it is a top level
> > query but there are no comments explaining what sort of queries we
> > don't want to log. Suppose I am executing a DDL statement inside a PL
> > then that will not be a top level statement so is your intention to
> > block that as well or that is an unintentional side effect?
> >
> > + /*
> > + * Consider logging the DDL command if logical logging is
> > enabled and this is
> > + * a top level query.
> > + */
> > + if (XLogLogicalInfoActive() && isTopLevel)
> > + LogLogicalDDLCommand(parsetree, queryString);
>
> Good catch. The reason for having isTopLevel in the condition is
> because I haven't decided if a DDL statement inside a PL should
> be replicated from the user point of view. For example, if I execute a
> plpgsql function or a stored procedure which creates a table under the hood,
> does it always make sense to replicate the DDL without running the same
> function or stored procedure on the subscriber? It probably depends on
> the specific
> use case. Maybe we can consider making this behavior configurable by the user.

But then this could be true for DML as well right? Like after
replicating the function to the subscriber if we are sending the DML
done by function then what's the problem in DDL. I mean if there is
no design issue in implementing this then I don't think there is much
point in blocking the same or even providing configuration for this.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Mar 23, 2022 at 4:09 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
>
> On Tue, 22 Mar 2022 at 04:56, Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > Hi Japin,
> >
> >> You should use a different user that has different length from your current one.
> >> For example:
> >>
> >> px(at)localhost$ make check-world
> >
> > This is fixed in the latest commit:
> > https://github.com/zli236/postgres/commits/ddl_replication
> >
>
> Thanks for fixing this. I rebase the patchset on current master (383f222119)
> and attach here for further review.
>

The patch no longer applies. The patch is a very good attempt, and I
would also like to contribute if required.
I have a few comments but will hold it till a rebased version is available.

regards,
Ajin Cherian
Fujitsu Australia

On Wed, Mar 23, 2022 at 10:39 AM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
> Thanks for fixing this. I rebase the patchset on current master (383f222119)
> and attach here for further review.
>

Some initial comments:
===================
1.
+/*
+ * Write logical decoding DDL message into XLog.
+ */
+XLogRecPtr
+LogLogicalDDLMessage(const char *prefix, Oid roleoid, const char *message,
+ size_t size, bool transactional)

I don't see anywhere the patch using a non-transactional message. Is
it required for any future usage? The non-transactional behavior has
some known consistency issues, see email [1].

2. For DDL replication, do we need to wait for a consistent point of
snapshot? For DMLs, that point is a convenient point to initialize
replication from, which is why we export a snapshot at that point,
which is used to read normal data. Do we have any similar needs for
DDL replication?

3. The patch seems to be creating an entry in pg_subscription_rel for
'create' message. Do we need some handling on Drop, if not, why? I
think adding some comments for this aspect would make it easier to
follow.

4. The handling related to partition tables seems missing because, on
the subscriber-side, it always creates a relation entry in
pg_subscription_rel which won't work. Check its interaction with
publish_via_partition_root.

[1] - https://www.postgresql.org/message-id/CAA4eK1KAFdQEULk%2B4C%3DieWA5UPSUtf_gtqKsFj9J9f2c%3D8hm4g%40mail.gmail.com

--
With Regards,
Amit Kapila.

On Tue, Mar 29, 2022 at 9:47 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Thu, Mar 24, 2022 at 11:24 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> >
> >
> > Good catch. The reason for having isTopLevel in the condition is
> > because I haven't decided if a DDL statement inside a PL should
> > be replicated from the user point of view. For example, if I execute a
> > plpgsql function or a stored procedure which creates a table under the hood,
> > does it always make sense to replicate the DDL without running the same
> > function or stored procedure on the subscriber? It probably depends on
> > the specific
> > use case. Maybe we can consider making this behavior configurable by the user.
>
> But then this could be true for DML as well right? Like after
> replicating the function to the subscriber if we are sending the DML
> done by function then what's the problem in DDL. I mean if there is
> no design issue in implementing this then I don't think there is much
> point in blocking the same or even providing configuration for this.
>

Valid point. I think if there are some design/implementation
constraints for this then it is better to document those(probably as
comments).

--
With Regards,
Amit Kapila.

On Tue, Mar 29, 2022 at 9:47 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > The idea is to force skipping any direct data population (which can
> > potentially cause data inconsistency on the subscriber)
> > in CREATE AS and SELECT INTO command on the subscriber by forcing the
> > skipData flag in the intoClause of the parsetree after
> > the logical replication worker parses the command. The data sync will
> > be taken care of by the DML replication after the DDL replication
> > finishes.
>
> Okay, something like that should work, I am not sure it is the best
> design though.
>

Even if this works, how will we make Alter Table statement work where
it needs to rewrite the table? There also I think we can face a
similar problem if we directly send the statement, once the table will
be updated due to the DDL statement and then again due to table
rewrite as that will have a separate WAL.

Another somewhat unrelated problem I see with this work is how to save
recursion of the same command between nodes (when the involved nodes
replicate DDLs). For DMLs, we can avoid that via replication origins
as is being done in the patch proposed [1] but not sure how will we
deal with that here?

[1] - https://commitfest.postgresql.org/38/3610/

--
With Regards,
Amit Kapila.

On Thu, Mar 17, 2022 at 3:36 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> Did you see some old code I wrote towards this goal?
> https://www.postgresql.org/message-id/[email protected]
> The intention was that DDL would produce some JSON blob that accurately
> describes the DDL that was run;
>

I have read that thread and found one of your emails [1] where you
seem to be saying that JSON representation is not required for BDR.
Will in some way going via JSON blob way make this project
easier/better?

> the caller can acquire that and use it
> to produce working DDL that doesn't depend on runtime conditions.
>

For runtime conditions, one of the things you have mentioned in that
thread is to add schema name in the statement at the required places
which this patch deals with in a different way by explicitly sending
it along with the DDL statement. The other cases where we might need
deparsing are Alter Table type cases (where we need to rewrite the
table) where we may want to send a different DDL. I haven't analyzed
but I think it is better to have a list where all we need deparsing
and what is the best way to deal with it. The simpler cases seem to be
working with the approach proposed by this patch but I am not sure if
it will work for all kinds of cases.

[1] - https://www.postgresql.org/message-id/20150504185721.GB2523%40alvh.no-ip.org

--
With Regards,
Amit Kapila.

On 2022-Apr-08, Amit Kapila wrote:

> On Thu, Mar 17, 2022 at 3:36 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > Did you see some old code I wrote towards this goal?
> > https://www.postgresql.org/message-id/[email protected]
> > The intention was that DDL would produce some JSON blob that accurately
> > describes the DDL that was run;
>
> I have read that thread and found one of your emails [1] where you
> seem to be saying that JSON representation is not required for BDR.
> Will in some way going via JSON blob way make this project
> easier/better?

I don't know if replication support will be easier by using JSON; I just
think that JSON makes the overall feature more easily usable for other
purposes.

I am not familiar with BDR replication nowadays.

> For runtime conditions, one of the things you have mentioned in that
> thread is to add schema name in the statement at the required places
> which this patch deals with in a different way by explicitly sending
> it along with the DDL statement.

Hmm, ok. The point of the JSON-blob route is that the publisher sends a
command representation that can be parsed/processed/transformed
arbitrarily by the subscriber using generic rules; it should be trivial
to use a JSON tool to change schema A to schema B in any arbitrary DDL
command, and produce another working DDL command without having to know
how to write that command specifically. So if I have a rule that
"schema A there is schema B here", all DDL commands can be replayed with
no further coding (without having to rely on getting the run-time
search_path correct.)

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"El sudor es la mejor cura para un pensamiento enfermo" (Bardia)

On Fri, Apr 8, 2022 at 7:34 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > For runtime conditions, one of the things you have mentioned in that
> > thread is to add schema name in the statement at the required places
> > which this patch deals with in a different way by explicitly sending
> > it along with the DDL statement.
>
> Hmm, ok. The point of the JSON-blob route is that the publisher sends a
> command representation that can be parsed/processed/transformed
> arbitrarily by the subscriber using generic rules; it should be trivial
> to use a JSON tool to change schema A to schema B in any arbitrary DDL
> command, and produce another working DDL command without having to know
> how to write that command specifically. So if I have a rule that
> "schema A there is schema B here", all DDL commands can be replayed with
> no further coding (without having to rely on getting the run-time
> search_path correct.)

Yeah, that was a really nice aspect of that approach.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Apr 7, 2022 at 3:46 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Mar 23, 2022 at 10:39 AM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
> 2. For DDL replication, do we need to wait for a consistent point of
> snapshot? For DMLs, that point is a convenient point to initialize
> replication from, which is why we export a snapshot at that point,
> which is used to read normal data. Do we have any similar needs for
> DDL replication?
>

I have thought a bit more about this and I think we need to build the
snapshot for DML replication as we need to read catalog tables to
decode the corresponding WAL but it is not clear to me if we have a
similar requirement for DDL replication. If the catalog access is
required then it makes sense to follow the current snapshot model,
otherwise, we may need to think differently for DDL replication.

One more related point is that for DML replication, we do ensure that
we copy the entire data of the table (via initial sync) which exists
even before the publication for that table exists, so do we want to do
something similar for DDLs? How do we sync the schema of the table
before the user has defined the publication? Say the table has been
created before the publication is defined and after that, there are
only Alter statements, so do we expect, users to create the table on
the subscriber and then we can replicate the Alter statements? And
even if we do that it won't be clear which Alter statements will be
replicated after publication is defined especially if those Alters
happened concurrently with defining publications?

--
With Regards,
Amit Kapila.

On Fri, Apr 8, 2022 at 5:04 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-Apr-08, Amit Kapila wrote:
>
>
> > For runtime conditions, one of the things you have mentioned in that
> > thread is to add schema name in the statement at the required places
> > which this patch deals with in a different way by explicitly sending
> > it along with the DDL statement.
>
> Hmm, ok. The point of the JSON-blob route is that the publisher sends a
> command representation that can be parsed/processed/transformed
> arbitrarily by the subscriber using generic rules; it should be trivial
> to use a JSON tool to change schema A to schema B in any arbitrary DDL
> command, and produce another working DDL command without having to know
> how to write that command specifically. So if I have a rule that
> "schema A there is schema B here", all DDL commands can be replayed with
> no further coding (without having to rely on getting the run-time
> search_path correct.)
>

Okay, thanks for the clarification.

--
With Regards,
Amit Kapila.

On Mon, Apr 11, 2022, at 2:00 AM, Amit Kapila wrote:
> On Thu, Apr 7, 2022 at 3:46 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Mar 23, 2022 at 10:39 AM Japin Li <japinli(at)hotmail(dot)com> wrote:
> >
> > 2. For DDL replication, do we need to wait for a consistent point of
> > snapshot? For DMLs, that point is a convenient point to initialize
> > replication from, which is why we export a snapshot at that point,
> > which is used to read normal data. Do we have any similar needs for
> > DDL replication?
> >
>
> I have thought a bit more about this and I think we need to build the
> snapshot for DML replication as we need to read catalog tables to
> decode the corresponding WAL but it is not clear to me if we have a
> similar requirement for DDL replication. If the catalog access is
> required then it makes sense to follow the current snapshot model,
> otherwise, we may need to think differently for DDL replication.
>
> One more related point is that for DML replication, we do ensure that
> we copy the entire data of the table (via initial sync) which exists
> even before the publication for that table exists, so do we want to do
> something similar for DDLs? How do we sync the schema of the table
> before the user has defined the publication? Say the table has been
> created before the publication is defined and after that, there are
> only Alter statements, so do we expect, users to create the table on
> the subscriber and then we can replicate the Alter statements? And
> even if we do that it won't be clear which Alter statements will be
> replicated after publication is defined especially if those Alters
> happened concurrently with defining publications?
The *initial* DDL replication is a different problem than DDL replication. The
former requires a snapshot to read the current catalog data and build a CREATE
command as part of the subscription process. The subsequent DDLs in that object
will be handled by a different approach that is being discussed here.

I'm planning to work on the initial DDL replication. I'll open a new thread as
soon as I write a design for it. Just as an example, the pglogical approach is
to use pg_dump behind the scenes to provide the schema [1]. It is a reasonable
approach but an optimal solution should be an API to provide the initial DDL
commands. I mean the main point of this feature is to have an API to create an
object that the logical replication can use it for initial schema
synchronization. This "DDL to create an object" was already discussed in the
past [2].

[1] https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_sync.c#L942
[2] https://www.postgresql.org/message-id/4E69156E.5060509%40dunslane.net

--
Euler Taveira
EDB https://www.enterprisedb.com/

Hi,

> > Good catch. The reason for having isTopLevel in the condition is
> > because I haven't decided if a DDL statement inside a PL should
> > be replicated from the user point of view. For example, if I execute a
> > plpgsql function or a stored procedure which creates a table under the hood,
> > does it always make sense to replicate the DDL without running the same
> > function or stored procedure on the subscriber? It probably depends on
> > the specific
> > use case. Maybe we can consider making this behavior configurable by the user.
>
> But then this could be true for DML as well right? Like after
> replicating the function to the subscriber if we are sending the DML
> done by function then what's the problem in DDL. I mean if there is
> no design issue in implementing this then I don't think there is much
> point in blocking the same or even providing configuration for this.

Agreed. I'll unblock DDLs in functions/procedures and test it out. I
find out some DDLs in functions are replicated multiple times on the
subscriber while they should only be replicated once. Still trying to
figure out why.

>The patch no longer applies. The patch is a very good attempt, and I
>would also like to contribute if required.
>I have a few comments but will hold it till a rebased version is available.

Hi, Ajin. That'll be great! I'll rebase my github branch to master
head. In the meantime, this github branch still works
https://github.com/zli236/postgres/tree/ddl_replication

Regards,
Zheng

Hi Amit,

> Some initial comments:
> ===================
> 1.
> +/*
> + * Write logical decoding DDL message into XLog.
> + */
> +XLogRecPtr
> +LogLogicalDDLMessage(const char *prefix, Oid roleoid, const char *message,
> + size_t size, bool transactional)
>
> I don't see anywhere the patch using a non-transactional message. Is
> it required for any future usage? The non-transactional behavior has
> some known consistency issues, see email [1].

The transactional flag is not required by the current usage. I thought
it might be useful if other logical decoding plugins want to log and
consume DDL messages in a non-transactional way. But I don't have a
specific use case yet.

> 2. For DDL replication, do we need to wait for a consistent point of
> snapshot? For DMLs, that point is a convenient point to initialize
> replication from, which is why we export a snapshot at that point,
> which is used to read normal data. Do we have any similar needs for
> DDL replication?

The current design requires manual schema initialization on the
subscriber before the logical replication setup.
As Euler Taveira pointed out, snapshot is needed in initial schema
synchronization. And that is a different topic.

> 3. The patch seems to be creating an entry in pg_subscription_rel for
> 'create' message. Do we need some handling on Drop, if not, why? I
> think adding some comments for this aspect would make it easier to
> follow.

It's already handled by existing logic in heap_drop_with_catalog:
https://github.com/zli236/postgres/blob/ddl_replication/src/backend/catalog/heap.c#L2005
I'll add some comment.

> 4. The handling related to partition tables seems missing because, on
> the subscriber-side, it always creates a relation entry in
> pg_subscription_rel which won't work. Check its interaction with
> publish_via_partition_root.

I will test it out.

>Even if this works, how will we make Alter Table statement work where
>it needs to rewrite the table? There also I think we can face a
>similar problem if we directly send the statement, once the table will
>be updated due to the DDL statement and then again due to table
>rewrite as that will have a separate WAL.

Yes, I think any DDL that can generate DML changes should be listed
out and handled properly or documented. Here is one extreme example
involving volatile functions:
ALTER TABLE nd_ddl ADD COLUMN t timestamp DEFAULT now().
Again, I think we need to somehow skip the data rewrite on the
subscriber when replicating such DDL and let DML replication handle
the rewrite.

>Another somewhat unrelated problem I see with this work is how to save
>recursion of the same command between nodes (when the involved nodes
>replicate DDLs). For DMLs, we can avoid that via replication origins
>as is being done in the patch proposed [1] but not sure how will we
>deal with that here?

I'll need to investigate "recursion of the same command between
nodes", could you provide an example?

Regards,
Zheng

> I'm planning to work on the initial DDL replication. I'll open a new thread as
> soon as I write a design for it. Just as an example, the pglogical approach is
> to use pg_dump behind the scenes to provide the schema [1]. It is a reasonable
> approach but an optimal solution should be an API to provide the initial DDL
> commands. I mean the main point of this feature is to have an API to create an
> object that the logical replication can use it for initial schema
> synchronization. This "DDL to create an object" was already discussed in the
> past [2].

Nice! I think automatic initial schema synchronization for replication
is a very useful feature.

Regards,
Zheng

On Mon, Apr 11, 2022 at 6:16 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Mon, Apr 11, 2022, at 2:00 AM, Amit Kapila wrote:
>
> On Thu, Apr 7, 2022 at 3:46 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Mar 23, 2022 at 10:39 AM Japin Li <japinli(at)hotmail(dot)com> wrote:
> >
> > 2. For DDL replication, do we need to wait for a consistent point of
> > snapshot? For DMLs, that point is a convenient point to initialize
> > replication from, which is why we export a snapshot at that point,
> > which is used to read normal data. Do we have any similar needs for
> > DDL replication?
> >
>
> I have thought a bit more about this and I think we need to build the
> snapshot for DML replication as we need to read catalog tables to
> decode the corresponding WAL but it is not clear to me if we have a
> similar requirement for DDL replication. If the catalog access is
> required then it makes sense to follow the current snapshot model,
> otherwise, we may need to think differently for DDL replication.
>
> One more related point is that for DML replication, we do ensure that
> we copy the entire data of the table (via initial sync) which exists
> even before the publication for that table exists, so do we want to do
> something similar for DDLs? How do we sync the schema of the table
> before the user has defined the publication? Say the table has been
> created before the publication is defined and after that, there are
> only Alter statements, so do we expect, users to create the table on
> the subscriber and then we can replicate the Alter statements? And
> even if we do that it won't be clear which Alter statements will be
> replicated after publication is defined especially if those Alters
> happened concurrently with defining publications?
>
> The *initial* DDL replication is a different problem than DDL replication. The
> former requires a snapshot to read the current catalog data and build a CREATE
> command as part of the subscription process. The subsequent DDLs in that object
> will be handled by a different approach that is being discussed here.
>

I think they are not completely independent because of the current way
to do initial sync followed by replication. The initial sync and
replication need some mechanism to ensure that one of those doesn't
overwrite the work done by the other. Now, the initial idea and patch
can be developed separately but I think both the patches have some
dependency.

--
With Regards,
Amit Kapila.

On Mon, Apr 11, 2022 at 11:01 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> >Even if this works, how will we make Alter Table statement work where
> >it needs to rewrite the table? There also I think we can face a
> >similar problem if we directly send the statement, once the table will
> >be updated due to the DDL statement and then again due to table
> >rewrite as that will have a separate WAL.
>
> Yes, I think any DDL that can generate DML changes should be listed
> out and handled properly or documented. Here is one extreme example
> involving volatile functions:
> ALTER TABLE nd_ddl ADD COLUMN t timestamp DEFAULT now().
> Again, I think we need to somehow skip the data rewrite on the
> subscriber when replicating such DDL and let DML replication handle
> the rewrite.
>

I am not sure what is the right way to deal with this but another idea
we can investigate is to probably rewrite the DDL such that it doesn't
rewrite the table.

> >Another somewhat unrelated problem I see with this work is how to save
> >recursion of the same command between nodes (when the involved nodes
> >replicate DDLs). For DMLs, we can avoid that via replication origins
> >as is being done in the patch proposed [1] but not sure how will we
> >deal with that here?
>
> I'll need to investigate "recursion of the same command between
> nodes", could you provide an example?
>

See email [1]. I think the same solution should work for your proposal
as well because I see that LogLogicalMessage includes origin but it is
better if you can confirm it.

[1] - https://www.postgresql.org/message-id/flat/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9=9orqubhjcQ(at)mail(dot)gmail(dot)com

--
With Regards,
Amit Kapila.

Hi,

Here is the rebased new branch
https://github.com/zli236/postgres/commits/ddl_replication

Regards,
Zheng

On Wed, Apr 13, 2022 at 5:49 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> Hi,
>
> Here is the rebased new branch
> https://github.com/zli236/postgres/commits/ddl_replication
>

Thanks, but it would be good if you can share it in the patch form as
well unless you need to send patches too frequently. It is easier to
give comments.

--
With Regards,
Amit Kapila.

On Tue, Apr 12, 2022 at 4:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> > The *initial* DDL replication is a different problem than DDL replication. The
> > former requires a snapshot to read the current catalog data and build a CREATE
> > command as part of the subscription process. The subsequent DDLs in that object
> > will be handled by a different approach that is being discussed here.
> >
>
> I think they are not completely independent because of the current way
> to do initial sync followed by replication. The initial sync and
> replication need some mechanism to ensure that one of those doesn't
> overwrite the work done by the other. Now, the initial idea and patch
> can be developed separately but I think both the patches have some
> dependency.

I agree with the point that their design can not be completely
independent. They have some logical relationship of what schema will
be copied by the initial sync and where is the exact boundary from
which we will start sending as replication. And suppose first we only
plan to implement the replication part then how the user will know
what all schema user has to create and what will be replicated using
DDL replication? Suppose the user takes a dump and copies all the
schema and then creates the subscription, then how we are we going to
handle the DDL concurrent to the subscription command?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 13, 2022 at 2:38 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Apr 12, 2022 at 4:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > > The *initial* DDL replication is a different problem than DDL replication. The
> > > former requires a snapshot to read the current catalog data and build a CREATE
> > > command as part of the subscription process. The subsequent DDLs in that object
> > > will be handled by a different approach that is being discussed here.
> > >
> >
> > I think they are not completely independent because of the current way
> > to do initial sync followed by replication. The initial sync and
> > replication need some mechanism to ensure that one of those doesn't
> > overwrite the work done by the other. Now, the initial idea and patch
> > can be developed separately but I think both the patches have some
> > dependency.
>
> I agree with the point that their design can not be completely
> independent. They have some logical relationship of what schema will
> be copied by the initial sync and where is the exact boundary from
> which we will start sending as replication. And suppose first we only
> plan to implement the replication part then how the user will know
> what all schema user has to create and what will be replicated using
> DDL replication? Suppose the user takes a dump and copies all the
> schema and then creates the subscription, then how we are we going to
> handle the DDL concurrent to the subscription command?
>

Right, I also don't see how it can be done in the current
implementation. So, I think even if we want to develop these two as
separate patches they need to be integrated to make the solution
complete.

--
With Regards,
Amit Kapila.

>Thanks, but it would be good if you can share it in the patch form as
>well unless you need to send patches too frequently. It is easier to
>give comments.

The rebased patches are attached. They apply on commit ed0fbc8.

> > I agree with the point that their design can not be completely
> > independent. They have some logical relationship of what schema will
> > be copied by the initial sync and where is the exact boundary from
> > which we will start sending as replication. And suppose first we only
> > plan to implement the replication part then how the user will know
> > what all schema user has to create and what will be replicated using
> > DDL replication? Suppose the user takes a dump and copies all the
> > schema and then creates the subscription, then how we are we going to
> > handle the DDL concurrent to the subscription command?
> >
>
> Right, I also don't see how it can be done in the current
> implementation. So, I think even if we want to develop these two as
> separate patches they need to be integrated to make the solution
> complete.

The concurrent DDL issue actually exists today even with DML
replication. i.e. user have to make sure table schema is in sync
before starting logical replication.

Here is a high level idea to implement schema initialization:
1. On source DB: dump the schema and create a logical replication slot
on a snapshot at the same time
2. On target DB: restore the schema
3. On target DB: create subscription by pointing it to the logical
replication slot and the snapshot in step 1, so that the initial sync
is based on the snapshot and concurrent changes are replicated using
the logical replication slot.

Regards,
Zheng

On Fri, Apr 8, 2022 at 4:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Mar 29, 2022 at 9:47 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > The idea is to force skipping any direct data population (which can
> > > potentially cause data inconsistency on the subscriber)
> > > in CREATE AS and SELECT INTO command on the subscriber by forcing the
> > > skipData flag in the intoClause of the parsetree after
> > > the logical replication worker parses the command. The data sync will
> > > be taken care of by the DML replication after the DDL replication
> > > finishes.
> >
> > Okay, something like that should work, I am not sure it is the best
> > design though.
> >
>
> Even if this works, how will we make Alter Table statement work where
> it needs to rewrite the table? There also I think we can face a
> similar problem if we directly send the statement, once the table will
> be updated due to the DDL statement and then again due to table
> rewrite as that will have a separate WAL.

I agree. But here the bigger question is what is the correct behavior
in case of the Alter Table? I mean for example in the publisher the
table gets rewritten due to the Access Method change then what should
be the behavior of the subscriber. One expected behavior is that on
subscriber also the access method gets changed and the data remains
the same as on the subscriber table(table can be locally rewritten
based on new AM). Which seems quite sensible behavior to me. But if
we want this behavior then we can not replay the logical messages
generated by DML WAL because of table rewrite, otherwise we will get
duplicate data, unless we plan to get rid of the current data and just
get all new data from the publisher. And if we do that then the data
will be as per the latest data in the table based on the publisher, so
I think first we need to define the correct behavior and then we can
design it accordingly.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 14, 2022, at 6:26 AM, Dilip Kumar wrote:
> I agree. But here the bigger question is what is the correct behavior
> in case of the Alter Table? I mean for example in the publisher the
> table gets rewritten due to the Access Method change then what should
> be the behavior of the subscriber. One expected behavior is that on
> subscriber also the access method gets changed and the data remains
> the same as on the subscriber table(table can be locally rewritten
> based on new AM). Which seems quite sensible behavior to me. But if
> we want this behavior then we can not replay the logical messages
> generated by DML WAL because of table rewrite, otherwise we will get
> duplicate data, unless we plan to get rid of the current data and just
> get all new data from the publisher. And if we do that then the data
> will be as per the latest data in the table based on the publisher, so
> I think first we need to define the correct behavior and then we can
> design it accordingly.
You should forbid it. Unless you can decompose the command into multiple SQL
commands to make it a safe operation for logical replication.

Let's say you want to add a column with a volatile default.

ALTER TABLE foo ADD COLUMN bar double precision DEFAULT random();

If you replicate the DDL command as is, you will have different data
downstream. You should forbid it. However, this operation can be supported if
the DDL command is decomposed in multiple steps.

-- add a new column without DEFAULT to avoid rewrite
ALTER TABLE foo ADD COLUMN bar double precision;

-- future rows could use the DEFAULT expression
-- it also doesn't rewrite the table
ALTER TABLE foo ALTER COLUMN bar SET DEFAULT random();

-- it effectively rewrites the table
-- all rows are built from one source node
-- data will be the same on all nodes
UPDATE foo SET bar = random();

The ALTER TABLE ... ALTER COLUMN ... TYPE has a similar issue. This DDL command
can be decomposed to avoid the rewrite. If you are changing the data type, in
general, you add a new column and updates all rows doing the proper conversion.
(If you are updating in batches, you usually add a trigger to automatically
adjust the new column value for INSERTs and UPDATEs. Another case is when you
are reducing the the typmod (for example, varchar(100) to varchar(20)). In this
case, the DDL command can de decomposed removing the typmod information (ALTER
TABLE ... ALTER COLUMN ... TYPE varchar) and replacing it with a CHECK
constraint.

I didn't review this patch in depth but we certainly need to impose some DDL
restrictions if we are replicating DDLs. There are other cases that should be
treated accordingly such as a TABLESPACE specification or a custom data type.

--
Euler Taveira
EDB https://www.enterprisedb.com/

> You should forbid it. Unless you can decompose the command into multiple SQL
> commands to make it a safe operation for logical replication.
>
> Let's say you want to add a column with a volatile default.
>
> ALTER TABLE foo ADD COLUMN bar double precision DEFAULT random();
>
> If you replicate the DDL command as is, you will have different data
> downstream. You should forbid it. However, this operation can be supported if
> the DDL command is decomposed in multiple steps.
>
> -- add a new column without DEFAULT to avoid rewrite
> ALTER TABLE foo ADD COLUMN bar double precision;
>
> -- future rows could use the DEFAULT expression
> -- it also doesn't rewrite the table
> ALTER TABLE foo ALTER COLUMN bar SET DEFAULT random();
>
> -- it effectively rewrites the table
> -- all rows are built from one source node
> -- data will be the same on all nodes
> UPDATE foo SET bar = random();
>
> The ALTER TABLE ... ALTER COLUMN ... TYPE has a similar issue. This DDL command
> can be decomposed to avoid the rewrite. If you are changing the data type, in
> general, you add a new column and updates all rows doing the proper conversion.
> (If you are updating in batches, you usually add a trigger to automatically
> adjust the new column value for INSERTs and UPDATEs. Another case is when you
> are reducing the the typmod (for example, varchar(100) to varchar(20)). In this
> case, the DDL command can be decomposed removing the typmod information (ALTER
> TABLE ... ALTER COLUMN ... TYPE varchar) and replacing it with a CHECK
> constraint.
>
> I didn't review this patch in depth but we certainly need to impose some DDL
> restrictions if we are replicating DDLs. There are other cases that should be
> treated accordingly such as a TABLESPACE specification or a custom data type.

This is helpful. Thanks.

Zheng

> > But then this could be true for DML as well right? Like after
> > replicating the function to the subscriber if we are sending the DML
> > done by function then what's the problem in DDL. I mean if there is
> > no design issue in implementing this then I don't think there is much
> > point in blocking the same or even providing configuration for this.
>
> Agreed. I'll unblock DDLs in functions/procedures and test it out. I
> find out some DDLs in functions are replicated multiple times on the
> subscriber while they should only be replicated once. Still trying to
> figure out why.

Here is the patch unblocking DDLs in function and procedures. Also
fixed a bug where DDL with sub-command may get logged twice.
github commit of the same patch:
https://github.com/zli236/postgres/commit/d0fe6065ee3cb6051e5b026f17b82f5220903e6f

Regards,
Zheng

Hello,

> You should forbid it. Unless you can decompose the command into multiple SQL
> commands to make it a safe operation for logical replication.
>
> Let's say you want to add a column with a volatile default.
>
> ALTER TABLE foo ADD COLUMN bar double precision DEFAULT random();
>
> If you replicate the DDL command as is, you will have different data
> downstream. You should forbid it. However, this operation can be supported if
> the DDL command is decomposed in multiple steps.
>
> -- add a new column without DEFAULT to avoid rewrite
> ALTER TABLE foo ADD COLUMN bar double precision;
>
> -- future rows could use the DEFAULT expression
> -- it also doesn't rewrite the table
> ALTER TABLE foo ALTER COLUMN bar SET DEFAULT random();
>
> -- it effectively rewrites the table
> -- all rows are built from one source node
> -- data will be the same on all nodes
> UPDATE foo SET bar = random();

I looked more into this. In order to support statements like "ALTER
TABLE foo ADD COLUMN bar double precision DEFAULT random();", we have
two potential solutions, but both of them are non-trivial to
implement:

1. As Euler pointed out, we could decompose the statement on the
publisher into multiple statements so that the table rewrite (using
volatile function) is handled by a DML sub-command. The decomposition
requires changes in parse analysis/transform. We also need new logic
to assemble the decomposed DDL commands string back from the parse
trees so we can log them for logical replication.

2. Force skipping table rewrite when executing the same command on the
subscriber, and let DML replication replicate the table rewrite from
the publisher. The problem is table rewrite is not replicated at all
today, and it doesn't seem easy to just enable it for logical
replication. Table rewrite is an expensive operation involving heap
file swap, details can be found in ATRewriteTables().

In light of this, I propose to temporarily block replication of such
DDL command on the replication worker until we figure out a better
solution. This is implemented in patch
0008-Fail-replication-worker-on-DDL-command-that-rewrites.patch.
Notice only DDL statements that rewrite table using a VOLATILE
expression will be blocked. I don't see a problem replicating
non-volatile expression.
Here is the github commit of the same patch:
https://github.com/zli236/postgres/commit/1e6115cb99a1286a61cb0a6a088f7476da29d0b9

> The ALTER TABLE ... ALTER COLUMN ... TYPE has a similar issue. This DDL command
> can be decomposed to avoid the rewrite. If you are changing the data type, in
> general, you add a new column and updates all rows doing the proper conversion.
> (If you are updating in batches, you usually add a trigger to automatically
> adjust the new column value for INSERTs and UPDATEs. Another case is when you
> are reducing the the typmod (for example, varchar(100) to varchar(20)). In this
> case, the DDL command can de decomposed removing the typmod information (ALTER
> TABLE ... ALTER COLUMN ... TYPE varchar) and replacing it with a CHECK
> constraint.

I tested ALTER TABLE ... ALTER COLUMN ... TYPE. It seems to be working
fine. Is there a particular case you're concerned about?

> --
> Euler Taveira
> EDB https://www.enterprisedb.com/
>

Regards,
Zheng Li

> > >Another somewhat unrelated problem I see with this work is how to save
> > >recursion of the same command between nodes (when the involved nodes
> > >replicate DDLs). For DMLs, we can avoid that via replication origins
> > >as is being done in the patch proposed [1] but not sure how will we
> > >deal with that here?
> >
> > I'll need to investigate "recursion of the same command between
> > nodes", could you provide an example?
> >
>
> See email [1]. I think the same solution should work for your proposal
> as well because I see that LogLogicalMessage includes origin but it is
> better if you can confirm it.
>
> [1] - https://www.postgresql.org/message-id/flat/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9=9orqubhjcQ(at)mail(dot)gmail(dot)com

Yes, I applied the patches from [1] and can confirm that the solution
for "infinite recursion of the same command(DDL command in this case)
between nodes" works with my patches.

Regards,
Zheng

On Fri, Apr 8, 2022 at 5:04 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-Apr-08, Amit Kapila wrote:
>
> > On Thu, Mar 17, 2022 at 3:36 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> > For runtime conditions, one of the things you have mentioned in that
> > thread is to add schema name in the statement at the required places
> > which this patch deals with in a different way by explicitly sending
> > it along with the DDL statement.
>
> Hmm, ok. The point of the JSON-blob route is that the publisher sends a
> command representation that can be parsed/processed/transformed
> arbitrarily by the subscriber using generic rules; it should be trivial
> to use a JSON tool to change schema A to schema B in any arbitrary DDL
> command, and produce another working DDL command without having to know
> how to write that command specifically.
>

Attached is a set of two patches as an attempt to evaluate this approach.

The first patch provides functions to deparse DDL commands. Currently,
it is restricted to just a simple CREATE TABLE statement, the required
code is extracted from one of the patches posted in the thread [1].

The second patch allows replicating simple CREATE TABLE DDL
replication. To do that we used an event trigger and DDL deparsing
facilities. While creating a publication, we register a command end
trigger that deparses the DDL as a JSON blob, and WAL logs it. The
event trigger is automatically removed at the time of drop
publication. The WALSender decodes the WAL and sends it downstream
similar to other DML commands. The subscriber then converts JSON back
to the DDL command string and executes it. In the subscriber, we also
add the newly added rel to pg_subscription_rel so that the DML changes
on the new table can be replicated without having to manually run
"ALTER SUBSCRIPTION ... REFRESH PUBLICATION". Some of the code related
to WAL logging and subscriber-side work is taken from the patch posted
by Zheng in this thread but there are quite a few changes in that as
we don't need schema, role, transaction vs. non-transactional
handling.

Note that for now, we have hacked Create Publication code such that
when the user specifies the "FOR ALL TABLES" clause, we invoke this
new functionality. So, this will work only for "FOR ALL TABLES"
publications. For example, we need to below to replicate the simple
Create Table command.

Publisher:
Create Publication pub1 For All Tables;

Subscriber:
Create Subscription sub1 Connection '...' Publication pub1;

Publisher:
Create Table t1(c1 int);

Subscriber:
\d should show t1.

As we have hacked CreatePublication function for this POC, the
regression tests are not passing but we can easily change it so that
we invoke new functionality with the syntax proposed in this thread or
with some other syntax and we shall do that in the next patch unless
this approach is not worth pursuing.

This POC is prepared by Ajin Cherian, Hou-San, and me.

Thoughts?

[1] - https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.no-ip.org

--
With Regards,
Amit Kapila.

> Attached is a set of two patches as an attempt to evaluate this approach.
>
> The first patch provides functions to deparse DDL commands. Currently,
> it is restricted to just a simple CREATE TABLE statement, the required
> code is extracted from one of the patches posted in the thread [1].
>
> The second patch allows replicating simple CREATE TABLE DDL
> replication. To do that we used an event trigger and DDL deparsing
> facilities. While creating a publication, we register a command end
> trigger that deparses the DDL as a JSON blob, and WAL logs it. The
> event trigger is automatically removed at the time of drop
> publication. The WALSender decodes the WAL and sends it downstream
> similar to other DML commands. The subscriber then converts JSON back
> to the DDL command string and executes it. In the subscriber, we also
> add the newly added rel to pg_subscription_rel so that the DML changes
> on the new table can be replicated without having to manually run
> "ALTER SUBSCRIPTION ... REFRESH PUBLICATION". Some of the code related
> to WAL logging and subscriber-side work is taken from the patch posted
> by Zheng in this thread but there are quite a few changes in that as
> we don't need schema, role, transaction vs. non-transactional
> handling.
>
> Note that for now, we have hacked Create Publication code such that
> when the user specifies the "FOR ALL TABLES" clause, we invoke this
> new functionality. So, this will work only for "FOR ALL TABLES"
> publications. For example, we need to below to replicate the simple
> Create Table command.
>
> Publisher:
> Create Publication pub1 For All Tables;
>
> Subscriber:
> Create Subscription sub1 Connection '...' Publication pub1;
>
> Publisher:
> Create Table t1(c1 int);
>
> Subscriber:
> \d should show t1.
>
> As we have hacked CreatePublication function for this POC, the
> regression tests are not passing but we can easily change it so that
> we invoke new functionality with the syntax proposed in this thread or
> with some other syntax and we shall do that in the next patch unless
> this approach is not worth pursuing.
>
> This POC is prepared by Ajin Cherian, Hou-San, and me.
>
> Thoughts?
>
> [1] - https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.no-ip.org

Hi,

Thanks for exploring this direction.

I read the deparsing thread and your patch. Here is my thought:
1. The main concern on maintainability of the deparsing code still
applies if we want to adapt it for DDL replication.
2. The search_path and role still need to be handled, in the deparsing
code. And I think it's actually more overhead to qualify every object
compared to just logging the search_path and enforcing it on the apply
worker.
3. I'm trying to understand if deparsing helps with edge cases like
"ALTER TABLE foo ADD COLUMN bar double precision DEFAULT random();". I
don't think it helps out of the box. The crux of separating table
rewrite and DDL still needs to be solved for such cases.

Regards,
Zheng

On Fri, May 6, 2022 at 10:21 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> > Attached is a set of two patches as an attempt to evaluate this approach.
> >
>
> Thanks for exploring this direction.
>
> I read the deparsing thread and your patch. Here is my thought:
> 1. The main concern on maintainability of the deparsing code still
> applies if we want to adapt it for DDL replication.
>

I agree that it adds to our maintainability effort, like every time we
enhance any DDL or add a new DDL that needs to be replicated, we
probably need to change the deparsing code. But OTOH this approach
seems to provide better flexibility. So, in the long run, maybe the
effort is worthwhile. I am not completely sure at this stage which
approach is better but I thought it is worth evaluating this approach
as Alvaro and Robert seem to prefer this idea.

> 2. The search_path and role still need to be handled, in the deparsing
> code. And I think it's actually more overhead to qualify every object
> compared to just logging the search_path and enforcing it on the apply
> worker.
>

But doing it in the deparsing code will have the benefit that the
other plugins won't have to develop similar logic.

--
With Regards,
Amit Kapila.

On Sat, May 7, 2022 at 9:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, May 6, 2022 at 10:21 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> >
> > > Attached is a set of two patches as an attempt to evaluate this approach.
> > >
> >
> > Thanks for exploring this direction.
> >
> > I read the deparsing thread and your patch. Here is my thought:
> > 1. The main concern on maintainability of the deparsing code still
> > applies if we want to adapt it for DDL replication.
> >
>
> I agree that it adds to our maintainability effort, like every time we
> enhance any DDL or add a new DDL that needs to be replicated, we
> probably need to change the deparsing code. But OTOH this approach
> seems to provide better flexibility. So, in the long run, maybe the
> effort is worthwhile. I am not completely sure at this stage which
> approach is better but I thought it is worth evaluating this approach
> as Alvaro and Robert seem to prefer this idea.

+1, IMHO with deparsing logic it would be easy to handle the mixed DDL
commands like ALTER TABLE REWRITE. But the only thing is that we will
have to write the deparsing code for all the utility commands so there
will be a huge amount of new code to maintain.

> > 2. The search_path and role still need to be handled, in the deparsing
> > code. And I think it's actually more overhead to qualify every object
> > compared to just logging the search_path and enforcing it on the apply
> > worker.
> >
>
> But doing it in the deparsing code will have the benefit that the
> other plugins won't have to develop similar logic.

Right, this makes sense.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sun, May 8, 2022 at 12:39 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Sat, May 7, 2022 at 9:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, May 6, 2022 at 10:21 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > >
> > > > Attached is a set of two patches as an attempt to evaluate this approach.
> > > >
> > >
> > > Thanks for exploring this direction.
> > >
> > > I read the deparsing thread and your patch. Here is my thought:
> > > 1. The main concern on maintainability of the deparsing code still
> > > applies if we want to adapt it for DDL replication.
> > >
> >
> > I agree that it adds to our maintainability effort, like every time we
> > enhance any DDL or add a new DDL that needs to be replicated, we
> > probably need to change the deparsing code. But OTOH this approach
> > seems to provide better flexibility. So, in the long run, maybe the
> > effort is worthwhile. I am not completely sure at this stage which
> > approach is better but I thought it is worth evaluating this approach
> > as Alvaro and Robert seem to prefer this idea.
>
> +1, IMHO with deparsing logic it would be easy to handle the mixed DDL
> commands like ALTER TABLE REWRITE. But the only thing is that we will
> have to write the deparsing code for all the utility commands so there
> will be a huge amount of new code to maintain.

I haven't gone through the entire thread, just trying to understand
the need of deparsing logic - do we need the DDL query text to be sent
to subscribers? If yes, why can't we WAL log the DDL query text as-is
and the logical decoding can send those to subscribers, as-is or in a
modified form? Or log the DDLs to a separate log on disk?

Regards,
Bharath Rupireddy.

On Mon, May 9, 2022 at 12:46 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Sun, May 8, 2022 at 12:39 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Sat, May 7, 2022 at 9:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, May 6, 2022 at 10:21 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > > >
> > > > > Attached is a set of two patches as an attempt to evaluate this approach.
> > > > >
> > > >
> > > > Thanks for exploring this direction.
> > > >
> > > > I read the deparsing thread and your patch. Here is my thought:
> > > > 1. The main concern on maintainability of the deparsing code still
> > > > applies if we want to adapt it for DDL replication.
> > > >
> > >
> > > I agree that it adds to our maintainability effort, like every time we
> > > enhance any DDL or add a new DDL that needs to be replicated, we
> > > probably need to change the deparsing code. But OTOH this approach
> > > seems to provide better flexibility. So, in the long run, maybe the
> > > effort is worthwhile. I am not completely sure at this stage which
> > > approach is better but I thought it is worth evaluating this approach
> > > as Alvaro and Robert seem to prefer this idea.
> >
> > +1, IMHO with deparsing logic it would be easy to handle the mixed DDL
> > commands like ALTER TABLE REWRITE. But the only thing is that we will
> > have to write the deparsing code for all the utility commands so there
> > will be a huge amount of new code to maintain.
>
> I haven't gone through the entire thread, just trying to understand
> the need of deparsing logic
>

That provided more flexibility, for example, see [1]. We can do things
like identifying the right schema for an object in a way that all
plugins don't need to do specific handling. I think the point Dilip is
highlighting about table rewrite is explained in the email [2]. The
point is that sometimes we can't replay the DDL statement as it is on
the subscriber and deparsing might help in some of those cases even if
not all. At this stage, we are just evaluating both approaches.

[1] - https://www.postgresql.org/message-id/202204081134.6tcmf5cxl3sz%40alvherre.pgsql
[2] - https://www.postgresql.org/message-id/3c646317-df34-4cb3-9365-14abeada6587%40www.fastmail.com

--
With Regards,
Amit Kapila.

On 2022-May-08, Dilip Kumar wrote:

> On Sat, May 7, 2022 at 9:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> > I agree that it adds to our maintainability effort, like every time we
> > enhance any DDL or add a new DDL that needs to be replicated, we
> > probably need to change the deparsing code. But OTOH this approach
> > seems to provide better flexibility. So, in the long run, maybe the
> > effort is worthwhile. I am not completely sure at this stage which
> > approach is better but I thought it is worth evaluating this approach
> > as Alvaro and Robert seem to prefer this idea.
>
> +1, IMHO with deparsing logic it would be easy to handle the mixed DDL
> commands like ALTER TABLE REWRITE. But the only thing is that we will
> have to write the deparsing code for all the utility commands so there
> will be a huge amount of new code to maintain.

Actually, the largest stumbling block on this, IMO, is having a good
mechanism to verify that all DDLs are supported. The deparsing code
itself is typically not very bad, as long as we have a sure way to twist
every contributor's hand into writing it, which is what an automated
test mechanism would give us.

The code in test_ddl_deparse is a pretty lame start, not nearly good
enough by a thousand miles. My real intention was to have a test
harness that would first run a special SQL script to install DDL
capture, then run all the regular src/test/regress scripts, and then at
the end ensure that all the DDL scripts were properly reproduced -- for
example transmit them to another database, replay them there, and dump
both databases and compare them. However, there were challenges which I
no longer remember and we were unable to complete this, and we are where
we are.

Thanks for rebasing that old code, BTW.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Wed, Apr 13, 2022 at 6:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Apr 13, 2022 at 2:38 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Tue, Apr 12, 2022 at 4:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > > The *initial* DDL replication is a different problem than DDL replication. The
> > > > former requires a snapshot to read the current catalog data and build a CREATE
> > > > command as part of the subscription process. The subsequent DDLs in that object
> > > > will be handled by a different approach that is being discussed here.
> > > >
> > >
> > > I think they are not completely independent because of the current way
> > > to do initial sync followed by replication. The initial sync and
> > > replication need some mechanism to ensure that one of those doesn't
> > > overwrite the work done by the other. Now, the initial idea and patch
> > > can be developed separately but I think both the patches have some
> > > dependency.
> >
> > I agree with the point that their design can not be completely
> > independent. They have some logical relationship of what schema will
> > be copied by the initial sync and where is the exact boundary from
> > which we will start sending as replication. And suppose first we only
> > plan to implement the replication part then how the user will know
> > what all schema user has to create and what will be replicated using
> > DDL replication? Suppose the user takes a dump and copies all the
> > schema and then creates the subscription, then how we are we going to
> > handle the DDL concurrent to the subscription command?
> >
>
> Right, I also don't see how it can be done in the current
> implementation. So, I think even if we want to develop these two as
> separate patches they need to be integrated to make the solution
> complete.

It would be better to develop them separately in terms of development
speed but, yes, we perhaps need to integrate them at some points.

I think that the initial DDL replication can be done when the
relation's state is SUBREL_STATE_INIT. That is, at the very beginning
of the table synchronization, the syncworker copies the table schema
somehow, then starts the initial data copy. After that, syncworker or
applyworker applies DML/DDL changes while catching up and streaming
changes, respectively. Probably we can have it optional whether to
copy schema only, data only, or both.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, May 10, 2022 at 12:32 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, Apr 13, 2022 at 6:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Apr 13, 2022 at 2:38 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Apr 12, 2022 at 4:25 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > > The *initial* DDL replication is a different problem than DDL replication. The
> > > > > former requires a snapshot to read the current catalog data and build a CREATE
> > > > > command as part of the subscription process. The subsequent DDLs in that object
> > > > > will be handled by a different approach that is being discussed here.
> > > > >
> > > >
> > > > I think they are not completely independent because of the current way
> > > > to do initial sync followed by replication. The initial sync and
> > > > replication need some mechanism to ensure that one of those doesn't
> > > > overwrite the work done by the other. Now, the initial idea and patch
> > > > can be developed separately but I think both the patches have some
> > > > dependency.
> > >
> > > I agree with the point that their design can not be completely
> > > independent. They have some logical relationship of what schema will
> > > be copied by the initial sync and where is the exact boundary from
> > > which we will start sending as replication. And suppose first we only
> > > plan to implement the replication part then how the user will know
> > > what all schema user has to create and what will be replicated using
> > > DDL replication? Suppose the user takes a dump and copies all the
> > > schema and then creates the subscription, then how we are we going to
> > > handle the DDL concurrent to the subscription command?
> > >
> >
> > Right, I also don't see how it can be done in the current
> > implementation. So, I think even if we want to develop these two as
> > separate patches they need to be integrated to make the solution
> > complete.
>
> It would be better to develop them separately in terms of development
> speed but, yes, we perhaps need to integrate them at some points.
>
> I think that the initial DDL replication can be done when the
> relation's state is SUBREL_STATE_INIT. That is, at the very beginning
> of the table synchronization, the syncworker copies the table schema
> somehow, then starts the initial data copy. After that, syncworker or
> applyworker applies DML/DDL changes while catching up and streaming
> changes, respectively. Probably we can have it optional whether to
> copy schema only, data only, or both.
>

This sounds okay for copying table schema but we can have other
objects like functions, procedures, views, etc. So, we may need
altogether a separate mechanism to copy all the published objects.

--
With Regards,
Amit Kapila.

On Fri, May 6, 2022 at 11:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> As we have hacked CreatePublication function for this POC, the
> regression tests are not passing but we can easily change it so that
> we invoke new functionality with the syntax proposed in this thread or
> with some other syntax and we shall do that in the next patch unless
> this approach is not worth pursuing.
>
> This POC is prepared by Ajin Cherian, Hou-San, and me.
>
> Thoughts?
>
> [1] - https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.no-ip.org

I have updated Amit's patch by including a public action "create" when
creating publication which is turned off by default.
Now the 'make check' tests pass. I also fixed a problem that failed to
create tables when the table has a primary key.

regards,
Ajin Cherian
Fujitsu Australia

On Tue, May 10, 2022 at 4:59 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, May 6, 2022 at 11:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > As we have hacked CreatePublication function for this POC, the
> > regression tests are not passing but we can easily change it so that
> > we invoke new functionality with the syntax proposed in this thread or
> > with some other syntax and we shall do that in the next patch unless
> > this approach is not worth pursuing.
> >
> > This POC is prepared by Ajin Cherian, Hou-San, and me.
> >
> > Thoughts?
> >
> > [1] - https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.no-ip.org
>
> I have updated Amit's patch by including a public action "create" when
> creating publication which is turned off by default.
> Now the 'make check' tests pass. I also fixed a problem that failed to
> create tables when the table has a primary key.
>

Are these the correct set of patches? I don't see a DDL support patch.

--
With Regards,
Amit Kapila.

On Tue, May 10, 2022 at 9:33 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, May 6, 2022 at 11:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > As we have hacked CreatePublication function for this POC, the
> > regression tests are not passing but we can easily change it so that
> > we invoke new functionality with the syntax proposed in this thread or
> > with some other syntax and we shall do that in the next patch unless
> > this approach is not worth pursuing.
> >
> > This POC is prepared by Ajin Cherian, Hou-San, and me.
> >
> > Thoughts?
> >
> > [1] - https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.no-ip.org
>
> I have updated Amit's patch by including a public action "create" when
> creating publication which is turned off by default.
> Now the 'make check' tests pass. I also fixed a problem that failed to
> create tables when the table has a primary key.

Sorry, I sent the wrong patches. Here is the correct one.

regards,
Ajin Cherian
Fujitsu Australia

> > > I agree that it adds to our maintainability effort, like every time we
> > > enhance any DDL or add a new DDL that needs to be replicated, we
> > > probably need to change the deparsing code. But OTOH this approach
> > > seems to provide better flexibility. So, in the long run, maybe the
> > > effort is worthwhile. I am not completely sure at this stage which
> > > approach is better but I thought it is worth evaluating this approach
> > > as Alvaro and Robert seem to prefer this idea.
> >
> > +1, IMHO with deparsing logic it would be easy to handle the mixed DDL
> > commands like ALTER TABLE REWRITE. But the only thing is that we will
> > have to write the deparsing code for all the utility commands so there
> > will be a huge amount of new code to maintain.
>
> Actually, the largest stumbling block on this, IMO, is having a good
> mechanism to verify that all DDLs are supported. The deparsing code
> itself is typically not very bad, as long as we have a sure way to twist
> every contributor's hand into writing it, which is what an automated
> test mechanism would give us.
>
> The code in test_ddl_deparse is a pretty lame start, not nearly good
> enough by a thousand miles. My real intention was to have a test
> harness that would first run a special SQL script to install DDL
> capture, then run all the regular src/test/regress scripts, and then at
> the end ensure that all the DDL scripts were properly reproduced -- for
> example transmit them to another database, replay them there, and dump
> both databases and compare them. However, there were challenges which I
> no longer remember and we were unable to complete this, and we are where
> we are.
>
> Thanks for rebasing that old code, BTW.

I agree that deparsing could be a very useful utility on its own. Not
only for SQL command replication between PostgreSQL servers, but also
potentially feasible for SQL command replication between PotgreSQL and
other database systems. For example, one could assemble the json
representation of the SQL parse tree back to a SQL command that can be
run in MySQL. But that requires different assembling rules and code
for different database systems. If we're envisioning this kind of
flexibility that the deparsing utility can offer, then I think it's
better to develop the deparsing utility as an extension itself.

Regards,
Zheng

On Wed, May 11, 2022 at 1:01 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> > > > I agree that it adds to our maintainability effort, like every time we
> > > > enhance any DDL or add a new DDL that needs to be replicated, we
> > > > probably need to change the deparsing code. But OTOH this approach
> > > > seems to provide better flexibility. So, in the long run, maybe the
> > > > effort is worthwhile. I am not completely sure at this stage which
> > > > approach is better but I thought it is worth evaluating this approach
> > > > as Alvaro and Robert seem to prefer this idea.
> > >
> > > +1, IMHO with deparsing logic it would be easy to handle the mixed DDL
> > > commands like ALTER TABLE REWRITE. But the only thing is that we will
> > > have to write the deparsing code for all the utility commands so there
> > > will be a huge amount of new code to maintain.
> >
> > Actually, the largest stumbling block on this, IMO, is having a good
> > mechanism to verify that all DDLs are supported. The deparsing code
> > itself is typically not very bad, as long as we have a sure way to twist
> > every contributor's hand into writing it, which is what an automated
> > test mechanism would give us.
> >
> > The code in test_ddl_deparse is a pretty lame start, not nearly good
> > enough by a thousand miles. My real intention was to have a test
> > harness that would first run a special SQL script to install DDL
> > capture, then run all the regular src/test/regress scripts, and then at
> > the end ensure that all the DDL scripts were properly reproduced -- for
> > example transmit them to another database, replay them there, and dump
> > both databases and compare them. However, there were challenges which I
> > no longer remember and we were unable to complete this, and we are where
> > we are.
> >
> > Thanks for rebasing that old code, BTW.
>
> I agree that deparsing could be a very useful utility on its own. Not
> only for SQL command replication between PostgreSQL servers, but also
> potentially feasible for SQL command replication between PotgreSQL and
> other database systems. For example, one could assemble the json
> representation of the SQL parse tree back to a SQL command that can be
> run in MySQL. But that requires different assembling rules and code
> for different database systems. If we're envisioning this kind of
> flexibility that the deparsing utility can offer, then I think it's
> better to develop the deparsing utility as an extension itself.

IIUC the event trigger can already provide such flexibility. That is,
one could create an extension that creates an event trigger and in the
event trigger function it deparses the SQL parse tree to a SQL command
that can be run in MySQL. While having such flexibility, I’m fine with
having built-in deparsing utility in the core.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, May 11, 2022 at 1:17 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:

> > I have updated Amit's patch by including a public action "create" when
> > creating publication which is turned off by default.
> > Now the 'make check' tests pass. I also fixed a problem that failed to
> > create tables when the table has a primary key.
>

I have rearranged the files around and removed
src/bin/pg_waldump/logicalddlmsgdesc.c
as it is a generated file.

regards,
Ajin Cherian
Fujitsu Australia

On Mon, May 9, 2022 at 6:05 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-May-08, Dilip Kumar wrote:
>
> > On Sat, May 7, 2022 at 9:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> > > I agree that it adds to our maintainability effort, like every time we
> > > enhance any DDL or add a new DDL that needs to be replicated, we
> > > probably need to change the deparsing code. But OTOH this approach
> > > seems to provide better flexibility. So, in the long run, maybe the
> > > effort is worthwhile. I am not completely sure at this stage which
> > > approach is better but I thought it is worth evaluating this approach
> > > as Alvaro and Robert seem to prefer this idea.
> >
> > +1, IMHO with deparsing logic it would be easy to handle the mixed DDL
> > commands like ALTER TABLE REWRITE. But the only thing is that we will
> > have to write the deparsing code for all the utility commands so there
> > will be a huge amount of new code to maintain.
>
> Actually, the largest stumbling block on this, IMO, is having a good
> mechanism to verify that all DDLs are supported. The deparsing code
> itself is typically not very bad, as long as we have a sure way to twist
> every contributor's hand into writing it, which is what an automated
> test mechanism would give us.

Agreed.

>
> The code in test_ddl_deparse is a pretty lame start, not nearly good
> enough by a thousand miles. My real intention was to have a test
> harness that would first run a special SQL script to install DDL
> capture, then run all the regular src/test/regress scripts, and then at
> the end ensure that all the DDL scripts were properly reproduced -- for
> example transmit them to another database, replay them there, and dump
> both databases and compare them. However, there were challenges which I
> no longer remember and we were unable to complete this, and we are where
> we are.

I think the regression test suite improvements in these few years make
it easier to implement such regression tests in order to check not
only the existing commands but also future changes. I'll try it if no
one is working on it, and let us see if there are challenges.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, May 11, 2022 at 1:09 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, May 9, 2022 at 6:05 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > On 2022-May-08, Dilip Kumar wrote:
>
> >
> > The code in test_ddl_deparse is a pretty lame start, not nearly good
> > enough by a thousand miles. My real intention was to have a test
> > harness that would first run a special SQL script to install DDL
> > capture, then run all the regular src/test/regress scripts, and then at
> > the end ensure that all the DDL scripts were properly reproduced -- for
> > example transmit them to another database, replay them there, and dump
> > both databases and compare them. However, there were challenges which I
> > no longer remember and we were unable to complete this, and we are where
> > we are.
>
> I think the regression test suite improvements in these few years make
> it easier to implement such regression tests in order to check not
> only the existing commands but also future changes. I'll try it if no
> one is working on it, and let us see if there are challenges.
>

I think it is a good idea to give it a try. However, one point to note
in this regard is that if we decide to use deparsing for DDL logical
replication then the tests for logical replication will automatically
test all the deparsing code.

--
With Regards,
Amit Kapila.

On Wed, May 11, 2022 at 6:15 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, May 11, 2022 at 1:09 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, May 9, 2022 at 6:05 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > >
> > > On 2022-May-08, Dilip Kumar wrote:
> >
> > >
> > > The code in test_ddl_deparse is a pretty lame start, not nearly good
> > > enough by a thousand miles. My real intention was to have a test
> > > harness that would first run a special SQL script to install DDL
> > > capture, then run all the regular src/test/regress scripts, and then at
> > > the end ensure that all the DDL scripts were properly reproduced -- for
> > > example transmit them to another database, replay them there, and dump
> > > both databases and compare them. However, there were challenges which I
> > > no longer remember and we were unable to complete this, and we are where
> > > we are.
> >
> > I think the regression test suite improvements in these few years make
> > it easier to implement such regression tests in order to check not
> > only the existing commands but also future changes. I'll try it if no
> > one is working on it, and let us see if there are challenges.
> >
>
> I think it is a good idea to give it a try. However, one point to note
> in this regard is that if we decide to use deparsing for DDL logical
> replication then the tests for logical replication will automatically
> test all the deparsing code.

Do you mean that in tests for logical replication we run regression
tests on the publisher and check if relations are
created/altered/dropped expectedly on the subscriber? If we rely on
tests for logical replication, I think logical replication will have
to support all future DDL changes/features.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, May 11, 2022 at 6:25 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Wed, May 11, 2022 at 6:15 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, May 11, 2022 at 1:09 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Mon, May 9, 2022 at 6:05 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > > >
> > > >
> > > > The code in test_ddl_deparse is a pretty lame start, not nearly good
> > > > enough by a thousand miles. My real intention was to have a test
> > > > harness that would first run a special SQL script to install DDL
> > > > capture, then run all the regular src/test/regress scripts, and then at
> > > > the end ensure that all the DDL scripts were properly reproduced -- for
> > > > example transmit them to another database, replay them there, and dump
> > > > both databases and compare them. However, there were challenges which I
> > > > no longer remember and we were unable to complete this, and we are where
> > > > we are.
> > >
> > > I think the regression test suite improvements in these few years make
> > > it easier to implement such regression tests in order to check not
> > > only the existing commands but also future changes. I'll try it if no
> > > one is working on it, and let us see if there are challenges.
> > >
> >
> > I think it is a good idea to give it a try. However, one point to note
> > in this regard is that if we decide to use deparsing for DDL logical
> > replication then the tests for logical replication will automatically
> > test all the deparsing code.
>
> Do you mean that in tests for logical replication we run regression
> tests on the publisher and check if relations are
> created/altered/dropped expectedly on the subscriber?
>

Yes.

> If we rely on
> tests for logical replication, I think logical replication will have
> to support all future DDL changes/features.
>

Agreed, so we can't completely rely on logical replication tests.

--
With Regards,
Amit Kapila.

> > 4. The handling related to partition tables seems missing because, on
> > the subscriber-side, it always creates a relation entry in
> > pg_subscription_rel which won't work. Check its interaction with
> > publish_via_partition_root.
>
> I will test it out.

Hi,

patch 0010 properly handles partitioned table creation on the apply
worker. Whether a replicated partitioned table should be added to
pg_subscription_rel catalog depends on the setting of
publish_via_partition_root of the publication. Thus we need to connect
to the source DB and check if the partitioned table is in
pg_catalog.pg_publication_tables after the apply worker creates the
partitioned table.

Thanks to Borui Yang for enabling and testing replication of DDL type
T_RenameStmt in patch 0009.

I've also rebased all the patches. Github branch of the same change
can be found here:
https://github.com/zli236/postgres/commits/ddl_replication

Regards,
Zheng

On Thu, Apr 14, 2022 at 7:45 PM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> You should forbid it. Unless you can decompose the command into multiple SQL
> commands to make it a safe operation for logical replication.
>
> Let's say you want to add a column with a volatile default.
>
> ALTER TABLE foo ADD COLUMN bar double precision DEFAULT random();
>
> If you replicate the DDL command as is, you will have different data
> downstream. You should forbid it. However, this operation can be supported if
> the DDL command is decomposed in multiple steps.
>
> -- add a new column without DEFAULT to avoid rewrite
> ALTER TABLE foo ADD COLUMN bar double precision;
>
> -- future rows could use the DEFAULT expression
> -- it also doesn't rewrite the table
> ALTER TABLE foo ALTER COLUMN bar SET DEFAULT random();
>
> -- it effectively rewrites the table
> -- all rows are built from one source node
> -- data will be the same on all nodes
> UPDATE foo SET bar = random();
>

While thinking about this, I see more to it than this. Say, we are
able to decompose/split the DDL command with the help of deparser, do
we want to update the additional rows on the subscriber that didn't
exist on the publisher? For example,

A table on the publisher side has rows:
ddl_test(foo)
a
----
1
2

The same table on the subscriber side has rows:
ddl_test(foo)
a
----
1
2
3
4

Now, say, the user has added a bar column with "ALTER TABLE foo ADD
COLUMN bar double precision NOT NULL DEFAULT random();" If we compare
with replication of DMLs like (UPDATE ddl_test SET bar = random();),
the replication won't update rows with values (3 and 4) on subscriber
as they don't exist on the publisher. However, if we follow the same
here for DDL replication of Alter, it will fail because of NOT NULL
constraint. So, it seems we should update all the existing rows on the
subscriber to make replication of such constraints successful. It
seems that IBM's replication solution allows replication of such DDLs
and does update all existing rows on the target table [1][2].

I think it would be tricky to update the rows in subscriber that
doesn't exist in the publisher as we need to distinguish/find such
rows during apply but I think we can find some way to achieve this if
we decide to go this way.

We can also conclude that we want to restrict the replication of Alter
Table for such cases but as other databases seem to support this, I
think it is worth trying to support such an operation. If it turns out
to be too complex or not at all feasible then we can always exclude it
from the first version.

Thoughts?

[1] - https://www.ibm.com/docs/en/idr/10.2.1?topic=replication-adding-existing-columns-subscription-unidirectional
(... Columns are added to the target table with the same data type,
null characteristic, and default value as the matching columns in the
source table... Rows that existed in the target table before the new
column is added will have a NULL or default value for the new column.
[2] - https://www.ibm.com/docs/en/idr/11.4.0?topic=replication-alter-add-column-command-multidirectional

--
With Regards,
Amit Kapila.

> Now, say, the user has added a bar column with "ALTER TABLE foo ADD
> COLUMN bar double precision NOT NULL DEFAULT random();" If we compare
> with replication of DMLs like (UPDATE ddl_test SET bar = random();),
> the replication won't update rows with values (3 and 4) on subscriber
> as they don't exist on the publisher. However, if we follow the same
> here for DDL replication of Alter, it will fail because of NOT NULL
> constraint. So, it seems we should update all the existing rows on the
> subscriber to make replication of such constraints successful. It
> seems that IBM's replication solution allows replication of such DDLs
> and does update all existing rows on the target table [1][2].
>
> I think it would be tricky to update the rows in subscriber that
> doesn't exist in the publisher as we need to distinguish/find such
> rows during apply but I think we can find some way to achieve this if
> we decide to go this way.

I think the behavior that makes most sense here is to replicate the default
values of the new column for existing rows from the publisher, and generate
default values for the additional rows on the subscriber.

> We can also conclude that we want to restrict the replication of Alter
> Table for such cases but as other databases seem to support this, I
> think it is worth trying to support such an operation. If it turns out
> to be too complex or not at all feasible then we can always exclude it
> from the first version.

I think it might be OK that we exclude such cases (fail apply worker on
occurence) for the first version. I've spent some time on it and it seems
to be a high effort, low reward task at the moment.

Regards,
Zheng

On Tue, May 10, 2022 at 9:33 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, May 6, 2022 at 11:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > As we have hacked CreatePublication function for this POC, the
> > regression tests are not passing but we can easily change it so that
> > we invoke new functionality with the syntax proposed in this thread or
> > with some other syntax and we shall do that in the next patch unless
> > this approach is not worth pursuing.
> >
> > This POC is prepared by Ajin Cherian, Hou-San, and me.
> >
> > Thoughts?
> >
> > [1] - https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.no-ip.org
>
> I have updated Amit's patch by including a public action "create" when
> creating publication which is turned off by default.
> Now the 'make check' tests pass. I also fixed a problem that failed to
> create tables when the table has a primary key.

I have updated this "deparse" patch-set to include support for 'drop table':

Only the drop command of those tables in the publication is replicated.

This is achieved with two event trigger handlers, 'command start' and
'command end' for drop table commands.

The 'command start' event handler logs a ddl message with the relids
of the tables that are dropped which the output plugin (pgoutput)
stores in its internal data structure after verifying that it is for a
table that is part of the publication. Later the 'command end' event
handler
sends the actual drop message. Pgoutput on receiving the command end,
only sends out the drop command only if it is for one of the
relids marked for deleting. The reason we have to do this is because,
once the logical decoder receives the 'command end' message,
the relid of the table is no longer valid as it has been deleted as
part of invalidations received for the drop table command.
It is no longer possible to verify if the table is part of the
publication list or not. To make this possible, I have added two more
elements
to the ddl xlog and ddl message, (relid and cmdtype).

We could have also handled all this on the subscriber side as well,
but that would mean sending spurious ddl messages for tables that
are not part of the publication.

Example:

publisher:

postgres=# create table test (a int, b text);
CREATE TABLE
postgres=# create table test1 (a int, b text);
CREATE TABLE
postgres=# create table test2 (a int, b text);
CREATE TABLE
postgres=# create publication tap_pub for TABLE test, test1 with
(publish='ddl'); CREATE PUBLICATION
postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+-------+-------+-------
public | test | table | ajin
public | test1 | table | ajin
public | test2 | table | ajin
(3 rows)

subscriber:

postgres=# create table test (a int, b text);
CREATE TABLE
postgres=# create table test1 (a int, b text);
CREATE TABLE
postgres=# create table test2 (a int, b text);
CREATE TABLE
postgres=# CREATE SUBSCRIPTION sub CONNECTION '<CONNINFO>' PUBLICATION tap_pub;
NOTICE: created replication slot "sub" on publisher
CREATE SUBSCRIPTION
postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+-------+-------+-------
public | test | table | ajin
public | test1 | table | ajin
public | test2 | table | ajin
(3 rows)

publisher:

postgres=# drop table test1;
DROP TABLE
postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+-------+-------+-------
public | test | table | ajin
public | test2 | table | ajin
(2 rows)

subscriber:

postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+-------+-------+-------
public | test | table | ajin
public | test2 | table | ajin
(2 rows)
>>>> DROP table replicated.

publisher:

postgres=# drop table test2;
DROP TABLE
postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+------+-------+-------
public | test | table | ajin
(1 row)

subscriber:

postgres=# \d
List of relations
Schema | Name | Type | Owner
--------+-------+-------+-------
public | test | table | ajin
public | test2 | table | ajin
(2 rows)

>>>DROP table not replicated since table not in publication list.

regards,
Ajin Cherian

On Sat, May 14, 2022 at 6:02 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> > > 4. The handling related to partition tables seems missing because, on
> > > the subscriber-side, it always creates a relation entry in
> > > pg_subscription_rel which won't work. Check its interaction with
> > > publish_via_partition_root.
> >
> > I will test it out.
>
> Hi,
>
> patch 0010 properly handles partitioned table creation on the apply
> worker. Whether a replicated partitioned table should be added to
> pg_subscription_rel catalog depends on the setting of
> publish_via_partition_root of the publication. Thus we need to connect
> to the source DB and check if the partitioned table is in
> pg_catalog.pg_publication_tables after the apply worker creates the
> partitioned table.
>
> Thanks to Borui Yang for enabling and testing replication of DDL type
> T_RenameStmt in patch 0009.
>
> I've also rebased all the patches. Github branch of the same change
> can be found here:
> https://github.com/zli236/postgres/commits/ddl_replication

Thank you for updating the patches!

I've not looked at these patches in-depth yet but with this approach,
what do you think we can handle the DDL syntax differences between
major versions? DDL syntax or behavior could be changed by future
changes and I think we need to somehow deal with the differences. For
example, if the user uses logical replication for major version
upgrade, the publisher is older than the subscriber. We might have to
rewrite the DDL before applying to the subscriber because the DDL
executed on the publisher no longer work on a new PostgreSQL version
or we might have to add some options to the DDL before the application
in order to keep the same behavior. This seems to require a different
solution from what the patch does for the problem you mentioned such
as "DDL involving multiple tables where only some tables are
replicated”.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

Hi Masahiko,

> Thank you for updating the patches!
>
> I've not looked at these patches in-depth yet but with this approach,
> what do you think we can handle the DDL syntax differences between
> major versions? DDL syntax or behavior could be changed by future
> changes and I think we need to somehow deal with the differences. For

> example, if the user uses logical replication for major version
> upgrade, the publisher is older than the subscriber. We might have to
> rewrite the DDL before applying to the subscriber because the DDL
> executed on the publisher no longer work on a new PostgreSQL version

I don't think we will allow this kind of situation to happen in the
first place for
backward compatibility. If a DDL no longer works on a new version of
PostgreSQL, the user will have to change the application code as well.
So even if it happens for
whatever reason, we could either
1. fail the apply worker and let the user fix such DDL because they'll
have to fix the application code anyway when this happens.
2. add guard rail logic in the apply worker to automatically fix such
DDL if possible, knowing the version of the source and target. Similar
logic must have been implemented for pg_dump/restore/upgrade.

> or we might have to add some options to the DDL before the application
> in order to keep the same behavior. This seems to require a different
> solution from what the patch does for the problem you mentioned such

> as "DDL involving multiple tables where only some tables are
> replicated”.

First of all, this case can only happen when the customer chooses to
only replicate a subset of the tables in a database in which case
table level DDL replication is chosen instead of database level DDL
replication (where all tables
and DDLs are replicated). I think the solution would be:
1. make best effort to detect such DDLs on the publisher and avoid
logging of such DDLs in table level DDL replication.
2. apply worker will fail to replay such command due to missing
objects if such DDLs didn't get filtered on the publisher for some
reason. This should be rare and I think it's OK even if it happens,
we'll find out
why and fix it.

Regards,
Zheng

On Fri, May 27, 2022 at 3:49 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> Hi Masahiko,
>
> > Thank you for updating the patches!
> >
> > I've not looked at these patches in-depth yet but with this approach,
> > what do you think we can handle the DDL syntax differences between
> > major versions? DDL syntax or behavior could be changed by future
> > changes and I think we need to somehow deal with the differences. For
>
> > example, if the user uses logical replication for major version
> > upgrade, the publisher is older than the subscriber. We might have to
> > rewrite the DDL before applying to the subscriber because the DDL
> > executed on the publisher no longer work on a new PostgreSQL version
>
> I don't think we will allow this kind of situation to happen in the
> first place for
> backward compatibility. If a DDL no longer works on a new version of
> PostgreSQL, the user will have to change the application code as well.
> So even if it happens for
> whatever reason, we could either
> 1. fail the apply worker and let the user fix such DDL because they'll
> have to fix the application code anyway when this happens.
> 2. add guard rail logic in the apply worker to automatically fix such
> DDL if possible, knowing the version of the source and target. Similar
> logic must have been implemented for pg_dump/restore/upgrade.
>
> > or we might have to add some options to the DDL before the application
> > in order to keep the same behavior. This seems to require a different
> > solution from what the patch does for the problem you mentioned such
>
> > as "DDL involving multiple tables where only some tables are
> > replicated”.
>
> First of all, this case can only happen when the customer chooses to
> only replicate a subset of the tables in a database in which case
> table level DDL replication is chosen instead of database level DDL
> replication (where all tables
> and DDLs are replicated). I think the solution would be:
> 1. make best effort to detect such DDLs on the publisher and avoid
> logging of such DDLs in table level DDL replication.
> 2. apply worker will fail to replay such command due to missing
> objects if such DDLs didn't get filtered on the publisher for some
> reason. This should be rare and I think it's OK even if it happens,
> we'll find out
> why and fix it.
>

FWIW, both these cases could be handled with the deparsing approach,
and the handling related to the drop of multiple tables where only a
few are published is already done in the last POC patch shared by Ajin
[1].

[1] - https://www.postgresql.org/message-id/CAFPTHDaBodoZ5c7U1uyokbvq%2BzUvhJ4ps-7H66nHGw45UnO0OQ%40mail.gmail.com

--
With Regards,
Amit Kapila.

On Thursday, May 26, 2022 1:46 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Tue, May 10, 2022 at 9:33 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > On Fri, May 6, 2022 at 11:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > > As we have hacked CreatePublication function for this POC, the
> > > regression tests are not passing but we can easily change it so that
> > > we invoke new functionality with the syntax proposed in this thread
> > > or with some other syntax and we shall do that in the next patch
> > > unless this approach is not worth pursuing.
> > >
> > > This POC is prepared by Ajin Cherian, Hou-San, and me.
> > >
> > > Thoughts?
> > >
> > > [1] -
> > >
> https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.n
> > > o-ip.org
> >
> > I have updated Amit's patch by including a public action "create" when
> > creating publication which is turned off by default.
> > Now the 'make check' tests pass. I also fixed a problem that failed to
> > create tables when the table has a primary key.
>
> I have updated this "deparse" patch-set to include support for 'drop table':
>

Here is the new version POC patches which add support for 'ALTER TABLE'

For non-rewrite ALTER TABLE command:
we deparse the command and WAL log the deparsed json string with the relid
of the altered table at table_rewrite event trigger. The WALSender decodes
the WAL and sends it to subscriber if the altered table is published. Most
of ALTER TABLE command are supported except some commands(DDL related to
PARTITIONED TABLE ...) that introduced recently which haven't been
supported by the current ddl_deparser, we will support that later.

For table_rewrite ALTER TABLE command:
(ALTER COLUMN TYPE, ADD COLUMN DEFAULT, SET LOGGED, SET ACCESS METHOD)

we deparse the command and WAL log the deparsed json string with the relid
of the altered table at table_rewrite event trigger. The WALSender decodes
the WAL and sends it to subscriber if the altered table is published.
Then, the WALSender will convert the upcoming rewrite INSERTs to UPDATEs
and send them to subscriber so that the data between publisher and
subscriber can always be consistent. Note that the tables that publish
rewrite ddl must have a replica identity configured in order to be able to
replicate the upcoming rewrite UPDATEs.

We do this way because of two reasons:
(1) The data before the rewrite ddl could already be different among
publisher and subscriber. To make sure the extra data in subscriber which
doesn't exist in publisher also get rewritten, we need to let the
subscriber execute the original rewrite ddl to rewrite all the data at
first.

(2) the data after executing rewrite ddl could be different among
publisher and subscriber(due to different functions/operators used during
rewrite), so we need to replicate the rewrite UPDATEs to keep the data
consistent.

Here is the example for the rewrite ddl: ALTER TABLE ADD COLUMN DEFAULT:

A table on the publisher side has rows:
ddl_test(a)
a
----
1
2

The same table on the subscriber side has rows:
ddl_test(a)
a
----
1
2
3
4

-----------------------------------
If we execute "ALTER TABLE ddl_test ADD COLUMN b int DEFAULT random();" on
publisher. The row(1,2) on subscriber will be updated by the rewrite
UPDATE received from publisher. The row(3,4) will be updated by exectuing
the "ADD COLUMN b int DEFAULT random();" on the subscriber.
-----------------------------------

ddl_test(a,b)
a|b
----
1|random num(pub)
2|random num(pub)

The same table on the subscriber side has rows:
ddl_test(a,b)
a|b
----
1|random num(pub)
2|random num(pub)
3|random num(sub)
4|random num(sub)

TO IMPROVE:

This approach could be improved by letting the subscriber try to update
the extra data itself instead of doing fully rewrite ddl and use the
upcoming rewrite UPDATEs to rewrite the rest data. To achieve this, we
could modify the deparsed json string to temporarily remove the rewrite
part and add some logic in subscriber to update the extra data.
Besides, we may not need to send rewrite changes for all type of rewrite
ddl, for example, it seems fine to skip sending rewrite changes for ALTER
TABLE SET LOGGED as the data in the table doesn't actually be changed. We
could use the deparser and event trigger to filter these ddls and skip
sending rewrite changes for them.

Best regards,
Hou zj

On Fri, May 27, 2022 at 7:19 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> Hi Masahiko,
>
> > Thank you for updating the patches!
> >
> > I've not looked at these patches in-depth yet but with this approach,
> > what do you think we can handle the DDL syntax differences between
> > major versions? DDL syntax or behavior could be changed by future
> > changes and I think we need to somehow deal with the differences. For
>
> > example, if the user uses logical replication for major version
> > upgrade, the publisher is older than the subscriber. We might have to
> > rewrite the DDL before applying to the subscriber because the DDL
> > executed on the publisher no longer work on a new PostgreSQL version
>
> I don't think we will allow this kind of situation to happen in the
> first place for
> backward compatibility.

It seems like a big limitation to me.

> If a DDL no longer works on a new version of
> PostgreSQL, the user will have to change the application code as well.
> So even if it happens for
> whatever reason, we could either
> 1. fail the apply worker and let the user fix such DDL because they'll
> have to fix the application code anyway when this happens.

Once the apply worker received the DDL, if the DDL doesn't work on the
subscriber, it will enter an infinite loop until the problem is fixed.
If the failure is due to a syntax error, how does the user fix it?

> 2. add guard rail logic in the apply worker to automatically fix such
> DDL if possible, knowing the version of the source and target. Similar
> logic must have been implemented for pg_dump/restore/upgrade.

If I'm not missing something, there is no such implementation in
pg_dump/restore/upgrade. When we use pg_dump/pg_restore for major
version upgrades, we usually use the newer version pg_dump to fetch
objects from the older version server, then restore the objects by
using the newer version pg_restore.

>
> > or we might have to add some options to the DDL before the application
> > in order to keep the same behavior. This seems to require a different
> > solution from what the patch does for the problem you mentioned such
>
> > as "DDL involving multiple tables where only some tables are
> > replicated”.
>
> First of all, this case can only happen when the customer chooses to
> only replicate a subset of the tables in a database in which case
> table level DDL replication is chosen instead of database level DDL
> replication (where all tables
> and DDLs are replicated). I think the solution would be:
> 1. make best effort to detect such DDLs on the publisher and avoid
> logging of such DDLs in table level DDL replication.

I think it's better to support this case.

> 2. apply worker will fail to replay such command due to missing
> objects if such DDLs didn't get filtered on the publisher for some
> reason. This should be rare and I think it's OK even if it happens,
> we'll find out
> why and fix it.

I'm not sure it's rare since replicating a subset of tables is a
common use case of logical replication. But even if we want to go this
way I think we should consider how to fix it at this stage, otherwise
we will end up redesigning it.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Fri, May 27, 2022 at 11:03 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, May 27, 2022 at 3:49 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> >
> > Hi Masahiko,
> >
> > > Thank you for updating the patches!
> > >
> > > I've not looked at these patches in-depth yet but with this approach,
> > > what do you think we can handle the DDL syntax differences between
> > > major versions? DDL syntax or behavior could be changed by future
> > > changes and I think we need to somehow deal with the differences. For
> >
> > > example, if the user uses logical replication for major version
> > > upgrade, the publisher is older than the subscriber. We might have to
> > > rewrite the DDL before applying to the subscriber because the DDL
> > > executed on the publisher no longer work on a new PostgreSQL version
> >
> > I don't think we will allow this kind of situation to happen in the
> > first place for
> > backward compatibility. If a DDL no longer works on a new version of
> > PostgreSQL, the user will have to change the application code as well.
> > So even if it happens for
> > whatever reason, we could either
> > 1. fail the apply worker and let the user fix such DDL because they'll
> > have to fix the application code anyway when this happens.
> > 2. add guard rail logic in the apply worker to automatically fix such
> > DDL if possible, knowing the version of the source and target. Similar
> > logic must have been implemented for pg_dump/restore/upgrade.
> >
> > > or we might have to add some options to the DDL before the application
> > > in order to keep the same behavior. This seems to require a different
> > > solution from what the patch does for the problem you mentioned such
> >
> > > as "DDL involving multiple tables where only some tables are
> > > replicated”.
> >
> > First of all, this case can only happen when the customer chooses to
> > only replicate a subset of the tables in a database in which case
> > table level DDL replication is chosen instead of database level DDL
> > replication (where all tables
> > and DDLs are replicated). I think the solution would be:
> > 1. make best effort to detect such DDLs on the publisher and avoid
> > logging of such DDLs in table level DDL replication.
> > 2. apply worker will fail to replay such command due to missing
> > objects if such DDLs didn't get filtered on the publisher for some
> > reason. This should be rare and I think it's OK even if it happens,
> > we'll find out
> > why and fix it.
> >
>
> FWIW, both these cases could be handled with the deparsing approach,
> and the handling related to the drop of multiple tables where only a
> few are published is already done in the last POC patch shared by Ajin
> [1].
>

Right. So I'm inclined to think that deparsing approach is better from
this point as well as the point mentioned by Álvaro before[1].

Regards,

[1] https://www.postgresql.org/message-id/202204081134.6tcmf5cxl3sz%40alvherre.pgsql

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Monday, May 30, 2022 2:52 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Fri, May 27, 2022 at 11:03 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, May 27, 2022 at 3:49 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > >
> > > Hi Masahiko,
> > >
> > > > Thank you for updating the patches!
> > > >
> > > > I've not looked at these patches in-depth yet but with this approach,
> > > > what do you think we can handle the DDL syntax differences between
> > > > major versions? DDL syntax or behavior could be changed by future
> > > > changes and I think we need to somehow deal with the differences. For
> > >
> > > > example, if the user uses logical replication for major version
> > > > upgrade, the publisher is older than the subscriber. We might have to
> > > > rewrite the DDL before applying to the subscriber because the DDL
> > > > executed on the publisher no longer work on a new PostgreSQL version
> > >
> > > I don't think we will allow this kind of situation to happen in the
> > > first place for
> > > backward compatibility. If a DDL no longer works on a new version of
> > > PostgreSQL, the user will have to change the application code as well.
> > > So even if it happens for
> > > whatever reason, we could either
> > > 1. fail the apply worker and let the user fix such DDL because they'll
> > > have to fix the application code anyway when this happens.
> > > 2. add guard rail logic in the apply worker to automatically fix such
> > > DDL if possible, knowing the version of the source and target. Similar
> > > logic must have been implemented for pg_dump/restore/upgrade.
> > >
> > > > or we might have to add some options to the DDL before the application
> > > > in order to keep the same behavior. This seems to require a different
> > > > solution from what the patch does for the problem you mentioned such
> > >
> > > > as "DDL involving multiple tables where only some tables are
> > > > replicated”.
> > >
> > > First of all, this case can only happen when the customer chooses to
> > > only replicate a subset of the tables in a database in which case
> > > table level DDL replication is chosen instead of database level DDL
> > > replication (where all tables
> > > and DDLs are replicated). I think the solution would be:
> > > 1. make best effort to detect such DDLs on the publisher and avoid
> > > logging of such DDLs in table level DDL replication.
> > > 2. apply worker will fail to replay such command due to missing
> > > objects if such DDLs didn't get filtered on the publisher for some
> > > reason. This should be rare and I think it's OK even if it happens,
> > > we'll find out
> > > why and fix it.
> > >
> >
> > FWIW, both these cases could be handled with the deparsing approach,
> > and the handling related to the drop of multiple tables where only a
> > few are published is already done in the last POC patch shared by Ajin
> > [1].
> >
>
> Right. So I'm inclined to think that deparsing approach is better from
> this point as well as the point mentioned by Álvaro before[1].

I agree. One more point about deparsing approach is that it can also
help to replicate CREATE TABLE AS/SELECT INTO in a better way.

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 temporarily use the second idea which introduce a new type event
trigger in the 0003 POC patch.

In the POC patch, we deparse the command in the table_create event trigger and
WAL log the deparsed json string. The walsender will send the string to
subscriber. And incoming INSERTs will also be replicated.

Best regards,
Hou zj

On Thu, Jun 2, 2022 at 5:44 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)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?

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?

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

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

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

--
With Regards,
Amit Kapila.

On Friday, June 3, 2022 7:16 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Jun 2, 2022 at 5:44 PM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)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

Hi,

I did some research for one potential problem[1] mentioned earlier which is related
to the function execution when replicating DDL.

[1]> 4. Statements that have nondeterministic side effects (e.g., as caused
> by triggers, stored procedures, user-defined functions) may result in
> different side effects occurring on each subscriber.
>
> Think how to handle triggers and functions with same name but different
> purpose.
>

Examples:
ALTER TABLE ADD CONSTRAINT func()
ALTER TABLE ADD COLUMN DEFAULT func()
CREATE TRIGGER ... execute procedure func()
...

When replication the above DDLs, there are some cases we need to think about.

----------------
1) The functions used in DDL have same definition among pub/sub.

In this case, if the functions include DML/DDL operations, it will result in
unexpected behavior.

For example:

--- both pub and sub have the same function test_func().
create function test_func()
returns integer as '
begin
CREATE TABLE test(a int);
INSERT INTO test values(1);
return 0;
end
' language plpgsql;

--- We replicate the following DDL
ALTER TABLE t ADD COLUMN a int DEFAULT test_func();

There are three SQLs that would be replicated to the subscriber:
CREATE TABLE test(a int);
INSERT(1);
ALTER TABLE t ADD COLUMN a int DEFAULT test_func();

Then, we would create the table "test" twice and insert same value twice(first by
executing DEFAULT function, second by replaying the CREATE TABLE and INSERT
command) on subcriber.

One possible idea is that we only allow replicating 'immutable/stable' function
which won't include write action so that we don't have this problem. But it
seems a bit restrictive.

----------------

2) The functions used in DDL have different definitions among pub/sub.

I am not sure how to handle this case as this could result in unpredictable
behavior based on the different definitions. And it's difficult to compare the
function definition among pub/sub because the function could call other
functions nested.

OTOH, the current behavior of this case on HEAD is that we don't check the
consistency of the functions executed on pub/sub. For example: the row triggers
will always be fired on subscriber(when enabled) without checking if the same
trigger exists on publisher. So, it might be acceptable if we document this
restriction, although I am not sure.

*******************

- About the solution for the above two points. I think one solution could be:

We can document that user should make sure the DDL to be replicated should not
execute any function which could execute DML/DDL. This seems acceptable as it's
a pretty rare case to execute DML/DDL in a CONSTRAINT function or DEFAULT
function. And the document[1] already suggest similar thing for CONSTRAINT
function. Besides, we can also document that and the functions should be
defined in a consistent way among pub/sub.

[1] https://www.postgresql.org/docs/devel/ddl-constraints.html
> PostgreSQL assumes that CHECK constraints' conditions are immutable, that is,
> they will always give the same result for the same input row. This assumption
> is what justifies examining CHECK constraints only when rows are inserted or
> updated, and not at other times. (The warning above about not referencing
> other table data is really a special case of this restriction.)

- Another solution could be:

We coud introduce a new function flag called (replica_safety) and the values
could be 'safe'/'unsafe'. 'safe' indicates that it's safe to be replicated to
subscriber and it's safe to be executed when replay the DDL from other
publisher.

When replicating the DDL on publisher, we first check if the functions used in
DDL are replica_safe, if not, we don't replicate that DDL. If yes, before
replicating the DDL, we first send a new type FUNCTIONS message which include
functions(used in DDL or ..) information to the subscriber which is similar to
the RELATION messages. And on subscriber, we check if the received functions
exist and if they are also marked replica safe on subscriber. if functions are
not replica safe on sub, we report an error and suggest user to adjust the
problematic function. I haven't tried to implement it so I could miss
something. Thoughts ?

BTW, attach the POC patch set which only fixed a CFbot error and
added some testcases provided by Osumi-san.

Best regards,
Hou zj

On Thu, Jun 9, 2022 at 5:14 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Hi,
>
> I did some research for one potential problem[1] mentioned earlier which is related
> to the function execution when replicating DDL.
>
> [1]> 4. Statements that have nondeterministic side effects (e.g., as caused
> > by triggers, stored procedures, user-defined functions) may result in
> > different side effects occurring on each subscriber.
> >
> > Think how to handle triggers and functions with same name but different
> > purpose.
> >
>
> Examples:
> ALTER TABLE ADD CONSTRAINT func()
> ALTER TABLE ADD COLUMN DEFAULT func()
> CREATE TRIGGER ... execute procedure func()
> ...
>
> When replication the above DDLs, there are some cases we need to think about.
>
> ----------------
> 1) The functions used in DDL have same definition among pub/sub.
>
> In this case, if the functions include DML/DDL operations, it will result in
> unexpected behavior.
>
> For example:
>
> --- both pub and sub have the same function test_func().
> create function test_func()
> returns integer as '
> begin
> CREATE TABLE test(a int);
> INSERT INTO test values(1);
> return 0;
> end
> ' language plpgsql;
>
> --- We replicate the following DDL
> ALTER TABLE t ADD COLUMN a int DEFAULT test_func();
>
> There are three SQLs that would be replicated to the subscriber:
> CREATE TABLE test(a int);
> INSERT(1);
> ALTER TABLE t ADD COLUMN a int DEFAULT test_func();
>
> Then, we would create the table "test" twice and insert same value twice(first by
> executing DEFAULT function, second by replaying the CREATE TABLE and INSERT
> command) on subcriber.
>

The other kind of problem is that we don't know whether the tables
being accessed by these DDL/DML are published or not. So blindly
allowing such functions can allow unintended clauses like the tables
accessed in those functions may not even exist on the subscriber-side.

> One possible idea is that we only allow replicating 'immutable/stable' function
> which won't include write action so that we don't have this problem. But it
> seems a bit restrictive.
>
> ----------------
>
> 2) The functions used in DDL have different definitions among pub/sub.
>
> I am not sure how to handle this case as this could result in unpredictable
> behavior based on the different definitions. And it's difficult to compare the
> function definition among pub/sub because the function could call other
> functions nested.
>
> OTOH, the current behavior of this case on HEAD is that we don't check the
> consistency of the functions executed on pub/sub. For example: the row triggers
> will always be fired on subscriber(when enabled) without checking if the same
> trigger exists on publisher. So, it might be acceptable if we document this
> restriction, although I am not sure.
>
> *******************
>
> - About the solution for the above two points. I think one solution could be:
>
> We can document that user should make sure the DDL to be replicated should not
> execute any function which could execute DML/DDL. This seems acceptable as it's
> a pretty rare case to execute DML/DDL in a CONSTRAINT function or DEFAULT
> function. And the document[1] already suggest similar thing for CONSTRAINT
> function. Besides, we can also document that and the functions should be
> defined in a consistent way among pub/sub.
>
> [1] https://www.postgresql.org/docs/devel/ddl-constraints.html
> > PostgreSQL assumes that CHECK constraints' conditions are immutable, that is,
> > they will always give the same result for the same input row. This assumption
> > is what justifies examining CHECK constraints only when rows are inserted or
> > updated, and not at other times. (The warning above about not referencing
> > other table data is really a special case of this restriction.)
>
> - Another solution could be:
>
> We coud introduce a new function flag called (replica_safety) and the values
> could be 'safe'/'unsafe'. 'safe' indicates that it's safe to be replicated to
> subscriber and it's safe to be executed when replay the DDL from other
> publisher.
>

Yeah, something like this could be a good idea but I think for the
first version we should simply raise a WARNING for such DDLs
indicating that they won't be replicated.

--
With Regards,
Amit Kapila.

> > > > > I've not looked at these patches in-depth yet but with this approach,
> > > > > what do you think we can handle the DDL syntax differences between
> > > > > major versions? DDL syntax or behavior could be changed by future
> > > > > changes and I think we need to somehow deal with the differences. For
> > > >
> > > > > example, if the user uses logical replication for major version
> > > > > upgrade, the publisher is older than the subscriber. We might have to
> > > > > rewrite the DDL before applying to the subscriber because the DDL
> > > > > executed on the publisher no longer work on a new PostgreSQL version
> > > >
> > > > I don't think we will allow this kind of situation to happen in the
> > > > first place for
> > > > backward compatibility. If a DDL no longer works on a new version of
> > > > PostgreSQL, the user will have to change the application code as well.
> > > > So even if it happens for
> > > > whatever reason, we could either
> > > > 1. fail the apply worker and let the user fix such DDL because they'll
> > > > have to fix the application code anyway when this happens.
> > > > 2. add guard rail logic in the apply worker to automatically fix such
> > > > DDL if possible, knowing the version of the source and target. Similar
> > > > logic must have been implemented for pg_dump/restore/upgrade.
> > > >
> > > > > or we might have to add some options to the DDL before the application
> > > > > in order to keep the same behavior. This seems to require a different
> > > > > solution from what the patch does for the problem you mentioned such
> > > >
> > > > > as "DDL involving multiple tables where only some tables are
> > > > > replicated”.
> > > >
> > > > First of all, this case can only happen when the customer chooses to
> > > > only replicate a subset of the tables in a database in which case
> > > > table level DDL replication is chosen instead of database level DDL
> > > > replication (where all tables
> > > > and DDLs are replicated). I think the solution would be:
> > > > 1. make best effort to detect such DDLs on the publisher and avoid
> > > > logging of such DDLs in table level DDL replication.
> > > > 2. apply worker will fail to replay such command due to missing
> > > > objects if such DDLs didn't get filtered on the publisher for some
> > > > reason. This should be rare and I think it's OK even if it happens,
> > > > we'll find out
> > > > why and fix it.
> > > >
> > >
> > > FWIW, both these cases could be handled with the deparsing approach,
> > > and the handling related to the drop of multiple tables where only a
> > > few are published is already done in the last POC patch shared by Ajin
> > > [1].
> > >
> >
> > Right. So I'm inclined to think that deparsing approach is better from
> > this point as well as the point mentioned by Álvaro before[1].
>
> I agree. One more point about deparsing approach is that it can also
> help to replicate CREATE TABLE AS/SELECT INTO in a better way.
>
> 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 temporarily use the second idea which introduce a new type event
> trigger in the 0003 POC patch.

Hi, I agree that an intermediate structured format (with all objects
schema qualified) makes it easier to handle syntax differences between
the publisher and the subscriber. Such structured format is also
likely easier to use by other logical replication consumers.
However, to make things more maintainable, would it be better to use
the existing serialization/deserialization functions in
out/readfuncs.c to generate the parsetree representation of the DDL
command?

It turns out support for DDL commands are mostly missing in
out/readfuncs at the moment. see the following comment from
outfuncs.c:

* Output support for raw parsetrees
* is somewhat incomplete, too; in particular, utility statements are
* almost entirely unsupported. We try to support everything that can
* appear in a raw SELECT, though.

So what about adding support for utility statements in out/readfuncs.c
so that nodeToString()/stringToNode() works on raw parsetrees of
utility statements? I think there'll be some benefits:
1. It's less code compared to introducing brand new
serialization/deserialization code for DDL commands.
2. It's more maintainable since hackers are already familiar with
out/readfuncs.c.
3. Support for utility statements in out/readfuncs.c may turn out to
be useful for future features that need serialization/deserialization
of DDL raw parsetrees.

In patch 0012 I explored the above idea by changing the logging format
of logical ddlmessage to that of the serialization of the raw
parsetree of the DDL command using nodeToString(), the apply worker
deserializes the message back to the raw parsetree by calling
stringToNode().
Here are the detailed changes:
- Adding serialization/deserialization functions in outfuncs.c/readfuncs.c
for CreateTableStmt, AlterTableStmt, DropStmt, CreateFunctionStmt and
AlterFunctionStmt.
- Modified the serialization process to always schema qualify object names,
this is done by outQualifiedName() and a change in _outRangeVar().
- Change the input of LogLogicalDDLMessage() to use nodeToString(parsetree).
- Change the apply worker to call stringToNode(ddlmessage) to get
the raw parsetree back and then directly execute the parsetree.

This patch doesn't introduce any deparsing functionality yet, but it
also provides the flexibility to edit the command being replayed by
directly modifying the raw parsetree on the apply worker (e.g. setting
the
missing_ok flag for DropStmt is equivalent to adding "IF EXIST" to the
statement).

Thoughts?

Regards,
Zheng Li

On Sunday, June 12, 2022 2:46 PM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
>
> > > > > > I've not looked at these patches in-depth yet but with this
> > > > > > approach, what do you think we can handle the DDL syntax
> > > > > > differences between major versions? DDL syntax or behavior
> > > > > > could be changed by future changes and I think we need to
> > > > > > somehow deal with the differences. For
> > > > >
> > > > > > example, if the user uses logical replication for major
> > > > > > version upgrade, the publisher is older than the subscriber.
> > > > > > We might have to rewrite the DDL before applying to the
> > > > > > subscriber because the DDL executed on the publisher no longer
> > > > > > work on a new PostgreSQL version
> > > > >
> > > > > I don't think we will allow this kind of situation to happen in
> > > > > the first place for backward compatibility. If a DDL no longer
> > > > > works on a new version of PostgreSQL, the user will have to
> > > > > change the application code as well.
> > > > > So even if it happens for
> > > > > whatever reason, we could either 1. fail the apply worker and
> > > > > let the user fix such DDL because they'll have to fix the
> > > > > application code anyway when this happens.
> > > > > 2. add guard rail logic in the apply worker to automatically fix
> > > > > such DDL if possible, knowing the version of the source and
> > > > > target. Similar logic must have been implemented for
> pg_dump/restore/upgrade.
> > > > >
> > > > > > or we might have to add some options to the DDL before the
> > > > > > application in order to keep the same behavior. This seems to
> > > > > > require a different solution from what the patch does for the
> > > > > > problem you mentioned such
> > > > >
> > > > > > as "DDL involving multiple tables where only some tables are
> > > > > > replicated”.
> > > > >
> > > > > First of all, this case can only happen when the customer
> > > > > chooses to only replicate a subset of the tables in a database
> > > > > in which case table level DDL replication is chosen instead of
> > > > > database level DDL replication (where all tables and DDLs are
> > > > > replicated). I think the solution would be:
> > > > > 1. make best effort to detect such DDLs on the publisher and
> > > > > avoid logging of such DDLs in table level DDL replication.
> > > > > 2. apply worker will fail to replay such command due to missing
> > > > > objects if such DDLs didn't get filtered on the publisher for
> > > > > some reason. This should be rare and I think it's OK even if it
> > > > > happens, we'll find out why and fix it.
> > > > >
> > > >
> > > > FWIW, both these cases could be handled with the deparsing
> > > > approach, and the handling related to the drop of multiple tables
> > > > where only a few are published is already done in the last POC
> > > > patch shared by Ajin [1].
> > > >
> > >
> > > Right. So I'm inclined to think that deparsing approach is better
> > > from this point as well as the point mentioned by Álvaro before[1].
> >
> > I agree. One more point about deparsing approach is that it can also
> > help to replicate CREATE TABLE AS/SELECT INTO in a better way.
> >
> > 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 temporarily use the second idea which
> > introduce a new type event trigger in the 0003 POC patch.
>
> Hi, I agree that an intermediate structured format (with all objects schema
> qualified) makes it easier to handle syntax differences between the publisher
> and the subscriber. Such structured format is also likely easier to use by other
> logical replication consumers.
> However, to make things more maintainable, would it be better to use the
> existing serialization/deserialization functions in out/readfuncs.c to generate
> the parsetree representation of the DDL command?
>
> It turns out support for DDL commands are mostly missing in out/readfuncs at
> the moment. see the following comment from
> outfuncs.c:
>
> * Output support for raw parsetrees
> * is somewhat incomplete, too; in particular, utility statements are
> * almost entirely unsupported. We try to support everything that can
> * appear in a raw SELECT, though.
>
> So what about adding support for utility statements in out/readfuncs.c so that
> nodeToString()/stringToNode() works on raw parsetrees of utility statements? I
> think there'll be some benefits:
> 1. It's less code compared to introducing brand new
> serialization/deserialization code for DDL commands.
> 2. It's more maintainable since hackers are already familiar with
> out/readfuncs.c.
> 3. Support for utility statements in out/readfuncs.c may turn out to be useful
> for future features that need serialization/deserialization of DDL raw
> parsetrees.
>
> In patch 0012 I explored the above idea by changing the logging format of
> logical ddlmessage to that of the serialization of the raw parsetree of the DDL
> command using nodeToString(), the apply worker deserializes the message
> back to the raw parsetree by calling stringToNode().
> Here are the detailed changes:
> - Adding serialization/deserialization functions in outfuncs.c/readfuncs.c
> for CreateTableStmt, AlterTableStmt, DropStmt, CreateFunctionStmt and
> AlterFunctionStmt.
> - Modified the serialization process to always schema qualify object names,
> this is done by outQualifiedName() and a change in _outRangeVar().
> - Change the input of LogLogicalDDLMessage() to use
> nodeToString(parsetree).
> - Change the apply worker to call stringToNode(ddlmessage) to get the
> raw parsetree back and then directly execute the parsetree.
>
> This patch doesn't introduce any deparsing functionality yet, but it also
> provides the flexibility to edit the command being replayed by directly
> modifying the raw parsetree on the apply worker (e.g. setting the missing_ok
> flag for DropStmt is equivalent to adding "IF EXIST" to the statement).
>
> Thoughts?