Thread: Testing DDL Deparser
Hello: I’m working on developing a testing harness for the DDL Deparser being worked on in [1], please apply the patches in [1] before apply this patch. I think the testing harness needs to achieve the following goals: 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. Based on these 3 goals, we think the deparser testing harness should have 2 parts: the first part is unit testing to cover the first two goals and the second part is integrating deparser test with pg_regress to cover the third goal. I think the unit test part can be based on https://git.postgresql.org/gitweb/?p=postgresql.git;a=tree;f=src/test/modules/test_ddl_deparse;hb=HEAD. We can improve upon this test by using it to validate the output JSON and the re-formed SQL from the deparsed JSON. [2] and [3] proposed using regression tests to test deparser and provided an implementation using TAP framework. I made some changes which enables testing any SQL file under the folder provided in $inputdir variable. This implementation enables us to run test cases under regression tests folder, or just any test cases using a SQL file. 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? 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. 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. 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? Your feedback is appreciated. Regards, Runqi Tian Amazon RDS/Aurora for PostgreSQL [1] https://www.postgresql.org/message-id/CALDaNm0VnaCg__huSDW%3Dn%3D_rSGGES90cpOtqwZeWnA6muoz3oA%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAD21AoBVCoPPRKvU_5-%3DwEXsa92GsNJFJOcYyXzvoSEJCx5dKw%40mail.gmail.com [3] https://www.postgresql.org/message-id/20150215044814.GL3391@alvh.no-ip.org
Attachment
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)
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)
Attachment
On 2022-Oct-20, Runqi Tian wrote: > 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. Ah, right. I don't remember why we made these commands be separate; but for instance if you try to add a SERIAL column you'll also see one command to create the sequence, then the table, then the sequence gets its OWNED BY the column. I think the point is that we want to have some regularity so that an application can inspect the JSON blobs and perhaps modify them; if you make a bunch of sub-objects, this becomes more difficult. For DDL replication purposes perhaps this isn't very useful (since you just grab it and execute on the other side as-is), but other use cases might have other ideas. > 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. It is expected. > 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. Well, the reason to use the regular regression tests rather than separate, is that when a developer adds a new feature, they will add test cases for it in regular regression tests, so deparsing of their command will be automatically picked up by the DDL-deparse testing framework. We discussed at the time that another option would be to have patch reviewers ensure that the added DDL commands are also tested in the DDL-deparse framework, but nobody wants to add yet another thing that we have to remember (or, more likely, forget). > 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 objection against this idea. > updated to: > > 1, The deparsed JSON is the same as the expected string I would rather say "has the same effects as". > 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? We have several runs of pg_regress already -- apart from the normal one, we run it once in recovery/t/027_stream_regress.pl and once in the pg_upgrade test. I'm not sure that we necessarily need to avoid another one here, particularly if avoiding it would potentially pollute the results for the regular tests. I am okay with solution 2 personally. If we really wanted to optimize this, perhaps we should try to drive all three uses (pg_upgrade, stream_regress, this new test) from a single pg_regress run. But ISTM we don't *have* to. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Hay dos momentos en la vida de un hombre en los que no debería especular: cuando puede permitírselo y cuando no puede" (Mark Twain)
Hi, Thanks for working on this and for the feedback! I've added the updated deparser testing module to the DDL replication thread in [1]. We'll add more test cases to the testing module and continue the discussion there. [1] https://www.postgresql.org/message-id/CAAD30U%2BA%3D2rjZ%2BxejNz%2Be1A%3DudWPQMxHD8W48nbhxwJRfw_qrA%40mail.gmail.com Regards, Zheng On Mon, Oct 24, 2022 at 7:29 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2022-Oct-20, Runqi Tian wrote: > > > 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. > > Ah, right. > > I don't remember why we made these commands be separate; but for > instance if you try to add a SERIAL column you'll also see one command > to create the sequence, then the table, then the sequence gets its OWNED > BY the column. > > I think the point is that we want to have some regularity so that an > application can inspect the JSON blobs and perhaps modify them; if you > make a bunch of sub-objects, this becomes more difficult. For DDL > replication purposes perhaps this isn't very useful (since you just grab > it and execute on the other side as-is), but other use cases might have > other ideas. > > > 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. > > It is expected. > > > 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. > > Well, the reason to use the regular regression tests rather than > separate, is that when a developer adds a new feature, they will add > test cases for it in regular regression tests, so deparsing of their > command will be automatically picked up by the DDL-deparse testing > framework. We discussed at the time that another option would be to > have patch reviewers ensure that the added DDL commands are also tested > in the DDL-deparse framework, but nobody wants to add yet another thing > that we have to remember (or, more likely, forget). > > > 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 objection against this idea. > > > updated to: > > > > 1, The deparsed JSON is the same as the expected string > > I would rather say "has the same effects as". > > > 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? > > We have several runs of pg_regress already -- apart from the normal one, > we run it once in recovery/t/027_stream_regress.pl and once in the > pg_upgrade test. I'm not sure that we necessarily need to avoid another > one here, particularly if avoiding it would potentially pollute the > results for the regular tests. I am okay with solution 2 personally. > > If we really wanted to optimize this, perhaps we should try to drive all > three uses (pg_upgrade, stream_regress, this new test) from a single > pg_regress run. But ISTM we don't *have* to. > > -- > Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ > "Hay dos momentos en la vida de un hombre en los que no debería > especular: cuando puede permitírselo y cuando no puede" (Mark Twain) > >