review: Deparsing DDL command strings

Hello Dimitri

* patching (success)

pavel ~/src/postgresql $ patch -p1 < binBUNnKQVPBP
patching file src/backend/catalog/heap.c
patching file src/backend/commands/event_trigger.c
patching file src/backend/commands/extension.c
patching file src/backend/commands/sequence.c
patching file src/backend/commands/view.c
patching file src/backend/tcop/utility.c
patching file src/backend/utils/adt/Makefile
patching file src/backend/utils/adt/ddl_rewrite.c
patching file src/backend/utils/adt/ruleutils.c
patching file src/backend/utils/cache/evtcache.c
patching file src/bin/psql/describe.c
patching file src/include/catalog/heap.h
patching file src/include/catalog/pg_event_trigger.h
patching file src/include/commands/event_trigger.h
patching file src/include/commands/extension.h
patching file src/include/commands/sequence.h
patching file src/include/commands/view.h
patching file src/include/utils/builtins.h
patching file src/include/utils/evtcache.h
patching file src/pl/plpgsql/src/pl_comp.c
patching file src/pl/plpgsql/src/pl_exec.c
patching file src/pl/plpgsql/src/plpgsql.h
patching file src/test/regress/expected/event_trigger.out
patching file src/test/regress/sql/event_trigger.sql

* compilation (success)

All of PostgreSQL successfully made. Ready to install.

* regress tests (success)

All 133 tests passed.

* issues

** missing doc

** statements are not deparsd for ddl_command_start event

postgres=# create table fooa(a int, b int);
NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: CREATE
TABLE, operation: CREATE, type: TABLE, schema: <NULL>, name: <NULL>
NOTICE: command: <NULL>
NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: CREATE
TABLE, operation: CREATE, type: TABLE, schema: <NULL>, name: <NULL>
NOTICE: command: <NULL>
NOTICE: event: ddl_command_end, context: TOPLEVEL, tag: CREATE TABLE,
operation: CREATE, type: TABLE, schema: public, name: fooa
NOTICE: command: CREATE TABLE public.fooa (a pg_catalog.int4, b
pg_catalog.int4);

** CREATE FUNCTION is not supported

postgres=# create or replace function bubu(a int) returns int as
$$select $1$$ language sql;
NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: CREATE
FUNCTION, operation: CREATE, type: FUNCTION, schema: <NULL>, name:
<NULL>
NOTICE: command: <NULL>
NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: CREATE
FUNCTION, operation: CREATE, type: FUNCTION, schema: <NULL>, name:
<NULL>
NOTICE: command: <NULL>
CREATE FUNCTION

* some wrong in deparsing - doesn't support constraints

postgres=# alter table a add column bbbsss int check (bbbsss > 0);
NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: ALTER
TABLE, operation: ALTER, type: TABLE, schema: <NULL>, name: <NULL>
NOTICE: command: <NULL>
NOTICE: event: ddl_command_end, context: TOPLEVEL, tag: ALTER TABLE,
operation: ALTER, type: TABLE, schema: public, name: a
NOTICE: command: ALTER TABLE public.a ADD COLUMN bbbsss pg_catalog.int4, ;
ALTER TABLE

Regards

Pavel

Hi,

Thank you Pavel for reviewing my patch! :)

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> * issues
>
> ** missing doc

Yes. I wanted to reach an agreement on why we need the de parsing for.
You can read the Last comment we made about it at the following link, I
didn't have any answer to my proposal.

http://archives.postgresql.org/message-id/m2391fu79m.fsf@2ndQuadrant.fr

In that email, I said:
> Also, I'm thinking that publishing the normalized command string is
> something that must be maintained in the core code, whereas the external
> stable format might be done as a contrib extension, coded in C, working
> with the Node *parsetree. Because it needs to ouput a compatible format
> when applied to different major versions of PostgreSQL, I think it suits
> quite well the model of C coded extensions.

I'd like to hear what people think about that position. I could be
working on the docs meanwhile I guess, but a non trivial amount of what
I'm going to document is to be thrown away if we end up deciding that we
don't want the normalized command string…

> ** statements are not deparsd for ddl_command_start event

Yes, that's by design, and that's why we have the ddl_command_trace
event alias too. Tom is right in saying that until the catalogs are
fully populated we can not rewrite most of the command string if we want
normalized output (types, constraints, namespaces, all the jazz).

> ** CREATE FUNCTION is not supported

Not yet. The goal of this patch in this CF is to get a green light on
providing a full implementation of what information to add to event
triggers and in particular about rewriting command strings.

That said, if you think it would help you as a reviewer, I may attack
the CREATE FUNCTION support right now.

> * some wrong in deparsing - doesn't support constraints
>
> postgres=# alter table a add column bbbsss int check (bbbsss > 0);
> NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: ALTER
> TABLE, operation: ALTER, type: TABLE, schema: <NULL>, name: <NULL>
> NOTICE: command: <NULL>
> NOTICE: event: ddl_command_end, context: TOPLEVEL, tag: ALTER TABLE,
> operation: ALTER, type: TABLE, schema: public, name: a
> NOTICE: command: ALTER TABLE public.a ADD COLUMN bbbsss pg_catalog.int4, ;
> ALTER TABLE

Took me some time to figure out how the constraint processing works in
CREATE TABLE, and apparently I didn't finish ALTER TABLE support yet.
Working on that, thanks.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Hello

2012/11/22 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Hi,
>
> Thank you Pavel for reviewing my patch! :)
>
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> * issues
>>
>> ** missing doc
>
> Yes. I wanted to reach an agreement on why we need the de parsing for.
> You can read the Last comment we made about it at the following link, I
> didn't have any answer to my proposal.
>
> http://archives.postgresql.org/message-id/m2391fu79m.fsf@2ndQuadrant.fr
>
> In that email, I said:
>> Also, I'm thinking that publishing the normalized command string is
>> something that must be maintained in the core code, whereas the external
>> stable format might be done as a contrib extension, coded in C, working
>> with the Node *parsetree. Because it needs to ouput a compatible format
>> when applied to different major versions of PostgreSQL, I think it suits
>> quite well the model of C coded extensions.
>
> I'd like to hear what people think about that position. I could be
> working on the docs meanwhile I guess, but a non trivial amount of what
> I'm going to document is to be thrown away if we end up deciding that we
> don't want the normalized command string…
>

I agree with you, so for PL languages just plain text is best format -
it is enough for all tasks that, I can imagine, can be solved by all
supported PL. And for complex tasks - exported parsetree is perfect.

My starting point is strategy, so everything should by possible from
PL/pgSQL, that is our most used PL - and there any complex format is
bad - the most work is doable via plan text and regular expressions.

There is precedent - EXPLAIN - we support more formats, but in PL, we
use usually just plain format.

>> ** statements are not deparsd for ddl_command_start event
>
> Yes, that's by design, and that's why we have the ddl_command_trace
> event alias too. Tom is right in saying that until the catalogs are
> fully populated we can not rewrite most of the command string if we want
> normalized output (types, constraints, namespaces, all the jazz).
>

ok

>> ** CREATE FUNCTION is not supported
>
> Not yet. The goal of this patch in this CF is to get a green light on
> providing a full implementation of what information to add to event
> triggers and in particular about rewriting command strings.

I don't have a objection. Any other ???

Regards

Pavel

>
> That said, if you think it would help you as a reviewer, I may attack
> the CREATE FUNCTION support right now.
>
>> * some wrong in deparsing - doesn't support constraints
>>
>> postgres=# alter table a add column bbbsss int check (bbbsss > 0);
>> NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: ALTER
>> TABLE, operation: ALTER, type: TABLE, schema: <NULL>, name: <NULL>
>> NOTICE: command: <NULL>
>> NOTICE: event: ddl_command_end, context: TOPLEVEL, tag: ALTER TABLE,
>> operation: ALTER, type: TABLE, schema: public, name: a
>> NOTICE: command: ALTER TABLE public.a ADD COLUMN bbbsss pg_catalog.int4, ;
>> ALTER TABLE
>
> Took me some time to figure out how the constraint processing works in
> CREATE TABLE, and apparently I didn't finish ALTER TABLE support yet.
> Working on that, thanks.
>

> Regards,
> --
> Dimitri Fontaine
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> * some wrong in deparsing - doesn't support constraints
>
> postgres=# alter table a add column bbbsss int check (bbbsss > 0);
> NOTICE: event: ddl_command_start, context: TOPLEVEL, tag: ALTER
> TABLE, operation: ALTER, type: TABLE, schema: <NULL>, name: <NULL>
> NOTICE: command: <NULL>
> NOTICE: event: ddl_command_end, context: TOPLEVEL, tag: ALTER TABLE,
> operation: ALTER, type: TABLE, schema: public, name: a
> NOTICE: command: ALTER TABLE public.a ADD COLUMN bbbsss pg_catalog.int4, ;
> ALTER TABLE

So apparently to be able to decipher what ALTER TABLE did actually do
you need more than just hook yourself after transformAlterTableStmt()
have been done, because the interesting stuff is happening in the alter
table "work queue", see ATController in src/backend/commands/tablecmds.c
for details.

Exporting that data, I'm now able to implement constraint rewriting this
way:

alter table a add column bbbsss int check (bbbsss > 0);
NOTICE: AT_AddConstraint: a_bbbsss_check
NOTICE: event: ddl_command_end, context: TOPLEVEL, tag: ALTER TABLE, operation: ALTER, type: TABLE, schema: public, name: a
NOTICE: command: ALTER TABLE public.a ADD COLUMN bbbsss pg_catalog.int4, ADD CONSTRAINT a_bbbsss_check CHECK ((bbbsss > 0));
ALTER TABLE

I did publish that work on the github repository (that I rebase every
time I'm pulling from master, beware):

https://github.com/dimitri/postgres/compare/evt_add_info

This implementation also shows to me that it's not possible to get the
command string from the parsetree directly nor after the DDL transform
step if you want such things as the constraint name that's been produced
automatically by the backend. And I don't see any way to implement that
from an extension, without first patching the backend.

As that's the kind of code we want to be able to break at will in
between releases (or to fix an important bug in a minor update), I think
we need to have the facility to provide users with the normalized
command string in core.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Hi,

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> ** missing doc

I still would prefer to have an ack about the premises of this patch
before spending time on writing docs for it, namely:

- we want a normalized command string (schema, auto naming of some
objects such as constraints and indexes, exporting default values,
etc);

- this normalisation can not happen as an extension given the current
source code organisation and the new ddl_rewrite module is a good fit
to solve that problem;

- we offer a new event alias "ddl_command_trace" that will get
activated start of a DROP and end of an ALTER or a CREATE command so
that it's easy to hook at the right place when you want to trace
things;

- it's now possible and easy to process GENERATED commands if you wish
to do so (explicit sequence and index operations for a create table
containing a serial primary key column).

I think no complaints is encouraging though, so I will probably begin
the docs effort soonish.

> ** CREATE FUNCTION is not supported

I've added support for create and drop function in the attached patch.
Not all commands are rewritten yet though, and not all of them even have
support for the TG_OBJECTID, TG_OBJECTNAME and TG_SCHEMANAME variables.

I think we could still apply that patch + docs and a limited set of
commands, and continue filling the gaps till the end of this cycle. Once
we agree on how to attack the problem at hand, do we really need support
for ALTER OPERATOR FAMILY for an intermediate commit? You tell me.

> * some wrong in deparsing - doesn't support constraints

I've been fixing that and reviewing ALTER TABLE rewriting, should be ok
now. Note that we have to analyze the alter table work queue, not the
parsetree, if we want to normalize automatic constraints names and such
like.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Hello

2012/11/27 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Hi,
>
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> ** missing doc
>
> I still would prefer to have an ack about the premises of this patch
> before spending time on writing docs for it, namely:
>
> - we want a normalized command string (schema, auto naming of some
> objects such as constraints and indexes, exporting default values,
> etc);
>
> - this normalisation can not happen as an extension given the current
> source code organisation and the new ddl_rewrite module is a good fit
> to solve that problem;
>
> - we offer a new event alias "ddl_command_trace" that will get
> activated start of a DROP and end of an ALTER or a CREATE command so
> that it's easy to hook at the right place when you want to trace
> things;
>
> - it's now possible and easy to process GENERATED commands if you wish
> to do so (explicit sequence and index operations for a create table
> containing a serial primary key column).
>

I agree with these premisses. I am sure so normalised commands strings
are very nice feature, that can be helpful for generation clean audit
logs and messages. I see a one issue - testing a consistency between
commands and their deparsed forms.

Do you have some idea, how to do these tests - Maybe we can write
extension, that will try repeated parsing and deparsing - normalised
string should be same. I am not sure, if this test should be part of
regressions test, but we have to thinking about some tool for
checking.

we can take commands via hooks and we can check.... ORIGINAL CMD ==>
tree1 ==> DEPARSED STRING ==> tree2 .... tree1 <=> tree2

Please, can others to write an agreement or any rejection now?

Does somebody know why this concept is not acceptable? Speak now, please.

> I think no complaints is encouraging though, so I will probably begin
> the docs effort soonish.
>
>> ** CREATE FUNCTION is not supported
>
> I've added support for create and drop function in the attached patch.
> Not all commands are rewritten yet though, and not all of them even have
> support for the TG_OBJECTID, TG_OBJECTNAME and TG_SCHEMANAME variables.
>

I have no problem with "defined" list of fully unsupported statements.

> I think we could still apply that patch + docs and a limited set of
> commands, and continue filling the gaps till the end of this cycle. Once
> we agree on how to attack the problem at hand, do we really need support
> for ALTER OPERATOR FAMILY for an intermediate commit? You tell me.
>
>> * some wrong in deparsing - doesn't support constraints
>
> I've been fixing that and reviewing ALTER TABLE rewriting, should be ok
> now. Note that we have to analyze the alter table work queue, not the
> parsetree, if we want to normalize automatic constraints names and such
> like.
>
> Regards,
> --
> Dimitri Fontaine
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
>

I had a small problem with patching and compilation produce warnings too

pavel ~/src/postgresql/src $ cat log | grep warning
scan.c:16247:23: warning: unused variable ‘yyg’ [-Wunused-variable]
../../../src/include/utils/ddl_rewrite.h:24:8: warning: extra tokens
at end of #endif directive [enabled by default]
../../../../src/include/utils/ddl_rewrite.h:24:8: warning: extra
tokens at end of #endif directive [enabled by default]
ddl_rewrite.c:1719:9: warning: variable ‘def’ set but not used
[-Wunused-but-set-variable]
ddl_rewrite.c:1777:21: warning: ‘languageOid’ is used uninitialized in
this function [-Wuninitialized]

All 133 tests passed.

when I tested

postgres=# create table bubuaa(a int unique not null check (a > 20));
NOTICE: snitch: ddl_command_end CREATE TABLE CREATE TABLE
public.bubuaa (a pg_catalog.int4 UNIQUE NOT NULL CHECK ((a > 20)),
CHECK ((a > 20)));
NOTICE: snitch: ddl_command_end CREATE INDEX CREATE UNIQUE INDEX ON
public.bubuaa USING btree (a);
CREATE TABLE

constraint is twice time there - it is probably same, but it is confusing

drop table generate command string

postgres=# drop table bubuaa;
NOTICE: snitch: ddl_command_end DROP TABLE <NULL>
DROP TABLE

functions are supported :)

backslash statement \dy doesn't work

postgres=# \dy
ERROR: column "evtctxs" does not exist
LINE 1: ... array_to_string(array(select x from unnest(evtctxs) a...

I was little bit surprised so general event trigger without tag
predicate was not triggered for CREATE FUNCTION statement. Is it
documented somewhere?

Regards

Pavel

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> I agree with these premisses. I am sure so normalised commands strings
> are very nice feature, that can be helpful for generation clean audit
> logs and messages. I see a one issue - testing a consistency between
> commands and their deparsed forms.

Thanks.

> Do you have some idea, how to do these tests - Maybe we can write
> extension, that will try repeated parsing and deparsing - normalised
> string should be same. I am not sure, if this test should be part of
> regressions test, but we have to thinking about some tool for
> checking.
>
> we can take commands via hooks and we can check.... ORIGINAL CMD ==>
> tree1 ==> DEPARSED STRING ==> tree2 .... tree1 <=> tree2

We could have an event trigger that stores the statements in a table,
does some object description like \d or \df+, then drops them all and
replay all the statements from the table and describe the objects again.

I'm not sure how to check that the descriptions are the same from the
regression tests themselves, but once that's manually/externally sorted
out the current facilities will sure provide guards against any
regression.

> Please, can others to write an agreement or any rejection now?
> Does somebody know why this concept is not acceptable? Speak now, please.

+1 :)

>> I've added support for create and drop function in the attached patch.
>> Not all commands are rewritten yet though, and not all of them even have
>> support for the TG_OBJECTID, TG_OBJECTNAME and TG_SCHEMANAME variables.
>
> I have no problem with "defined" list of fully unsupported statements.

Cool. The goal still is to support all functions, but I need aproval
first. It's too much work to be wasted 2 months from now.

> I had a small problem with patching and compilation produce warnings too

I will look into that. I know I had to switch in between -O0 and -O2 to
be able to debug some really strange things that happened to have been
due to our build infrastructure not being able to cope with some header
changes. Days like that I really hate the C developper tooling.

> postgres=# create table bubuaa(a int unique not null check (a > 20));
> NOTICE: snitch: ddl_command_end CREATE TABLE CREATE TABLE
> public.bubuaa (a pg_catalog.int4 UNIQUE NOT NULL CHECK ((a > 20)),
> CHECK ((a > 20)));
> NOTICE: snitch: ddl_command_end CREATE INDEX CREATE UNIQUE INDEX ON
> public.bubuaa USING btree (a);
> CREATE TABLE
>
> constraint is twice time there - it is probably same, but it is confusing

That might be due to some refactoring I just did for the ALTER TABLE.
Sharing code means "action at a distance" sometimes. I need to begin
adding regression tests, but they will look a lot like the ones Robert
did insist on removing last time…

> postgres=# \dy
> ERROR: column "evtctxs" does not exist
> LINE 1: ... array_to_string(array(select x from unnest(evtctxs) a...

My guess is that you didn't initdb the version you tested. As usual I
didn't include the catalog bump in my patch, but initdb still is needed.

> I was little bit surprised so general event trigger without tag
> predicate was not triggered for CREATE FUNCTION statement. Is it
> documented somewhere?

Works for me (from memory). Sequels of the initdb problem?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

2012/12/4 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> I agree with these premisses. I am sure so normalised commands strings
>> are very nice feature, that can be helpful for generation clean audit
>> logs and messages. I see a one issue - testing a consistency between
>> commands and their deparsed forms.
>
> Thanks.
>
>> Do you have some idea, how to do these tests - Maybe we can write
>> extension, that will try repeated parsing and deparsing - normalised
>> string should be same. I am not sure, if this test should be part of
>> regressions test, but we have to thinking about some tool for
>> checking.
>>
>> we can take commands via hooks and we can check.... ORIGINAL CMD ==>
>> tree1 ==> DEPARSED STRING ==> tree2 .... tree1 <=> tree2
>
> We could have an event trigger that stores the statements in a table,
> does some object description like \d or \df+, then drops them all and
> replay all the statements from the table and describe the objects again.
>
> I'm not sure how to check that the descriptions are the same from the
> regression tests themselves, but once that's manually/externally sorted
> out the current facilities will sure provide guards against any
> regression.
>
>> Please, can others to write an agreement or any rejection now?
>> Does somebody know why this concept is not acceptable? Speak now, please.
>
> +1 :)
>
>>> I've added support for create and drop function in the attached patch.
>>> Not all commands are rewritten yet though, and not all of them even have
>>> support for the TG_OBJECTID, TG_OBJECTNAME and TG_SCHEMANAME variables.
>>
>> I have no problem with "defined" list of fully unsupported statements.
>
> Cool. The goal still is to support all functions, but I need aproval
> first. It's too much work to be wasted 2 months from now.
>
>> I had a small problem with patching and compilation produce warnings too
>
> I will look into that. I know I had to switch in between -O0 and -O2 to
> be able to debug some really strange things that happened to have been
> due to our build infrastructure not being able to cope with some header
> changes. Days like that I really hate the C developper tooling.
>
>> postgres=# create table bubuaa(a int unique not null check (a > 20));
>> NOTICE: snitch: ddl_command_end CREATE TABLE CREATE TABLE
>> public.bubuaa (a pg_catalog.int4 UNIQUE NOT NULL CHECK ((a > 20)),
>> CHECK ((a > 20)));
>> NOTICE: snitch: ddl_command_end CREATE INDEX CREATE UNIQUE INDEX ON
>> public.bubuaa USING btree (a);
>> CREATE TABLE
>>
>> constraint is twice time there - it is probably same, but it is confusing
>
> That might be due to some refactoring I just did for the ALTER TABLE.
> Sharing code means "action at a distance" sometimes. I need to begin
> adding regression tests, but they will look a lot like the ones Robert
> did insist on removing last time…
>
>> postgres=# \dy
>> ERROR: column "evtctxs" does not exist
>> LINE 1: ... array_to_string(array(select x from unnest(evtctxs) a...
>
> My guess is that you didn't initdb the version you tested. As usual I
> didn't include the catalog bump in my patch, but initdb still is needed.
>
>> I was little bit surprised so general event trigger without tag
>> predicate was not triggered for CREATE FUNCTION statement. Is it
>> documented somewhere?
>
> Works for me (from memory). Sequels of the initdb problem?

yes, last two issue are related to missing initdb

Regards

Pavel

>
> Regards,
> --
> Dimitri Fontaine
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support