Re: Testing DDL Deparser - Mailing list pgsql-hackers

Hello:

Many thanks for providing feedbacks and sorry for late reply. I
updated the testing harness, please apply patches in [1] before apply
the attached patch.

>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.

My question regarding subcommand is actually on commands other than
ALTER TABLE. Let me give an example (You can also find this example in
the patch), when command like

CREATE SCHEMA element_test CREATE TABLE foo (id int)

is caught by ddl_command_end trigger, function
pg_event_trigger_ddl_commands() will return 2 records which I called
as subcommands in the previous email. One is deparsed as

CREATE SCHEMA element_test,
another is deparsed as
CREATE TABLE element_test.foo (id pg_catalog.int4 ).

Is this behavior expected? I thought the deparser is designed to
deparse the entire command to a single command instead of dividing
them into 2 commands.

>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

It seems that keeping separate test cases in deparser tests folder is
better than using the test cases in core regression tests folder
directly. I will write some test cases in the new deparser test
folder.

>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.

I see, it seems that a function to deparse DROP command to JSON output
is needed but not provided yet. I implemented a function
deparse_drop_ddl() in the testing harness, maybe we could consider
exposing a function in engine to deparse DROP command as
deparse_ddl_command() does.

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

I see. If the ddl command is not supported, the sub node will not
receive the ddl command, so the sub node will dump different results.
We are still able to detect the error. But because the error message
is not explicit, the developer needs to do some investigations in log
to find out the cause.

About the new implementation, I divided the 3 testing goals into 4 goals, from:

>1. The deparsed JSON output is as expected.
>2. The SQL commands re-formed from deparsed JSON should make the same schema change as the original SQL commands.
>3. Any DDL change without modifying the deparser should fail the testing harness.

updated to:

1, The deparsed JSON is the same as the expected string
2, The reformed SQL command is the same as the expected string
3, The original command and re-formed command can dump the same schema
4, Any DDL change without modifying the deparser should fail the
testing harness.

This new implementation achieves the first 3 goals. For the first 2
goals, the implementation is the same as test_ddl_deparse, I just
changed the notice content from command tag to deparsed JSON and
reformed SQL command. For the 3rd goal, it’s implemented using TAP
framework. The pub node will run the same test cases again, the SQL
files will be executed sequentially. After the execution of each SQL
file, the reformed commands will be applied to the sub node and the
dumped results from pub node and sub node will be compared.

For the next step, I’m going to support goal 4. Because if any DDL
change is made, the core regression test cases should also be changed.
We need to execute the core regression test cases with DDL event
triggers and the DDL deparser enabled. There are 2 solutions in my
mind:

1, Build DDL event triggers and DDL deparser into pg_regress tests so
that DDL commands in these tests can be captured and deparsed.
2, Let the goal 3 implementation, aka the TAP test to execute test
cases from pg_regress, if sub and pub node don’t dump the same
results, some DDL commands must be changed.

Solution 1 is more lighter weight as we only need to run pg_regress
once. Any other thoughts?

Next I’m going to add more test cases for each command type into this framework.

Regards,
Runqi Tian
Amazon RDS/Aurora for PostgreSQL

[1] https://www.postgresql.org/message-id/CALDaNm08gZq9a7xnsbaJMmHmi29_kbEuyShHHfxAKLXPh6btWQ%40mail.gmail.com


On Thu, Oct 6, 2022 at 12:22 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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)