Add CREATE support to event triggers

On Fri, Nov 8, 2013 at 10:33 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Attached you can find a very-much-WIP patch to add CREATE info support
for event triggers (normalized commands). This patch builds mainly on
two things:

1. Dimitri's "DDL rewrite" patch he submitted way back, in
/messages/by-id/m2zk1j9c44.fsf@2ndQuadrant.fr

I borrowed the whole ddl_rewrite.c code, and tweaked it a bit. There
are several things still wrong with it and which will need to be fixed
before a final patch can even be contemplated; but there are some
questions that require a consensus answer before I go and fix it all,
because what it will look like will depend on said answers.

I'm still unhappy with this whole concept. It adds a significant
maintenance burden that must be carried by everyone who adds new DDL
syntax in the future, and it's easy to imagine this area of the code
ending up very poorly tested and rife with bugs. After all, event
triggers, as nifty as they are, are only going to be used by a small
minority of users. And new DDL features are typically going to be
things that are fairly obscure, so again they will only be used by a
small minority of users. I think we need to avoid the situation where
we have bugs that can only get found when those minorities happen to
intersect. If we're going to have DDL rewrite code, then we need a
way of making sure that it gets tested in a comprehensive way on a
regular basis.

Here's one idea: create a contrib module that (somehow, via APIs to be
invented) runs every DDL command that gets executed through the
deparsing code, and then parses the result and executes *that* instead
of the original command. Then, add a build target that runs the
regression test suite in that mode, and get the buildfarm configured
to run that build target regularly on at least some machines. That
way, adding syntax to the regular regression test suite also serves to
test that the deparsing logic for that syntax is working. If we do
this, there's still some maintenance burden associated with having DDL
deparsing code, but at least our chances of noticing when we've failed
to maintain it should be pretty good.

The other thing that bothers me here is that, while a normalized
command string sounds great in theory, as soon as you want to allow
(for example) mapping schema A on node 1 to schema B on node 2, the
wheels come off: you'll have to deparse that normalized command string
so you can change out the schema name and then reassemble it back into
a command string again. So we're going to parse the user input, then
deparse it, hand over the results to the application code, which will
then parse it, modify that, and deparse it again. At every step of
that process, any mistake will lead to subtle bugs in the resulting
system. Larry Wall once wrote (approximately) that a good programming
language makes simple things simple and hard things possible; I think
this design fails the second prong of that test.

Now, I guess I can live with that if it's what everyone else wants,
but I don't have a great feeling about the long-term utility of it.
Exposing the data from the DDL statement in some structured way - like
what we've done with drops, or a JSON blob, or something like that,
feels much more robust and useful than a normalized command string to
me. If the normalized command string can easily be built up from that
data, then you don't really need to expose the command string
separately. If it can't, then you're not doing a good job exposing
the data in a usable form. Saying "well, people can always parse the
normalized command string" is a cop-out. Parsing SQL is *hard*; the
only external project I know of that parses our SQL syntax well is
pgpool, and that's because they copy our parser wholesale, surely not
the sort of solution we want to foist off on event trigger authors.

Finally, I'm very skeptical of the word "normalized". To me, that
sounds like an alias for "modifying the command string in unspecified
ways that big brother thinks will be useful to event trigger authors".
Color me skeptical. What if somebody doesn't want their command
string normalized? What if they want it normalized in a way that's
different from the way that we've chosen to normalize it? I fear that
this whole direction amounts to "we don't know how to design a real
API so let's just do surgery on the command string and call whatever
pops out the API". Maybe that's harsh, but if it is I don't know why.
The normalization steps we build into this process constitute
assumptions about how the feature will be used, and they back the user
into using that feature in just that way and no other.

2. The ideas we used to build DROP support. Mainly, the interesting
thing here is the fact that we use a SRF to report, at
ddl_command_end, all the objects that were created during execution
of that command. We do this by collecting them in a list in some raw
form somewhere during ProcessUtility, and then spitting them out if
the SRF is called. I think the general idea is sound, although of
course I admit there might be bugs in the implementation.

Agreed.

Note this patch doesn't try to add any kind of ALTER support. I think
this is fine in principle, because we agreed that we would attack each
kind of command separately (divide to conquer and all that);

Also agreed.

but there
is a slight problem for some kind of objects that are represented partly
as ALTER state during creation; for example creating a table with a
sequence uses ALTER SEQ/OWNED BY internally at some point. There might
be other cases I'm missing, also. (The REFRESH command is nominally
also supported.)

There are lots of places in the DDL code where we pass around
constructed parse trees as a substitute for real argument lists. I
expect that many of those places will eventually get refactored away,
so it's important that this feature does not end up relying on
accidents of the current code structure. For example, an
AlterTableStmt can actually do a whole bunch of different things in a
single statement: SOME of those are handled by a loop in
ProcessUtilitySlow() and OTHERS are handled internally by AlterTable.
I'm pretty well convinced that that division of labor is a bad design,
and I think it's important that this feature doesn't make that dubious
design decision into documented behavior.

Now about the questions I mentioned above:

a) It doesn't work to reverse-parse the statement nodes in all cases;
there are several unfixable bugs if we only do that. In order to create
always-correct statements, we need access to the catalogs for the
created objects. But if we are doing catalog access, then it seems to
me that we can do away with the statement parse nodes completely and
just reconstruct the objects from catalog information. Shall we go that
route?

That works well for CREATE and is definitely appealing in some ways;
it probably means needing a whole lot of what's in pg_dump in the
backend also. Of course, converting the pg_dump code to a library
that can be linked into either a client or the server would make a lot
of people happy. Making pg_dump much dumber (at least as regards
future versions) and relying on new backend code to serve the same
purpose would perhaps be reasonable as well, although I know Tom is
against it. But having two copies of that code doesn't sound very
good; and we'd need some way of thoroughly testing the one living in
the backend.

c) The current patch stashes all objects in a list, whenever there's an
event trigger function. But perhaps some users want to have event
triggers and not have any use for the CREATE statements. So one idea is
to require users that want the objects reported to call a special
function in a ddl_command_start event trigger which enables collection;
if it's not called, objects are not reported. This eliminates
performance impact for people not using it, but then maybe it will be
surprising for people that call the SRF and find that it always returns
empty.

This seems like premature optimization to me, but I think I lost the
last iteration of this argument.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Nov 8, 2013 at 10:33 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Hello,

Attached you can find a very-much-WIP patch to add CREATE info support
for event triggers (normalized commands). This patch builds mainly on
two things:

1. Dimitri's "DDL rewrite" patch he submitted way back, in
/messages/by-id/m2zk1j9c44.fsf@2ndQuadrant.fr

I borrowed the whole ddl_rewrite.c code, and tweaked it a bit. There
are several things still wrong with it and which will need to be fixed
before a final patch can even be contemplated; but there are some
questions that require a consensus answer before I go and fix it all,
because what it will look like will depend on said answers.

I have tried this out; the patch applies fine.

Note that it induces (modulo my environment) a failure in "make check".

The "opr_sanity" test fails.

postgres@cbbrowne ~/p/s/t/regress> diff expected/opr_sanity.out
results/opr_sanity.out
348,350c348,351
< oid | proname
< -----+---------
< (0 rows)
---

oid | proname
------+------------------------------------------
3567 | pg_event_trigger_get_normalized_commands
(1 row)

That's a minor problem; the trouble there is that the new function is not
yet documented. Not a concern at this stage.

2. The ideas we used to build DROP support. Mainly, the interesting
thing here is the fact that we use a SRF to report, at
ddl_command_end, all the objects that were created during execution
of that command. We do this by collecting them in a list in some raw
form somewhere during ProcessUtility, and then spitting them out if
the SRF is called. I think the general idea is sound, although of
course I admit there might be bugs in the implementation.

Note this patch doesn't try to add any kind of ALTER support. I think
this is fine in principle, because we agreed that we would attack each
kind of command separately (divide to conquer and all that); but there
is a slight problem for some kind of objects that are represented partly
as ALTER state during creation; for example creating a table with a
sequence uses ALTER SEQ/OWNED BY internally at some point. There might
be other cases I'm missing, also. (The REFRESH command is nominally
also supported.)

I imagine that the things we create in earlier stages may help with later
stages, so it's worth *some* planning so we can hope not to build bits
now that push later enhancements into corners that they can't get out of.

But I'm not disagreeing at all.

Now about the questions I mentioned above:

a) It doesn't work to reverse-parse the statement nodes in all cases;
there are several unfixable bugs if we only do that. In order to create
always-correct statements, we need access to the catalogs for the
created objects. But if we are doing catalog access, then it seems to
me that we can do away with the statement parse nodes completely and
just reconstruct the objects from catalog information. Shall we go that
route?

Here's a case where it doesn't work.

testevent@localhost-> create schema foo;
CREATE SCHEMA
testevent@localhost-> create domain foo.bar integer;
CREATE DOMAIN
testevent@localhost-> CREATE OR REPLACE FUNCTION snitch() RETURNS
event_trigger LANGUAGE plpgsql AS $$
testevent$# DECLARE
testevent$# r RECORD;
testevent$# BEGIN
testevent$# FOR r IN SELECT * FROM
pg_event_trigger_get_normalized_commands()
testevent$# LOOP
testevent$# RAISE NOTICE 'object created: id %, statement %',
testevent$# r.identity, r.command;
testevent$# END LOOP;
testevent$# END;
testevent$# $$;
CREATE FUNCTION
testevent@localhost-> CREATE EVENT TRIGGER snitch ON ddl_command_end
EXECUTE PROCEDURE snitch();
CREATE EVENT TRIGGER
testevent@localhost-> set search_path to public, foo;
SET
testevent@localhost-> create table foo.foo2 (acolumn bar);
NOTICE: object created: id foo.foo2, statement CREATE TABLE foo.foo2
(acolumn bar)
CREATE TABLE

The trouble is that you have only normalized the table name. The
domain, bar, needs its name normalized as well.

b) What's the best design of the SRF output? This patch proposes two
columns, object identity and create statement. Is there use for
anything else? Class/object OIDs perhaps, schema OIDs for objects types
that have it? I don't see any immediate need to that info, but perhaps
someone does.

Probably an object type is needed as well, to know if it's a table or
a domain or a sequence or whatever.

I suspect that what will be needed to make it all usable is some sort of
"structured" form. That is in keeping with Robert Haas' discomfort with
the normalized form.

My "minor" gripe is that you haven't normalized enough (e.g. - it should be
CREATE TABLE foo.foo2 (acolumn foo.bar), capturing the normalization of
data types that are referenced).

But Robert's quite right that users may want more than just to capture that
literally; they may want to modify it, for instance, by shifting to another
schema. And it will be no fun at all if you have to construct an SQL parser
in order to change it.

The "big change" in Slony 2.2 was essentially to solve that problem; we had
been capturing logged updates as near-raw SQL, and discovered that we
really needed to capture it in a form that allows restructuring it without
needing to reparse it. It seems to me that the same need applies here.

The answer we came up with in Slony was to change from "SQL fragment"
to "array of attribute names and attribute values." I expect that is quite
unsuitable for CREATE events, being too "flat" a representation.

c) The current patch stashes all objects in a list, whenever there's an
event trigger function. But perhaps some users want to have event
triggers and not have any use for the CREATE statements. So one idea is
to require users that want the objects reported to call a special
function in a ddl_command_start event trigger which enables collection;
if it's not called, objects are not reported. This eliminates
performance impact for people not using it, but then maybe it will be
surprising for people that call the SRF and find that it always returns
empty.

Hmm. I'm not sure I follow what the issue is here (I think Robert has
the same problem; if that's so, then it's probably worth a separate
explanation/discussion).

I think there are actually more options here than just worrying about
CREATE...

I can see caring/not caring about CREATE events. Also caring/not
caring about different sorts of objects.

Thus, an event trigger might either filter on event type
(CREATE versus DROP versus ALTER vs ...), as well as on object
type (TABLE vs VIEW vs SCHEMA vs DOMAIN vs ...).

I could see that being either part of the trigger definition or as an
attribute established within the trigger.

Thus...

create event trigger foo snitch on ddl_command_end
on create, drop, alter
for table, view
execute procedure snitch();

And having internals

create or replace function snitch() returns event_trigger language plpgsql as
$$
declare
r record;
begin
for r in select * from pg_event_trigger_get_normalized_commands loop
raise notice 'action: % object type % object name % object id %
normalized statement %',
r.type, r.identity, r.id, r.command;
end loop
end;
$$;

And I suspect that there needs to be another value returned (not
necessarily by the same function) that is some form of the parse tree.

If the parse tree is already available in memory on the C side of things, then
making it accessible by calling a function that allows "walking" the tree and
invoking methods on the elements is a perfectly reasonable idea. Ideally,
there's something that can be usefully called that is written in pl/pgsql;
mandating that we punt to C to do arbitrary magic doesn't seem nice.
(If that's not reading coherently, well, blame it on my mayor being on
crack! ;-) )

d) There's a new routine uncookConstraintOrDefault. This takes a raw
expression, runs transformExpr() on it, and then deparses it (possibly
setting up a deparse context based on some relation). This is a
somewhat funny thing to be doing, so if there are other ideas on how to
handle this, I'm all ears.

Is that perhaps some of the "magic" to address my perhaps incoherent
bit about "punt to C"? It looks a bit small to be that...
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 21, 2013 at 2:36 AM, Christopher Browne <cbbrowne@gmail.com> wrote:

b) What's the best design of the SRF output? This patch proposes two
columns, object identity and create statement. Is there use for
anything else? Class/object OIDs perhaps, schema OIDs for objects types
that have it? I don't see any immediate need to that info, but perhaps
someone does.

Probably an object type is needed as well, to know if it's a table or
a domain or a sequence or whatever.

I suspect that what will be needed to make it all usable is some sort of
"structured" form. That is in keeping with Robert Haas' discomfort with
the normalized form.

My "minor" gripe is that you haven't normalized enough (e.g. - it should be
CREATE TABLE foo.foo2 (acolumn foo.bar), capturing the normalization of
data types that are referenced).

But Robert's quite right that users may want more than just to capture that
literally; they may want to modify it, for instance, by shifting to another
schema. And it will be no fun at all if you have to construct an SQL parser
in order to change it.

It's certainly much easier to transform a structured representation
into a valid SQL command string than it is to do the inverse.

You may be interested in an extension that I'm working on for a
client, which provides relation_create, relation_alter, and
relation_drop event triggers for 9.3:

https://bitbucket.org/malloclabs/pg_schema_triggers

I decided to create a composite type for each event, which can be
accessed from within the event trigger by calling a special function.
For example, the relation_alter event supplies the relation Oid and
the "old" and "new" pg_class rows. It's easy to then examine the old
vs. new rows and determine what has changed.

Kind regards,
Andrew Tipton

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas escribi�:

The other thing that bothers me here is that, while a normalized
command string sounds great in theory, as soon as you want to allow
(for example) mapping schema A on node 1 to schema B on node 2, the
wheels come off: you'll have to deparse that normalized command string
so you can change out the schema name and then reassemble it back into
a command string again. So we're going to parse the user input, then
deparse it, hand over the results to the application code, which will
then parse it, modify that, and deparse it again.

I have considered several ideas on this front, but most of them turn out
to be useless or too cumbersome to use. What seems most adequate is to
build a command string containing certain patterns, and an array of
replacement values for such patterns; each pattern corresponds to one
element that somebody might want modified in the command. As a trivial
example, a command such as

CREATE TABLE foo (bar INTEGER);

would return a string like
CREATE TABLE ${table_schema}.${table_name} (bar INTEGER);

and the replacement array would be
{table_schema => "public", table_name => "foo"}

If we additionally provide a function to expand the replacements in the
string, we would have the base funcionality of a normalized command
string. If somebody wants to move the table to some other schema, they
can simply modify the array to suit their taste, and again expand using
the provided function; this doesn't require parsing SQL. It's likely
that there are lots of fine details that need exploring before this is a
fully workable idea -- I have just started work on it, so please bear
with me.

I think this is basically what you call "a JSON blob".

Finally, I'm very skeptical of the word "normalized". To me, that
sounds like an alias for "modifying the command string in unspecified
ways that big brother thinks will be useful to event trigger authors".
Color me skeptical. What if somebody doesn't want their command
string normalized? What if they want it normalized in a way that's
different from the way that we've chosen to normalize it? I fear that
this whole direction amounts to "we don't know how to design a real
API so let's just do surgery on the command string and call whatever
pops out the API".

You might criticize the example above by saying that I haven't
considered using a JSON array for the list of table elements; in a
sense, I would be being Big Brother and deciding that you (as the user)
don't need to mess up with the column/constraints list in a table you're
creating. I thought about it and wasn't sure if there was a need to
implement that bit in the first iteration of this implementation. One
neat thing about this string+replaceables idea is that we can later
change what replaceable elements the string has, thus providing more
functionality (thus, for example, perhaps the column list can be altered
in v2 that was a "constant" in v1), without breaking existing users of
the v1.

but there
is a slight problem for some kind of objects that are represented partly
as ALTER state during creation; for example creating a table with a
sequence uses ALTER SEQ/OWNED BY internally at some point. There might
be other cases I'm missing, also. (The REFRESH command is nominally
also supported.)

There are lots of places in the DDL code where we pass around
constructed parse trees as a substitute for real argument lists. I
expect that many of those places will eventually get refactored away,
so it's important that this feature does not end up relying on
accidents of the current code structure. For example, an
AlterTableStmt can actually do a whole bunch of different things in a
single statement: SOME of those are handled by a loop in
ProcessUtilitySlow() and OTHERS are handled internally by AlterTable.
I'm pretty well convinced that that division of labor is a bad design,
and I think it's important that this feature doesn't make that dubious
design decision into documented behavior.

Yeah, the submitted patch took care of these elements by invoking the
appropriate collection function at all the right places. Most of it
happened right in ProcessUtilitySlow, but other bits were elsewhere (for
instance, sub-objects created in a complex CREATE SCHEMA command). I
mentioned the ALTER SEQUENCE example above because that happens in a
code path that wasn't even close to the rest of the stuff.

Now about the questions I mentioned above:

a) It doesn't work to reverse-parse the statement nodes in all cases;
there are several unfixable bugs if we only do that. In order to create
always-correct statements, we need access to the catalogs for the
created objects. But if we are doing catalog access, then it seems to
me that we can do away with the statement parse nodes completely and
just reconstruct the objects from catalog information. Shall we go that
route?

That works well for CREATE and is definitely appealing in some ways;
it probably means needing a whole lot of what's in pg_dump in the
backend also. Of course, converting the pg_dump code to a library
that can be linked into either a client or the server would make a lot
of people happy. Making pg_dump much dumber (at least as regards
future versions) and relying on new backend code to serve the same
purpose would perhaps be reasonable as well, although I know Tom is
against it. But having two copies of that code doesn't sound very
good; and we'd need some way of thoroughly testing the one living in
the backend.

Agreed on requiring thorough testing.

c) The current patch stashes all objects in a list, whenever there's an
event trigger function. But perhaps some users want to have event
triggers and not have any use for the CREATE statements. So one idea is
to require users that want the objects reported to call a special
function in a ddl_command_start event trigger which enables collection;
if it's not called, objects are not reported. This eliminates
performance impact for people not using it, but then maybe it will be
surprising for people that call the SRF and find that it always returns
empty.

This seems like premature optimization to me, but I think I lost the
last iteration of this argument.

I don't think you did; we ended up using a design that both doesn't
require explicit user action, nor causes a performance hit. I wasn't
sure about this, but thought I would mention just it in case. Maybe we
will end up doing the same here and have a create_sql_objects event or
something like that --- not real sure of this part yet, as there's a lot
of infrastructure to design and code first.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 3, 2014 at 9:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

The other thing that bothers me here is that, while a normalized
command string sounds great in theory, as soon as you want to allow
(for example) mapping schema A on node 1 to schema B on node 2, the
wheels come off: you'll have to deparse that normalized command string
so you can change out the schema name and then reassemble it back into
a command string again. So we're going to parse the user input, then
deparse it, hand over the results to the application code, which will
then parse it, modify that, and deparse it again.

I have considered several ideas on this front, but most of them turn out
to be useless or too cumbersome to use. What seems most adequate is to
build a command string containing certain patterns, and an array of
replacement values for such patterns; each pattern corresponds to one
element that somebody might want modified in the command. As a trivial
example, a command such as

CREATE TABLE foo (bar INTEGER);

would return a string like
CREATE TABLE ${table_schema}.${table_name} (bar INTEGER);

and the replacement array would be
{table_schema => "public", table_name => "foo"}

If we additionally provide a function to expand the replacements in the
string, we would have the base funcionality of a normalized command
string. If somebody wants to move the table to some other schema, they
can simply modify the array to suit their taste, and again expand using
the provided function; this doesn't require parsing SQL. It's likely
that there are lots of fine details that need exploring before this is a
fully workable idea -- I have just started work on it, so please bear
with me.

I think this is basically what you call "a JSON blob".

I think this direction has some potential. I'm not sure it's right in
detail. The exact scheme you propose above won't work if you want to
leave out the schema name altogether, and more generally it's not
going to help very much with anything other than substituting in
identifiers. What if you want to add a column called satellite_id to
every table that gets created, for example? What if you want to make
the tables UNLOGGED? I don't see how that kind of things is going to
work at all cleanly.

What I can imagine that might work is that you get a JSON blob for a
create table statement and then you have a function
make_a_create_table_statement(json) that will turn it back into SQL.
You can pass a modified blob (adding, removing, or changing the
schema_name or any other element) instead and it will do its thing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas escribi�:

I think this direction has some potential. I'm not sure it's right in
detail. The exact scheme you propose above won't work if you want to
leave out the schema name altogether, and more generally it's not
going to help very much with anything other than substituting in
identifiers. What if you want to add a column called satellite_id to
every table that gets created, for example? What if you want to make
the tables UNLOGGED? I don't see how that kind of things is going to
work at all cleanly.

Thanks for the discussion. I am building some basic infrastructure to
make this possible, and will explore ideas to cover these oversights
(not posting anything concrete yet because I expect several iterations
to crash and burn before I have something sensible to post).

What I can imagine that might work is that you get a JSON blob for a
create table statement and then you have a function
make_a_create_table_statement(json) that will turn it back into SQL.
You can pass a modified blob (adding, removing, or changing the
schema_name or any other element) instead and it will do its thing.

I agree, except that I would prefer not to have one function for each
DDL statement type; instead we would have a single function that knows
to expand arbitrary strings using arbitrary JSON parameter objects, and
let ddl_rewrite.c (or whatever we call that file) deal with all the
ugliness.

One function per statement would increase the maintenance cost more, I
think (three extra pg_proc entries every time you add a new object type?
Ugh.)

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Jan 6, 2014 at 1:17 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I agree, except that I would prefer not to have one function for each
DDL statement type; instead we would have a single function that knows
to expand arbitrary strings using arbitrary JSON parameter objects, and
let ddl_rewrite.c (or whatever we call that file) deal with all the
ugliness.

Yeah, that might work.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera escribi�:

Robert Haas escribi�:

I think this direction has some potential. I'm not sure it's right in
detail. The exact scheme you propose above won't work if you want to
leave out the schema name altogether, and more generally it's not
going to help very much with anything other than substituting in
identifiers. What if you want to add a column called satellite_id to
every table that gets created, for example? What if you want to make
the tables UNLOGGED? I don't see how that kind of things is going to
work at all cleanly.

Thanks for the discussion. I am building some basic infrastructure to
make this possible, and will explore ideas to cover these oversights
(not posting anything concrete yet because I expect several iterations
to crash and burn before I have something sensible to post).

Here's a working example. Suppose the user runs

CREATE SCHEMA IF NOT EXISTS "some schema" AUTHORIZATION "some guy";

In an event trigger, the function pg_event_trigger_get_creation_commands()
returns the following JSON blob:

{"authorization":{"authorization_role":"some guy",
"output":"AUTHORIZATION %i{authorization_role}"},
"if_not_exists":"IF NOT EXISTS",
"name":"some schema",
"output":"CREATE SCHEMA %{if_not_exists} %i{name} %{authorization}"}

wherein I have chosen to have a JSON element with the hardcoded name of
"output" which is what needs to be expanded; for each %{} parameter
found in it, there is an equally-named element in the JSON blob. This
can be a string, a NULL, or another JSON object.

If it's a string, it expands to that value; if it's an object,
recursively an "output" element is expanded in the same way, and the
expanded string is used.

If there's a NULL element when expanding an object, the whole thing
expands to empty. For example, if no AUTHORIZATION
clause is specified, "authorization" element is still there, but the
"authorization_role" element within it is NULL, and so the whole
AUTHORIZATION clause expands to empty and the resulting command contains
no authorization clause. This is useful to support the case that
someone doesn't have an AUTHORIZATION clause in the CREATE SCHEMA
command, and the event trigger injects one simply by setting the
authorization_role to some role name.

IF NOT EXISTS is handled by defining it to either the string IF NOT
EXISTS or to empty if no such clause was specified.

The user can modify elements in the JSON to get a different version of
the command. (I reckon the "output" can also be modified, but this is
probably a bad idea in most/all cases. I don't think there's a need to
prohibit this explicitely.) Also, someone might define "if_not_exists"
to something completely unrelated, but that would be their own fault.
(Maybe we can have some cross-check that the if_not_exists element in
JSON cannot be anything other than "IF NOT EXISTS" or the empty string;
and that the "output" element remains the same at expansion time than it
was at generation time. Perhaps we should even hide the "output"
element from the user completely and only add them to the JSON at time
of expansion. Not sure it's worth the trouble.)

There is another function,
pg_event_trigger_expand_creation_command(json), which will expand the
above JSON blob and return the following text:

CREATE SCHEMA IF NOT EXISTS "some schema" AUTHORIZATION "some guy"

Note the identifiers are properly quoted (there are quotes in the JSON
blob, but they correspond to JSON's own delimiters). I have defined a
'i' modifier to have %i{} elements, which means that the element is an
identifier which might need quoting.

I have also defined a %d{} modifier that means to use the element to
expand a possibly-qualified dotted name. (There would be no "output"
element in this case.) This is to support the case where you have

CREATE TABLE public.foo
which results in
{"table_name":{"schema":"public",
"relname":"foo"}}

and you want to edit the "table_name" element in the root JSON and set
the schema to something else (perhaps NULL), so in the event trigger
after expansion you can end up with "CREATE TABLE foo" or "CREATE TABLE
private.foo" or whatever.

Most likely there are some more rules that will need to be created, but
so far this looks sensible.

I'm going to play some more with the %d{} stuff, and also with the idea
of representing table elements such as columns and constraints as an
array. In the meantime please let me know whether this makes sense.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello

I don't like this direction. What we can do with JSON from plpgsql? More,
JSON is not too robust format against some future changes.

Regards

Pavel
Dne 8.1.2014 21:43 "Alvaro Herrera" <alvherre@2ndquadrant.com> napsal(a):

Pavel Stehule escribi�:

Hello

I don't like this direction. What we can do with JSON from plpgsql?

We have plenty of JSON functions and operators in SQL, and more to come
soon. Is that not enough?

More, JSON is not too robust format against some future changes.

Not sure what you mean. This JSON is generated and consumed by our own
code, so we only need to concern ourselves with making sure that we can
consume (in the expansion function) what we generated in the previous
phase. There is no transmission to the exterior.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

CC to hackers restored.

Pavel Stehule escribi�:

Dne 8.1.2014 23:17 "Alvaro Herrera" <alvherre@2ndquadrant.com> napsal(a):

Pavel Stehule escribi�:

Hello

I don't like this direction. What we can do with JSON from plpgsql?

We have plenty of JSON functions and operators in SQL, and more to come
soon. Is that not enough?

No, is not. Im sure. It is wrong a request to parse system internal data,
that is available in structured form. You create string that should be
parsed same time.

Few functions with OUT parameters are beter than any semistructured string.

That was shot down, for good reasons: we assume that the users of this
are going to want to modify the command before considering it final.
Maybe they want to add a column to each table being created, or they
want to change the tablespace if the table name ends with "_big", or
they want to change the schema in which it is created.

This JSON representations lets you receive the table creation data in a
well-known JSON schema; you can tweak individual elements without having
to parse the SQL command. And when you're done tweaking, there's a
function that lets you produce the SQL command that corresponds to the
original with the tweaks you just did.

(Please note that, thus far, this facility DOES NOT let you change the
table that was created, at least not directly: these event triggers are
run AFTER the creation command has completed. You can tweak the command
that would be sent to a remote server in a replication swarm, for
example. Or create a mirror table for audit purposes. Or perhaps even
generate an ALTER TABLE command for the new table.)

If by "few functions with OUT parameters" you mean that we need to have
one record type that is able to receive all possible CREATE TABLE
options, so that you can change them as a record in plpgsql, this
doesn't sound too good to me, for three reasons: a) it's going to
require too many types and functions (one per statement type); b)
cramming the stuff in pg_type.h / pg_proc.h is going to be a horrid
task; c) any change is going to require an initdb.

--
Alvaro Herrera

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 01/09/2014 04:42 AM, Alvaro Herrera wrote:

If there's a NULL element when expanding an object, the whole thing
expands to empty. For example, if no AUTHORIZATION
clause is specified, "authorization" element is still there, but the
"authorization_role" element within it is NULL, and so the whole
AUTHORIZATION clause expands to empty and the resulting command contains
no authorization clause.

I'd like to see this applied consistently to argument-less clauses like
IF NOT EXISTS too. So the same rules apply.

IF NOT EXISTS is handled by defining it to either the string IF NOT
EXISTS or to empty if no such clause was specified.

I'm not keen on this bit. It puts clauses of syntax into value strings
other than the special "output" key. Those keys aren't easily
distinguished from user data without clause specific knowledge. I'm not
keen on that.

Instead, can't we use your already proposed subclause structure?

{"authorization":{"authorization_role":"some guy",
"output":"AUTHORIZATION %i{authorization_role}"},
"if_not_exists": {"output": "IF NOT EXISTS"},
"name":"some schema",
"output":"CREATE SCHEMA %{if_not_exists} %i{name} %{authorization}"}

i.e. "if_not_exists" becomes an object. All clauses are objects, all
non-object values are user data. (right?). If the clause is absent, the
"output" key is the empty string.

The issue with that (and with your original proposal) is that you can't
tell what these clauses are supposed to be if they're not present in the
original query. You can't *enable* "IF NOT EXISTS" without pulling
knowledge of that syntax from somewhere else.

Depending on the problem you intend to solve there, that might be fine.

If it isn't, then instead there just needs to be a key to flag such
clauses as present or not.

{"authorization":{"authorization_role":"some guy",
"output":"AUTHORIZATION %i{authorization_role}"},
"if_not_exists": {"output": "IF NOT EXISTS"
"present": true},
"name":"some schema",
"output":"CREATE SCHEMA %{if_not_exists} %i{name} %{authorization}"}

Am I just over-complicating something simple here?

My reasoning is that it'd be good to be able to easily tell the
difference between *structure* and *user data* in these query trees and
do so without possibly version-specific and certainly
syntax/clause-specific knowledge about the meaning of every key of every
clause.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Craig Ringer escribi�:

Instead, can't we use your already proposed subclause structure?

{"authorization":{"authorization_role":"some guy",
"output":"AUTHORIZATION %i{authorization_role}"},
"if_not_exists": {"output": "IF NOT EXISTS"},
"name":"some schema",
"output":"CREATE SCHEMA %{if_not_exists} %i{name} %{authorization}"}

i.e. "if_not_exists" becomes an object. All clauses are objects, all
non-object values are user data. (right?). If the clause is absent, the
"output" key is the empty string.

The issue with that (and with your original proposal) is that you can't
tell what these clauses are supposed to be if they're not present in the
original query. You can't *enable* "IF NOT EXISTS" without pulling
knowledge of that syntax from somewhere else.

Depending on the problem you intend to solve there, that might be fine.

Hmm. This seems like a reasonable thing to do, except that I would like
the "output" to always be the constant, and have some other way to
enable the clause or disable it. With your "present" boolean:
so

"if_not_exists": {"output": "IF NOT EXISTS",
"present": true/false}

In fact, I'm now wondering whether this is a better idea than not
emitting anything when some element in the output expands to NULL; so it
would apply to "authorization" as well; if the command includes the
clause, it'd be

{"authorization":{"authorization_role":"some guy",
"present": true,
"output":"AUTHORIZATION %i{authorization_role}"},

and if there wasn't anything, you'd have

{"authorization":{"authorization_role": null,
"present": false,
"output":"AUTHORIZATION %i{authorization_role}"},

so if you want to turn it on and it wasn't, you need to change both the
present boolean and also set the authorization_role element; and if you
want to turn it off when it was present, just set present to false.

Am I just over-complicating something simple here?

I think it's a fair point.

My reasoning is that it'd be good to be able to easily tell the
difference between *structure* and *user data* in these query trees and
do so without possibly version-specific and certainly
syntax/clause-specific knowledge about the meaning of every key of every
clause.

Sounds reasonable.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jan 8, 2014 at 10:27 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Craig Ringer escribió:

Instead, can't we use your already proposed subclause structure?

{"authorization":{"authorization_role":"some guy",
"output":"AUTHORIZATION %i{authorization_role}"},
"if_not_exists": {"output": "IF NOT EXISTS"},
"name":"some schema",
"output":"CREATE SCHEMA %{if_not_exists} %i{name} %{authorization}"}

i.e. "if_not_exists" becomes an object. All clauses are objects, all
non-object values are user data. (right?). If the clause is absent, the
"output" key is the empty string.

The issue with that (and with your original proposal) is that you can't
tell what these clauses are supposed to be if they're not present in the
original query. You can't *enable* "IF NOT EXISTS" without pulling
knowledge of that syntax from somewhere else.

Depending on the problem you intend to solve there, that might be fine.

Hmm. This seems like a reasonable thing to do, except that I would like
the "output" to always be the constant, and have some other way to
enable the clause or disable it. With your "present" boolean:
so

"if_not_exists": {"output": "IF NOT EXISTS",
"present": true/false}

Why not:

"if_not_exists": true/false

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas escribi�:

On Wed, Jan 8, 2014 at 10:27 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Hmm. This seems like a reasonable thing to do, except that I would like
the "output" to always be the constant, and have some other way to
enable the clause or disable it. With your "present" boolean:
so

"if_not_exists": {"output": "IF NOT EXISTS",
"present": true/false}

Why not:

"if_not_exists": true/false

Yeah, that's another option. If we do this, though, the expansion
function would have to know that an "if_not_exist" element expands to IF
NOT EXISTS. Maybe that's okay. Right now, the expansion function is
pretty stupid, which is nice.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1/9/14, 11:58 AM, Alvaro Herrera wrote:

Robert Haas escribi�:

On Wed, Jan 8, 2014 at 10:27 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Hmm. This seems like a reasonable thing to do, except that I would like
the "output" to always be the constant, and have some other way to
enable the clause or disable it. With your "present" boolean:
so

"if_not_exists": {"output": "IF NOT EXISTS",
"present": true/false}

Why not:

"if_not_exists": true/false

Yeah, that's another option. If we do this, though, the expansion
function would have to know that an "if_not_exist" element expands to IF
NOT EXISTS. Maybe that's okay. Right now, the expansion function is
pretty stupid, which is nice.

Yeah, the source side of this will always have to understand the nuances of every command; it'd be really nice to not burden the other side with that as well. The only downside I see is a larger JSON output, but meh.

Another advantage is if you really wanted to you could modify the output formatting in the JSON doc to do something radically different if so inclined...
--
Jim C. Nasby, Data Architect jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8 January 2014 20:42, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

CREATE SCHEMA IF NOT EXISTS "some schema" AUTHORIZATION "some guy";

Hmm, given in 9.3 it was OK to have only DROP event triggers, I think
it should be equally acceptable to have just CREATE, but without every
option on CREATE. CREATE SCHEMA is easily the most complex thing here
and would be the command/event to deprioritise if we had any issues
getting this done/agreeing something for 9.4.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jan 9, 2014 at 5:17 PM, Jim Nasby <jim@nasby.net> wrote:

On 1/9/14, 11:58 AM, Alvaro Herrera wrote:

Robert Haas escribió:

On Wed, Jan 8, 2014 at 10:27 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Hmm. This seems like a reasonable thing to do, except that I would like
the "output" to always be the constant, and have some other way to
enable the clause or disable it. With your "present" boolean:
so

"if_not_exists": {"output": "IF NOT EXISTS",
"present": true/false}

Why not:

"if_not_exists": true/false

Yeah, that's another option. If we do this, though, the expansion
function would have to know that an "if_not_exist" element expands to IF
NOT EXISTS. Maybe that's okay. Right now, the expansion function is
pretty stupid, which is nice.

Yeah, the source side of this will always have to understand the nuances of
every command; it'd be really nice to not burden the other side with that as
well. The only downside I see is a larger JSON output, but meh.

Another advantage is if you really wanted to you could modify the output
formatting in the JSON doc to do something radically different if so
inclined...

Yeah. I wasn't necessarily objecting to the way Alvaro did it, just
asking why he did it that way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 10, 2014 at 10:36 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 8 January 2014 20:42, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

CREATE SCHEMA IF NOT EXISTS "some schema" AUTHORIZATION "some guy";

Hmm, given in 9.3 it was OK to have only DROP event triggers, I think
it should be equally acceptable to have just CREATE, but without every
option on CREATE. CREATE SCHEMA is easily the most complex thing here
and would be the command/event to deprioritise if we had any issues
getting this done/agreeing something for 9.4.

I don't know that I agree with that, but I guess we can cross that
bridge when we come to it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers