Re: Testing DDL Deparser

Hello

Overall, many thanks for working on this.  I hope that the objectives
can be fulfilled, so that we can have dependable DDL replication soon.

I haven't read the patch at all, so I can't comment on what you've done,
but I have comments to some of your questions:

On 2022-Oct-05, Runqi Tian wrote:

> I came up with some ideas during the investigation and want to collect
> some feedback:
> 1, Currently we want to utilize the test cases from regression tests.
> However you will find that many test cases end with DROP commands. In
> current deparser testing approach proposed in [2] and [3], we compare
> the pg_dump schema results between the original SQL scripts and
> deparser generated commands. Because most test cases end with DROP
> command, the schema will not be shown in pg_dump, so the test coverage
> is vastly reduced. Any suggestion to this problem?

The whole reason some objects are *not* dropped is precisely so that
this type of testing has something to work on.  If we find that there
are object types that would be good to have in order to increase
coverage, what we can do is change the .sql files to not drop those
objects.  This should be as minimal as possible (i.e. we don't need tons
of tables that are all essentially identical, just a representative
bunch of objects of different types).

> 2, We found that DROP command are not returned by
> pg_event_trigger_ddl_commands() fucntion in ddl_command_end trigger,
> but it’s caught by ddl_command_end trigger. Currently, we catch DROP
> command in sql_drop trigger. It’s unclear why
> pg_event_trigger_ddl_commands() function is designed to not return
> DROP command.

Yeah, the idea is that a DDL processor needs to handle both the DROP and
the other cases separately in these two event types.  As I recall, we
needed to handle DROP separately because there was no way to collect the
necessary info otherwise.

> 3, For unsupported DDL commands by the deparser, the current
> implementation just skips them silently. So we cannot detect
> unsupported DDL commands easily. Customers may also want the deparser
> related features like logical replication to be executed in a strict
> mode, so that the system can warn them when deparser can not deparse
> some DDL command. So I propose to introduce a new GUC such as
> “StopOnDeparserUnsupportedCommand = true/false” to allow the deparser
> to execute in strict mode, in which an unsupported DDL command will
> raise an error.

No opinion on this.  I don't think the deparser should be controlled by
individual GUCs, since it will probably have multiple simultaneous uses.

> 4, We found that the event trigger function
> pg_event_trigger_ddl_commands() only returns subcommands, and deparser
> is deparsing subcommands returned by this function. The deparser works
> on subcommand level by using this function, but the deparser is
> designed to deparse the complete command to JSON output. So there is a
> mismatch here, what do you think about this problem? Should the
> deparser work at subcommand level? Or should we provide some event
> trigger function which can return the complete command instead of
> subcommands?

Not clear on what this means.  Are you talking about ALTER TABLE
subcommands?  If so, then what you have to do (ISTM) is construct a
single ALTER TABLE command containing several subcommands, when that is
what is being deparsed; the reason for the user to submit several
subcommands is to apply optimizations such as avoiding multiple table
rewrites for several operations, when they can all share a single table
rewrite.  Therefore I think replaying such a command should try and do
the same, if at all possible.


-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Crear es tan difícil como ser libre" (Elsa Triolet)