Re: Support logical replication of DDLs

On Fri, Nov 25, 2022 at 5:23 PM Zheng Li <zhengli10@gmail.com> wrote:
>
> Hello,
>
> Thanks for the feedback.
>
> > I have been following this patch for a long time.
> > Recently, I started to try to test it. I found several bugs
> > here and want to give you feedback.
> >
> > 1. CREATE TABLE LIKE
> >   I found that this case may be repication incorrectly.
> >    You can run the following SQL statement:
> >    ```
> >    CREATE TABLE ctlt1 (a text CHECK (length(a) > 2) PRIMARY KEY, b text);
> > ALTER TABLE ctlt1 ALTER COLUMN a SET STORAGE MAIN;
> > ALTER TABLE ctlt1 ALTER COLUMN b SET STORAGE EXTERNAL;
> > CREATE TABLE ctlt1_like (LIKE ctlt1 INCLUDING ALL);
> >    ```
> >    The ctlt1_like table will not be able to correct the replication.
> >   I think this is because create table like statement is captured by
> >   the event trigger to a create table statement and multiple alter table statements.
> >   There are some overlaps between them, and an error is reported when downstream replication occurs.
>
> I looked into this case. The root cause is the statement
>
> CREATE TABLE ctlt1_like (LIKE ctlt1 INCLUDING ALL);
>
> is executed internally using 3 DDLs:
> 1. CREATE TABLE ctlt1_like (LIKE ctlt1 INCLUDING ALL); --The top level command
> 2. ALTER TABLE ctlt1_like ADD CONSTRAINT ctlt1_a_check CHECK
> (length(a) > 2); --The first subcommand
> 3. CREATE UNIQUE INDEX ctlt1_like_pkey on ctlt1_like (a); --The second
> subcommand that creates the primary key index
>
> All three commands are captured by the event trigger. The first and
> second command ends up getting deparsed, WAL-logged and
> replayed on the subscriber. The replay of the ALTER TABLE command
> causes a duplicate constraint error. The problem is that
> while subcommands are captured by event triggers by default, they
> don't need to be deparsed and WAL-logged for DDL replication.
> To do that we can pass the isCompleteQuery variable in
> ProcessUtilitySlow to EventTriggerCollectSimpleCommand() and
> EventTriggerAlterTableEnd() and make this information available in
> CollectedCommand so that any subcommands can be skipped.

Attaching the proposed fix in
v40-0005-Do-not-generate-WAL-log-for-non-top-level-DDL-comman.patch.
This patch adds a new boolean field isTopLevelCommand to
CollectedCommand so that non-top level command
can be skipped in the DDL replication event trigger functions. The
patch also makes the information available by
passing the isTopLevel variable in ProcessUtilitySlow to several
EventTriggerCollect functions such as
EventTriggerCollectSimpleCommand and EventTriggerAlterTableStart.

> 2. ALTER TABLE (inherits)
> case:
> ```
> CREATE TABLE gtest30 (
> a int,
> b int GENERATED ALWAYS AS (a * 2) STORED
> );
> CREATE TABLE gtest30_1 () INHERITS (gtest30);
> ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
> ```
> After this case is executed in the publication, the following error occurs in the subscription :
>
> ERROR:  column "b" of relation "gtest30" is not a stored generated column
> STATEMENT:  ALTER TABLE public.gtest30 ALTER COLUMN b DROP EXPRESSION, ALTER COLUMN b DROP EXPRESSION
>
> Obviously, the column modifications of the inherited table were also captured,

Yes, I can confirm that the column modifications of the inherited
table were also captured as a subcommand of "ALTER TABLE gtest30 ALTER
COLUMN b DROP EXPRESSION;". This feels wrong to me, because the
subcommand
On Fri, Nov 25, 2022 at 5:23 PM Zheng Li <zhengli10@gmail.com> wrote:
>
> Hello,
>
> Thanks for the feedback.
>
> > I have been following this patch for a long time.
> > Recently, I started to try to test it. I found several bugs
> > here and want to give you feedback.
> >
> > 1. CREATE TABLE LIKE
> >   I found that this case may be repication incorrectly.
> >    You can run the following SQL statement:
> >    ```
> >    CREATE TABLE ctlt1 (a text CHECK (length(a) > 2) PRIMARY KEY, b text);
> > ALTER TABLE ctlt1 ALTER COLUMN a SET STORAGE MAIN;
> > ALTER TABLE ctlt1 ALTER COLUMN b SET STORAGE EXTERNAL;
> > CREATE TABLE ctlt1_like (LIKE ctlt1 INCLUDING ALL);
> >    ```
> >    The ctlt1_like table will not be able to correct the replication.
> >   I think this is because create table like statement is captured by
> >   the event trigger to a create table statement and multiple alter table statements.
> >   There are some overlaps between them, and an error is reported when downstream replication occurs.
>
> I looked into this case. The root cause is the statement
>
> CREATE TABLE ctlt1_like (LIKE ctlt1 INCLUDING ALL);
>
> is executed internally using 3 DDLs:
> 1. CREATE TABLE ctlt1_like (LIKE ctlt1 INCLUDING ALL); --The top level command
> 2. ALTER TABLE ctlt1_like ADD CONSTRAINT ctlt1_a_check CHECK
> (length(a) > 2); --The first subcommand
> 3. CREATE UNIQUE INDEX ctlt1_like_pkey on ctlt1_like (a); --The second
> subcommand that creates the primary key index
>
> All three commands are captured by the event trigger. The first and
> second command ends up getting deparsed, WAL-logged and
> replayed on the subscriber. The replay of the ALTER TABLE command
> causes a duplicate constraint error. The problem is that
> while subcommands are captured by event triggers by default, they
> don't need to be deparsed and WAL-logged for DDL replication.
> To do that we can pass the isCompleteQuery variable in
> ProcessUtilitySlow to EventTriggerCollectSimpleCommand() and
> EventTriggerAlterTableEnd() and make this information available in
> CollectedCommand so that any subcommands can be skipped.

Attaching the proposed fix in
v40-0005-Do-not-generate-WAL-log-for-non-top-level-DDL-comman.patch.
This patch adds a new boolean field isTopLevelCommand to
CollectedCommand so that non-top level command
can be skipped in the DDL replication event trigger functions. The
patch also makes the information available by
passing the isTopLevel variable in ProcessUtilitySlow to several
EventTriggerCollect functions such as
EventTriggerCollectSimpleCommand and EventTriggerAlterTableStart.

> 2. ALTER TABLE (inherits)
> case:
> ```
> CREATE TABLE gtest30 (
> a int,
> b int GENERATED ALWAYS AS (a * 2) STORED
> );
> CREATE TABLE gtest30_1 () INHERITS (gtest30);
> ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
> ```
> After this case is executed in the publication, the following error occurs in the subscription :
>
> ERROR:  column "b" of relation "gtest30" is not a stored generated column
> STATEMENT:  ALTER TABLE public.gtest30 ALTER COLUMN b DROP EXPRESSION, ALTER COLUMN b DROP EXPRESSION
>
> Obviously, the column modifications of the inherited table were also captured,

Yes, I can confirm that the column modifications of the inherited
table gtest30_1 were also captured as a subcommand of "ALTER TABLE
gtest30 ALTER COLUMN b DROP EXPRESSION;". This feels wrong to me,
because the subcommand "ALTER COLUMN b DROP EXPRESSION" is collected
for ALTER TABLE gtest30 but it's actually meant for the inherited
table gtest30_1. I think we should fix the capture of the subcommand
in a way that we know it's meant to be executed on the inherited table
gtest30_1.

Regards,
Zheng