Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order views by dependency order in SCRIPT output (#2391) #3022

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fkalinski
Copy link

Hi! I've just encountered effects of #2390 and #2391 in my work.
We use SCRIPT to snapshot a Liquibase managed database for use in integration tests to speed up subsequent runs.

Unfortunately some of the views are exported before the tables and some views are exported before the views they depend on.
Test cases added to the commit

I've prepared a solution, by ordering the views using topological sorting.
The fix provided in #2390 looks fine, this is just more generic.

Comment on lines +7106 to +7108

----- Issue#2390 -----
CREATE TABLE TEST_TABLE ("ID" NUMBER(19, 0), "TEXT" VARCHAR(100));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please, don't add new tests to testScript.sql and testSimple.sql, use some files relevant to the command that you want to fix.
  2. Use explicit CREATE MEMORY TABLE in tests of SCRIPT command to avoid failures during tests of persistent databases.
  3. NUMBER is not a native data type of H2; it would be better to use NUMERIC(19) or any other standard or H2's own data type.

@katzyn
Copy link
Contributor

katzyn commented Feb 1, 2021

Thank you for your contribution!

Unfortunately, there is exactly the same problem as in #2982. In some real-word applications some tables depend on other views due to different reasons.

In H2 they may depend through standard default and generated expressions and also through non-standard ON UPDATE expressions.

See the following simplified example (not really useful, it only illustrates the problem):

CREATE TABLE T1(ID INT);
CREATE VIEW V AS SELECT * FROM T1;
CREATE TABLE T2(ID INT, V BIGINT DEFAULT SELECT COUNT(*) FROM V);
SCRIPT;

Before your changes everything is exported as expected:

CREATE MEMORY TABLE "PUBLIC"."T1"(
    "ID" INTEGER
);

CREATE FORCE VIEW "PUBLIC"."V"("ID") AS
SELECT
    "PUBLIC"."T1"."ID"
FROM "PUBLIC"."T1";

CREATE MEMORY TABLE "PUBLIC"."T2"(
    "ID" INTEGER,
    "V" BIGINT DEFAULT (SELECT
    COUNT(*)
FROM "PUBLIC"."V")
);

But after them objects are reordered incorrectly:

CREATE MEMORY TABLE "PUBLIC"."T1"(
    "ID" INTEGER
);

CREATE MEMORY TABLE "PUBLIC"."T2"(
    "ID" INTEGER,
    "V" BIGINT DEFAULT (SELECT
    COUNT(*)
FROM "PUBLIC"."V")
);

CREATE FORCE VIEW "PUBLIC"."V"("ID") AS
SELECT
    "PUBLIC"."T1"."ID"
FROM "PUBLIC"."T1";

This issue is much more complicated than it looks, that's why it isn't fixed yet. There are no simple solutions.

Hypothetically we can extract initialization of default and on update expressions into separate ALTER TABLE commands, but I don't see any good solutions for generated columns. Perhaps a complete graph of all dependencies should be constructed.

@grandinj
Copy link
Contributor

grandinj commented Feb 1, 2021

Hypothetically we can extract initialization of default and on update expressions into separate ALTER TABLE commands, but I don't see any good solutions for generated columns. Perhaps a complete graph of all dependencies should be constructed.

There are times that there WILL be circular dependencies.

Possibly we should export all views using FORCE when we create such a script.

We can possible add a FORCE parameter for other things too, like defaults that contain queries.

Then the ordering becomes less important.

@katzyn
Copy link
Contributor

katzyn commented Feb 1, 2021

I guess H2 will be unable to start with circular dependencies between tables.

Actually their creation is normally not possible:

CREATE TABLE T1(A INT);
CREATE TABLE T2(A INT, B INT DEFAULT SELECT COUNT(*) FROM T1);
ALTER TABLE T1 ADD COLUMN B INT DEFAULT SELECT COUNT(*) FROM T2;
> Cannot drop "PUBLIC.T1" because "PUBLIC.T2" depends on it; SQL statement:
> DROP TABLE "PUBLIC"."T1" IGNORE

(and yes, error message here is very weird too).

CREATE FORCE VIEW V AS TABLE V2;
CREATE FORCE VIEW V2 AS TABLE V;
> View "V" is invalid: "not compiled"; SQL statement:
> CREATE FORCE VIEW V2 AS TABLE V 

And so on.

Maybe there are other ways to create them, for example, with domains (I didn't test that).

So for almost all real use cases a complete graph of dependencies may really help, but code should be protected against infinite loops or stack overflow with

We can possible add a FORCE parameter for other things too, like defaults that contain queries.

We have a problem with a parser, I run into it when I tried to wrote an SQL/PSM implementation for pure SQL triggers and other objects. Parser resolves some objects immediately, but all names objects should be resolved only during execution of the command to allow parsing of anything potentially valid. There is a trick for CREATE FORCE VIEW due to such implementation, this command has a fallback code to read everything to the end of input or to semicolon. This simple workaround will not be possible for default, generated, and on update expressions, because we can't determine where expression ends so easy. It's a large work.

@lqc
Copy link

lqc commented Mar 12, 2021

Before your changes everything is exported as expected:

Is it really exported as expected? #2390 and #2391 rather suggest that it's a coincidence that this works rather than a feature of the implementation.

Currently, there really is no rule to avoiding this problem. This change (as well as others proposed) at least makes it a fair game: if your tables do not depend on views, everything will work fine.

From a pragmatic point-of-view, how often people create "tables that depend on views" vs. "views that depend on tables" ? If there is no simple way of making it work in 100% of cases, at least lets make it work in 80% of cases that are most used.

@katzyn
Copy link
Contributor

katzyn commented Mar 12, 2021

I provided a reduced test case with valid schema that works in current H2, but doesn't work with changes from this PR.

In the most cases H2 is able to export the schema properly. In some cases H2 can't do that, this PR fixes some (or maybe many) of them, but it also breaks some others that work with current H2. Sorry, but we can't replace one bug with another.

It is possible to fix the most of problematic cases without such regressions. It's enough to check all dependencies properly and take them into account. PR with such fix will be accepted.

But the complete fix requires a lot of changes everywhere.

It's actually possible to use the another approach and detach problematic expressions and set them separately. Maybe H2 should do that not only is the SCRIPT output, but in the meta table too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants