Re: Support logical replication of DDLs

On Tue, Jun 21, 2022 at 5:49 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, June 20, 2022 11:32 AM houzj(dot)fnst(at)fujitsu(dot)com <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Saturday, June 18, 2022 3:38 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > > On Wed, Jun 15, 2022 at 12:00 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > > wrote:
> > > >
> > > > On Wed, Jun 15, 2022 at 5:44 AM Zheng Li <zhengli10(at)gmail(dot)com> wrote:
> > > > >
> > > > >
> > > > > While I agree that the deparser is needed to handle the potential
> > > > > syntax differences between the pub/sub, I think it's only relevant
> > > > > for the use cases where only a subset of tables in the database are
> > > > > replicated. For other use cases where all tables, functions and
> > > > > other objects need to be replicated, (for example, creating a
> > > > > logical replica for major version upgrade) there won't be any syntax
> > > > > difference to handle and the schemas are supposed to match exactly
> > > > > between the pub/sub. In other words the user seeks to create an
> > > > > identical replica of the source database and the DDLs should be
> > > > > replicated as is in this case.
> > > > >
> > > >
> > > > I think even for database-level replication we can't assume that
> > > > source and target will always have the same data in which case "Create
> > > > Table As ..", "Alter Table .. " kind of statements can't be replicated
> > > > as it is because that can lead to different results.
> > > "Create Table As .." is already handled by setting the skipData flag of the
> > > statement parsetreee before replay:
> > >
> > > /*
> > > * Force skipping data population to avoid data inconsistency.
> > > * Data should be replicated from the publisher instead.
> > > */
> > > castmt->into->skipData = true;
> > >
> > > "Alter Table .. " that rewrites with volatile expressions can also be handled
> > > without any syntax change, by enabling the table rewrite replication and
> > > converting the rewrite inserts to updates. ZJ's patch introduced this solution.
> > > I've also adopted this approach in my latest patch
> > > 0012-Support-replication-of-ALTER-TABLE-commands-that-rew.patch
> > >
> > > > The other point
> > > > is tomorrow we can extend the database level option/syntax to exclude
> > > > a few objects (something like [1]) as well in which case we again need
> > > > to filter at the publisher level
> > >
> > > I think for such cases it's not full database replication and we could treat it as
> > > table level DDL replication, i.e. use the the deparser format.
> >
> > Hi,
> >
> > Here are some points in my mind about the two approaches discussed here.
> >
> > 1) search_patch vs schema qualify
> >
> > Again, I still think it will bring more flexibility and security by schema qualify the
> > objects in DDL command as mentioned before[1].
> >
> > Besides, a schema qualified DDL is also more appropriate for other use
> > cases(e.g. a table-level replication). As it's possible the schema is different
> > between pub/sub and it's easy to cause unexpected and undetectable failure if
> > we just log the search_path.
> >
> > It makes more sense to me to have the same style WAL log(schema qualified)
> > for
> > both database level or table level replication as it will bring more
> > flexibility.
> >
> >
> > > "Create Table As .." is already handled by setting the skipData flag of the
> > > statement parsetreee before replay:
> >
> > 2) About the handling of CREATE TABLE AS:
> >
> > I think it's not a appropriate approach to set the skipdata flag on subscriber
> > as it cannot handle EXECUTE command in CTAS.
> >
> > CREATE TABLE q5_prep_results AS EXECUTE q5(200, 'DTAAAA');
> >
> > The Prepared statement is a temporary object which we don't replicate. So if
> > you directly execute the original SQL on subscriber, even if you set skipdata
> > it will fail.
> >
> > I think it difficult to make this work as you need handle the create/drop of
> > this prepared statement. And even if we extended subscriber's code to make it
> > work, it doesn't seems like a standard and elegant approach.
> >
> >
> > > "Alter Table .. " that rewrites with volatile expressions can also be handled
> > > without any syntax change, by enabling the table rewrite replication and
> > > converting the rewrite inserts to updates. ZJ's patch introduced this solution.
> >
> > 3) About the handling of ALTER TABLE rewrite.
> >
> > The approach I proposed before is based on the event trigger + deparser
> > approach. We were able to improve that approach as we don't need to
> > replicate
> > the rewrite in many cases. For example: we don't need to replicate rewrite dml
> > if there is no volatile/mutable function. We should check and filter these case
> > at publisher (e.g. via deparser) instead of checking that at subscriber.
> >
> > Besides, as discussed, we need to give warning or error for the cases when DDL
> > contains volatile function which would be executed[2]. We should check this at
> > publisher as well(via deparser).
> >
> >
> > > I think for such cases it's not full database replication and we could treat it as
> > > table level DDL replication, i.e. use the the deparser format.
> >
> > 4) I think the point could be that we should make the WAL log format
> > extendable
> > so that we can extend it to support more useful feature(table filter/schema
> > maps/DDL filter). If we just WAL log the original SQL, it seems it's difficult
> > to extend it in the future ?
>
> Attach the new version patch set which added support for CREATE/DROP/ATER
> Sequence and CREATE/DROP Schema ddl commands which are provided by Ajin
> Cherian off list.
>
> The new version patch will also check function's volatility[1] in ALTER TABLE
> command. If any function to be executed is volatile, we report an ERROR.
> Whether WARNING is better to be used here is still under consideration.
>
> [1] https://www.postgresql.org/message-id/CAA4eK1JVynFsj%2BmcRWj9sewR2yNUs6LuNxJ0eN-gNJ83oKcUOQ%40mail.gmail.com

I noticed a couple of things while trying to apply the patch and
review the patch:
1) Creation of temporary table fails infinitely in the subscriber.
CREATE TEMPORARY TABLE temp1 (a int primary key);

The above statement is converted to the below format:
CREATE TEMPORARY TABLE pg_temp.temp1 (a pg_catalog.int4 ,
CONSTRAINT temp1_pkey PRIMARY KEY (a));
While handling the creation of temporary table in the worker, the
worker fails continuously with the following error:
2022-06-22 14:24:01.317 IST [240872] ERROR: schema "pg_temp" does not exist
2022-06-22 14:24:01.317 IST [240872] CONTEXT: processing remote data
for replication origin "pg_16384" during "DDL" in transaction 725
finished at 0/14BBDA8

This error comes from handle_create_table->get_namespace_oid. Here it
checks that the pg_temp namespace is present in the system or not. As
pg_temp namespace is not present it will throw an error.
I saw one issue regarding the partition table mentioned in the commit
message. We should include this in the commit message till this issue
is resolved.

2) There are few whitespace errors while applying patch
git am v9-0001-Functions-to-deparse-DDL-commands.patch
Applying: Functions to deparse DDL commands.
.git/rebase-apply/patch:1480: indent with spaces.
ObjTree *tmp;
.git/rebase-apply/patch:1481: indent with spaces.
char *tmpstr;
.git/rebase-apply/patch:1483: indent with spaces.
tmpstr = psprintf(INT64_FORMAT, seqdata->seqcache);
.git/rebase-apply/patch:1484: indent with spaces.
tmp = new_objtree_VA("CACHE %{value}s",
.git/rebase-apply/patch:1485: indent with spaces.
2,
warning: squelched 140 whitespace errors
warning: 145 lines add whitespace errors.

Regards,
Vignesh