Re: Testing DDL Deparser - Mailing list pgsql-hackers

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