Re: Testing DDL Deparser - Mailing list pgsql-hackers

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)