Re: Support logical replication of DDLs

On Fri, Jun 9, 2023 at 5:35 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Jun 8, 2023 at 9:12 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > Please find new set of patches addressing below:
> > a) Issue mentioned by Wang-san in [1],
> > b) Comments from Peter given in [2]
> > c) Comments from Amit given in the last 2 emails.
> >
> > [1]: https://www.postgresql.org/message-id/OS3PR01MB62750D43D4F7F075B33BD2609E52A%40OS3PR01MB6275.jpnprd01.prod.outlook.com
> > [2]: https://www.postgresql.org/message-id/CAHut%2BPv9vPbUQc0fzrKmDkKOsS_bj-hup_E%2BsLHNEX%2B6F%2BSY5Q%40mail.gmail.com
> >
> > Thank You Vignesh for handling (a), Ajin for handling (b), Shi-san and
> > Hou-san for contributing in (c).
> >
>
> I have some questions about DDL deparser:
>
> I've tested deparsed and reformed DDL statements with the following
> function and event trigger borrowed from the regression tests:
>
> CREATE OR REPLACE FUNCTION test_ddl_deparse()
>   RETURNS event_trigger LANGUAGE plpgsql AS
> $$
> DECLARE
>         r record;
>         deparsed_json text;
> BEGIN
>         FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
>                 -- Some TABLE commands generate sequence-related
> commands, also deparse them.
>                 WHERE command_tag in ('ALTER FOREIGN TABLE', 'ALTER TABLE',
>                                                           'CREATE
> FOREIGN TABLE', 'CREATE TABLE',
>                                                           'CREATE
> TABLE AS', 'DROP FOREIGN TABLE',
>                                                           'DROP
> TABLE', 'ALTER SEQUENCE',
>                                                           'CREATE
> SEQUENCE', 'DROP SEQUENCE')
>         LOOP
>                 deparsed_json = pg_catalog.ddl_deparse_to_json(r.command);
>                 RAISE NOTICE 'deparsed json: %', deparsed_json;
>                 RAISE NOTICE 're-formed command: %',
> pg_catalog.ddl_deparse_expand_command(deparsed_json);
>         END LOOP;
> END;
> $$;
>
> CREATE EVENT TRIGGER test_ddl_deparse
> ON ddl_command_end EXECUTE PROCEDURE test_ddl_deparse();
>
> The query "CREATE TABLE TEST AS SELECT 1" is deparsed and reformed by
> DDL deparse to:
>
> CREATE  TABLE  public.test ("?column?" pg_catalog.int4 STORAGE PLAIN      )
>
> I agree that we need to deparse CTAS in such a way for logical
> replication but IIUC DDL deparse feature (e.g., ddl_deparse_to_json()
> and ddl_deparse_expand_command()) is a generic feature in principle so
> I'm concerned that there might be users who want to get the DDL
> statement that is actually executed. If so, we might want to have a
> switch to get the actual DDL statement instead.
>

I agree with an additional switch here but OTOH I think we should
consider excluding CTAS and SELECT INTO in the first version to avoid
further complications to a bigger patch. Let's just do it for
CREATE/ALTER/DROP table.

> Also, the table and column data type are schema qualified, but is
> there any reason why the reformed query doesn't explicitly specify
> tablespace, toast compression and access method? Since their default
> values depend on GUC parameters, the table could be created in a
> different tablespace on the subscriber if the subscriber set a
> different value to default_tablespace GUC parameter. IIUC the reason
> why we use schema-qualified names instead of sending along with
> search_path is to prevent tables from being created in a different
> schema depending on search_patch setting on the subscriber.
>

I think we do send schema name during DML replication as well, so
probably doing the same for DDL replication makes sense from that
angle as well. The other related point is that apply worker always set
search_path as an empty string.

> So I
> wondered why we don't do that for other GUC-depends propety.
>

Yeah, that would probably be a good idea but do we want to do it in
default mode? I think if we want to optimize the default mode such
that we only WAL log the DDL with user-specified options then
appending options for default GUCs would make the string to be WAL
logged unnecessarily long. I am thinking that in default mode we can
allow subscribers to perform operations with their default respective
GUCs.

-- 
With Regards,
Amit Kapila.