Re: Support logical replication of DDLs

Hi,

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.

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. So I
wondered why we don't do that for other GUC-depends propety.

---
I got a SEGV in the following case:

=# create event trigger test_trigger ON ddl_command_start execute
function publication_deparse_ddl_command_end();
CREATE EVENT TRIGGER
=# create table t ();

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com