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

RFC 1010: Global variables #49

Merged
merged 3 commits into from Mar 31, 2022
Merged

RFC 1010: Global variables #49

merged 3 commits into from Mar 31, 2022

Conversation

elprans
Copy link
Member

@elprans elprans commented Jan 18, 2022

No description provided.

Copy link
Contributor

@tailhook tailhook left a comment

Choose a reason for hiding this comment

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

What's written here is pretty simple, and almost uncontroversial. But I can't see a bigger picture within the current text of RFC:

  1. How it will help with the ACL? Some example queries, please (could be a separate "discussion", but important to get this spec right)
  2. How session authentication will work, given that database declares globals, but they can't be set yet.
  3. How typical queries will behave if global is in default value or unset? Will they (a) expose the whole database or (b) yield no records instead? If (b) is typical, how we can ensure that it's always (b) and users will not mistakenly expose their database?
  4. The (3) above assumes that (GLOBAL x) yields empty set if not set. But does it? Or is it an error to execute such query?
  5. How Python/JS API will look like? Does it mean you have to acquire a connection?
  6. How much impact it will have on performance? Given that right abstraction will probably have to set session variables at the start of incoming request processing even if these globals are not used by specific query.
  7. what are ways to mitigate performance issues? Will we declare required globals in the result of Prepare? Will we allow pushing variables within the Execute/OptimisticExecute?
  8. Is there a way to introspect globals? (Probably DESCRIBE GLOBAL ?)
  9. Is there a way to introspect globals, withough parsing the declaration, so Repl/Studio can ask user to type globals? One way described in (7), but there might be another way that isn't connected to a specific query. There is probably hack like SELECT { x:= (GLOBAL x)}. But this requires converting input codec to output codec, etc. I'm not sure we want to rely on hacks.
  10. What are interactions between globals and migrations? Off the top of my head: what happens if required is set on a property that has (GLOBAL xx) in the default? Can I use/set globals in data migrations?

Comment on lines 73 to 74
If a global is declared as ``REQUIRED``, it *must* provide a default value, or,
if the global is computed, the expression must be non-optional.
Copy link
Contributor

@tailhook tailhook Jan 18, 2022

Choose a reason for hiding this comment

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

What are examples of required globals? Is it something like:

scalar type Context extending enum<System, User>;
create required global context -> Context {
  set default := Context::System
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll add some examples

queries and there is no way to pass such context to schema expressions at all.


Specification
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we mark every statement, the capabilities they use? I.e. CREATE and DROP are DDLs and SET and RESET are session config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

SET GLOBAL <name> := <expr> ;


RESET GLOBAL
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have RESET ALL GLOBALS. For pool.release(conn)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need for that, I think. Globals are by definition session-local, and sessions are purely a client-side concept! I'll elaborate on the proposed protocol changes to clarify that.

The new ``GLOBAL <name>`` expression is used to refer to the value of the
given global variable in queries. For example::

SELECT User FILTER .id = (GLOBAL user_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the precedence? Always using lisp-style parenthesis is not convenient.

Alternative might be something like $global.user_id.

Copy link
Member Author

Choose a reason for hiding this comment

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

The proposed precedence is like a unary minus, so the parentheses are optional.

$global.user_id looks like an object argument $global with a user_id property. It's OK to think of all globals as properties/links of a Global singleton, but we shouldn't indicate it using the argument syntax, I think. We can use the same approach as for Config, but then we would need to add a mechanism to indicate singleton objects in schema, so that cardinality inference of Global.user_id is correct.

text/1010-global-vars.rst Show resolved Hide resolved

Synopsis::

SET GLOBAL <name> := <expr> ;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about setting multiple variables? Making each of them separate query (so separate roundtrip or 3 if there is no cache) is very unfortunate.

Copy link
Member Author

Choose a reason for hiding this comment

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

You don't have to have 3 roundtrips, you can send them semicolon-separated as a single block. Other SET (and CONFIGURE) statements also operate on per-variable basis, I don't really see a need to invent a more complex syntax here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes we have execute with multiple statements, but we don't have as far as I remember, right?

Although, if this is mostly for REPL and this is not the main way to set session variables in client bindings, then fine.

Comment on lines +143 to +144
The new ``GLOBAL <name>`` expression is used to refer to the value of the
given global variable in queries. For example::
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be more specific, where global expressions can be:

  1. In queries, for sure
  2. In computed properties and links?
  3. In defaults of properties and links?
  4. In other globals, for sure
  5. In alias definitions?
  6. Probably not in constraints, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anything that is a regular EdgeQL query or can be interpolated into a query, e.g. defaults, computeds, aliases.

Copy link
Contributor

Choose a reason for hiding this comment

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

But not in constraints, right? Constraints are technically an EdgeQL query too, right? Or do we have some other term for constraints?

Copy link
Member Author

Choose a reason for hiding this comment

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

Constraints cannot use volatile expressions, so no.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why it's "volatile" and not "stable"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, technically globals are stable (unless it's a computed global with a volatile expression), but it doesn't matter because the real showstopper here is that it's impossible to pass the global value to a constraint reasonably.

@elprans
Copy link
Member Author

elprans commented Jan 18, 2022

  1. How it will help with the ACL? Some example queries, please (could be a separate "discussion", but important to get this spec right)

I'll post a separate RFC with examples soon.

  1. The (3) above assumes that (GLOBAL x) yields empty set if not set. But does it? Or is it an error to execute such query?

Globals behave exactly like any other expression, including cardinality inference. If you want your queries to fail if a global is unset, then you must declare it as required and either use a default, a known non-empty constant expression, or assert_exists.

  1. How Python/JS API will look like? Does it mean you have to acquire a connection?

Something like:

conn = conn.with_globals(foo=bar)

The values of globals will be sent encoded in a header together with Execute or OptimisticExecute.

  1. How much impact it will have on performance? Given that right abstraction will probably have to set session variables at the start of incoming request processing even if these globals are not used by specific query.

Globals will compile into SQL query arguments, and the server will know when and how to pass the extra arguments. There should be no noticeable impact on perf other than the need to encode/decode extra data in "globals" message headers, but those are also easily cacheable.

  1. Is there a way to introspect globals? (Probably DESCRIBE GLOBAL ?)

Like every other schema object you can introspect globals via the introspection schema or get its text definition via DESCRIBE.

  1. What are interactions between globals and migrations? Off the top of my head: what happens if required is set on a property that has (GLOBAL xx) in the default? Can I use/set globals in data migrations?

Globals obey the same cardinality inference rules as other expressions. So, if you change an enclosing DDL object in a way that is incompatible with the expression containing a reference to a global, it's a schema error.

stands in place of a material set of objects (and this masquerades as an
object type), a computed global stands in for a settable global. Additionally,
the alias cardinality is usually ``multi``, whereas most globals would normally
be ``single``.
Copy link
Member

Choose a reason for hiding this comment

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

We could skip having computed globals, right, and just use aliases? The main difference would be whether you write global when referring to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yes. I think the distinction is important enough, though.

Copy link
Member

Choose a reason for hiding this comment

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

The distinction is essentially just a question of documentation of intended purpose, though, right? To be honest it almost seems to me that we almost want aliases to just be subsumed by computed globals, though it's probably too late for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a question of being able to identify references to global state in queries. The primary intended purpose of aliases is to masquerade as schema types, i.e. they look and behave as if they were persistent sets (mostly). Computed globals, on the other hand, masquerade as global variables, i.e. it is expected that they'll be defined in terms of other (non-computed) globals. The other distinction is scope: globals are per-session whereas aliases usually refer to persistent data.

Copy link
Member

Choose a reason for hiding this comment

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

Right. I think I'd feel better about this distinction if aliases weren't allowed to refer to global variables? That would be inconsistent with the idea that they are allowed anywhere, though?

I actually don't really love being able to have global variables anywhere, since it, like you said, makes it harder to identify references to global state, but allowing it would be nice in a bunch of cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm OK with restricting globals to explicit queries and rewrite policy initially.

@tailhook
Copy link
Contributor

19. What are interactions between globals and migrations? Off the top of my head: what happens if required is set on a property that has (GLOBAL xx) in the default? Can I use/set globals in data migrations?

Globals obey the same cardinality inference rules as other expressions. So, if you change an enclosing DDL object in a way that is incompatible with the expression containing a reference to a global, it's a schema error.

I mean more complicated scenarios. Like:

ALTER TYPE User {
    ALTER PROPERTY name {
        SET default := GLOBAL X;
    }
};
ALTER TYPE User {
    ALTER PROPERTY name {
        SET REQUIRED;
    }
};

Does this populate property with value for all objects when migration is applied?

If above is no-op. What about this scenario:

CREATE ALIAS Name := (SELECT GLOBAL default_name);
ALTER TYPE User {
    ALTER PROPERTY name {
        SET REQUIRED USING (SELECT Name); // or whatever syntax we have to populate required?
    }
};

Does this work? If user types (SELECT Name) in shell when doing migration, do we have a mechanism that will ask them to type GLOBAL default_name or is it an error?

Although if we "restricting globals to explicit queries and rewrite policy initially", this should not be a problem.

@elprans
Copy link
Member Author

elprans commented Jan 19, 2022

Does this populate property with value for all objects when migration is applied?

When a pointer is made required you must provide a fill expression that has lower cardinality bound of non-zero, which means that either 1) your GLOBAL is declared as required; or 2) you use assert_exists. Any query that depends on a required GLOBAL that is unset is automatically an error.

do we have a mechanism that will ask them to type GLOBAL default_name or is it an error?

It's an error to reference an unset REQUIRED GLOBAL. The error message can have a suggestion to SET GLOBAL in its hint and the CLI may even handle that and present a prompt.

@tailhook
Copy link
Contributor

do we have a mechanism that will ask them to type GLOBAL default_name or is it an error?

It's an error to reference an unset REQUIRED GLOBAL. The error message can have a suggestion to SET GLOBAL in its hint and the CLI may even handle that and present a prompt.

I'm confused. What is an unset global? My reading of this RFC was that global always have either default value or equals to an empty set.

Can I make a REQUIRED global, and only set it when queries that refer to that global are used?

If if by unset you mean "= empty set" then the problem is that assert_single(GLOBAL user_id) will only trigger error on migration commit. So suggesting to SET GLOBAL at this point is unfortunate.

@elprans
Copy link
Member Author

elprans commented Jan 20, 2022

I'm confused. What is an unset global? My reading of this RFC was that global always have either default value or equals to an empty set.

Ah yes, I see the inconsistency. Default shouldn't be mandatory for required globals. Instead, a required global essentially inserts an implicit assert_exists around itself when referenced.

Can I make a REQUIRED global, and only set it when queries that refer to that global are used?

Yes.

If if by unset you mean "= empty set" then the problem is that assert_single(GLOBAL user_id) will only trigger error on migration commit. So suggesting to SET GLOBAL at this point is unfortunate.

Presumably you'd set it with set global in the migration script, otherwise it's a broken migration. Regardless, I think we'll restrict where globals can be used for now and worry about these details later.

@msullivan
Copy link
Member

I'm going to merge this now, and then probably do some updates as I work on things

@msullivan msullivan merged commit 80f2f11 into master Mar 31, 2022
@msullivan msullivan deleted the 1010-globals branch March 31, 2022 03:23
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

3 participants