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 <alvhe...@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) > >
